Messages in this thread Patch in this message | | | From | Emil Renner Berthing <> | Date | Wed, 16 Nov 2022 18:41:06 +0100 | Subject | Re: [PATCH v2] pwm: sifive: Always let the first pwm_apply_state succeed |
| |
On Wed, 9 Nov 2022 at 16:33, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, Nov 09, 2022 at 01:45:43PM +0100, Emil Renner Berthing wrote: > > On Wed, 9 Nov 2022 at 13:01, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > Hello Emil, > > > > > > On Wed, Nov 09, 2022 at 12:37:24PM +0100, 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@canonical.com> > > > > --- > > > > drivers/pwm/pwm-sifive.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > > > index 2d4fa5e5fdd4..b3c60ec72a6e 100644 > > > > --- a/drivers/pwm/pwm-sifive.c > > > > +++ b/drivers/pwm/pwm-sifive.c > > > > @@ -159,7 +159,13 @@ 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) { > > > > + /* > > > > + * Don't let a 2nd user change the period underneath the 1st user. > > > > + * However if ddate->approx_period == 0 this is the first time we set > > > > + * any period, so let whoever gets here first set the period so other > > > > + * users who agree on the period won't fail. > > > > + */ > > > > + if (ddata->user_count != 1 && ddata->approx_period) { > > > > > > While I'm convinced this works, we'd get some more uniform behaviour > > > compared to other hardwares with similar restrictions if you lock the > > > period on enabling the PWM instead of at request time. See for example > > > drivers/pwm/pwm-pca9685.c. > > > > Hmm.. that driver uses a pwms_enabled bitmap rather than a user count, > > but it still sets the bit in the request method and refuses to change > > period in the apply method if more than 1 bit is set. > > Note there are two different bitmaps. The one modified in .request is > for gpio stuff and the other in .apply() for locking the common period > length.
Yeah, there is the pwms_enabled and pwms_inuse bitmaps, but pwms_enabled is used both in .request and .apply.
> > So as far as I > > can tell it still suffers from the same race. However using a bitmap > > instead of a user count would let us handle everything in the apply > > method if we don't set the bit in the request method, but then the > > behaviour would still be different. In any case it would still be a > > large change to this driver. > > > > How about we merge this bug fix that can easily be backported first > > and then look at how it should be handled properly? > > I thought it wouldn't be that hard to do it right from the start, > but I admit it's harder than I expected to get right. My prototype looks > as follows:
This works for me (modulo the two extra {'s). I'd still prefer merging the simpler version and then this on top for ease of backporting, but as long as the race is fixed I'm fine. Will you send a cleaned up version of this?
/Emil
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > index 2d4fa5e5fdd4..89846d95bfc0 100644 > --- a/drivers/pwm/pwm-sifive.c > +++ b/drivers/pwm/pwm-sifive.c > @@ -41,13 +41,13 @@ > > struct pwm_sifive_ddata { > struct pwm_chip chip; > - struct mutex lock; /* lock to protect user_count and approx_period */ > + struct mutex lock; /* lock to protect approx_period */ > struct notifier_block notifier; > struct clk *clk; > void __iomem *regs; > unsigned int real_period; > unsigned int approx_period; > - int user_count; > + DECLARE_BITMAP(pwms_enabled, 4); > }; > > static inline > @@ -59,10 +59,16 @@ struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c) > static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip); > + u32 val = readl(ddata->regs + PWM_SIFIVE_PWMCFG); > > - mutex_lock(&ddata->lock); > - ddata->user_count++; > - mutex_unlock(&ddata->lock); > + if (val & PWM_SIFIVE_PWMCFG_EN_ALWAYS) { > + val = readl(ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm)); > + if (val > 0) { > + mutex_lock(&ddata->lock); > + __set_bit(pwm->hwpwm, ddata->pwms_enabled); > + mutex_unlock(&ddata->lock); > + } > + } > > return 0; > } > @@ -72,7 +78,7 @@ static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *pwm) > struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip); > > mutex_lock(&ddata->lock); > - ddata->user_count--; > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled); > mutex_unlock(&ddata->lock); > } > > @@ -158,11 +164,18 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > mutex_lock(&ddata->lock); > + > + if (state->enabled) { > + __set_bit(pwm->hwpwm, ddata->pwms_enabled); > + > if (state->period != ddata->approx_period) { > - if (ddata->user_count != 1) { > + if (bitmap_weight(ddata->pwms_enabled, 4) > 1) { > + if (!enabled) { > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled); > mutex_unlock(&ddata->lock); > return -EBUSY; > } > + > ddata->approx_period = state->period; > pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk)); > } > @@ -177,14 +190,23 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > ret = clk_enable(ddata->clk); > if (ret) { > dev_err(ddata->chip.dev, "Enable clk failed\n"); > + if (state->enabled) { > + mutex_lock(&ddata->lock); > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled); > + mutex_unlock(&ddata->lock); > + } > return ret; > } > } > > writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm)); > > - if (!state->enabled) > + if (!state->enabled) { > + mutex_lock(&ddata->lock); > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled); > + mutex_unlock(&ddata->lock); > clk_disable(ddata->clk); > + } > > return 0; > } > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
| |