From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Mon, 05 Oct 2015 11:19:12 +0000 Subject: Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM" Message-Id: <20151005131912.4da38169@bbrezillon> List-Id: References: <1443295482-18687-1-git-send-email-robert.jarzmik@free.fr> <87bncjemdk.fsf@belgarion.home> <560CE816.1060307@atmel.com> <20151005093543.GB30219@ulmo> In-Reply-To: <20151005093543.GB30219@ulmo> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Thierry Reding Cc: Nicolas Ferre , Robert Jarzmik , linux-pwm@vger.kernel.org, Alexandre Belloni , Jingoo Han , Lee Jones , Jean-Christophe Plagniol-Villard , Tomi Valkeinen , linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org Hi Thierry, On Mon, 5 Oct 2015 11:35:43 +0200 Thierry Reding wrote: > On Thu, Oct 01, 2015 at 10:00:22AM +0200, Nicolas Ferre wrote: > > Le 30/09/2015 21:29, Robert Jarzmik a =C3=A9crit : > > > Robert Jarzmik writes: > > >=20 > > >> 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 there= fore > > >> chosen =3D 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 =3D PTR_ERR(pb->pwm); > > >> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret); > > >> if (ret =3D -EPROBE_DEFER) > > >> goto err_alloc; > > >> > > >> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > >> pb->legacy =3D true; > > >> pb->pwm =3D 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 > > > 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. > >=20 > > Well, as I answered on the linux-pwm mailing-list (I was not in copy) h= ere: > > http://article.gmane.org/gmane.linux.pwm/2744 > > I wonder if it's not easier to fix the platforms and add the pwm tables= ... > >=20 > > 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_de= vice *pdev) > > pb->pwm =3D devm_pwm_get(&pdev->dev, NULL); > > if (IS_ERR(pb->pwm)) { > > ret =3D PTR_ERR(pb->pwm); > > - if (ret =3D -EPROBE_DEFER) > > - goto err_alloc; > > =20 > > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); > > pb->legacy =3D true; > > pb->pwm =3D pwm_request(data->pwm_id, "pwm-backlight"); > > if (IS_ERR(pb->pwm)) { > > dev_err(&pdev->dev, "unable to request legacy PWM\n"); > > - ret =3D PTR_ERR(pb->pwm); > > + if (ret !=3D -EPROBE_DEFER) > > + ret =3D PTR_ERR(pb->pwm); > > + > > goto err_alloc; > > } > > } > >=20 > > which is not tested and may add an extra non-valid error log. >=20 > 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 =3D 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 --=20 Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com