Regression in deaeeda2051f ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state")

Regression in deaeeda2051f ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state")

[Uwe Kleine-König]

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

Hello,

hmm, the subject is wrong, this is about commit deaeeda2051f
("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive
state") and not 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only
once per backlight toggle"). I fixed it accordingly.

On Wed, Mar 22, 2023 at 05:13:24AM +0000, Aisheng Dong wrote:
> It seems this patch changed the behavior of pwm_backlight_suspend a little bit in
> pwm_backlight_power_off() that pwm state keep unchanged during suspend.
> Then pwm_imx_tpm_suspend() will return -Ebusy due to tpm->enable_count > 0.
> Was this intended behavior? Should we fix pwm core or the driver?

A I see. The problem is the combination of the following facts:

 - Some PWMs don't emit a constant inactive signal when disabled, so a
   consumer who wants a constant inactive signal must not disable the
   PWM.

 - A used PWM is supposed to be disabled by its consumer on suspend.
   (This is right IMHO because on suspend the PWM is likely to stop
   oscillating and if the consumer requested some output wave form a
   suspend usually stops to adhere to the consumer's request.)

So the options to fix this are (in order of my preference):

 a) Improve the check in the pwm_bl driver when it's safe to disable the
    PWM.

 b) Disable the PWM on suspend in the pwm_bl driver.

 c) If the pwm-imx-tpm's PWM output is configured with duty_cycle = 0 and
    is known not to continue driving a constant inactive signal on
    suspend, it might continue to suspend.

I think a) is not possible in general. To determine that: Which machine
do you experience this regression on?

b) is the right one from the PWM framework's POV. If you have a PWM like
pwm-imx27 that might result in the backlight going on on suspend. That's
bad, but compared to the pre-deaeeda2051f state it's still an
improvement (as there the backlight went on on disable *and* suspend).
Depending on the machine the backlight might or might not go off again
later when suspend progresses.

c) isn't that nice because that's an a bit special behaviour and people
who intend to write code that is correct for all PWMs but only have an
pwm-imx-tpm to test might fail to do so.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

RE: Regression in deaeeda2051f ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state")

[Aisheng Dong]

Hi Uwe,

> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2023年3月22日 15:04
> 
> Hello,
> 
> hmm, the subject is wrong, this is about commit deaeeda2051f
> ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive
> state") and not 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> per backlight toggle"). I fixed it accordingly.

I just double checked that only revert deaeeda2051f can't fix the issue.
I have to revert 00e7e698bff1 as well.
Below are my top commits:
2c8b0985593a (HEAD -> next-20230315) Revert "backlight: pwm_bl: Configure pwm only once per backlight toggle"
e6d0aba366a7 Revert "backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state"
225b6b81afe6 (tag: next-20230315, linux-next-cu/master) Add linux-next specific files for 20230315
...

> 
> On Wed, Mar 22, 2023 at 05:13:24AM +0000, Aisheng Dong wrote:
> > It seems this patch changed the behavior of pwm_backlight_suspend a
> > little bit in
> > pwm_backlight_power_off() that pwm state keep unchanged during
> suspend.
> > Then pwm_imx_tpm_suspend() will return -Ebusy due to
> tpm->enable_count > 0.
> > Was this intended behavior? Should we fix pwm core or the driver?
> 
> A I see. The problem is the combination of the following facts:
> 
>  - Some PWMs don't emit a constant inactive signal when disabled, so a
>    consumer who wants a constant inactive signal must not disable the
>    PWM.
> 
>  - A used PWM is supposed to be disabled by its consumer on suspend.
>    (This is right IMHO because on suspend the PWM is likely to stop
>    oscillating and if the consumer requested some output wave form a
>    suspend usually stops to adhere to the consumer's request.)
> 
> So the options to fix this are (in order of my preference):
> 
>  a) Improve the check in the pwm_bl driver when it's safe to disable the
>     PWM.
> 
>  b) Disable the PWM on suspend in the pwm_bl driver.
> 
>  c) If the pwm-imx-tpm's PWM output is configured with duty_cycle = 0 and
>     is known not to continue driving a constant inactive signal on
>     suspend, it might continue to suspend.
> 
> I think a) is not possible in general. To determine that: Which machine do you
> experience this regression on?

Imx7ulp evk.

> 
> b) is the right one from the PWM framework's POV. If you have a PWM like
> pwm-imx27 that might result in the backlight going on on suspend. That's bad,
> but compared to the pre-deaeeda2051f state it's still an improvement (as
> there the backlight went on on disable *and* suspend).
> Depending on the machine the backlight might or might not go off again later
> when suspend progresses.
> 

This seems like the previous working behavior on mx7ulp without this patch.
Would you help make a patch to fix it?

Regards
Aisheng

> c) isn't that nice because that's an a bit special behaviour and people who
> intend to write code that is correct for all PWMs but only have an
> pwm-imx-tpm to test might fail to do so.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH] backlight: pwm_bl: Disable PWM on shutdown and suspend disabled PWM emiting inactive state")

[Uwe Kleine-König]

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

Since commit 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
per backlight toggle") calling pwm_backlight_power_off() doesn't disable
the PWM any more. However this is necessary to suspend, because PWM
drivers usually refuse to suspend if they are still enabled.

Also adapt shutdown to disable the PWM for similar reasons.

Fixes: 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once per backlight toggle")
Reported-by: Aisheng Dong <aisheng.dong@nxp.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
On Wed, Mar 22, 2023 at 08:10:54AM +0000, Aisheng Dong wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: 2023年3月22日 15:04
> > 
> > Hello,
> > 
> > hmm, the subject is wrong, this is about commit deaeeda2051f
> > ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive
> > state") and not 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> > per backlight toggle"). I fixed it accordingly.
> 
> I just double checked that only revert deaeeda2051f can't fix the issue.
> I have to revert 00e7e698bff1 as well.

Ah, I see, because of 00e7e698bff1 the pwm isn't modified any more if
only pwm_backlight_power_off() (but not pwm_backlight_update_status())
is called. But that is what the suspend callback (and also shutdown)
does.

> > On Wed, Mar 22, 2023 at 05:13:24AM +0000, Aisheng Dong wrote:
> > > It seems this patch changed the behavior of pwm_backlight_suspend a
> > > little bit in
> > > pwm_backlight_power_off() that pwm state keep unchanged during
> > suspend.
> > > Then pwm_imx_tpm_suspend() will return -Ebusy due to
> > tpm->enable_count > 0.
> > > Was this intended behavior? Should we fix pwm core or the driver?
> > 
> > A I see. The problem is the combination of the following facts:
> > 
> >  - Some PWMs don't emit a constant inactive signal when disabled, so a
> >    consumer who wants a constant inactive signal must not disable the
> >    PWM.
> > 
> >  - A used PWM is supposed to be disabled by its consumer on suspend.
> >    (This is right IMHO because on suspend the PWM is likely to stop
> >    oscillating and if the consumer requested some output wave form a
> >    suspend usually stops to adhere to the consumer's request.)
> > 
> > So the options to fix this are (in order of my preference):
> > 
> >  a) Improve the check in the pwm_bl driver when it's safe to disable the
> >     PWM.
> > 
> >  b) Disable the PWM on suspend in the pwm_bl driver.
> > 
> >  c) If the pwm-imx-tpm's PWM output is configured with duty_cycle = 0 and
> >     is known not to continue driving a constant inactive signal on
> >     suspend, it might continue to suspend.
> > 
> > I think a) is not possible in general. To determine that: Which machine do you
> > experience this regression on?
> 
> Imx7ulp evk.

OK, so a backlight with neither an enable-gpio nor a regulator. So this
is a configuration where the PWM isn't disabled any more when
brightness=0 is set. Iff the driver emits its inactive state when
disabled (for both polarities), it might disable if .duty_cycle = 0 is
requested to safe some power.

> > b) is the right one from the PWM framework's POV. If you have a PWM like
> > pwm-imx27 that might result in the backlight going on on suspend. That's bad,
> > but compared to the pre-deaeeda2051f state it's still an improvement (as
> > there the backlight went on on disable *and* suspend).
> > Depending on the machine the backlight might or might not go off again later
> > when suspend progresses.
> > 
> 
> This seems like the previous working behavior on mx7ulp without this patch.
> Would you help make a patch to fix it?

Sure. I expect this fixes your issue, but I didn't test it. So if you
give it a shot that would be great.

Best regards
Uwe

 drivers/video/backlight/pwm_bl.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index fb388148d98f..577714e41694 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -643,8 +643,13 @@ static void pwm_backlight_shutdown(struct platform_device *pdev)
 {
 	struct backlight_device *bl = platform_get_drvdata(pdev);
 	struct pwm_bl_data *pb = bl_get_data(bl);
+	struct pwm_state state;
 
 	pwm_backlight_power_off(pb);
+	pwm_get_state(pb->pwm, &state);
+	state.duty_cycle = 0;
+	state.enabled = false;
+	pwm_apply_state(pb->pwm, &state);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -652,12 +657,24 @@ static int pwm_backlight_suspend(struct device *dev)
 {
 	struct backlight_device *bl = dev_get_drvdata(dev);
 	struct pwm_bl_data *pb = bl_get_data(bl);
+	struct pwm_state state;
 
 	if (pb->notify)
 		pb->notify(pb->dev, 0);
 
 	pwm_backlight_power_off(pb);
 
+	/*
+	 * Note that disabling the PWM doesn't guarantee that the output stays
+	 * at its inactive state. However without the PWM disabled, the PWM
+	 * driver refuses to suspend. So disable here even though this might
+	 * enable the backlight on poorly designed boards.
+	 */
+	pwm_get_state(pb->pwm, &state);
+	state.duty_cycle = 0;
+	state.enabled = false;
+	pwm_apply_state(pb->pwm, &state);
+
 	if (pb->notify_after)
 		pb->notify_after(pb->dev, 0);
 
-- 
2.39.2

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

RE: [PATCH] backlight: pwm_bl: Disable PWM on shutdown and suspend disabled PWM emiting inactive state")

[Aisheng Dong]

> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2023年3月22日 16:51
> 
> Since commit 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> per backlight toggle") calling pwm_backlight_power_off() doesn't disable the
> PWM any more. However this is necessary to suspend, because PWM drivers
> usually refuse to suspend if they are still enabled.
> 
> Also adapt shutdown to disable the PWM for similar reasons.
> 
> Fixes: 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once per
> backlight toggle")
> Reported-by: Aisheng Dong <aisheng.dong@nxp.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for the quick fix.
Tested-by: Aisheng Dong <asheng.dong@nxp.com>

Regards
Aisheng

> ---
> On Wed, Mar 22, 2023 at 08:10:54AM +0000, Aisheng Dong wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Sent: 2023年3月22日 15:04
> > >
> > > Hello,
> > >
> > > hmm, the subject is wrong, this is about commit deaeeda2051f
> > > ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive
> > > state") and not 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only
> > > once per backlight toggle"). I fixed it accordingly.
> >
> > I just double checked that only revert deaeeda2051f can't fix the issue.
> > I have to revert 00e7e698bff1 as well.
> 
> Ah, I see, because of 00e7e698bff1 the pwm isn't modified any more if only
> pwm_backlight_power_off() (but not pwm_backlight_update_status()) is called.
> But that is what the suspend callback (and also shutdown) does.
> 
> > > On Wed, Mar 22, 2023 at 05:13:24AM +0000, Aisheng Dong wrote:
> > > > It seems this patch changed the behavior of pwm_backlight_suspend
> > > > a little bit in
> > > > pwm_backlight_power_off() that pwm state keep unchanged during
> > > suspend.
> > > > Then pwm_imx_tpm_suspend() will return -Ebusy due to
> > > tpm->enable_count > 0.
> > > > Was this intended behavior? Should we fix pwm core or the driver?
> > >
> > > A I see. The problem is the combination of the following facts:
> > >
> > >  - Some PWMs don't emit a constant inactive signal when disabled, so a
> > >    consumer who wants a constant inactive signal must not disable the
> > >    PWM.
> > >
> > >  - A used PWM is supposed to be disabled by its consumer on suspend.
> > >    (This is right IMHO because on suspend the PWM is likely to stop
> > >    oscillating and if the consumer requested some output wave form a
> > >    suspend usually stops to adhere to the consumer's request.)
> > >
> > > So the options to fix this are (in order of my preference):
> > >
> > >  a) Improve the check in the pwm_bl driver when it's safe to disable the
> > >     PWM.
> > >
> > >  b) Disable the PWM on suspend in the pwm_bl driver.
> > >
> > >  c) If the pwm-imx-tpm's PWM output is configured with duty_cycle = 0
> and
> > >     is known not to continue driving a constant inactive signal on
> > >     suspend, it might continue to suspend.
> > >
> > > I think a) is not possible in general. To determine that: Which
> > > machine do you experience this regression on?
> >
> > Imx7ulp evk.
> 
> OK, so a backlight with neither an enable-gpio nor a regulator. So this is a
> configuration where the PWM isn't disabled any more when
> brightness=0 is set. Iff the driver emits its inactive state when disabled (for
> both polarities), it might disable if .duty_cycle = 0 is requested to safe some
> power.
> 
> > > b) is the right one from the PWM framework's POV. If you have a PWM
> > > like
> > > pwm-imx27 that might result in the backlight going on on suspend.
> > > That's bad, but compared to the pre-deaeeda2051f state it's still an
> > > improvement (as there the backlight went on on disable *and* suspend).
> > > Depending on the machine the backlight might or might not go off
> > > again later when suspend progresses.
> > >
> >
> > This seems like the previous working behavior on mx7ulp without this patch.
> > Would you help make a patch to fix it?
> 
> Sure. I expect this fixes your issue, but I didn't test it. So if you give it a shot
> that would be great.
> 
> Best regards
> Uwe
> 
>  drivers/video/backlight/pwm_bl.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index fb388148d98f..577714e41694 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -643,8 +643,13 @@ static void pwm_backlight_shutdown(struct
> platform_device *pdev)  {
>  	struct backlight_device *bl = platform_get_drvdata(pdev);
>  	struct pwm_bl_data *pb = bl_get_data(bl);
> +	struct pwm_state state;
> 
>  	pwm_backlight_power_off(pb);
> +	pwm_get_state(pb->pwm, &state);
> +	state.duty_cycle = 0;
> +	state.enabled = false;
> +	pwm_apply_state(pb->pwm, &state);
>  }
> 
>  #ifdef CONFIG_PM_SLEEP
> @@ -652,12 +657,24 @@ static int pwm_backlight_suspend(struct device
> *dev)  {
>  	struct backlight_device *bl = dev_get_drvdata(dev);
>  	struct pwm_bl_data *pb = bl_get_data(bl);
> +	struct pwm_state state;
> 
>  	if (pb->notify)
>  		pb->notify(pb->dev, 0);
> 
>  	pwm_backlight_power_off(pb);
> 
> +	/*
> +	 * Note that disabling the PWM doesn't guarantee that the output stays
> +	 * at its inactive state. However without the PWM disabled, the PWM
> +	 * driver refuses to suspend. So disable here even though this might
> +	 * enable the backlight on poorly designed boards.
> +	 */
> +	pwm_get_state(pb->pwm, &state);
> +	state.duty_cycle = 0;
> +	state.enabled = false;
> +	pwm_apply_state(pb->pwm, &state);
> +
>  	if (pb->notify_after)
>  		pb->notify_after(pb->dev, 0);
> 
> --
> 2.39.2
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] backlight: pwm_bl: Disable PWM on shutdown and suspend disabled PWM emiting inactive state")

[Uwe Kleine-König]

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

Hello,

On Wed, Mar 22, 2023 at 09:51:29AM +0100, Uwe Kleine-König wrote:
> Since commit 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> per backlight toggle") calling pwm_backlight_power_off() doesn't disable
> the PWM any more. However this is necessary to suspend, because PWM
> drivers usually refuse to suspend if they are still enabled.
> 
> Also adapt shutdown to disable the PWM for similar reasons.
> 
> Fixes: 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once per backlight toggle")
> Reported-by: Aisheng Dong <aisheng.dong@nxp.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This patch was never applied but I think it is still needed. I assume it
fell through the cracks?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] backlight: pwm_bl: Disable PWM on shutdown and suspend disabled PWM emiting inactive state")

[Lee Jones]

On Tue, 26 Sep 2023, Uwe Kleine-König wrote:

> Hello,
> 
> On Wed, Mar 22, 2023 at 09:51:29AM +0100, Uwe Kleine-König wrote:
> > Since commit 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> > per backlight toggle") calling pwm_backlight_power_off() doesn't disable
> > the PWM any more. However this is necessary to suspend, because PWM
> > drivers usually refuse to suspend if they are still enabled.
> > 
> > Also adapt shutdown to disable the PWM for similar reasons.
> > 
> > Fixes: 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once per backlight toggle")
> > Reported-by: Aisheng Dong <aisheng.dong@nxp.com>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> This patch was never applied but I think it is still needed. I assume it
> fell through the cracks?

This "patch" was sent half way through a thread and when opened in my
mail client looks like a mail reply due to the quotes below the '---'.

I'd suggest sending this again.

-- 
Lee Jones [李琼斯]

Re: [PATCH] backlight: pwm_bl: Disable PWM on shutdown and suspend disabled PWM emiting inactive state")

[Lee Jones]

On Tue, 26 Sep 2023, Lee Jones wrote:

> On Tue, 26 Sep 2023, Uwe Kleine-König wrote:
> 
> > Hello,
> > 
> > On Wed, Mar 22, 2023 at 09:51:29AM +0100, Uwe Kleine-König wrote:
> > > Since commit 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> > > per backlight toggle") calling pwm_backlight_power_off() doesn't disable
> > > the PWM any more. However this is necessary to suspend, because PWM
> > > drivers usually refuse to suspend if they are still enabled.
> > > 
> > > Also adapt shutdown to disable the PWM for similar reasons.
> > > 
> > > Fixes: 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once per backlight toggle")
> > > Reported-by: Aisheng Dong <aisheng.dong@nxp.com>
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > This patch was never applied but I think it is still needed. I assume it
> > fell through the cracks?
> 
> This "patch" was sent half way through a thread and when opened in my
> mail client looks like a mail reply due to the quotes below the '---'.
> 
> I'd suggest sending this again.

You also have a copy/paste error in the subject line.

-- 
Lee Jones [李琼斯]