| Message ID | 20221109113724.519021-1-emil.renner.berthing@canonical.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | [v2] pwm: sifive: Always let the first pwm_apply_state succeed | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/patch_count | success | Link |
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be fixes |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/build_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
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. Best regards Uwe
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. 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? /Emil
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. > 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
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/ |
On Wed, 16 Nov 2022 at 18:41, Emil Renner Berthing <emil.renner.berthing@canonical.com> wrote: > > 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. Oh, I think you might have looked at the pca9685_pwm_gpio_request function and not pca9685_pwm_request. > > > 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/ |
Hello, On Wed, Nov 16, 2022 at 06:41:06PM +0100, Emil Renner Berthing wrote: > 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: > > > 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 Yeah, didn't even try to compile it. > 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? I'd let Thierry decide what he want here. Depending on his choice I can clean up my suggestion (inclusive compile test) on the then relevant state of the driver. Best regards Uwe
On Mon, 28 Nov 2022 at 09:24, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello, > > On Wed, Nov 16, 2022 at 06:41:06PM +0100, Emil Renner Berthing wrote: > > 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: > > > > 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 > > Yeah, didn't even try to compile it. > > > 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? > > I'd let Thierry decide what he want here. Depending on his choice I can > clean up my suggestion (inclusive compile test) on the then relevant > state of the driver. @Thierry, it seems this is stuck on some opinion from you. /Emil > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
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(-) I've applied this as-is for now. What I'm wondering is if perhaps we want to implement something into the PWM core to deal with this, now fairly common, situation. Thierry
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) { mutex_unlock(&ddata->lock); return -EBUSY; }
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(-)