[PATCH v2] pwm: sifive: Always let the first pwm_apply_state succeed
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Wed Nov 9 07:33:11 PST 2022
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 at 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 at 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.
> 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:
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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20221109/b73886fa/attachment.sig>
More information about the linux-riscv
mailing list