* [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
@ 2023-05-11 18:18 Marek Vasut
2023-05-11 21:08 ` Brian Norris
2023-05-12 6:22 ` Uwe Kleine-König
0 siblings, 2 replies; 8+ messages in thread
From: Marek Vasut @ 2023-05-11 18:18 UTC (permalink / raw)
To: linux-pwm
Cc: Marek Vasut, Brian Norris, Uwe Kleine-König,
Geert Uytterhoeven, Thierry Reding, Yoshihiro Shimoda
In case the PWM is not enabled, the period can still be left unconfigured,
i.e. zero . Currently the pwm_class_apply_state() errors out in such a case.
This e.g. makes suspend fail on systems where pwmchip has been exported via
sysfs interface, but left unconfigured before suspend occurred.
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
...
"
Permit unset period in pwm_class_apply_state() in case the PWM is disabled
to fix this issue.
Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
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/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3dacceaef4a9b..87252b70f1c81 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
*/
might_sleep();
- if (!pwm || !state || !state->period ||
- state->duty_cycle > state->period)
+ if (!pwm || !state || (state->enabled &&
+ (!state->period || state->duty_cycle > state->period)))
return -EINVAL;
chip = pwm->chip;
--
2.39.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
2023-05-11 18:18 [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs Marek Vasut
@ 2023-05-11 21:08 ` Brian Norris
2023-05-11 23:32 ` Marek Vasut
2023-05-12 6:22 ` Uwe Kleine-König
1 sibling, 1 reply; 8+ messages in thread
From: Brian Norris @ 2023-05-11 21:08 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pwm, Uwe Kleine-König, Geert Uytterhoeven,
Thierry Reding, Yoshihiro Shimoda
Hi,
On Thu, May 11, 2023 at 08:18:53PM +0200, Marek Vasut wrote:
> In case the PWM is not enabled, the period can still be left unconfigured,
> i.e. zero . Currently the pwm_class_apply_state() errors out in such a case.
> This e.g. makes suspend fail on systems where pwmchip has been exported via
> sysfs interface, but left unconfigured before suspend occurred.
>
> 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
> ...
> "
>
> Permit unset period in pwm_class_apply_state() in case the PWM is disabled
> to fix this issue.
>
> Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
> 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/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3dacceaef4a9b..87252b70f1c81 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> */
> might_sleep();
>
> - if (!pwm || !state || !state->period ||
> - state->duty_cycle > state->period)
> + if (!pwm || !state || (state->enabled &&
> + (!state->period || state->duty_cycle > state->period)))
> return -EINVAL;
>
> chip = pwm->chip;
By making the period assertions conditional, you're allowing people to
write garbage period values via sysfs. However you fix the (legitimate)
bug you point out, you shouldn't regress that. (Now, that's sounding
like we could use some unit tests for the PWM framework...)
You could, for example, also add the bounds checks to
drviers/pwm/sysfs.c's period_store().
Or perhaps you could teach the suspend/resume functions to not bother
calling pwm_apply_state() on a disabled PWM.
Brian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
2023-05-11 21:08 ` Brian Norris
@ 2023-05-11 23:32 ` Marek Vasut
2023-05-12 0:51 ` Brian Norris
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2023-05-11 23:32 UTC (permalink / raw)
To: Brian Norris
Cc: linux-pwm, Uwe Kleine-König, Geert Uytterhoeven,
Thierry Reding, Yoshihiro Shimoda
On 5/11/23 23:08, Brian Norris wrote:
> Hi,
Hi,
> On Thu, May 11, 2023 at 08:18:53PM +0200, Marek Vasut wrote:
>> In case the PWM is not enabled, the period can still be left unconfigured,
>> i.e. zero . Currently the pwm_class_apply_state() errors out in such a case.
>> This e.g. makes suspend fail on systems where pwmchip has been exported via
>> sysfs interface, but left unconfigured before suspend occurred.
>>
>> 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
>> ...
>> "
>>
>> Permit unset period in pwm_class_apply_state() in case the PWM is disabled
>> to fix this issue.
>>
>> Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
>> 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/core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 3dacceaef4a9b..87252b70f1c81 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>> */
>> might_sleep();
>>
>> - if (!pwm || !state || !state->period ||
>> - state->duty_cycle > state->period)
>> + if (!pwm || !state || (state->enabled &&
>> + (!state->period || state->duty_cycle > state->period)))
>> return -EINVAL;
>>
>> chip = pwm->chip;
>
> By making the period assertions conditional, you're allowing people to
> write garbage period values via sysfs. However you fix the (legitimate)
> bug you point out, you shouldn't regress that.
I wanted to say, it might be best to fix userspace so that it wouldn't
export pwmchip and then suspend without configuring it. But (!) this
actually allows userspace to export pwmchip and that way, block suspend
completely, because with pwmchip exported and not configured, the system
just would not suspend. So, yes, this is a legitimate fix for a real
bug, right ?
> (Now, that's sounding
> like we could use some unit tests for the PWM framework...)
Not just the PWM framework ...
> You could, for example, also add the bounds checks to
> drviers/pwm/sysfs.c's period_store().
>
> Or perhaps you could teach the suspend/resume functions to not bother
> calling pwm_apply_state() on a disabled PWM.
Right, I think it boils down to -- should this be fixed on the sysfs ABI
side, or in the pwm core ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
2023-05-11 23:32 ` Marek Vasut
@ 2023-05-12 0:51 ` Brian Norris
2023-05-12 16:50 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2023-05-12 0:51 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pwm, Uwe Kleine-König, Geert Uytterhoeven,
Thierry Reding, Yoshihiro Shimoda
On Fri, May 12, 2023 at 01:32:49AM +0200, Marek Vasut wrote:
> On 5/11/23 23:08, Brian Norris wrote:
> > On Thu, May 11, 2023 at 08:18:53PM +0200, Marek Vasut wrote:
> > > Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
> > > 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/core.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index 3dacceaef4a9b..87252b70f1c81 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > > */
> > > might_sleep();
> > > - if (!pwm || !state || !state->period ||
> > > - state->duty_cycle > state->period)
> > > + if (!pwm || !state || (state->enabled &&
> > > + (!state->period || state->duty_cycle > state->period)))
> > > return -EINVAL;
> > > chip = pwm->chip;
> >
> > By making the period assertions conditional, you're allowing people to
> > write garbage period values via sysfs. However you fix the (legitimate)
> > bug you point out, you shouldn't regress that.
>
> I wanted to say, it might be best to fix userspace so that it wouldn't
> export pwmchip and then suspend without configuring it. But (!) this
> actually allows userspace to export pwmchip and that way, block suspend
> completely, because with pwmchip exported and not configured, the system
> just would not suspend. So, yes, this is a legitimate fix for a real bug,
> right ?
It's a real bug, yes. (Quoting myself, "(legitimate) bug you point
out".)
But you're introducing the old one again, so I wouldn't call it a
"legitimate fix."
commit ef2bf4997f7da6efa8540d9cf726c44bf2b863af
[...]
In particular, I noted that we are now allowing invalid period
selections, e.g.:
# echo 1 > /sys/class/pwm/pwmchip0/export
# cat /sys/class/pwm/pwmchip0/pwm1/period
100
# echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
[... driver may or may not reject the value, or trigger some logic bug ...]
The only difference is that we'll still *eventually* reject it somewhere
(probably when we try to enable the PWM), but just not at the
"echo 101 > .../duty_cycle" phase.
> > (Now, that's sounding
> > like we could use some unit tests for the PWM framework...)
>
> Not just the PWM framework ...
>
> > You could, for example, also add the bounds checks to
> > drviers/pwm/sysfs.c's period_store().
> >
> > Or perhaps you could teach the suspend/resume functions to not bother
> > calling pwm_apply_state() on a disabled PWM.
>
> Right, I think it boils down to -- should this be fixed on the sysfs ABI
> side, or in the pwm core ?
I don't know if I have a strong preference (I haven't tried to write it
out to see what looks cleaner / has the fewest holes), I just would
prefer that this isn't allowed:
# echo 1 > /sys/class/pwm/pwmchip0/export
# echo 100 > /sys/class/pwm/pwmchip0/pwm1/period
### Should fail with EINVAL:
# echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
Brian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
2023-05-11 18:18 [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs Marek Vasut
2023-05-11 21:08 ` Brian Norris
@ 2023-05-12 6:22 ` Uwe Kleine-König
2023-05-12 12:20 ` Marek Vasut
1 sibling, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2023-05-12 6:22 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pwm, Brian Norris, Geert Uytterhoeven, Thierry Reding,
Yoshihiro Shimoda
[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]
Hello Marek,
On Thu, May 11, 2023 at 08:18:53PM +0200, Marek Vasut wrote:
> In case the PWM is not enabled, the period can still be left unconfigured,
> i.e. zero . Currently the pwm_class_apply_state() errors out in such a case.
> This e.g. makes suspend fail on systems where pwmchip has been exported via
> sysfs interface, but left unconfigured before suspend occurred.
>
> 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
> ...
> "
>
> Permit unset period in pwm_class_apply_state() in case the PWM is disabled
> to fix this issue.
>
> Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
> 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/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3dacceaef4a9b..87252b70f1c81 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> */
> might_sleep();
>
> - if (!pwm || !state || !state->period ||
> - state->duty_cycle > state->period)
> + if (!pwm || !state || (state->enabled &&
> + (!state->period || state->duty_cycle > state->period)))
While I tend to agree that the checks about period and duty_cycle are
(somewhat) irrelevant if enabled == false, I fear we'd break assumptions
here as some drivers configure duty_cycle/period in hardware even with
.enabled == false, and so proably rely on period > 0 and duty_cycle <=
period.
Another approach would be to skip pwm_apply_state() in
pwm_class_suspend() if the state is already disabled. That one sounds
safer.
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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
2023-05-12 6:22 ` Uwe Kleine-König
@ 2023-05-12 12:20 ` Marek Vasut
2023-05-16 9:42 ` Thierry Reding
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2023-05-12 12:20 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, Brian Norris, Geert Uytterhoeven, Thierry Reding,
Yoshihiro Shimoda
On 5/12/23 08:22, Uwe Kleine-König wrote:
> Hello Marek,
>
> On Thu, May 11, 2023 at 08:18:53PM +0200, Marek Vasut wrote:
>> In case the PWM is not enabled, the period can still be left unconfigured,
>> i.e. zero . Currently the pwm_class_apply_state() errors out in such a case.
>> This e.g. makes suspend fail on systems where pwmchip has been exported via
>> sysfs interface, but left unconfigured before suspend occurred.
>>
>> 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
>> ...
>> "
>>
>> Permit unset period in pwm_class_apply_state() in case the PWM is disabled
>> to fix this issue.
>>
>> Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
>> 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/core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 3dacceaef4a9b..87252b70f1c81 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>> */
>> might_sleep();
>>
>> - if (!pwm || !state || !state->period ||
>> - state->duty_cycle > state->period)
>> + if (!pwm || !state || (state->enabled &&
>> + (!state->period || state->duty_cycle > state->period)))
>
> While I tend to agree that the checks about period and duty_cycle are
> (somewhat) irrelevant if enabled == false, I fear we'd break assumptions
> here as some drivers configure duty_cycle/period in hardware even with
> .enabled == false, and so proably rely on period > 0 and duty_cycle <=
> period.
>
> Another approach would be to skip pwm_apply_state() in
> pwm_class_suspend() if the state is already disabled. That one sounds
> safer.
I am not convinced that would be identical behavior.
If we skip apply_state call on PWMs exported via sysfs interface which
are enabled=false , then the drivers wouldn't have their apply_state
callback called to disable the PWM before suspend ... I think ... which
seems like a problem to me ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
2023-05-12 0:51 ` Brian Norris
@ 2023-05-12 16:50 ` Marek Vasut
0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2023-05-12 16:50 UTC (permalink / raw)
To: Brian Norris
Cc: linux-pwm, Uwe Kleine-König, Geert Uytterhoeven,
Thierry Reding, Yoshihiro Shimoda
On 5/12/23 02:51, Brian Norris wrote:
> On Fri, May 12, 2023 at 01:32:49AM +0200, Marek Vasut wrote:
>> On 5/11/23 23:08, Brian Norris wrote:
>>> On Thu, May 11, 2023 at 08:18:53PM +0200, Marek Vasut wrote:
>>>> Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
>>>> 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/core.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>> index 3dacceaef4a9b..87252b70f1c81 100644
>>>> --- a/drivers/pwm/core.c
>>>> +++ b/drivers/pwm/core.c
>>>> @@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>>>> */
>>>> might_sleep();
>>>> - if (!pwm || !state || !state->period ||
>>>> - state->duty_cycle > state->period)
>>>> + if (!pwm || !state || (state->enabled &&
>>>> + (!state->period || state->duty_cycle > state->period)))
>>>> return -EINVAL;
>>>> chip = pwm->chip;
>>>
>>> By making the period assertions conditional, you're allowing people to
>>> write garbage period values via sysfs. However you fix the (legitimate)
>>> bug you point out, you shouldn't regress that.
>>
>> I wanted to say, it might be best to fix userspace so that it wouldn't
>> export pwmchip and then suspend without configuring it. But (!) this
>> actually allows userspace to export pwmchip and that way, block suspend
>> completely, because with pwmchip exported and not configured, the system
>> just would not suspend. So, yes, this is a legitimate fix for a real bug,
>> right ?
>
> It's a real bug, yes. (Quoting myself, "(legitimate) bug you point
> out".)
>
> But you're introducing the old one again, so I wouldn't call it a
> "legitimate fix."
>
> commit ef2bf4997f7da6efa8540d9cf726c44bf2b863af
> [...]
> In particular, I noted that we are now allowing invalid period
> selections, e.g.:
>
> # echo 1 > /sys/class/pwm/pwmchip0/export
> # cat /sys/class/pwm/pwmchip0/pwm1/period
> 100
> # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> [... driver may or may not reject the value, or trigger some logic bug ...]
>
> The only difference is that we'll still *eventually* reject it somewhere
> (probably when we try to enable the PWM), but just not at the
> "echo 101 > .../duty_cycle" phase.
>
>>> (Now, that's sounding
>>> like we could use some unit tests for the PWM framework...)
>>
>> Not just the PWM framework ...
>>
>>> You could, for example, also add the bounds checks to
>>> drviers/pwm/sysfs.c's period_store().
>>>
>>> Or perhaps you could teach the suspend/resume functions to not bother
>>> calling pwm_apply_state() on a disabled PWM.
>>
>> Right, I think it boils down to -- should this be fixed on the sysfs ABI
>> side, or in the pwm core ?
>
> I don't know if I have a strong preference (I haven't tried to write it
> out to see what looks cleaner / has the fewest holes), I just would
> prefer that this isn't allowed:
>
> # echo 1 > /sys/class/pwm/pwmchip0/export
> # echo 100 > /sys/class/pwm/pwmchip0/pwm1/period
> ### Should fail with EINVAL:
> # echo 101 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
So, I sent the sysfs variant of this patch just now. I think maybe that
is the better option.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs
2023-05-12 12:20 ` Marek Vasut
@ 2023-05-16 9:42 ` Thierry Reding
0 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2023-05-16 9:42 UTC (permalink / raw)
To: Marek Vasut
Cc: Uwe Kleine-König, linux-pwm, Brian Norris,
Geert Uytterhoeven, Yoshihiro Shimoda
[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]
On Fri, May 12, 2023 at 02:20:12PM +0200, Marek Vasut wrote:
> On 5/12/23 08:22, Uwe Kleine-König wrote:
> > Hello Marek,
> >
> > On Thu, May 11, 2023 at 08:18:53PM +0200, Marek Vasut wrote:
> > > In case the PWM is not enabled, the period can still be left unconfigured,
> > > i.e. zero . Currently the pwm_class_apply_state() errors out in such a case.
> > > This e.g. makes suspend fail on systems where pwmchip has been exported via
> > > sysfs interface, but left unconfigured before suspend occurred.
> > >
> > > 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
> > > ...
> > > "
> > >
> > > Permit unset period in pwm_class_apply_state() in case the PWM is disabled
> > > to fix this issue.
> > >
> > > Fixes: ef2bf4997f7d ("pwm: Improve args checking in pwm_apply_state()")
> > > 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/core.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index 3dacceaef4a9b..87252b70f1c81 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -510,8 +510,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > > */
> > > might_sleep();
> > > - if (!pwm || !state || !state->period ||
> > > - state->duty_cycle > state->period)
> > > + if (!pwm || !state || (state->enabled &&
> > > + (!state->period || state->duty_cycle > state->period)))
> >
> > While I tend to agree that the checks about period and duty_cycle are
> > (somewhat) irrelevant if enabled == false, I fear we'd break assumptions
> > here as some drivers configure duty_cycle/period in hardware even with
> > .enabled == false, and so proably rely on period > 0 and duty_cycle <=
> > period.
> >
> > Another approach would be to skip pwm_apply_state() in
> > pwm_class_suspend() if the state is already disabled. That one sounds
> > safer.
>
> I am not convinced that would be identical behavior.
>
> If we skip apply_state call on PWMs exported via sysfs interface which are
> enabled=false , then the drivers wouldn't have their apply_state callback
> called to disable the PWM before suspend ... I think ... which seems like a
> problem to me ?
If a PWM exported via sysfs has enabled=false, then it should already be
disabled, so calling pwm_apply_state() on them to disable them should be
a no-op.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-05-16 9:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-11 18:18 [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs Marek Vasut
2023-05-11 21:08 ` Brian Norris
2023-05-11 23:32 ` Marek Vasut
2023-05-12 0:51 ` Brian Norris
2023-05-12 16:50 ` Marek Vasut
2023-05-12 6:22 ` Uwe Kleine-König
2023-05-12 12:20 ` Marek Vasut
2023-05-16 9:42 ` Thierry Reding
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).