Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM"

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com