[PATCH] pwm: sysfs: Do not apply state to already disabled PWMs

[PATCH] pwm: sysfs: Do not apply state to already disabled PWMs

[Marek Vasut]

If the PWM is exported but not enabled, do not call pwm_class_apply_state().
First of all, in this case, period may still be unconfigured and this would
make pwm_class_apply_state() return -EINVAL, and then suspend would fail.
Second, it makes little sense to apply state onto PWM that is not enabled
before suspend.

Failing case:
"
$ echo 1 > /sys/class/pwm/pwmchip4/export
$ echo mem > /sys/power/state
...
pwm pwmchip4: PM: dpm_run_callback(): pwm_class_suspend+0x1/0xa8 returns -22
pwm pwmchip4: PM: failed to suspend: error -22
PM: Some devices failed to suspend, or early wake event detected
"

Working case:
"
$ echo 1 > /sys/class/pwm/pwmchip4/export
$ echo 100 > /sys/class/pwm/pwmchip4/pwm1/period
$ echo 10 > /sys/class/pwm/pwmchip4/pwm1/duty_cycle
$ echo mem > /sys/power/state
...
"

Do not call pwm_class_apply_state() in case the PWM is disabled
to fix this issue.

Fixes: 7fd4edc57bbae ("pwm: sysfs: Add suspend/resume support")
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Brian Norris <briannorris@chromium.org>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-pwm@vger.kernel.org
---
 drivers/pwm/sysfs.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 1a106ec329392..8d1254761e4dd 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -424,6 +424,13 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
 		if (!export)
 			continue;
 
+		/* If pwmchip was not enabled before suspend, do nothing. */
+		if (!export->suspend.enabled) {
+			/* release lock taken in pwm_class_get_state */
+			mutex_unlock(&export->lock);
+			continue;
+		}
+
 		state.enabled = export->suspend.enabled;
 		ret = pwm_class_apply_state(export, pwm, &state);
 		if (ret < 0)
@@ -448,7 +455,17 @@ static int pwm_class_suspend(struct device *parent)
 		if (!export)
 			continue;
 
+		/*
+		 * If pwmchip was not enabled before suspend, save
+		 * state for resume time and do nothing else.
+		 */
 		export->suspend = state;
+		if (!state.enabled) {
+			/* release lock taken in pwm_class_get_state */
+			mutex_unlock(&export->lock);
+			continue;
+		}
+
 		state.enabled = false;
 		ret = pwm_class_apply_state(export, pwm, &state);
 		if (ret < 0) {
-- 
2.39.2

Re: [PATCH] pwm: sysfs: Do not apply state to already disabled PWMs

[Uwe Kleine-König]

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

Hello Marek,

On Fri, May 12, 2023 at 06:47:36PM +0200, Marek Vasut wrote:
> If the PWM is exported but not enabled, do not call pwm_class_apply_state().
> First of all, in this case, period may still be unconfigured and this would
> make pwm_class_apply_state() return -EINVAL, and then suspend would fail.
> Second, it makes little sense to apply state onto PWM that is not enabled
> before suspend.
> 
> Failing case:
> "
> $ echo 1 > /sys/class/pwm/pwmchip4/export
> $ echo mem > /sys/power/state
> ...
> pwm pwmchip4: PM: dpm_run_callback(): pwm_class_suspend+0x1/0xa8 returns -22
> pwm pwmchip4: PM: failed to suspend: error -22
> PM: Some devices failed to suspend, or early wake event detected
> "
> 
> Working case:
> "
> $ echo 1 > /sys/class/pwm/pwmchip4/export
> $ echo 100 > /sys/class/pwm/pwmchip4/pwm1/period
> $ echo 10 > /sys/class/pwm/pwmchip4/pwm1/duty_cycle
> $ echo mem > /sys/power/state
> ...
> "
> 
> Do not call pwm_class_apply_state() in case the PWM is disabled
> to fix this issue.
> 
> Fixes: 7fd4edc57bbae ("pwm: sysfs: Add suspend/resume support")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-pwm@vger.kernel.org
> ---
>  drivers/pwm/sysfs.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 1a106ec329392..8d1254761e4dd 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -424,6 +424,13 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
>  		if (!export)
>  			continue;
>  
> +		/* If pwmchip was not enabled before suspend, do nothing. */
> +		if (!export->suspend.enabled) {
> +			/* release lock taken in pwm_class_get_state */
> +			mutex_unlock(&export->lock);
> +			continue;
> +		}
> +
>  		state.enabled = export->suspend.enabled;
>  		ret = pwm_class_apply_state(export, pwm, &state);
>  		if (ret < 0)
> @@ -448,7 +455,17 @@ static int pwm_class_suspend(struct device *parent)
>  		if (!export)
>  			continue;
>  
> +		/*
> +		 * If pwmchip was not enabled before suspend, save
> +		 * state for resume time and do nothing else.
> +		 */
>  		export->suspend = state;
> +		if (!state.enabled) {
> +			/* release lock taken in pwm_class_get_state */
> +			mutex_unlock(&export->lock);
> +			continue;
> +		}
> +
>  		state.enabled = false;
>  		ret = pwm_class_apply_state(export, pwm, &state);
>  		if (ret < 0) {

The locking and code flow in these functions is really ugly. I see
several patch opportunities here ...

Anyhow, these are orthogonal to your patch which looks good.

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
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] pwm: sysfs: Do not apply state to already disabled PWMs

[Brian Norris]

Hi,

On Fri, May 12, 2023 at 06:47:36PM +0200, Marek Vasut wrote:
> If the PWM is exported but not enabled, do not call pwm_class_apply_state().
> First of all, in this case, period may still be unconfigured and this would
> make pwm_class_apply_state() return -EINVAL, and then suspend would fail.
> Second, it makes little sense to apply state onto PWM that is not enabled
> before suspend.
> 
> Failing case:
> "
> $ echo 1 > /sys/class/pwm/pwmchip4/export
> $ echo mem > /sys/power/state
> ...
> pwm pwmchip4: PM: dpm_run_callback(): pwm_class_suspend+0x1/0xa8 returns -22
> pwm pwmchip4: PM: failed to suspend: error -22
> PM: Some devices failed to suspend, or early wake event detected
> "
> 
> Working case:
> "
> $ echo 1 > /sys/class/pwm/pwmchip4/export
> $ echo 100 > /sys/class/pwm/pwmchip4/pwm1/period
> $ echo 10 > /sys/class/pwm/pwmchip4/pwm1/duty_cycle
> $ echo mem > /sys/power/state
> ...
> "
> 
> Do not call pwm_class_apply_state() in case the PWM is disabled
> to fix this issue.
> 
> Fixes: 7fd4edc57bbae ("pwm: sysfs: Add suspend/resume support")

My first thought was that this still belongs as:

Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")

but then I realized sysfs suspend/resume support was added *after* that,
so indeed, your Fixes tag makes the most sense.

And yes, I think this solution (addressing sysfs.c directly) is best
too:

Reviewed-by: Brian Norris <briannorris@chromium.org>

Side question: I wonder if this belongs in linux-stable. It's definitely
a bug fix, but the bug has been around a while, with a
{under,non}-specified ABI, and it's easy enough to work around I
suppose. But inevitably, *any* patch with a Fixes tag gets picked up by
somebody's cherry-picking bot, so maybe it doesn't matter...
...Anyway, I guess I'm saying it's probably going to go to linux-stable,
whether we want it to or not; and maybe that's OK :)

Thanks,
Brian

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-pwm@vger.kernel.org
> ---
>  drivers/pwm/sysfs.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Re: [PATCH] pwm: sysfs: Do not apply state to already disabled PWMs

[Marek Vasut]

On 5/15/23 20:26, Brian Norris wrote:
> Hi,

Hi,

> On Fri, May 12, 2023 at 06:47:36PM +0200, Marek Vasut wrote:
>> If the PWM is exported but not enabled, do not call pwm_class_apply_state().
>> First of all, in this case, period may still be unconfigured and this would
>> make pwm_class_apply_state() return -EINVAL, and then suspend would fail.
>> Second, it makes little sense to apply state onto PWM that is not enabled
>> before suspend.
>>
>> Failing case:
>> "
>> $ echo 1 > /sys/class/pwm/pwmchip4/export
>> $ echo mem > /sys/power/state
>> ...
>> pwm pwmchip4: PM: dpm_run_callback(): pwm_class_suspend+0x1/0xa8 returns -22
>> pwm pwmchip4: PM: failed to suspend: error -22
>> PM: Some devices failed to suspend, or early wake event detected
>> "
>>
>> Working case:
>> "
>> $ echo 1 > /sys/class/pwm/pwmchip4/export
>> $ echo 100 > /sys/class/pwm/pwmchip4/pwm1/period
>> $ echo 10 > /sys/class/pwm/pwmchip4/pwm1/duty_cycle
>> $ echo mem > /sys/power/state
>> ...
>> "
>>
>> Do not call pwm_class_apply_state() in case the PWM is disabled
>> to fix this issue.
>>
>> Fixes: 7fd4edc57bbae ("pwm: sysfs: Add suspend/resume support")
> 
> My first thought was that this still belongs as:
> 
> Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
> 
> but then I realized sysfs suspend/resume support was added *after* that,
> so indeed, your Fixes tag makes the most sense.
> 
> And yes, I think this solution (addressing sysfs.c directly) is best
> too:
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
> Side question: I wonder if this belongs in linux-stable. It's definitely
> a bug fix, but the bug has been around a while, with a
> {under,non}-specified ABI, and it's easy enough to work around I
> suppose. But inevitably, *any* patch with a Fixes tag gets picked up by
> somebody's cherry-picking bot, so maybe it doesn't matter...
> ...Anyway, I guess I'm saying it's probably going to go to linux-stable,
> whether we want it to or not; and maybe that's OK :)

To provide a bit more context. This was discovered a person writing 
userspace application who couldn't suspend the machine because of this, 
but they also couldn't find out why. So I think this is good candidate 
for stable as it makes this edge a little less sharp.

I agree it can be worked around and I told them how to update the user 
application, but still, this was inobvious and obscure.

Re: [PATCH] pwm: sysfs: Do not apply state to already disabled PWMs

[Thierry Reding]

On Fri, 12 May 2023 18:47:36 +0200, Marek Vasut wrote:
> If the PWM is exported but not enabled, do not call pwm_class_apply_state().
> First of all, in this case, period may still be unconfigured and this would
> make pwm_class_apply_state() return -EINVAL, and then suspend would fail.
> Second, it makes little sense to apply state onto PWM that is not enabled
> before suspend.
> 
> Failing case:
> "
> $ echo 1 > /sys/class/pwm/pwmchip4/export
> $ echo mem > /sys/power/state
> ...
> pwm pwmchip4: PM: dpm_run_callback(): pwm_class_suspend+0x1/0xa8 returns -22
> pwm pwmchip4: PM: failed to suspend: error -22
> PM: Some devices failed to suspend, or early wake event detected
> "
> 
> [...]

Applied, thanks!

[1/1] pwm: sysfs: Do not apply state to already disabled PWMs
      commit: 38ba83598633373f47951384cfc389181c8d1bed

Best regards,
-- 
Thierry Reding <thierry.reding@gmail.com>