Re: [PATCH] pwm: core: Permit unset period when applying configuration of disabled PWMs

[Brian Norris <briannorris@chromium.org>]

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