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

[Emil Renner Berthing]

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.

/Emil

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