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

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

[Robert Jarzmik]

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>
---
 drivers/video/backlight/pwm_bl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b234cc..57cb9ec8be43 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -272,10 +272,6 @@ 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");
-- 
2.1.4

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

[Robert Jarzmik]

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.

Cheers.

--
Robert

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

[Nicolas Ferre]

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.

Bye,
-- 
Nicolas Ferre

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

[Robert Jarzmik]

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.

-- 
Robert

PS: I have not received http://article.gmane.org/gmane.linux.pwm/2744.
    Is it my mailer or MUA which is broken, ie. was I in the "To:" of the mail ?

PPS: Sorry to having forgotten to join you to the revert

PPPS: As long as an other patch is not submitted to fix the issue (other than
      the Revert), I NAK the NAK. There is a breakage introduced, and I consider
      it a strong enough reason to be maintained for the -rc serie.

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

[Robert Jarzmik]

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.

-- 
Robert

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3616 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Boris Brezillon]

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

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4225 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Boris Brezillon]

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6483 bytes --]

On Mon, Oct 05, 2015 at 03:30:24PM +0200, 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: 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).

Right, the non-DT, slightly less legacy case...

> 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

Haha, I came up with exactly this earlier and I've been trying to think
of ways in which it could potentially break.

Thierry

--- >8 ---
From f7fee34e0c414b4268c59e97937c51e0c91a74cf Mon Sep 17 00:00:00 2001
From: Thierry Reding <thierry.reding@gmail.com>
Date: Mon, 5 Oct 2015 14:38:32 +0200
Subject: [PATCH] pwm: Return -ENODEV if no PWM lookup match is found

When looking up a PWM using the lookup table, assume that all entries
will have been added already, so failure to find a match means that no
corresponding entry has been registered.

This fixes an issue where -EPROBE_DEFER would be returned if the PWM
lookup table is empty. After this fix, -EPROBE_DEFER is reserved for
situations where no provider has yet registered for a matching entry.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3ea3350..94e5af123660 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -719,8 +719,10 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 		}
 	}
 
-	if (!chosen)
+	if (!chosen) {
+		pwm = ERR_PTR(-ENODEV);
 		goto out;
+	}
 
 	chip = pwmchip_find_by_name(chosen->provider);
 	if (!chip)
-- 
2.5.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Boris Brezillon]

On Mon, 5 Oct 2015 16:07:41 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Oct 05, 2015 at 03:30:24PM +0200, 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: 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).
> 
> Right, the non-DT, slightly less legacy case...
> 
> > 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
> 
> Haha, I came up with exactly this earlier and I've been trying to think
> of ways in which it could potentially break.

From a quick glance, I don't see any obvious problem in this approach.

> 
> Thierry
> 
> --- >8 ---
> From f7fee34e0c414b4268c59e97937c51e0c91a74cf Mon Sep 17 00:00:00 2001
> From: Thierry Reding <thierry.reding@gmail.com>
> Date: Mon, 5 Oct 2015 14:38:32 +0200
> Subject: [PATCH] pwm: Return -ENODEV if no PWM lookup match is found
> 
> When looking up a PWM using the lookup table, assume that all entries
> will have been added already, so failure to find a match means that no
> corresponding entry has been registered.
> 
> This fixes an issue where -EPROBE_DEFER would be returned if the PWM
> lookup table is empty. After this fix, -EPROBE_DEFER is reserved for
> situations where no provider has yet registered for a matching entry.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>

Not sure it has any value since I proposed the same patch, but here
is my

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/pwm/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3f9df3ea3350..94e5af123660 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -719,8 +719,10 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>  		}
>  	}
>  
> -	if (!chosen)
> +	if (!chosen) {
> +		pwm = ERR_PTR(-ENODEV);
>  		goto out;
> +	}
>  
>  	chip = pwmchip_find_by_name(chosen->provider);
>  	if (!chip)



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

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

[Vladimir Zapolskiy]

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