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

[Thierry Reding]

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@xxxxxxx> 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@xxxxxxx>
> > 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

Attachment: signature.asc
Description: PGP signature