Hi, Thorsten here, the Linux kernel's regression tracker.
Uwe, I noticed a report about a regression in bugzilla.kernel.org that
apparently is caused by a change of yours. As many (most?) kernel
developers don't keep an eye on it, I decided to forward it by mail.
Note, you have to use bugzilla to reach the reporter, as I sadly[1] can
not CCed them in mails like this.
Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=218559 :
> Commit 76fe464c8e64e71b2e4af11edeef0e5d85eeb6aa ("leds: pwm: Don't
> disable the PWM when the LED should be off") prevents stm32mp1xx based
> targets from entering suspend if pwm-leds exist, as the stm32 PWM driver
> refuses to enter suspend if any PWM channels are still active ("PWM 0
> still in use by consumer" see stm32_pwm_suspend in drivers/pwm/stm32-pwm.c).
>
> Reverting the mentioned commit fixes this behaviour but I'm not
> certain if this is a problem with stm32-pwm or pwm-leds (what is the
> usual behaviour for suspend with active PWM channels?).
See the ticket for more details.
[TLDR for the rest of this mail: I'm adding this report to the list of
tracked Linux kernel regressions; the text you find below is based on a
few templates paragraphs you might have encountered already in similar
form.]
BTW, let me use this mail to also add the report to the list of tracked
regressions to ensure it's doesn't fall through the cracks:
#regzbot introduced: 76fe464c8e64e71b
#regzbot duplicate https://bugzilla.kernel.org/show_bug.cgi?id=218559
#regzbot title: stm32mp1xx based targets stopped entering suspend if
pwm-leds exist
#regzbot ignore-activity
This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.
Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (e.g. the buzgzilla ticket and maybe this mail as well, if
this thread sees some discussion). See page linked in footer for details.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
[1] because bugzilla.kernel.org tells users upon registration their
"email address will never be displayed to logged out users"
Hello,
On Wed, Mar 06, 2024 at 08:05:15AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> Hi, Thorsten here, the Linux kernel's regression tracker.
>
> Uwe, I noticed a report about a regression in bugzilla.kernel.org that
> apparently is caused by a change of yours. As many (most?) kernel
> developers don't keep an eye on it, I decided to forward it by mail.
>
> Note, you have to use bugzilla to reach the reporter, as I sadly[1] can
> not CCed them in mails like this.
>
> Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=218559 :
>
> > Commit 76fe464c8e64e71b2e4af11edeef0e5d85eeb6aa ("leds: pwm: Don't
> > disable the PWM when the LED should be off") prevents stm32mp1xx based
> > targets from entering suspend if pwm-leds exist, as the stm32 PWM driver
> > refuses to enter suspend if any PWM channels are still active ("PWM 0
> > still in use by consumer" see stm32_pwm_suspend in drivers/pwm/stm32-pwm.c).
> >
> > Reverting the mentioned commit fixes this behaviour but I'm not
> > certain if this is a problem with stm32-pwm or pwm-leds (what is the
> > usual behaviour for suspend with active PWM channels?).
I'd assume the following patch fixes this report. I didn't test it
though.
Best regards
Uwe
---->8----
From: Uwe Kleine-K?nig <[email protected]>
Subject: [PATCH] leds: pwm: Disable PWM when going to suspend
On stm32mp1xx based machines (and others) a PWM consumer has to disable
the PWM because an enabled PWM refuses to suspend. So check the
LED_SUSPENDED flag and depending on that set the .enabled property.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
Signed-off-by: Uwe Kleine-K?nig <[email protected]>
---
drivers/leds/leds-pwm.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 4e3936a39d0e..e1b414b40353 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -53,7 +53,13 @@ static int led_pwm_set(struct led_classdev *led_cdev,
duty = led_dat->pwmstate.period - duty;
led_dat->pwmstate.duty_cycle = duty;
- led_dat->pwmstate.enabled = true;
+ /*
+ * Disabling a PWM doesn't guarantee that it emits the inactive level.
+ * So keep it on. Only for suspending the PWM should be disabled because
+ * otherwise it refuses to suspend. The possible downside is that the
+ * LED might stay (or even go) on.
+ */
+ led_dat->pwmstate.enabled = !(led_cdev->flags & LED_SUSPENDED);
return pwm_apply_might_sleep(led_dat->pwm, &led_dat->pwmstate);
}
base-commit: 15facbd7bd3dbfa04721cb71e69954eb4686cb9e
---->8----
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On 06.03.24 09:18, Uwe Kleine-König wrote:
> On Wed, Mar 06, 2024 at 08:05:15AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>>
>> Uwe, I noticed a report about a regression in bugzilla.kernel.org that
>> apparently is caused by a change of yours. As many (most?) kernel
>> developers don't keep an eye on it, I decided to forward it by mail.
>>
>> Note, you have to use bugzilla to reach the reporter, as I sadly[1] can
>> not CCed them in mails like this.
>>
>> Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=218559 :
>>
>>> Commit 76fe464c8e64e71b2e4af11edeef0e5d85eeb6aa ("leds: pwm: Don't
>>> disable the PWM when the LED should be off") prevents stm32mp1xx based
>>> targets from entering suspend if pwm-leds exist, as the stm32 PWM driver
>>> refuses to enter suspend if any PWM channels are still active ("PWM 0
>>> still in use by consumer" see stm32_pwm_suspend in drivers/pwm/stm32-pwm.c).
>>>
>>> Reverting the mentioned commit fixes this behaviour but I'm not
>>> certain if this is a problem with stm32-pwm or pwm-leds (what is the
>>> usual behaviour for suspend with active PWM channels?).
>
> I'd assume the following patch fixes this report. I didn't test it
> though.
Jakob confirmed it helped in the bugzilla ticket. But the patch since
then didn't make any progress afaics -- or did it and I just missed it
in my search?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot poke
> ---->8----
> From: Uwe Kleine-König <[email protected]>
> Subject: [PATCH] leds: pwm: Disable PWM when going to suspend
>
> On stm32mp1xx based machines (and others) a PWM consumer has to disable
> the PWM because an enabled PWM refuses to suspend. So check the
> LED_SUSPENDED flag and depending on that set the .enabled property.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
> Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> drivers/leds/leds-pwm.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 4e3936a39d0e..e1b414b40353 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -53,7 +53,13 @@ static int led_pwm_set(struct led_classdev *led_cdev,
> duty = led_dat->pwmstate.period - duty;
>
> led_dat->pwmstate.duty_cycle = duty;
> - led_dat->pwmstate.enabled = true;
> + /*
> + * Disabling a PWM doesn't guarantee that it emits the inactive level.
> + * So keep it on. Only for suspending the PWM should be disabled because
> + * otherwise it refuses to suspend. The possible downside is that the
> + * LED might stay (or even go) on.
> + */
> + led_dat->pwmstate.enabled = !(led_cdev->flags & LED_SUSPENDED);
> return pwm_apply_might_sleep(led_dat->pwm, &led_dat->pwmstate);
> }
>
> base-commit: 15facbd7bd3dbfa04721cb71e69954eb4686cb9e
> ---->8----
>
On Tue, 16 Apr 2024, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 06.03.24 09:18, Uwe Kleine-König wrote:
> > On Wed, Mar 06, 2024 at 08:05:15AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> >>
> >> Uwe, I noticed a report about a regression in bugzilla.kernel.org that
> >> apparently is caused by a change of yours. As many (most?) kernel
> >> developers don't keep an eye on it, I decided to forward it by mail.
> >>
> >> Note, you have to use bugzilla to reach the reporter, as I sadly[1] can
> >> not CCed them in mails like this.
> >>
> >> Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=218559 :
> >>
> >>> Commit 76fe464c8e64e71b2e4af11edeef0e5d85eeb6aa ("leds: pwm: Don't
> >>> disable the PWM when the LED should be off") prevents stm32mp1xx based
> >>> targets from entering suspend if pwm-leds exist, as the stm32 PWM driver
> >>> refuses to enter suspend if any PWM channels are still active ("PWM 0
> >>> still in use by consumer" see stm32_pwm_suspend in drivers/pwm/stm32-pwm.c).
> >>>
> >>> Reverting the mentioned commit fixes this behaviour but I'm not
> >>> certain if this is a problem with stm32-pwm or pwm-leds (what is the
> >>> usual behaviour for suspend with active PWM channels?).
> >
> > I'd assume the following patch fixes this report. I didn't test it
> > though.
>
> Jakob confirmed it helped in the bugzilla ticket. But the patch since
> then didn't make any progress afaics -- or did it and I just missed it
> in my search?
[...]
> > ---->8----
> > From: Uwe Kleine-König <[email protected]>
> > Subject: [PATCH] leds: pwm: Disable PWM when going to suspend
> >
> > On stm32mp1xx based machines (and others) a PWM consumer has to disable
> > the PWM because an enabled PWM refuses to suspend. So check the
> > LED_SUSPENDED flag and depending on that set the .enabled property.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
> > Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
> > Signed-off-by: Uwe Kleine-König <[email protected]>
> > ---
> > drivers/leds/leds-pwm.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> > index 4e3936a39d0e..e1b414b40353 100644
> > --- a/drivers/leds/leds-pwm.c
> > +++ b/drivers/leds/leds-pwm.c
> > @@ -53,7 +53,13 @@ static int led_pwm_set(struct led_classdev *led_cdev,
> > duty = led_dat->pwmstate.period - duty;
> >
> > led_dat->pwmstate.duty_cycle = duty;
> > - led_dat->pwmstate.enabled = true;
> > + /*
> > + * Disabling a PWM doesn't guarantee that it emits the inactive level.
> > + * So keep it on. Only for suspending the PWM should be disabled because
> > + * otherwise it refuses to suspend. The possible downside is that the
> > + * LED might stay (or even go) on.
> > + */
> > + led_dat->pwmstate.enabled = !(led_cdev->flags & LED_SUSPENDED);
> > return pwm_apply_might_sleep(led_dat->pwm, &led_dat->pwmstate);
> > }
> >
> > base-commit: 15facbd7bd3dbfa04721cb71e69954eb4686cb9e
> > ---->8----
Did you submit this? I don't see it in LORE or in my inbox.
--
Lee Jones [李琼斯]
Hey Lee,
On Tue, Apr 16, 2024 at 01:15:19PM +0100, Lee Jones wrote:
> On Tue, 16 Apr 2024, Linux regression tracking (Thorsten Leemhuis) wrote:
>
> > On 06.03.24 09:18, Uwe Kleine-K?nig wrote:
> > > On Wed, Mar 06, 2024 at 08:05:15AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> > > > [...]
> >
> > Jakob confirmed it helped in the bugzilla ticket. But the patch since
> > then didn't make any progress afaics -- or did it and I just missed it
> > in my search?
>
> [...]
>
> > > ---->8----
> > > From: Uwe Kleine-K?nig <[email protected]>
> > > Subject: [PATCH] leds: pwm: Disable PWM when going to suspend
> > >
> > > On stm32mp1xx based machines (and others) a PWM consumer has to disable
> > > the PWM because an enabled PWM refuses to suspend. So check the
> > > LED_SUSPENDED flag and depending on that set the .enabled property.
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
> > > Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
> > > Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> > > ---
> > > [...]
> > > ---->8----
>
> Did you submit this? I don't see it in LORE or in my inbox.
Yeah sure, apply it using:
curl -s https://lore.kernel.org/all/2vbwacjy25z5vekylle3ehwi3be4urm6bssrbg6bxobtdlekt4@mazicwtgf4qb/raw | git am --scissors -s
:-)
If you don't consider that suitable, I can create a patch that is easier
to pick up.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, 16 Apr 2024, Uwe Kleine-König wrote:
> Hey Lee,
>
> On Tue, Apr 16, 2024 at 01:15:19PM +0100, Lee Jones wrote:
> > On Tue, 16 Apr 2024, Linux regression tracking (Thorsten Leemhuis) wrote:
> >
> > > On 06.03.24 09:18, Uwe Kleine-König wrote:
> > > > On Wed, Mar 06, 2024 at 08:05:15AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> > > > > [...]
> > >
> > > Jakob confirmed it helped in the bugzilla ticket. But the patch since
> > > then didn't make any progress afaics -- or did it and I just missed it
> > > in my search?
> >
> > [...]
> >
> > > > ---->8----
> > > > From: Uwe Kleine-König <[email protected]>
> > > > Subject: [PATCH] leds: pwm: Disable PWM when going to suspend
> > > >
> > > > On stm32mp1xx based machines (and others) a PWM consumer has to disable
> > > > the PWM because an enabled PWM refuses to suspend. So check the
> > > > LED_SUSPENDED flag and depending on that set the .enabled property.
> > > >
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
> > > > Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
> > > > Signed-off-by: Uwe Kleine-König <[email protected]>
> > > > ---
> > > > [...]
> > > > ---->8----
> >
> > Did you submit this? I don't see it in LORE or in my inbox.
>
> Yeah sure, apply it using:
>
> curl -s https://lore.kernel.org/all/2vbwacjy25z5vekylle3ehwi3be4urm6bssrbg6bxobtdlekt4@mazicwtgf4qb/raw | git am --scissors -s
Nice try!
> :-)
>
> If you don't consider that suitable, I can create a patch that is easier
> to pick up.
Yes, please submit it properly.
--
Lee Jones [李琼斯]
On stm32mp1xx based machines (and others) a PWM consumer has to disable
the PWM because an enabled PWM refuses to suspend. So check the
LED_SUSPENDED flag and depending on that set the .enabled property.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
Signed-off-by: Uwe Kleine-König <[email protected]>
---
Hello,
On Wed, Apr 17, 2024 at 03:49:43PM +0100, Lee Jones wrote:
> On Tue, 16 Apr 2024, Uwe Kleine-König wrote:
> > If you don't consider that suitable, I can create a patch that is easier
> > to pick up.
>
> Yes, please submit it properly.
Here it comes.
Best regards
Uwe
drivers/leds/leds-pwm.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 4e3936a39d0e..e1b414b40353 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -53,7 +53,13 @@ static int led_pwm_set(struct led_classdev *led_cdev,
duty = led_dat->pwmstate.period - duty;
led_dat->pwmstate.duty_cycle = duty;
- led_dat->pwmstate.enabled = true;
+ /*
+ * Disabling a PWM doesn't guarantee that it emits the inactive level.
+ * So keep it on. Only for suspending the PWM should be disabled because
+ * otherwise it refuses to suspend. The possible downside is that the
+ * LED might stay (or even go) on.
+ */
+ led_dat->pwmstate.enabled = !(led_cdev->flags & LED_SUSPENDED);
return pwm_apply_might_sleep(led_dat->pwm, &led_dat->pwmstate);
}
base-commit: 4eab358930711bbeb85bf5ee267d0d42d3394c2c
--
2.43.0
Hello Lee,
On Wed, Apr 17, 2024 at 05:38:47PM +0200, Uwe Kleine-K?nig wrote:
> On stm32mp1xx based machines (and others) a PWM consumer has to disable
> the PWM because an enabled PWM refuses to suspend. So check the
> LED_SUSPENDED flag and depending on that set the .enabled property.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
> Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> ---
> Hello,
>
> On Wed, Apr 17, 2024 at 03:49:43PM +0100, Lee Jones wrote:
> > On Tue, 16 Apr 2024, Uwe Kleine-K?nig wrote:
> > > If you don't consider that suitable, I can create a patch that is easier
> > > to pick up.
> >
> > Yes, please submit it properly.
Gentle ping. Even given the regression was introduced in v6.7-rc1
already, I think this should go into v6.9. If you don't agree that's
fine, but then getting it onto next to queue it for v6.10-rc1 at least
would be great.
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On 26.04.24 08:17, Uwe Kleine-König wrote:
> On Wed, Apr 17, 2024 at 05:38:47PM +0200, Uwe Kleine-König wrote:
>> On stm32mp1xx based machines (and others) a PWM consumer has to disable
>> the PWM because an enabled PWM refuses to suspend. So check the
>> LED_SUSPENDED flag and depending on that set the .enabled property.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218559
>> Fixes: 76fe464c8e64 ("leds: pwm: Don't disable the PWM when the LED should be off")
>> Signed-off-by: Uwe Kleine-König <[email protected]>
>> ---
>> Hello,
>>
>> On Wed, Apr 17, 2024 at 03:49:43PM +0100, Lee Jones wrote:
>>> On Tue, 16 Apr 2024, Uwe Kleine-König wrote:
>>>> If you don't consider that suitable, I can create a patch that is easier
>>>> to pick up.
>>>
>>> Yes, please submit it properly.
>
> Gentle ping. Even given the regression was introduced in v6.7-rc1
> already, I think this should go into v6.9. [...]
FWIW, I guess Linus would agree (even if it's the second to last release
in this case):
https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
On Wed, 17 Apr 2024 17:38:47 +0200, Uwe Kleine-König wrote:
> On stm32mp1xx based machines (and others) a PWM consumer has to disable
> the PWM because an enabled PWM refuses to suspend. So check the
> LED_SUSPENDED flag and depending on that set the .enabled property.
>
>
Applied, thanks!
[1/1] leds: pwm: Disable PWM when going to suspend
commit: 974afccd37947a6951a052ef8118c961e57eaf7b
--
Lee Jones [李琼斯]
On Thu, 02 May 2024, Lee Jones wrote:
> On Wed, 17 Apr 2024 17:38:47 +0200, Uwe Kleine-König wrote:
> > On stm32mp1xx based machines (and others) a PWM consumer has to disable
> > the PWM because an enabled PWM refuses to suspend. So check the
> > LED_SUSPENDED flag and depending on that set the .enabled property.
> >
> >
>
> Applied, thanks!
>
> [1/1] leds: pwm: Disable PWM when going to suspend
> commit: 974afccd37947a6951a052ef8118c961e57eaf7b
If this is important, you may wish to retroactively send it to Stable
once it's been merged into Mainline.
--
Lee Jones [李琼斯]