diff mbox series

[v2] pwm: sifive: Always let the first pwm_apply_state succeed

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

Checks

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

Commit Message

Emil Renner Berthing Nov. 9, 2022, 11:37 a.m. UTC
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(-)

Comments

Uwe Kleine-König Nov. 9, 2022, 12:01 p.m. UTC | #1
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
Emil Renner Berthing Nov. 9, 2022, 12:45 p.m. UTC | #2
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
Uwe Kleine-König Nov. 9, 2022, 3:33 p.m. UTC | #3
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
Emil Renner Berthing Nov. 16, 2022, 5:41 p.m. UTC | #4
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/ |
Emil Renner Berthing Nov. 16, 2022, 5:46 p.m. UTC | #5
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/ |
Uwe Kleine-König Nov. 28, 2022, 8:24 a.m. UTC | #6
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
Emil Renner Berthing Dec. 5, 2022, 1:49 p.m. UTC | #7
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/ |
Thierry Reding Jan. 30, 2023, 3:36 p.m. UTC | #8
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 mbox series

Patch

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;
 		}