| Message ID | 560CE816.1060307@atmel.com |
|---|---|
| State | Superseded |
| Headers | show |
Nicolas Ferre <nicolas.ferre@atmel.com> writes: > Le 30/09/2015 21:29, Robert Jarzmik a écrit : >> Robert Jarzmik <robert.jarzmik@free.fr> writes: >> >>> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f. >>> This commit breaks legacy platforms, for which : >>> (a) no pwm table is added (legacy platforms) >>> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore >>> chosen == NULL, and therefore pwm_get() returns NULL, and pwm_get() >>> returns -EPROBE_DEFER >>> (c) as a consequence, this code is unreachable in pwm_bl.c : >>> if (IS_ERR(pb->pwm)) { >>> ret = PTR_ERR(pb->pwm); >>> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); >>> if (ret == -EPROBE_DEFER) >>> goto err_alloc; >>> >>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); >>> pb->legacy = true; >>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); >>> >>> As this code is unreachable, all legacy platforms relying on pwm_id are >>> broken, amongst which pxa have been tested as broken. >>> >>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> >> Thierry, would you have a look please ? >> As I said before, all legacy platform relying on pwm_id are broken. I'd like to >> be sure this lands in the next -rc series. > > Well, as I answered on the linux-pwm mailing-list (I was not in copy) here: > http://article.gmane.org/gmane.linux.pwm/2744 > I wonder if it's not easier to fix the platforms and add the pwm tables... No it's not, at least not for a -rc cycle. It's the long term solution you're talking about, not the fix one. > Otherwise, Boris proposed this fix: > 8<----------------------------------------------------------- > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index eff379b..00483d4 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > if (IS_ERR(pb->pwm)) { > ret = PTR_ERR(pb->pwm); > - if (ret == -EPROBE_DEFER) > - goto err_alloc; > > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > pb->legacy = true; > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > if (IS_ERR(pb->pwm)) { > dev_err(&pdev->dev, "unable to request legacy PWM\n"); > - ret = PTR_ERR(pb->pwm); > + if (ret != -EPROBE_DEFER) > + ret = PTR_ERR(pb->pwm); > + > goto err_alloc; > } > } > > which is not tested and may add an extra non-valid error log. I can test that, today, it looks an interesting alternative. If both solutions do work, someone (Boris) can post a patch for this -rc instead of the revert. If no patch is posted, I maintain my Revert, as this patch _does_ break platforms (omap is broken too AFAICS). Cheers.
Nicolas Ferre <nicolas.ferre@atmel.com> writes: > Le 30/09/2015 21:29, Robert Jarzmik a écrit : >> Robert Jarzmik <robert.jarzmik@free.fr> writes: >> >>> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f. >>> This commit breaks legacy platforms, for which : >>> (a) no pwm table is added (legacy platforms) >>> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore >>> chosen == NULL, and therefore pwm_get() returns NULL, and pwm_get() >>> returns -EPROBE_DEFER >>> (c) as a consequence, this code is unreachable in pwm_bl.c : >>> if (IS_ERR(pb->pwm)) { >>> ret = PTR_ERR(pb->pwm); >>> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); >>> if (ret == -EPROBE_DEFER) >>> goto err_alloc; >>> >>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); >>> pb->legacy = true; >>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); >>> >>> As this code is unreachable, all legacy platforms relying on pwm_id are >>> broken, amongst which pxa have been tested as broken. >>> >>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> >> Thierry, would you have a look please ? >> As I said before, all legacy platform relying on pwm_id are broken. I'd like to >> be sure this lands in the next -rc series. > > Otherwise, Boris proposed this fix: > 8<----------------------------------------------------------- > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index eff379b..00483d4 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > if (IS_ERR(pb->pwm)) { > ret = PTR_ERR(pb->pwm); > - if (ret == -EPROBE_DEFER) > - goto err_alloc; > > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > pb->legacy = true; > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > if (IS_ERR(pb->pwm)) { > dev_err(&pdev->dev, "unable to request legacy PWM\n"); > - ret = PTR_ERR(pb->pwm); > + if (ret != -EPROBE_DEFER) > + ret = PTR_ERR(pb->pwm); > + > goto err_alloc; > } > } > > which is not tested and may add an extra non-valid error log. Now it is tested, and it works correctly AFAICS. So could you or Borris post this patch, with : Fixes: 68feaca0b13e ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM") Tested-by: Robert Jarzmik <robert.jarzmik@free.fr> That would be a good replacement for the revert. Maybe stable tree should be joined too ... Cheers.
On Thu, Oct 01, 2015 at 10:00:22AM +0200, Nicolas Ferre wrote: > Le 30/09/2015 21:29, Robert Jarzmik a écrit : > > Robert Jarzmik <robert.jarzmik@free.fr> writes: > > > >> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f. > >> This commit breaks legacy platforms, for which : > >> (a) no pwm table is added (legacy platforms) > >> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore > >> chosen == NULL, and therefore pwm_get() returns NULL, and pwm_get() > >> returns -EPROBE_DEFER > >> (c) as a consequence, this code is unreachable in pwm_bl.c : > >> if (IS_ERR(pb->pwm)) { > >> ret = PTR_ERR(pb->pwm); > >> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); > >> if (ret == -EPROBE_DEFER) > >> goto err_alloc; > >> > >> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > >> pb->legacy = true; > >> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > >> > >> As this code is unreachable, all legacy platforms relying on pwm_id are > >> broken, amongst which pxa have been tested as broken. > >> > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > Thierry, would you have a look please ? > > As I said before, all legacy platform relying on pwm_id are broken. I'd like to > > be sure this lands in the next -rc series. > > Well, as I answered on the linux-pwm mailing-list (I was not in copy) here: > http://article.gmane.org/gmane.linux.pwm/2744 > I wonder if it's not easier to fix the platforms and add the pwm tables... > > Otherwise, Boris proposed this fix: > 8<----------------------------------------------------------- > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index eff379b..00483d4 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > if (IS_ERR(pb->pwm)) { > ret = PTR_ERR(pb->pwm); > - if (ret == -EPROBE_DEFER) > - goto err_alloc; > > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > pb->legacy = true; > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > if (IS_ERR(pb->pwm)) { > dev_err(&pdev->dev, "unable to request legacy PWM\n"); > - ret = PTR_ERR(pb->pwm); > + if (ret != -EPROBE_DEFER) > + ret = PTR_ERR(pb->pwm); > + > goto err_alloc; > } > } > > which is not tested and may add an extra non-valid error log. This is a little risky in my opinion. Not only does it print two error messages for non-legacy platforms (that would be another regression if you want to be nit-picking), but it is subtly buggy. If you have a system with multiple PWM providers, you could end up failing the first pwm_get() with -EPROBE_DEFER but then continue to the legacy case, and this could succeed because data->pwm_id == 0, and that other provider could be exporting the PWM with this ID. If I remember correctly this was one of the reasons why the offending commit was merged in the first place. I'm afraid that fixing up the legacy platforms to use PWM lookup tables will be the only proper fix that doesn't risk breaking everyone else. I sent out patches to do that for PXA a couple of minutes ago. Looking at the history of the pwm-pxa driver it seems like quite a few PXA boards must have been broken ever since v3.6 because of the PWM ID assignment mismatch, and those should be fixed with the patches I sent as well. Thierry
Hi Thierry, On Mon, 5 Oct 2015 11:35:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Oct 01, 2015 at 10:00:22AM +0200, Nicolas Ferre wrote: > > Le 30/09/2015 21:29, Robert Jarzmik a écrit : > > > Robert Jarzmik <robert.jarzmik@free.fr> writes: > > > > > >> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f. > > >> This commit breaks legacy platforms, for which : > > >> (a) no pwm table is added (legacy platforms) > > >> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore > > >> chosen == NULL, and therefore pwm_get() returns NULL, and pwm_get() > > >> returns -EPROBE_DEFER > > >> (c) as a consequence, this code is unreachable in pwm_bl.c : > > >> if (IS_ERR(pb->pwm)) { > > >> ret = PTR_ERR(pb->pwm); > > >> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); > > >> if (ret == -EPROBE_DEFER) > > >> goto err_alloc; > > >> > > >> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > >> pb->legacy = true; > > >> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > > >> > > >> As this code is unreachable, all legacy platforms relying on pwm_id are > > >> broken, amongst which pxa have been tested as broken. > > >> > > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > > Thierry, would you have a look please ? > > > As I said before, all legacy platform relying on pwm_id are broken. I'd like to > > > be sure this lands in the next -rc series. > > > > Well, as I answered on the linux-pwm mailing-list (I was not in copy) here: > > http://article.gmane.org/gmane.linux.pwm/2744 > > I wonder if it's not easier to fix the platforms and add the pwm tables... > > > > Otherwise, Boris proposed this fix: > > 8<----------------------------------------------------------- > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > index eff379b..00483d4 100644 > > --- a/drivers/video/backlight/pwm_bl.c > > +++ b/drivers/video/backlight/pwm_bl.c > > @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > > if (IS_ERR(pb->pwm)) { > > ret = PTR_ERR(pb->pwm); > > - if (ret == -EPROBE_DEFER) > > - goto err_alloc; > > > > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > pb->legacy = true; > > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > > if (IS_ERR(pb->pwm)) { > > dev_err(&pdev->dev, "unable to request legacy PWM\n"); > > - ret = PTR_ERR(pb->pwm); > > + if (ret != -EPROBE_DEFER) > > + ret = PTR_ERR(pb->pwm); > > + > > goto err_alloc; > > } > > } > > > > which is not tested and may add an extra non-valid error log. > > This is a little risky in my opinion. Not only does it print two error > messages for non-legacy platforms (that would be another regression if > you want to be nit-picking), but it is subtly buggy. If you have a > system with multiple PWM providers, you could end up failing the first > pwm_get() with -EPROBE_DEFER but then continue to the legacy case, and > this could succeed because data->pwm_id == 0, and that other provider > could be exporting the PWM with this ID. If I remember correctly this > was one of the reasons why the offending commit was merged in the first > place. Just for the record, when I proposed this fix to Nicolas, I clearly stated that this was not the way to go, and that fixing the offending platforms to use PWM lookup table was the only sane solution, though I didn't thought about the invalid PWM id case leading to buggy behavior. Best Regards, Boris
On Mon, Oct 05, 2015 at 01:19:12PM +0200, Boris Brezillon wrote: > Hi Thierry, > > On Mon, 5 Oct 2015 11:35:43 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Thu, Oct 01, 2015 at 10:00:22AM +0200, Nicolas Ferre wrote: > > > Le 30/09/2015 21:29, Robert Jarzmik a écrit : > > > > Robert Jarzmik <robert.jarzmik@free.fr> writes: > > > > > > > >> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f. > > > >> This commit breaks legacy platforms, for which : > > > >> (a) no pwm table is added (legacy platforms) > > > >> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore > > > >> chosen == NULL, and therefore pwm_get() returns NULL, and pwm_get() > > > >> returns -EPROBE_DEFER > > > >> (c) as a consequence, this code is unreachable in pwm_bl.c : > > > >> if (IS_ERR(pb->pwm)) { > > > >> ret = PTR_ERR(pb->pwm); > > > >> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); > > > >> if (ret == -EPROBE_DEFER) > > > >> goto err_alloc; > > > >> > > > >> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > > >> pb->legacy = true; > > > >> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > > > >> > > > >> As this code is unreachable, all legacy platforms relying on pwm_id are > > > >> broken, amongst which pxa have been tested as broken. > > > >> > > > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > > > Thierry, would you have a look please ? > > > > As I said before, all legacy platform relying on pwm_id are broken. I'd like to > > > > be sure this lands in the next -rc series. > > > > > > Well, as I answered on the linux-pwm mailing-list (I was not in copy) here: > > > http://article.gmane.org/gmane.linux.pwm/2744 > > > I wonder if it's not easier to fix the platforms and add the pwm tables... > > > > > > Otherwise, Boris proposed this fix: > > > 8<----------------------------------------------------------- > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > index eff379b..00483d4 100644 > > > --- a/drivers/video/backlight/pwm_bl.c > > > +++ b/drivers/video/backlight/pwm_bl.c > > > @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > > > if (IS_ERR(pb->pwm)) { > > > ret = PTR_ERR(pb->pwm); > > > - if (ret == -EPROBE_DEFER) > > > - goto err_alloc; > > > > > > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > > pb->legacy = true; > > > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > > > if (IS_ERR(pb->pwm)) { > > > dev_err(&pdev->dev, "unable to request legacy PWM\n"); > > > - ret = PTR_ERR(pb->pwm); > > > + if (ret != -EPROBE_DEFER) > > > + ret = PTR_ERR(pb->pwm); > > > + > > > goto err_alloc; > > > } > > > } > > > > > > which is not tested and may add an extra non-valid error log. > > > > This is a little risky in my opinion. Not only does it print two error > > messages for non-legacy platforms (that would be another regression if > > you want to be nit-picking), but it is subtly buggy. If you have a > > system with multiple PWM providers, you could end up failing the first > > pwm_get() with -EPROBE_DEFER but then continue to the legacy case, and > > this could succeed because data->pwm_id == 0, and that other provider > > could be exporting the PWM with this ID. If I remember correctly this > > was one of the reasons why the offending commit was merged in the first > > place. > > Just for the record, when I proposed this fix to Nicolas, I clearly > stated that this was not the way to go, and that fixing the offending > platforms to use PWM lookup table was the only sane solution, though I > didn't thought about the invalid PWM id case leading to buggy behavior. As chance would have it, this bubbled to the top of my inbox today: http://patchwork.ozlabs.org/patch/483993/ I think that conflicts with Nicolas' -EPROBE_DEFER patch, but I think reverting Nicolas' patch and then applying the above on top might fix this nicely for everybody. Thierry
On Mon, 5 Oct 2015 14:58:03 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Oct 05, 2015 at 01:19:12PM +0200, Boris Brezillon wrote: > > Hi Thierry, > > > > On Mon, 5 Oct 2015 11:35:43 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Thu, Oct 01, 2015 at 10:00:22AM +0200, Nicolas Ferre wrote: > > > > Le 30/09/2015 21:29, Robert Jarzmik a écrit : > > > > > Robert Jarzmik <robert.jarzmik@free.fr> writes: > > > > > > > > > >> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f. > > > > >> This commit breaks legacy platforms, for which : > > > > >> (a) no pwm table is added (legacy platforms) > > > > >> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore > > > > >> chosen == NULL, and therefore pwm_get() returns NULL, and pwm_get() > > > > >> returns -EPROBE_DEFER > > > > >> (c) as a consequence, this code is unreachable in pwm_bl.c : > > > > >> if (IS_ERR(pb->pwm)) { > > > > >> ret = PTR_ERR(pb->pwm); > > > > >> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); > > > > >> if (ret == -EPROBE_DEFER) > > > > >> goto err_alloc; > > > > >> > > > > >> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > > > >> pb->legacy = true; > > > > >> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > > > > >> > > > > >> As this code is unreachable, all legacy platforms relying on pwm_id are > > > > >> broken, amongst which pxa have been tested as broken. > > > > >> > > > > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > > > > Thierry, would you have a look please ? > > > > > As I said before, all legacy platform relying on pwm_id are broken. I'd like to > > > > > be sure this lands in the next -rc series. > > > > > > > > Well, as I answered on the linux-pwm mailing-list (I was not in copy) here: > > > > http://article.gmane.org/gmane.linux.pwm/2744 > > > > I wonder if it's not easier to fix the platforms and add the pwm tables... > > > > > > > > Otherwise, Boris proposed this fix: > > > > 8<----------------------------------------------------------- > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > index eff379b..00483d4 100644 > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > > > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > > > > if (IS_ERR(pb->pwm)) { > > > > ret = PTR_ERR(pb->pwm); > > > > - if (ret == -EPROBE_DEFER) > > > > - goto err_alloc; > > > > > > > > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > > > pb->legacy = true; > > > > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > > > > if (IS_ERR(pb->pwm)) { > > > > dev_err(&pdev->dev, "unable to request legacy PWM\n"); > > > > - ret = PTR_ERR(pb->pwm); > > > > + if (ret != -EPROBE_DEFER) > > > > + ret = PTR_ERR(pb->pwm); > > > > + > > > > goto err_alloc; > > > > } > > > > } > > > > > > > > which is not tested and may add an extra non-valid error log. > > > > > > This is a little risky in my opinion. Not only does it print two error > > > messages for non-legacy platforms (that would be another regression if > > > you want to be nit-picking), but it is subtly buggy. If you have a > > > system with multiple PWM providers, you could end up failing the first > > > pwm_get() with -EPROBE_DEFER but then continue to the legacy case, and > > > this could succeed because data->pwm_id == 0, and that other provider > > > could be exporting the PWM with this ID. If I remember correctly this > > > was one of the reasons why the offending commit was merged in the first > > > place. > > > > Just for the record, when I proposed this fix to Nicolas, I clearly > > stated that this was not the way to go, and that fixing the offending > > platforms to use PWM lookup table was the only sane solution, though I > > didn't thought about the invalid PWM id case leading to buggy behavior. > > As chance would have it, this bubbled to the top of my inbox today: > > http://patchwork.ozlabs.org/patch/483993/ AFAICT, this is not valid either. This patch is assuming -EPROBE_DEFER can only be returned in the DT case, which is not the case: it is also returned if the PWMs were declared with a lookup table but the driver is not registered yet (module not loaded, or driver registration taking place after the PWM backlight driver). If we were about to differentiate the missing PWM definition from the missing driver case, we should do something like this [1]. Best Regards, Boris [1]http://code.bulix.org/2oozbq-89125
On 05.10.2015 16:30, Boris Brezillon wrote: > On Mon, 5 Oct 2015 14:58:03 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > >> On Mon, Oct 05, 2015 at 01:19:12PM +0200, Boris Brezillon wrote: >>> Hi Thierry, >>> >>> On Mon, 5 Oct 2015 11:35:43 +0200 >>> Thierry Reding <thierry.reding@gmail.com> wrote: >>> >>>> On Thu, Oct 01, 2015 at 10:00:22AM +0200, Nicolas Ferre wrote: >>>>> Le 30/09/2015 21:29, Robert Jarzmik a écrit : >>>>>> Robert Jarzmik <robert.jarzmik@free.fr> writes: >>>>>> >>>>>>> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f. >>>>>>> This commit breaks legacy platforms, for which : >>>>>>> (a) no pwm table is added (legacy platforms) >>>>>>> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore >>>>>>> chosen == NULL, and therefore pwm_get() returns NULL, and pwm_get() >>>>>>> returns -EPROBE_DEFER >>>>>>> (c) as a consequence, this code is unreachable in pwm_bl.c : >>>>>>> if (IS_ERR(pb->pwm)) { >>>>>>> ret = PTR_ERR(pb->pwm); >>>>>>> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); >>>>>>> if (ret == -EPROBE_DEFER) >>>>>>> goto err_alloc; >>>>>>> >>>>>>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); >>>>>>> pb->legacy = true; >>>>>>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); >>>>>>> >>>>>>> As this code is unreachable, all legacy platforms relying on pwm_id are >>>>>>> broken, amongst which pxa have been tested as broken. >>>>>>> >>>>>>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> >>>>>> Thierry, would you have a look please ? >>>>>> As I said before, all legacy platform relying on pwm_id are broken. I'd like to >>>>>> be sure this lands in the next -rc series. >>>>> >>>>> Well, as I answered on the linux-pwm mailing-list (I was not in copy) here: >>>>> http://article.gmane.org/gmane.linux.pwm/2744 >>>>> I wonder if it's not easier to fix the platforms and add the pwm tables... >>>>> >>>>> Otherwise, Boris proposed this fix: >>>>> 8<----------------------------------------------------------- >>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c >>>>> index eff379b..00483d4 100644 >>>>> --- a/drivers/video/backlight/pwm_bl.c >>>>> +++ b/drivers/video/backlight/pwm_bl.c >>>>> @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) >>>>> pb->pwm = devm_pwm_get(&pdev->dev, NULL); >>>>> if (IS_ERR(pb->pwm)) { >>>>> ret = PTR_ERR(pb->pwm); >>>>> - if (ret == -EPROBE_DEFER) >>>>> - goto err_alloc; >>>>> >>>>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); >>>>> pb->legacy = true; >>>>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); >>>>> if (IS_ERR(pb->pwm)) { >>>>> dev_err(&pdev->dev, "unable to request legacy PWM\n"); >>>>> - ret = PTR_ERR(pb->pwm); >>>>> + if (ret != -EPROBE_DEFER) >>>>> + ret = PTR_ERR(pb->pwm); >>>>> + >>>>> goto err_alloc; >>>>> } >>>>> } >>>>> >>>>> which is not tested and may add an extra non-valid error log. >>>> >>>> This is a little risky in my opinion. Not only does it print two error >>>> messages for non-legacy platforms (that would be another regression if >>>> you want to be nit-picking), but it is subtly buggy. If you have a >>>> system with multiple PWM providers, you could end up failing the first >>>> pwm_get() with -EPROBE_DEFER but then continue to the legacy case, and >>>> this could succeed because data->pwm_id == 0, and that other provider >>>> could be exporting the PWM with this ID. If I remember correctly this >>>> was one of the reasons why the offending commit was merged in the first >>>> place. >>> >>> Just for the record, when I proposed this fix to Nicolas, I clearly >>> stated that this was not the way to go, and that fixing the offending >>> platforms to use PWM lookup table was the only sane solution, though I >>> didn't thought about the invalid PWM id case leading to buggy behavior. >> >> As chance would have it, this bubbled to the top of my inbox today: >> >> http://patchwork.ozlabs.org/patch/483993/ > > AFAICT, this is not valid either. This patch is assuming -EPROBE_DEFER > can only be returned in the DT case, which is not the case: FYI at the time I've created and sent the change for review (Oct. 11, 2014 (!), https://patchwork.ozlabs.org/patch/398849/) there was no any known to me intention to handle -EPROBE_DEFER . Last week Lee mentioned that the patch does not apply due to added -EPROBE_DEFER handling, so I've sent today to linux-pwm a rebased version of the same change, which hopefully is acceptable, please take a look. > it is also returned if the PWMs were declared with a lookup table > but the driver is not registered yet (module not loaded, or driver > registration taking place after the PWM backlight driver). > > If we were about to differentiate the missing PWM definition from > the missing driver case, we should do something like this [1]. > > Best Regards, > > Boris > > [1]http://code.bulix.org/2oozbq-89125 > > -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index eff379b..00483d4 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->pwm = devm_pwm_get(&pdev->dev, NULL); if (IS_ERR(pb->pwm)) { ret = PTR_ERR(pb->pwm); - if (ret == -EPROBE_DEFER) - goto err_alloc; dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); pb->legacy = true; pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); if (IS_ERR(pb->pwm)) { dev_err(&pdev->dev, "unable to request legacy PWM\n"); - ret = PTR_ERR(pb->pwm); + if (ret != -EPROBE_DEFER) + ret = PTR_ERR(pb->pwm); + goto err_alloc; } }