Re: [PATCH v1] pwm: sifive: Always let the first pwm_apply_state succeed

[Emil Renner Berthing]

On Tue, 18 Oct 2022 at 16:56, Emil Renner Berthing
<emil.renner.berthing@xxxxxxxxxxxxx> wrote:
>
> On Tue, 18 Oct 2022 at 15:29, Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> >
> > Hello,
> >
> > On Tue, Oct 18, 2022 at 11:13:16AM +0200, Emil Renner Berthing wrote:
> > > Commit 2cfe9bbec56ea579135cdd92409fff371841904f added support for the
> > > RGB and green PWM controlled LEDs on the HiFive Unmatched board
> > > managed by the leds-pwm-multicolor and leds-pwm drivers respectively.
> > > All three colours of the RGB LED and the green LED run from different
> > > lines of the same PWM, but with the same period so this works fine when
> > > the LED drivers are loaded one after the other.
> > >
> > > Unfortunately it does expose a race in the PWM driver when both LED
> > > drivers are loaded at roughly the same time. Here is an example:
> > >
> > >   |          Thread A           |          Thread B           |
> > >   |  led_pwm_mc_probe           |  led_pwm_probe              |
> > >   |    devm_fwnode_pwm_get      |                             |
> > >   |      pwm_sifive_request     |                             |
> > >   |        ddata->user_count++  |                             |
> > >   |                             |    devm_fwnode_pwm_get      |
> > >   |                             |      pwm_sifive_request     |
> > >   |                             |        ddata->user_count++  |
> > >   |         ...                 |          ...                |
> > >   |    pwm_state_apply          |    pwm_state_apply          |
> > >   |      pwm_sifive_apply       |      pwm_sifive_apply       |
> > >
> > > Now both calls to pwm_sifive_apply will see that ddata->approx_period,
> > > initially 0, is different from the requested period and the clock needs
> > > to be updated. But since ddata->user_count >= 2 both calls will fail
> > > with -EBUSY, which will then cause both LED drivers to fail to probe.
> > >
> > > Fix it by letting the first call to pwm_sifive_apply update the clock
> > > even when ddata->user_count != 1.
> > >
> > > Fixes: 9e37a53eb051 ("pwm: sifive: Add a driver for SiFive SoC PWM")
> > > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/pwm/pwm-sifive.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > index 2d4fa5e5fdd4..ccdf92045f34 100644
> > > --- a/drivers/pwm/pwm-sifive.c
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -159,7 +159,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >
> > >       mutex_lock(&ddata->lock);
> > >       if (state->period != ddata->approx_period) {
> > > -             if (ddata->user_count != 1) {
> > > +             if (ddata->user_count != 1 && ddata->approx_period) {
> >
> > IMHO this needs a code comment. It should among others mention that
> > approx_period is only zero if .apply() wasn't called before.
>
> Agreed. I'll add in v2.
>
> > Let me note this is inconsistent. I didn't check the details, but let's
> > assume the PWM can implement .period = 500 and .period = 514 and nothing
> > in between. So if the the first PWM requests 512 ns it gets (I hope) 500
> > ns. Then when the second requests comes in requesting 511 it fails and
> > if it requests 512 is succeeds also getting 500 ns. Hmm.
>
> Yes, if two different consumers wants different periods then whoever
> gets to take the mutex in pwm_sifive_apply first gets to set the clock
> for its requested period and the other consumer will get -EBUSY. I
> don't see how this lets one consumer call pwm_state_apply successfully
> but still get a different period though.

Sorry now I see what you mean. You're saying that if different
consumers want different periods, but they round to the same, then
that shouldn't fail, but now it does. I think that's a corner case I'd
happily live with.

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