[PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Sasha Levin]

From: Matthieu CASTET <matthieu.castet@parrot.com>

[ Upstream commit 2b83ff96f51d0b039c4561b9f95c824d7bddb85c ]

With the current code, the following sequence won't work :
echo timer > trigger

echo 0 >  delay_off
* at this point we call
** led_delay_off_store
** led_blink_set
*** stop timer
** led_blink_setup
** led_set_software_blink
*** if !delay_on, led off
*** if !delay_off, set led_set_brightness_nosleep <--- LED_BLINK_SW is set but timer is stop
*** otherwise start timer/set LED_BLINK_SW flag

echo xxx > brightness
* led_set_brightness
** if LED_BLINK_SW
*** if brightness=0, led off
*** else apply brightness if next timer <--- timer is stop, and will never apply new setting
** otherwise set led_set_brightness_nosleep

To fix that, when we delete the timer, we should clear LED_BLINK_SW.

Cc: linux-leds@vger.kernel.org
Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
---
 drivers/leds/led-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index ef1360445413..af630c11c284 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -187,7 +187,7 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
-	del_timer_sync(&led_cdev->blink_timer);
+	led_stop_software_blink(led_cdev);
 
 	clear_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags);
 	clear_bit(LED_BLINK_ONESHOT_STOP, &led_cdev->work_flags);
-- 
2.11.0

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Pavel Machek]

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

On Sat 2018-02-03 18:00:59, Sasha Levin wrote:
> From: Matthieu CASTET <matthieu.castet@parrot.com>
> 
> [ Upstream commit 2b83ff96f51d0b039c4561b9f95c824d7bddb85c ]
> 
> With the current code, the following sequence won't work :
> echo timer > trigger
> 
> echo 0 >  delay_off
> * at this point we call
> ** led_delay_off_store
> ** led_blink_set
> *** stop timer
> ** led_blink_setup
> ** led_set_software_blink
> *** if !delay_on, led off
> *** if !delay_off, set led_set_brightness_nosleep <--- LED_BLINK_SW is set but timer is stop
> *** otherwise start timer/set LED_BLINK_SW flag
> 
> echo xxx > brightness
> * led_set_brightness
> ** if LED_BLINK_SW
> *** if brightness=0, led off
> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> ** otherwise set led_set_brightness_nosleep
> 
> To fix that, when we delete the timer, we should clear LED_BLINK_SW.

Can you run the tests on the affected stable kernels? I have feeling
that the problem described might not be present there.

Thanks,
								Pavel

> Cc: linux-leds@vger.kernel.org
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
> ---
>  drivers/leds/led-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ef1360445413..af630c11c284 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -187,7 +187,7 @@ void led_blink_set(struct led_classdev *led_cdev,
>  		   unsigned long *delay_on,
>  		   unsigned long *delay_off)
>  {
> -	del_timer_sync(&led_cdev->blink_timer);
> +	led_stop_software_blink(led_cdev);
>  
>  	clear_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags);
>  	clear_bit(LED_BLINK_ONESHOT_STOP, &led_cdev->work_flags);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Sasha Levin]

On Sat, Feb 03, 2018 at 09:35:26PM +0100, Pavel Machek wrote:
>On Sat 2018-02-03 18:00:59, Sasha Levin wrote:
>> From: Matthieu CASTET <matthieu.castet@parrot.com>
>>
>> [ Upstream commit 2b83ff96f51d0b039c4561b9f95c824d7bddb85c ]
>>
>> With the current code, the following sequence won't work :
>> echo timer > trigger
>>
>> echo 0 >  delay_off
>> * at this point we call
>> ** led_delay_off_store
>> ** led_blink_set
>> *** stop timer
>> ** led_blink_setup
>> ** led_set_software_blink
>> *** if !delay_on, led off
>> *** if !delay_off, set led_set_brightness_nosleep <--- LED_BLINK_SW is set but timer is stop
>> *** otherwise start timer/set LED_BLINK_SW flag
>>
>> echo xxx > brightness
>> * led_set_brightness
>> ** if LED_BLINK_SW
>> *** if brightness=0, led off
>> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
>> ** otherwise set led_set_brightness_nosleep
>>
>> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
>
>Can you run the tests on the affected stable kernels? I have feeling
>that the problem described might not be present there.

Hm, I don't seem to have HW to test that out. Maybe someone else does?

-- 

Thanks,
Sasha

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Pavel Machek]

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

On Sun 2018-02-04 00:30:36, Sasha Levin wrote:
> On Sat, Feb 03, 2018 at 09:35:26PM +0100, Pavel Machek wrote:
> >On Sat 2018-02-03 18:00:59, Sasha Levin wrote:
> >> From: Matthieu CASTET <matthieu.castet@parrot.com>
> >>
> >> [ Upstream commit 2b83ff96f51d0b039c4561b9f95c824d7bddb85c ]
> >>
> >> With the current code, the following sequence won't work :
> >> echo timer > trigger
> >>
> >> echo 0 >  delay_off
> >> * at this point we call
> >> ** led_delay_off_store
> >> ** led_blink_set
> >> *** stop timer
> >> ** led_blink_setup
> >> ** led_set_software_blink
> >> *** if !delay_on, led off
> >> *** if !delay_off, set led_set_brightness_nosleep <--- LED_BLINK_SW is set but timer is stop
> >> *** otherwise start timer/set LED_BLINK_SW flag
> >>
> >> echo xxx > brightness
> >> * led_set_brightness
> >> ** if LED_BLINK_SW
> >> *** if brightness=0, led off
> >> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> >> ** otherwise set led_set_brightness_nosleep
> >>
> >> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
> >
> >Can you run the tests on the affected stable kernels? I have feeling
> >that the problem described might not be present there.
> 
> Hm, I don't seem to have HW to test that out. Maybe someone else does?

Why are you submitting patches you have no way to test?

Plus... you don't have PC keyboard? You don't have thinkpad notebook?


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Greg KH]

On Sun, Feb 04, 2018 at 10:05:31AM +0100, Pavel Machek wrote:
> On Sun 2018-02-04 00:30:36, Sasha Levin wrote:
> > On Sat, Feb 03, 2018 at 09:35:26PM +0100, Pavel Machek wrote:
> > >On Sat 2018-02-03 18:00:59, Sasha Levin wrote:
> > >> From: Matthieu CASTET <matthieu.castet@parrot.com>
> > >>
> > >> [ Upstream commit 2b83ff96f51d0b039c4561b9f95c824d7bddb85c ]
> > >>
> > >> With the current code, the following sequence won't work :
> > >> echo timer > trigger
> > >>
> > >> echo 0 >  delay_off
> > >> * at this point we call
> > >> ** led_delay_off_store
> > >> ** led_blink_set
> > >> *** stop timer
> > >> ** led_blink_setup
> > >> ** led_set_software_blink
> > >> *** if !delay_on, led off
> > >> *** if !delay_off, set led_set_brightness_nosleep <--- LED_BLINK_SW is set but timer is stop
> > >> *** otherwise start timer/set LED_BLINK_SW flag
> > >>
> > >> echo xxx > brightness
> > >> * led_set_brightness
> > >> ** if LED_BLINK_SW
> > >> *** if brightness=0, led off
> > >> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> > >> ** otherwise set led_set_brightness_nosleep
> > >>
> > >> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
> > >
> > >Can you run the tests on the affected stable kernels? I have feeling
> > >that the problem described might not be present there.
> > 
> > Hm, I don't seem to have HW to test that out. Maybe someone else does?
> 
> Why are you submitting patches you have no way to test?

What?  This is stable tree backporting, why are you trying to make a
requirement for something that we have never had before?

This is a backport of a patch that is already upstream.  If it doesn't
belong in a stable tree, great, let us know that, saying why it is so.

thanks,

greg k-h

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Sasha Levin]

On Sun, Feb 04, 2018 at 10:05:31AM +0100, Pavel Machek wrote:
>On Sun 2018-02-04 00:30:36, Sasha Levin wrote:
>> On Sat, Feb 03, 2018 at 09:35:26PM +0100, Pavel Machek wrote:
>> >On Sat 2018-02-03 18:00:59, Sasha Levin wrote:
>> >> From: Matthieu CASTET <matthieu.castet@parrot.com>
>> >>
>> >> [ Upstream commit 2b83ff96f51d0b039c4561b9f95c824d7bddb85c ]
>> >>
>> >> With the current code, the following sequence won't work :
>> >> echo timer > trigger
>> >>
>> >> echo 0 >  delay_off
>> >> * at this point we call
>> >> ** led_delay_off_store
>> >> ** led_blink_set
>> >> *** stop timer
>> >> ** led_blink_setup
>> >> ** led_set_software_blink
>> >> *** if !delay_on, led off
>> >> *** if !delay_off, set led_set_brightness_nosleep <--- LED_BLINK_SW is set but timer is stop
>> >> *** otherwise start timer/set LED_BLINK_SW flag
>> >>
>> >> echo xxx > brightness
>> >> * led_set_brightness
>> >> ** if LED_BLINK_SW
>> >> *** if brightness=0, led off
>> >> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
>> >> ** otherwise set led_set_brightness_nosleep
>> >>
>> >> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
>> >
>> >Can you run the tests on the affected stable kernels? I have feeling
>> >that the problem described might not be present there.
>>
>> Hm, I don't seem to have HW to test that out. Maybe someone else does?
>
>Why are you submitting patches you have no way to test?

Because.... that's how the process works? -stable maintainers don't test
every single patch that goes in. Never happened and I doubt it ever
will.

>Plus... you don't have PC keyboard? You don't have thinkpad notebook?

Nope, all the (limited) testing I do happens in VMs, which don't expose
leds as far as I know.

-- 

Thanks,
Sasha

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Pavel Machek]

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


> > > >> *** if brightness=0, led off
> > > >> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> > > >> ** otherwise set led_set_brightness_nosleep
> > > >>
> > > >> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
> > > >
> > > >Can you run the tests on the affected stable kernels? I have feeling
> > > >that the problem described might not be present there.
> > > 
> > > Hm, I don't seem to have HW to test that out. Maybe someone else does?
> > 
> > Why are you submitting patches you have no way to test?
> 
> What?  This is stable tree backporting, why are you trying to make a
> requirement for something that we have never had before?

I don't think random patches should be sent to stable just because
they appeared in mainline. Plus, I don't think I'm making new rules:

submit-checklist.rst:

13) Has been build- and runtime tested with and without ``CONFIG_SMP``
and
    ``CONFIG_PREEMPT.``

stable-kernel-rules.rst:

Rules on what kind of patches are accepted, and which ones are not,
into the "-stable" tree:

 - It must be obviously correct and tested.
 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).
   
> This is a backport of a patch that is already upstream.  If it doesn't
> belong in a stable tree, great, let us know that, saying why it is so.

See jacek.anaszewski@gmail.com 's explanation.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Pavel Machek]

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

On Sun 2018-02-04 15:49:06, Sasha Levin wrote:
> On Sun, Feb 04, 2018 at 10:05:31AM +0100, Pavel Machek wrote:
> >On Sun 2018-02-04 00:30:36, Sasha Levin wrote:
> >> On Sat, Feb 03, 2018 at 09:35:26PM +0100, Pavel Machek wrote:
> >> >On Sat 2018-02-03 18:00:59, Sasha Levin wrote:
> >> >> From: Matthieu CASTET <matthieu.castet@parrot.com>
> >> >>
> >> >> [ Upstream commit 2b83ff96f51d0b039c4561b9f95c824d7bddb85c ]
> >> >>
> >> >> With the current code, the following sequence won't work :
> >> >> echo timer > trigger
> >> >>
> >> >> echo 0 >  delay_off
> >> >> * at this point we call
> >> >> ** led_delay_off_store
> >> >> ** led_blink_set
> >> >> *** stop timer
> >> >> ** led_blink_setup
> >> >> ** led_set_software_blink
> >> >> *** if !delay_on, led off
> >> >> *** if !delay_off, set led_set_brightness_nosleep <--- LED_BLINK_SW is set but timer is stop
> >> >> *** otherwise start timer/set LED_BLINK_SW flag
> >> >>
> >> >> echo xxx > brightness
> >> >> * led_set_brightness
> >> >> ** if LED_BLINK_SW
> >> >> *** if brightness=0, led off
> >> >> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> >> >> ** otherwise set led_set_brightness_nosleep
> >> >>
> >> >> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
> >> >
> >> >Can you run the tests on the affected stable kernels? I have feeling
> >> >that the problem described might not be present there.
> >>
> >> Hm, I don't seem to have HW to test that out. Maybe someone else does?
> >
> >Why are you submitting patches you have no way to test?
> 
> Because.... that's how the process works? -stable maintainers don't test
> every single patch that goes in. Never happened and I doubt it ever
> will.

Well, see my reply to Greg. According to documentation, stable tree is
for

 - It must be obviously correct and tested.
 - It must fix a real bug that bothers people (not a, "This could
   be a   problem..." type thing).
 - It must fix a problem that causes a build error (but not for
   things marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, 
   something critical.
    
. This one was not tested in the stable trees, and it was not really
bothering anyone. It was not even present in some of the kernels... It
was not an oops, hang... etc.

> >Plus... you don't have PC keyboard? You don't have thinkpad notebook?
> 
> Nope, all the (limited) testing I do happens in VMs, which don't expose
> leds as far as I know.

You still use keyboard to access the VMs, right?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Sasha Levin]

On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
>
>> > > >> *** if brightness=0, led off
>> > > >> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
>> > > >> ** otherwise set led_set_brightness_nosleep
>> > > >>
>> > > >> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
>> > > >
>> > > >Can you run the tests on the affected stable kernels? I have feeling
>> > > >that the problem described might not be present there.
>> > >
>> > > Hm, I don't seem to have HW to test that out. Maybe someone else does?
>> >
>> > Why are you submitting patches you have no way to test?
>>
>> What?  This is stable tree backporting, why are you trying to make a
>> requirement for something that we have never had before?
>
>I don't think random patches should be sent to stable just because
>they appeared in mainline. Plus, I don't think I'm making new rules:
>
>submit-checklist.rst:
>
>13) Has been build- and runtime tested with and without ``CONFIG_SMP``
>and
>    ``CONFIG_PREEMPT.``
>
>stable-kernel-rules.rst:
>
>Rules on what kind of patches are accepted, and which ones are not,
>into the "-stable" tree:
>
> - It must be obviously correct and tested.
> - It must fix a real bug that bothers people (not a, "This could be a
>   problem..." type thing).

So you're saying that this doesn't qualify as a bug?

>> This is a backport of a patch that is already upstream.  If it doesn't
>> belong in a stable tree, great, let us know that, saying why it is so.
>
>See jacek.anaszewski@gmail.com 's explanation.

I might be missing something, but Jacek suggested I pull another patch
before this one?

-- 

Thanks,
Sasha

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Jacek Anaszewski]

On 02/06/2018 03:02 AM, Sasha Levin wrote:
> On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
>>
>>>>>>> *** if brightness=0, led off
>>>>>>> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
>>>>>>> ** otherwise set led_set_brightness_nosleep
>>>>>>>
>>>>>>> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
>>>>>>
>>>>>> Can you run the tests on the affected stable kernels? I have feeling
>>>>>> that the problem described might not be present there.
>>>>>
>>>>> Hm, I don't seem to have HW to test that out. Maybe someone else does?
>>>>
>>>> Why are you submitting patches you have no way to test?
>>>
>>> What?  This is stable tree backporting, why are you trying to make a
>>> requirement for something that we have never had before?
>>
>> I don't think random patches should be sent to stable just because
>> they appeared in mainline. Plus, I don't think I'm making new rules:
>>
>> submit-checklist.rst:
>>
>> 13) Has been build- and runtime tested with and without ``CONFIG_SMP``
>> and
>>    ``CONFIG_PREEMPT.``
>>
>> stable-kernel-rules.rst:
>>
>> Rules on what kind of patches are accepted, and which ones are not,
>> into the "-stable" tree:
>>
>> - It must be obviously correct and tested.
>> - It must fix a real bug that bothers people (not a, "This could be a
>>   problem..." type thing).
> 
> So you're saying that this doesn't qualify as a bug?
> 
>>> This is a backport of a patch that is already upstream.  If it doesn't
>>> belong in a stable tree, great, let us know that, saying why it is so.
>>
>> See jacek.anaszewski@gmail.com 's explanation.
> 
> I might be missing something, but Jacek suggested I pull another patch
> before this one?

Just to clarify:

For 4.14 below patches are chosen correctly:

[PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when
setting delay_off=0
[PATCH AUTOSEL for 4.14 094/110] leds: core: Fix regression caused by
commit 2b83ff96f51d

For 4.9 both above patches are needed preceded by:

eb1610b4c273 ("led: core: Fix blink_brightness setting race")

The issue the patch [PATCH AUTOSEL for 4.14 065/110] fixes was
introduced in 4.7, and thus it should be removed from the series
for 3.18 and 4.4.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Pavel Machek]

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

On Tue 2018-02-06 02:02:19, Sasha Levin wrote:
> On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
> >
> >> > > >> *** if brightness=0, led off
> >> > > >> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> >> > > >> ** otherwise set led_set_brightness_nosleep
> >> > > >>
> >> > > >> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
> >> > > >
> >> > > >Can you run the tests on the affected stable kernels? I have feeling
> >> > > >that the problem described might not be present there.
> >> > >
> >> > > Hm, I don't seem to have HW to test that out. Maybe someone else does?
> >> >
> >> > Why are you submitting patches you have no way to test?
> >>
> >> What?  This is stable tree backporting, why are you trying to make a
> >> requirement for something that we have never had before?
> >
> >I don't think random patches should be sent to stable just because
> >they appeared in mainline. Plus, I don't think I'm making new rules:
> >
> >submit-checklist.rst:
> >
> >13) Has been build- and runtime tested with and without ``CONFIG_SMP``
> >and
> >    ``CONFIG_PREEMPT.``
> >
> >stable-kernel-rules.rst:
> >
> >Rules on what kind of patches are accepted, and which ones are not,
> >into the "-stable" tree:
> >
> > - It must be obviously correct and tested.
> > - It must fix a real bug that bothers people (not a, "This could be a
> >   problem..." type thing).
> 
> So you're saying that this doesn't qualify as a bug?

I'm saying that this does not qualitfy as severe enough
bug. stable-kernel-rules.rst describes what bugs are severe enough,
and this is not one of them.

Best regards,
									Pavel	
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 2631 bytes --]

On 02/06/2018 09:44 PM, Jacek Anaszewski wrote:
> On 02/06/2018 03:02 AM, Sasha Levin wrote:
>> On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
>>>
>>>>>>>> *** if brightness=0, led off
>>>>>>>> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
>>>>>>>> ** otherwise set led_set_brightness_nosleep
>>>>>>>>
>>>>>>>> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
>>>>>>>
>>>>>>> Can you run the tests on the affected stable kernels? I have feeling
>>>>>>> that the problem described might not be present there.
>>>>>>
>>>>>> Hm, I don't seem to have HW to test that out. Maybe someone else does?
>>>>>
>>>>> Why are you submitting patches you have no way to test?
>>>>
>>>> What?  This is stable tree backporting, why are you trying to make a
>>>> requirement for something that we have never had before?
>>>
>>> I don't think random patches should be sent to stable just because
>>> they appeared in mainline. Plus, I don't think I'm making new rules:
>>>
>>> submit-checklist.rst:
>>>
>>> 13) Has been build- and runtime tested with and without ``CONFIG_SMP``
>>> and
>>>    ``CONFIG_PREEMPT.``
>>>
>>> stable-kernel-rules.rst:
>>>
>>> Rules on what kind of patches are accepted, and which ones are not,
>>> into the "-stable" tree:
>>>
>>> - It must be obviously correct and tested.
>>> - It must fix a real bug that bothers people (not a, "This could be a
>>>   problem..." type thing).
>>
>> So you're saying that this doesn't qualify as a bug?
>>
>>>> This is a backport of a patch that is already upstream.  If it doesn't
>>>> belong in a stable tree, great, let us know that, saying why it is so.
>>>
>>> See jacek.anaszewski@gmail.com 's explanation.
>>
>> I might be missing something, but Jacek suggested I pull another patch
>> before this one?
> 
> Just to clarify:
> 
> For 4.14 below patches are chosen correctly:
> 
> [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when
> setting delay_off=0
> [PATCH AUTOSEL for 4.14 094/110] leds: core: Fix regression caused by
> commit 2b83ff96f51d
> 
> For 4.9 both above patches are needed preceded by:
> 
> eb1610b4c273 ("led: core: Fix blink_brightness setting race")
> 
> The issue the patch [PATCH AUTOSEL for 4.14 065/110] fixes was
> introduced in 4.7, and thus it should be removed from the series
> for 3.18 and 4.4.
> 

It seems only "led: core: Fix brightness setting when setting delay_off=0"
was applied to 4.9. Could we get the regression fixes backported to 4.9 as
well?

Thanks,
Matthias


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Greg KH]

On Mon, Mar 12, 2018 at 04:00:01PM +0100, Matthias Schiffer wrote:
> On 02/06/2018 09:44 PM, Jacek Anaszewski wrote:
> > On 02/06/2018 03:02 AM, Sasha Levin wrote:
> >> On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
> >>>
> >>>>>>>> *** if brightness=0, led off
> >>>>>>>> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> >>>>>>>> ** otherwise set led_set_brightness_nosleep
> >>>>>>>>
> >>>>>>>> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
> >>>>>>>
> >>>>>>> Can you run the tests on the affected stable kernels? I have feeling
> >>>>>>> that the problem described might not be present there.
> >>>>>>
> >>>>>> Hm, I don't seem to have HW to test that out. Maybe someone else does?
> >>>>>
> >>>>> Why are you submitting patches you have no way to test?
> >>>>
> >>>> What?  This is stable tree backporting, why are you trying to make a
> >>>> requirement for something that we have never had before?
> >>>
> >>> I don't think random patches should be sent to stable just because
> >>> they appeared in mainline. Plus, I don't think I'm making new rules:
> >>>
> >>> submit-checklist.rst:
> >>>
> >>> 13) Has been build- and runtime tested with and without ``CONFIG_SMP``
> >>> and
> >>>    ``CONFIG_PREEMPT.``
> >>>
> >>> stable-kernel-rules.rst:
> >>>
> >>> Rules on what kind of patches are accepted, and which ones are not,
> >>> into the "-stable" tree:
> >>>
> >>> - It must be obviously correct and tested.
> >>> - It must fix a real bug that bothers people (not a, "This could be a
> >>>   problem..." type thing).
> >>
> >> So you're saying that this doesn't qualify as a bug?
> >>
> >>>> This is a backport of a patch that is already upstream.  If it doesn't
> >>>> belong in a stable tree, great, let us know that, saying why it is so.
> >>>
> >>> See jacek.anaszewski@gmail.com 's explanation.
> >>
> >> I might be missing something, but Jacek suggested I pull another patch
> >> before this one?
> > 
> > Just to clarify:
> > 
> > For 4.14 below patches are chosen correctly:
> > 
> > [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when
> > setting delay_off=0
> > [PATCH AUTOSEL for 4.14 094/110] leds: core: Fix regression caused by
> > commit 2b83ff96f51d
> > 
> > For 4.9 both above patches are needed preceded by:
> > 
> > eb1610b4c273 ("led: core: Fix blink_brightness setting race")
> > 
> > The issue the patch [PATCH AUTOSEL for 4.14 065/110] fixes was
> > introduced in 4.7, and thus it should be removed from the series
> > for 3.18 and 4.4.
> > 
> 
> It seems only "led: core: Fix brightness setting when setting delay_off=0"
> was applied to 4.9. Could we get the regression fixes backported to 4.9 as
> well?

What exact fixes were they?  I'll be glad to apply them if I have a git
commit id.

thanks,

greg k-h

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 3552 bytes --]

On 03/12/2018 04:28 PM, Greg KH wrote:
> On Mon, Mar 12, 2018 at 04:00:01PM +0100, Matthias Schiffer wrote:
>> On 02/06/2018 09:44 PM, Jacek Anaszewski wrote:
>>> On 02/06/2018 03:02 AM, Sasha Levin wrote:
>>>> On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
>>>>>
>>>>>>>>>> *** if brightness=0, led off
>>>>>>>>>> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
>>>>>>>>>> ** otherwise set led_set_brightness_nosleep
>>>>>>>>>>
>>>>>>>>>> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
>>>>>>>>>
>>>>>>>>> Can you run the tests on the affected stable kernels? I have feeling
>>>>>>>>> that the problem described might not be present there.
>>>>>>>>
>>>>>>>> Hm, I don't seem to have HW to test that out. Maybe someone else does?
>>>>>>>
>>>>>>> Why are you submitting patches you have no way to test?
>>>>>>
>>>>>> What?  This is stable tree backporting, why are you trying to make a
>>>>>> requirement for something that we have never had before?
>>>>>
>>>>> I don't think random patches should be sent to stable just because
>>>>> they appeared in mainline. Plus, I don't think I'm making new rules:
>>>>>
>>>>> submit-checklist.rst:
>>>>>
>>>>> 13) Has been build- and runtime tested with and without ``CONFIG_SMP``
>>>>> and
>>>>>    ``CONFIG_PREEMPT.``
>>>>>
>>>>> stable-kernel-rules.rst:
>>>>>
>>>>> Rules on what kind of patches are accepted, and which ones are not,
>>>>> into the "-stable" tree:
>>>>>
>>>>> - It must be obviously correct and tested.
>>>>> - It must fix a real bug that bothers people (not a, "This could be a
>>>>>   problem..." type thing).
>>>>
>>>> So you're saying that this doesn't qualify as a bug?
>>>>
>>>>>> This is a backport of a patch that is already upstream.  If it doesn't
>>>>>> belong in a stable tree, great, let us know that, saying why it is so.
>>>>>
>>>>> See jacek.anaszewski@gmail.com 's explanation.
>>>>
>>>> I might be missing something, but Jacek suggested I pull another patch
>>>> before this one?
>>>
>>> Just to clarify:
>>>
>>> For 4.14 below patches are chosen correctly:
>>>
>>> [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when
>>> setting delay_off=0
>>> [PATCH AUTOSEL for 4.14 094/110] leds: core: Fix regression caused by
>>> commit 2b83ff96f51d
>>>
>>> For 4.9 both above patches are needed preceded by:
>>>
>>> eb1610b4c273 ("led: core: Fix blink_brightness setting race")
>>>
>>> The issue the patch [PATCH AUTOSEL for 4.14 065/110] fixes was
>>> introduced in 4.7, and thus it should be removed from the series
>>> for 3.18 and 4.4.
>>>
>>
>> It seems only "led: core: Fix brightness setting when setting delay_off=0"
>> was applied to 4.9. Could we get the regression fixes backported to 4.9 as
>> well?
> 
> What exact fixes were they?  I'll be glad to apply them if I have a git
> commit id.
> 
> thanks,
> 
> greg k-h
> 

At least 7b6af2c531 ("leds: core: Fix regression caused by commit
2b83ff96f51d") is missing, causing visible regressions (LEDs not working at
all) on some OpenWrt devices. This was fixed in 4.4.121 by reverting the
offending commit, but if I followed the discussion correctly, 4.9 should
get the follow-up commit 7b6af2c531 instead (like 4.14 already did).

Jacek's mail I replied to mentions that eb1610b4c273 ("led: core: Fix
blink_brightness setting race") should be included in 4.9 as well, but I
don't know the impact of the issue it fixes.

Matthias


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Jacek Anaszewski]

On 03/12/2018 04:45 PM, Matthias Schiffer wrote:
> On 03/12/2018 04:28 PM, Greg KH wrote:
>> On Mon, Mar 12, 2018 at 04:00:01PM +0100, Matthias Schiffer wrote:
>>> On 02/06/2018 09:44 PM, Jacek Anaszewski wrote:
>>>> On 02/06/2018 03:02 AM, Sasha Levin wrote:
>>>>> On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
>>>>>>
>>>>>>>>>>> *** if brightness=0, led off
>>>>>>>>>>> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
>>>>>>>>>>> ** otherwise set led_set_brightness_nosleep
>>>>>>>>>>>
>>>>>>>>>>> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
>>>>>>>>>>
>>>>>>>>>> Can you run the tests on the affected stable kernels? I have feeling
>>>>>>>>>> that the problem described might not be present there.
>>>>>>>>>
>>>>>>>>> Hm, I don't seem to have HW to test that out. Maybe someone else does?
>>>>>>>>
>>>>>>>> Why are you submitting patches you have no way to test?
>>>>>>>
>>>>>>> What?  This is stable tree backporting, why are you trying to make a
>>>>>>> requirement for something that we have never had before?
>>>>>>
>>>>>> I don't think random patches should be sent to stable just because
>>>>>> they appeared in mainline. Plus, I don't think I'm making new rules:
>>>>>>
>>>>>> submit-checklist.rst:
>>>>>>
>>>>>> 13) Has been build- and runtime tested with and without ``CONFIG_SMP``
>>>>>> and
>>>>>>    ``CONFIG_PREEMPT.``
>>>>>>
>>>>>> stable-kernel-rules.rst:
>>>>>>
>>>>>> Rules on what kind of patches are accepted, and which ones are not,
>>>>>> into the "-stable" tree:
>>>>>>
>>>>>> - It must be obviously correct and tested.
>>>>>> - It must fix a real bug that bothers people (not a, "This could be a
>>>>>>   problem..." type thing).
>>>>>
>>>>> So you're saying that this doesn't qualify as a bug?
>>>>>
>>>>>>> This is a backport of a patch that is already upstream.  If it doesn't
>>>>>>> belong in a stable tree, great, let us know that, saying why it is so.
>>>>>>
>>>>>> See jacek.anaszewski@gmail.com 's explanation.
>>>>>
>>>>> I might be missing something, but Jacek suggested I pull another patch
>>>>> before this one?
>>>>
>>>> Just to clarify:
>>>>
>>>> For 4.14 below patches are chosen correctly:
>>>>
>>>> [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when
>>>> setting delay_off=0
>>>> [PATCH AUTOSEL for 4.14 094/110] leds: core: Fix regression caused by
>>>> commit 2b83ff96f51d
>>>>
>>>> For 4.9 both above patches are needed preceded by:
>>>>
>>>> eb1610b4c273 ("led: core: Fix blink_brightness setting race")
>>>>
>>>> The issue the patch [PATCH AUTOSEL for 4.14 065/110] fixes was
>>>> introduced in 4.7, and thus it should be removed from the series
>>>> for 3.18 and 4.4.
>>>>
>>>
>>> It seems only "led: core: Fix brightness setting when setting delay_off=0"
>>> was applied to 4.9. Could we get the regression fixes backported to 4.9 as
>>> well?
>>
>> What exact fixes were they?  I'll be glad to apply them if I have a git
>> commit id.
>>
>> thanks,
>>
>> greg k-h
>>
> 
> At least 7b6af2c531 ("leds: core: Fix regression caused by commit
> 2b83ff96f51d") is missing, causing visible regressions (LEDs not working at
> all) on some OpenWrt devices. This was fixed in 4.4.121 by reverting the
> offending commit, but if I followed the discussion correctly, 4.9 should
> get the follow-up commit 7b6af2c531 instead (like 4.14 already did).
> 
> Jacek's mail I replied to mentions that eb1610b4c273 ("led: core: Fix
> blink_brightness setting race") should be included in 4.9 as well, but I
> don't know the impact of the issue it fixes.

It doesn't fix any reported issue, but is just an improvement
aiming at preventing potential races while changing blink brightness.

After taking closer look it turns out that for the patches in question
to apply cleanly we need in 4.9 also a patch which introduces atomic
bit fields for blink flags.

Effectively, here is the list of patches required in 4.9 stable:

Revert "led: core: Fix brightness setting when setting delay_off=0"

followed by:

a9c6ce57ec ("led: core: Use atomic bit-field for the blink-flags")
eb1610b4c2 ("led: core: Fix blink_brightness setting race")
2b83ff96f5 ("led: core: Fix brightness setting when setting delay_off=0")
7b6af2c531 ("leds: core: Fix regression caused by commit 2b83ff96f51d")

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Pavel Machek]

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

Hi!

> > At least 7b6af2c531 ("leds: core: Fix regression caused by commit
> > 2b83ff96f51d") is missing, causing visible regressions (LEDs not working at
> > all) on some OpenWrt devices. This was fixed in 4.4.121 by reverting the
> > offending commit, but if I followed the discussion correctly, 4.9 should
> > get the follow-up commit 7b6af2c531 instead (like 4.14 already did).
> > 
> > Jacek's mail I replied to mentions that eb1610b4c273 ("led: core: Fix
> > blink_brightness setting race") should be included in 4.9 as well, but I
> > don't know the impact of the issue it fixes.
> 
> It doesn't fix any reported issue, but is just an improvement
> aiming at preventing potential races while changing blink brightness.
> 
> After taking closer look it turns out that for the patches in question
> to apply cleanly we need in 4.9 also a patch which introduces atomic
> bit fields for blink flags.
> 
> Effectively, here is the list of patches required in 4.9 stable:
> 
> Revert "led: core: Fix brightness setting when setting delay_off=0"
> 
> followed by:
> 
> a9c6ce57ec ("led: core: Use atomic bit-field for the blink-flags")
> eb1610b4c2 ("led: core: Fix blink_brightness setting race")
> 2b83ff96f5 ("led: core: Fix brightness setting when setting delay_off=0")
> 7b6af2c531 ("leds: core: Fix regression caused by commit 2b83ff96f51d")

For the record... there's no serious problem in LED subsystem in
v4.9. Just leave LED subsystem alone in v4.9-stable (reverting any
patches that were backported by some kind of bot that did not even
test them), and things will be fine.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Greg KH]

On Mon, Mar 12, 2018 at 09:20:48PM +0100, Jacek Anaszewski wrote:
> On 03/12/2018 04:45 PM, Matthias Schiffer wrote:
> > On 03/12/2018 04:28 PM, Greg KH wrote:
> >> On Mon, Mar 12, 2018 at 04:00:01PM +0100, Matthias Schiffer wrote:
> >>> On 02/06/2018 09:44 PM, Jacek Anaszewski wrote:
> >>>> On 02/06/2018 03:02 AM, Sasha Levin wrote:
> >>>>> On Sun, Feb 04, 2018 at 06:17:36PM +0100, Pavel Machek wrote:
> >>>>>>
> >>>>>>>>>>> *** if brightness=0, led off
> >>>>>>>>>>> *** else apply brightness if next timer <--- timer is stop, and will never apply new setting
> >>>>>>>>>>> ** otherwise set led_set_brightness_nosleep
> >>>>>>>>>>>
> >>>>>>>>>>> To fix that, when we delete the timer, we should clear LED_BLINK_SW.
> >>>>>>>>>>
> >>>>>>>>>> Can you run the tests on the affected stable kernels? I have feeling
> >>>>>>>>>> that the problem described might not be present there.
> >>>>>>>>>
> >>>>>>>>> Hm, I don't seem to have HW to test that out. Maybe someone else does?
> >>>>>>>>
> >>>>>>>> Why are you submitting patches you have no way to test?
> >>>>>>>
> >>>>>>> What?  This is stable tree backporting, why are you trying to make a
> >>>>>>> requirement for something that we have never had before?
> >>>>>>
> >>>>>> I don't think random patches should be sent to stable just because
> >>>>>> they appeared in mainline. Plus, I don't think I'm making new rules:
> >>>>>>
> >>>>>> submit-checklist.rst:
> >>>>>>
> >>>>>> 13) Has been build- and runtime tested with and without ``CONFIG_SMP``
> >>>>>> and
> >>>>>>    ``CONFIG_PREEMPT.``
> >>>>>>
> >>>>>> stable-kernel-rules.rst:
> >>>>>>
> >>>>>> Rules on what kind of patches are accepted, and which ones are not,
> >>>>>> into the "-stable" tree:
> >>>>>>
> >>>>>> - It must be obviously correct and tested.
> >>>>>> - It must fix a real bug that bothers people (not a, "This could be a
> >>>>>>   problem..." type thing).
> >>>>>
> >>>>> So you're saying that this doesn't qualify as a bug?
> >>>>>
> >>>>>>> This is a backport of a patch that is already upstream.  If it doesn't
> >>>>>>> belong in a stable tree, great, let us know that, saying why it is so.
> >>>>>>
> >>>>>> See jacek.anaszewski@gmail.com 's explanation.
> >>>>>
> >>>>> I might be missing something, but Jacek suggested I pull another patch
> >>>>> before this one?
> >>>>
> >>>> Just to clarify:
> >>>>
> >>>> For 4.14 below patches are chosen correctly:
> >>>>
> >>>> [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when
> >>>> setting delay_off=0
> >>>> [PATCH AUTOSEL for 4.14 094/110] leds: core: Fix regression caused by
> >>>> commit 2b83ff96f51d
> >>>>
> >>>> For 4.9 both above patches are needed preceded by:
> >>>>
> >>>> eb1610b4c273 ("led: core: Fix blink_brightness setting race")
> >>>>
> >>>> The issue the patch [PATCH AUTOSEL for 4.14 065/110] fixes was
> >>>> introduced in 4.7, and thus it should be removed from the series
> >>>> for 3.18 and 4.4.
> >>>>
> >>>
> >>> It seems only "led: core: Fix brightness setting when setting delay_off=0"
> >>> was applied to 4.9. Could we get the regression fixes backported to 4.9 as
> >>> well?
> >>
> >> What exact fixes were they?  I'll be glad to apply them if I have a git
> >> commit id.
> >>
> >> thanks,
> >>
> >> greg k-h
> >>
> > 
> > At least 7b6af2c531 ("leds: core: Fix regression caused by commit
> > 2b83ff96f51d") is missing, causing visible regressions (LEDs not working at
> > all) on some OpenWrt devices. This was fixed in 4.4.121 by reverting the
> > offending commit, but if I followed the discussion correctly, 4.9 should
> > get the follow-up commit 7b6af2c531 instead (like 4.14 already did).
> > 
> > Jacek's mail I replied to mentions that eb1610b4c273 ("led: core: Fix
> > blink_brightness setting race") should be included in 4.9 as well, but I
> > don't know the impact of the issue it fixes.
> 
> It doesn't fix any reported issue, but is just an improvement
> aiming at preventing potential races while changing blink brightness.
> 
> After taking closer look it turns out that for the patches in question
> to apply cleanly we need in 4.9 also a patch which introduces atomic
> bit fields for blink flags.
> 
> Effectively, here is the list of patches required in 4.9 stable:
> 
> Revert "led: core: Fix brightness setting when setting delay_off=0"
> 
> followed by:
> 
> a9c6ce57ec ("led: core: Use atomic bit-field for the blink-flags")
> eb1610b4c2 ("led: core: Fix blink_brightness setting race")
> 2b83ff96f5 ("led: core: Fix brightness setting when setting delay_off=0")
> 7b6af2c531 ("leds: core: Fix regression caused by commit 2b83ff96f51d")

Odd, I just got another report that the 4.9.87 release fixed some
reported LED issues, so why do I need all of these?

Should I just revert the single 2b83ff96f51d commit here instead?

Totally confused...

greg k-h

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Pavel Machek]

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

Hi!

> > > At least 7b6af2c531 ("leds: core: Fix regression caused by commit
> > > 2b83ff96f51d") is missing, causing visible regressions (LEDs not working at
> > > all) on some OpenWrt devices. This was fixed in 4.4.121 by reverting the
> > > offending commit, but if I followed the discussion correctly, 4.9 should
> > > get the follow-up commit 7b6af2c531 instead (like 4.14 already did).
> > > 
> > > Jacek's mail I replied to mentions that eb1610b4c273 ("led: core: Fix
> > > blink_brightness setting race") should be included in 4.9 as well, but I
> > > don't know the impact of the issue it fixes.
> > 
> > It doesn't fix any reported issue, but is just an improvement
> > aiming at preventing potential races while changing blink brightness.
> > 
> > After taking closer look it turns out that for the patches in question
> > to apply cleanly we need in 4.9 also a patch which introduces atomic
> > bit fields for blink flags.
> > 
> > Effectively, here is the list of patches required in 4.9 stable:
> > 
> > Revert "led: core: Fix brightness setting when setting delay_off=0"
> > 
> > followed by:
> > 
> > a9c6ce57ec ("led: core: Use atomic bit-field for the blink-flags")
> > eb1610b4c2 ("led: core: Fix blink_brightness setting race")
> > 2b83ff96f5 ("led: core: Fix brightness setting when setting delay_off=0")
> > 7b6af2c531 ("leds: core: Fix regression caused by commit 2b83ff96f51d")
> 
> Odd, I just got another report that the 4.9.87 release fixed some
> reported LED issues, so why do I need all of these?
> 
> Should I just revert the single 2b83ff96f51d commit here instead?

I believe so, yes.

I'm not aware of any _really bad_ issues with LED subsystem in
4.9. Take a look at changelog of
2b83ff96f51d0b039c4561b9f95c824d7bddb85c -- it fixes rather
theoretical issue; user can reproduce it by hand in shell, but,
well... don't do it then.

The rest of fixes ... fix some more theoretical races. I don't think
it is -stable material, as I pointed out before.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Jacek Anaszewski]

On 03/13/2018 02:27 PM, Pavel Machek wrote:
> Hi!
> 
>>>> At least 7b6af2c531 ("leds: core: Fix regression caused by commit
>>>> 2b83ff96f51d") is missing, causing visible regressions (LEDs not working at
>>>> all) on some OpenWrt devices. This was fixed in 4.4.121 by reverting the
>>>> offending commit, but if I followed the discussion correctly, 4.9 should
>>>> get the follow-up commit 7b6af2c531 instead (like 4.14 already did).
>>>>
>>>> Jacek's mail I replied to mentions that eb1610b4c273 ("led: core: Fix
>>>> blink_brightness setting race") should be included in 4.9 as well, but I
>>>> don't know the impact of the issue it fixes.
>>>
>>> It doesn't fix any reported issue, but is just an improvement
>>> aiming at preventing potential races while changing blink brightness.
>>>
>>> After taking closer look it turns out that for the patches in question
>>> to apply cleanly we need in 4.9 also a patch which introduces atomic
>>> bit fields for blink flags.
>>>
>>> Effectively, here is the list of patches required in 4.9 stable:
>>>
>>> Revert "led: core: Fix brightness setting when setting delay_off=0"
>>>
>>> followed by:
>>>
>>> a9c6ce57ec ("led: core: Use atomic bit-field for the blink-flags")
>>> eb1610b4c2 ("led: core: Fix blink_brightness setting race")
>>> 2b83ff96f5 ("led: core: Fix brightness setting when setting delay_off=0")
>>> 7b6af2c531 ("leds: core: Fix regression caused by commit 2b83ff96f51d")
>>
>> Odd, I just got another report that the 4.9.87 release fixed some
>> reported LED issues, so why do I need all of these?

Because 2b83ff96f5 introduces another bug, fixed in 7b6af2c531.
7b6af2c531 in turn uses atomic blink flags introduced in a9c6ce57ec.

eb1610b4c2 fixes theoretical races, actually we can do without it
in stable.

In order to avoid applying patch a9c6ce57ec, we could come up with the
below change which does exactly what 7b6af2c531 intended, but without
atomic blink flags, which are irrelevant for this bug.


diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 3bce448..454ed4d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -188,6 +188,7 @@ void led_blink_set(struct led_classdev *led_cdev,
 {
        del_timer_sync(&led_cdev->blink_timer);

+       led_cdev->flags &= ~LED_BLINK_SW;
        led_cdev->flags &= ~LED_BLINK_ONESHOT;
        led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;


I can submit it to stable if it is preferred.

In every case tha patch 2b83ff96f5 needs to be reverted beforehand,
since otherwise none of the discussed patches will apply cleanly
(besides the aforementioned reasoning it has a truncated commit
message).


>> Should I just revert the single 2b83ff96f51d commit here instead?
> 
> I believe so, yes.
> 
> I'm not aware of any _really bad_ issues with LED subsystem in
> 4.9. Take a look at changelog of
> 2b83ff96f51d0b039c4561b9f95c824d7bddb85c -- it fixes rather
> theoretical issue; user can reproduce it by hand in shell, but,
> well... don't do it then.

Greg mentioned that 4.9.87 release fixed some LED issue for someone,
and it was the only LED related patch in that release.

> The rest of fixes ... fix some more theoretical races. I don't think
> it is -stable material, as I pointed out before.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH AUTOSEL for 4.14 065/110] led: core: Fix brightness setting when setting delay_off=0

[Greg KH]

On Tue, Mar 13, 2018 at 08:44:49PM +0100, Jacek Anaszewski wrote:
> On 03/13/2018 02:27 PM, Pavel Machek wrote:
> > Hi!
> > 
> >>>> At least 7b6af2c531 ("leds: core: Fix regression caused by commit
> >>>> 2b83ff96f51d") is missing, causing visible regressions (LEDs not working at
> >>>> all) on some OpenWrt devices. This was fixed in 4.4.121 by reverting the
> >>>> offending commit, but if I followed the discussion correctly, 4.9 should
> >>>> get the follow-up commit 7b6af2c531 instead (like 4.14 already did).
> >>>>
> >>>> Jacek's mail I replied to mentions that eb1610b4c273 ("led: core: Fix
> >>>> blink_brightness setting race") should be included in 4.9 as well, but I
> >>>> don't know the impact of the issue it fixes.
> >>>
> >>> It doesn't fix any reported issue, but is just an improvement
> >>> aiming at preventing potential races while changing blink brightness.
> >>>
> >>> After taking closer look it turns out that for the patches in question
> >>> to apply cleanly we need in 4.9 also a patch which introduces atomic
> >>> bit fields for blink flags.
> >>>
> >>> Effectively, here is the list of patches required in 4.9 stable:
> >>>
> >>> Revert "led: core: Fix brightness setting when setting delay_off=0"
> >>>
> >>> followed by:
> >>>
> >>> a9c6ce57ec ("led: core: Use atomic bit-field for the blink-flags")
> >>> eb1610b4c2 ("led: core: Fix blink_brightness setting race")
> >>> 2b83ff96f5 ("led: core: Fix brightness setting when setting delay_off=0")
> >>> 7b6af2c531 ("leds: core: Fix regression caused by commit 2b83ff96f51d")
> >>
> >> Odd, I just got another report that the 4.9.87 release fixed some
> >> reported LED issues, so why do I need all of these?
> 
> Because 2b83ff96f5 introduces another bug, fixed in 7b6af2c531.
> 7b6af2c531 in turn uses atomic blink flags introduced in a9c6ce57ec.
> 
> eb1610b4c2 fixes theoretical races, actually we can do without it
> in stable.
> 
> In order to avoid applying patch a9c6ce57ec, we could come up with the
> below change which does exactly what 7b6af2c531 intended, but without
> atomic blink flags, which are irrelevant for this bug.
> 
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 3bce448..454ed4d 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -188,6 +188,7 @@ void led_blink_set(struct led_classdev *led_cdev,
>  {
>         del_timer_sync(&led_cdev->blink_timer);
> 
> +       led_cdev->flags &= ~LED_BLINK_SW;
>         led_cdev->flags &= ~LED_BLINK_ONESHOT;
>         led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> 
> 
> I can submit it to stable if it is preferred.
> 
> In every case tha patch 2b83ff96f5 needs to be reverted beforehand,
> since otherwise none of the discussed patches will apply cleanly
> (besides the aforementioned reasoning it has a truncated commit
> message).

Yes, please submit it to stable, along with a very simple "please
add/revert these patches" so I know what in the world to do as this
thread is really confusing at the moment :)

thanks,

greg k-h