diff mbox series

[1/2] pwm: atmel-tcb: Fix race condition and convert to guards

Message ID 20240709101806.52394-3-u.kleine-koenig@baylibre.com (mailing list archive)
State New, archived
Headers show
Series [1/2] pwm: atmel-tcb: Fix race condition and convert to guards | expand

Commit Message

Uwe Kleine-König July 9, 2024, 10:18 a.m. UTC 
The hardware only supports a single period length for both PWM outputs. So
atmel_tcb_pwm_config() checks the configuration of the other output if it's
compatible with the currently requested setting. The register values are
then actually updated in atmel_tcb_pwm_enable(). To make this race free
the lock must be held during the whole process, so grab the lock in
.apply() instead of individually in atmel_tcb_pwm_disable() and
atmel_tcb_pwm_enable() which then also covers atmel_tcb_pwm_config().

To simplify handling, use the guard helper to let the compiler care for
unlocking. Otherwise unlocking would be more difficult as there is more
than one exit path in atmel_tcb_pwm_apply().

Fixes: 9421bade0765 ("pwm: atmel: add Timer Counter Block PWM driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-atmel-tcb.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)


base-commit: 120a528213b6693214e3cbc24a9c3052a4b1024b

Comments

Nicolas Ferre July 10, 2024, 2:17 p.m. UTC | #1 
On 09/07/2024 at 12:18, Uwe Kleine-König wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The hardware only supports a single period length for both PWM outputs. So
> atmel_tcb_pwm_config() checks the configuration of the other output if it's
> compatible with the currently requested setting. The register values are
> then actually updated in atmel_tcb_pwm_enable(). To make this race free
> the lock must be held during the whole process, so grab the lock in
> .apply() instead of individually in atmel_tcb_pwm_disable() and
> atmel_tcb_pwm_enable() which then also covers atmel_tcb_pwm_config().
> 
> To simplify handling, use the guard helper to let the compiler care for
> unlocking. Otherwise unlocking would be more difficult as there is more
> than one exit path in atmel_tcb_pwm_apply().
> 
> Fixes: 9421bade0765 ("pwm: atmel: add Timer Counter Block PWM driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

And I like the conversion to the "guard" lock helper.
Best regards,
   Nicolas

> ---
>   drivers/pwm/pwm-atmel-tcb.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index 528e54c5999d..aca11493239a 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -81,7 +81,8 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
>          tcbpwm->period = 0;
>          tcbpwm->div = 0;
> 
> -       spin_lock(&tcbpwmc->lock);
> +       guard(spinlock)(&tcbpwmc->lock);
> +
>          regmap_read(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), &cmr);
>          /*
>           * Get init config from Timer Counter registers if
> @@ -107,7 +108,6 @@ static int atmel_tcb_pwm_request(struct pwm_chip *chip,
> 
>          cmr |= ATMEL_TC_WAVE | ATMEL_TC_WAVESEL_UP_AUTO | ATMEL_TC_EEVT_XC0;
>          regmap_write(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), cmr);
> -       spin_unlock(&tcbpwmc->lock);
> 
>          return 0;
>   }
> @@ -137,7 +137,6 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
>          if (tcbpwm->duty == 0)
>                  polarity = !polarity;
> 
> -       spin_lock(&tcbpwmc->lock);
>          regmap_read(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), &cmr);
> 
>          /* flush old setting and set the new one */
> @@ -172,8 +171,6 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
>                               ATMEL_TC_SWTRG);
>                  tcbpwmc->bkup.enabled = 0;
>          }
> -
> -       spin_unlock(&tcbpwmc->lock);
>   }
> 
>   static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -194,7 +191,6 @@ static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
>          if (tcbpwm->duty == 0)
>                  polarity = !polarity;
> 
> -       spin_lock(&tcbpwmc->lock);
>          regmap_read(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), &cmr);
> 
>          /* flush old setting and set the new one */
> @@ -256,7 +252,6 @@ static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
>          regmap_write(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CCR),
>                       ATMEL_TC_SWTRG | ATMEL_TC_CLKEN);
>          tcbpwmc->bkup.enabled = 1;
> -       spin_unlock(&tcbpwmc->lock);
>          return 0;
>   }
> 
> @@ -341,9 +336,12 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>                                 const struct pwm_state *state)
>   {
> +       struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
>          int duty_cycle, period;
>          int ret;
> 
> +       guard(spinlock)(&tcbpwmc->lock);
> +
>          if (!state->enabled) {
>                  atmel_tcb_pwm_disable(chip, pwm, state->polarity);
>                  return 0;
> 
> base-commit: 120a528213b6693214e3cbc24a9c3052a4b1024b
> --
> 2.43.0
>
Uwe Kleine-König July 10, 2024, 3:31 p.m. UTC | #2 
Hello Nicolas,

On Wed, Jul 10, 2024 at 04:17:09PM +0200, Nicolas Ferre wrote:
> On 09/07/2024 at 12:18, Uwe Kleine-König wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > The hardware only supports a single period length for both PWM outputs. So
> > atmel_tcb_pwm_config() checks the configuration of the other output if it's
> > compatible with the currently requested setting. The register values are
> > then actually updated in atmel_tcb_pwm_enable(). To make this race free
> > the lock must be held during the whole process, so grab the lock in
> > .apply() instead of individually in atmel_tcb_pwm_disable() and
> > atmel_tcb_pwm_enable() which then also covers atmel_tcb_pwm_config().
> > 
> > To simplify handling, use the guard helper to let the compiler care for
> > unlocking. Otherwise unlocking would be more difficult as there is more
> > than one exit path in atmel_tcb_pwm_apply().
> > 
> > Fixes: 9421bade0765 ("pwm: atmel: add Timer Counter Block PWM driver")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> And I like the conversion to the "guard" lock helper.

I hesitated a bit to add it because it will make backporting to stable
harder. But I guess we will just not backport it, the problem doesn't
seem to matter in practise given that it was found by looking at code
and not hit in real life more more than 11 years after its introduction.

Best regards and thanks for your Acks,
Uwe
Nicolas Ferre July 10, 2024, 4:13 p.m. UTC | #3 
On 10/07/2024 at 17:31, Uwe Kleine-König wrote:
> Hello Nicolas,
> 
> On Wed, Jul 10, 2024 at 04:17:09PM +0200, Nicolas Ferre wrote:
>> On 09/07/2024 at 12:18, Uwe Kleine-König wrote:
>>> The hardware only supports a single period length for both PWM outputs. So
>>> atmel_tcb_pwm_config() checks the configuration of the other output if it's
>>> compatible with the currently requested setting. The register values are
>>> then actually updated in atmel_tcb_pwm_enable(). To make this race free
>>> the lock must be held during the whole process, so grab the lock in
>>> .apply() instead of individually in atmel_tcb_pwm_disable() and
>>> atmel_tcb_pwm_enable() which then also covers atmel_tcb_pwm_config().
>>>
>>> To simplify handling, use the guard helper to let the compiler care for
>>> unlocking. Otherwise unlocking would be more difficult as there is more
>>> than one exit path in atmel_tcb_pwm_apply().
>>>
>>> Fixes: 9421bade0765 ("pwm: atmel: add Timer Counter Block PWM driver")
>>> Signed-off-by: Uwe Kleine-König<u.kleine-koenig@baylibre.com>
>> Acked-by: Nicolas Ferre<nicolas.ferre@microchip.com>
>>
>> And I like the conversion to the "guard" lock helper.
> I hesitated a bit to add it because it will make backporting to stable
> harder. But I guess we will just not backport it, the problem doesn't
> seem to matter in practise given that it was found by looking at code
> and not hit in real life more more than 11 years after its introduction.

Fair indeed: +1.

> Best regards and thanks for your Acks,
> Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 528e54c5999d..aca11493239a 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -81,7 +81,8 @@  static int atmel_tcb_pwm_request(struct pwm_chip *chip,
 	tcbpwm->period = 0;
 	tcbpwm->div = 0;
 
-	spin_lock(&tcbpwmc->lock);
+	guard(spinlock)(&tcbpwmc->lock);
+
 	regmap_read(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), &cmr);
 	/*
 	 * Get init config from Timer Counter registers if
@@ -107,7 +108,6 @@  static int atmel_tcb_pwm_request(struct pwm_chip *chip,
 
 	cmr |= ATMEL_TC_WAVE | ATMEL_TC_WAVESEL_UP_AUTO | ATMEL_TC_EEVT_XC0;
 	regmap_write(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), cmr);
-	spin_unlock(&tcbpwmc->lock);
 
 	return 0;
 }
@@ -137,7 +137,6 @@  static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (tcbpwm->duty == 0)
 		polarity = !polarity;
 
-	spin_lock(&tcbpwmc->lock);
 	regmap_read(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), &cmr);
 
 	/* flush old setting and set the new one */
@@ -172,8 +171,6 @@  static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
 			     ATMEL_TC_SWTRG);
 		tcbpwmc->bkup.enabled = 0;
 	}
-
-	spin_unlock(&tcbpwmc->lock);
 }
 
 static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -194,7 +191,6 @@  static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (tcbpwm->duty == 0)
 		polarity = !polarity;
 
-	spin_lock(&tcbpwmc->lock);
 	regmap_read(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CMR), &cmr);
 
 	/* flush old setting and set the new one */
@@ -256,7 +252,6 @@  static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
 	regmap_write(tcbpwmc->regmap, ATMEL_TC_REG(tcbpwmc->channel, CCR),
 		     ATMEL_TC_SWTRG | ATMEL_TC_CLKEN);
 	tcbpwmc->bkup.enabled = 1;
-	spin_unlock(&tcbpwmc->lock);
 	return 0;
 }
 
@@ -341,9 +336,12 @@  static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			       const struct pwm_state *state)
 {
+	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
 	int duty_cycle, period;
 	int ret;
 
+	guard(spinlock)(&tcbpwmc->lock);
+
 	if (!state->enabled) {
 		atmel_tcb_pwm_disable(chip, pwm, state->polarity);
 		return 0;