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

  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(-)
  

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