diff mbox series

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

Message ID 20221018091316.415685-1-emil.renner.berthing@canonical.com
State New
Headers
Series [v1] pwm: sifive: Always let the first pwm_apply_state succeed |

Commit Message

Emil Renner Berthing Oct. 18, 2022, 9:13 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Uwe Kleine-König Oct. 18, 2022, 1:29 p.m. UTC | #1 
Hello,

On Tue, Oct 18, 2022 at 11:13:16AM +0200, 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> index 2d4fa5e5fdd4..ccdf92045f34 100644
> --- a/drivers/pwm/pwm-sifive.c
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -159,7 +159,7 @@ 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) {
> +		if (ddata->user_count != 1 && ddata->approx_period) {

IMHO this needs a code comment. It should among others mention that
approx_period is only zero if .apply() wasn't called before.

Let me note this is inconsistent. I didn't check the details, but let's
assume the PWM can implement .period = 500 and .period = 514 and nothing
in between. So if the the first PWM requests 512 ns it gets (I hope) 500
ns. Then when the second requests comes in requesting 511 it fails and
if it requests 512 is succeeds also getting 500 ns. Hmm.

Best regards
Uwe
Emil Renner Berthing Oct. 18, 2022, 2:56 p.m. UTC | #2 
On Tue, 18 Oct 2022 at 15:29, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Tue, Oct 18, 2022 at 11:13:16AM +0200, 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > index 2d4fa5e5fdd4..ccdf92045f34 100644
> > --- a/drivers/pwm/pwm-sifive.c
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -159,7 +159,7 @@ 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) {
> > +             if (ddata->user_count != 1 && ddata->approx_period) {
>
> IMHO this needs a code comment. It should among others mention that
> approx_period is only zero if .apply() wasn't called before.

Agreed. I'll add in v2.

> Let me note this is inconsistent. I didn't check the details, but let's
> assume the PWM can implement .period = 500 and .period = 514 and nothing
> in between. So if the the first PWM requests 512 ns it gets (I hope) 500
> ns. Then when the second requests comes in requesting 511 it fails and
> if it requests 512 is succeeds also getting 500 ns. Hmm.

Yes, if two different consumers wants different periods then whoever
gets to take the mutex in pwm_sifive_apply first gets to set the clock
for its requested period and the other consumer will get -EBUSY. I
don't see how this lets one consumer call pwm_state_apply successfully
but still get a different period though.

/Emil

> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Emil Renner Berthing Oct. 18, 2022, 3:01 p.m. UTC | #3 
On Tue, 18 Oct 2022 at 16:56, Emil Renner Berthing
<emil.renner.berthing@canonical.com> wrote:
>
> On Tue, 18 Oct 2022 at 15:29, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Tue, Oct 18, 2022 at 11:13:16AM +0200, 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 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > index 2d4fa5e5fdd4..ccdf92045f34 100644
> > > --- a/drivers/pwm/pwm-sifive.c
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -159,7 +159,7 @@ 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) {
> > > +             if (ddata->user_count != 1 && ddata->approx_period) {
> >
> > IMHO this needs a code comment. It should among others mention that
> > approx_period is only zero if .apply() wasn't called before.
>
> Agreed. I'll add in v2.
>
> > Let me note this is inconsistent. I didn't check the details, but let's
> > assume the PWM can implement .period = 500 and .period = 514 and nothing
> > in between. So if the the first PWM requests 512 ns it gets (I hope) 500
> > ns. Then when the second requests comes in requesting 511 it fails and
> > if it requests 512 is succeeds also getting 500 ns. Hmm.
>
> Yes, if two different consumers wants different periods then whoever
> gets to take the mutex in pwm_sifive_apply first gets to set the clock
> for its requested period and the other consumer will get -EBUSY. I
> don't see how this lets one consumer call pwm_state_apply successfully
> but still get a different period though.

Sorry now I see what you mean. You're saying that if different
consumers want different periods, but they round to the same, then
that shouldn't fail, but now it does. I think that's a corner case I'd
happily live with.

> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K.                           | Uwe Kleine-König            |
> > Industrial Linux Solutions                 | https://www.pengutronix.de/ |
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index 2d4fa5e5fdd4..ccdf92045f34 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -159,7 +159,7 @@  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) {
+		if (ddata->user_count != 1 && ddata->approx_period) {
 			mutex_unlock(&ddata->lock);
 			return -EBUSY;
 		}