linux-kernel - Re: [PATCH] Revert "backlight: pwm: Handle EPROBE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151005093543.GB30219@ulmo>
Date:	Mon, 5 Oct 2015 11:35:43 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Nicolas Ferre <nicolas.ferre@...el.com>
Cc:	Robert Jarzmik <robert.jarzmik@...e.fr>, linux-pwm@...r.kernel.org,
	Boris BREZILLON <boris.brezillon@...e-electrons.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Lee Jones <lee.jones@...aro.org>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Tomi Valkeinen <tomi.valkeinen@...com>,
	linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while
 requesting the PWM"

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@...e.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@...e.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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)