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

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

[Uwe Kleine-König]

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

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

[PATCH 2/2] pwm: atmel-tcb: Simplify checking the companion output

[Uwe Kleine-König]

The two outputs provided by the supported hardware share some settings,
so access to the other PWM is required when one of them is configured.

Instead of an explicit if to deterimine the other PWM just use
hwpwm ^ 1. Further atcbpwm is never NULL, so drop the corresponding
check.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-atmel-tcb.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index aca11493239a..2feee3744b50 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -260,7 +260,8 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
 	struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
-	struct atmel_tcb_pwm_device *atcbpwm = NULL;
+	/* companion PWM sharing register values period and div */
+	struct atmel_tcb_pwm_device *atcbpwm = &tcbpwmc->pwms[pwm->hwpwm ^ 1];
 	int i = 0;
 	int slowclk = 0;
 	unsigned period;
@@ -305,11 +306,6 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	duty = div_u64(duty_ns, min);
 	period = div_u64(period_ns, min);
 
-	if (pwm->hwpwm == 0)
-		atcbpwm = &tcbpwmc->pwms[1];
-	else
-		atcbpwm = &tcbpwmc->pwms[0];
-
 	/*
 	 * PWM devices provided by the TCB driver are grouped by 2.
 	 * PWM devices in a given group must be configured with the
@@ -318,8 +314,7 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * We're checking the period value of the second PWM device
 	 * in this group before applying the new config.
 	 */
-	if ((atcbpwm && atcbpwm->duty > 0 &&
-			atcbpwm->duty != atcbpwm->period) &&
+	if ((atcbpwm->duty > 0 && atcbpwm->duty != atcbpwm->period) &&
 		(atcbpwm->div != i || atcbpwm->period != period)) {
 		dev_err(pwmchip_parent(chip),
 			"failed to configure period_ns: PWM group already configured with a different value\n");
-- 
2.43.0

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

[Nicolas Ferre]

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
> 

Re: [PATCH 2/2] pwm: atmel-tcb: Simplify checking the companion output

[Nicolas Ferre]

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 two outputs provided by the supported hardware share some settings,
> so access to the other PWM is required when one of them is configured.
> 
> Instead of an explicit if to deterimine the other PWM just use
> hwpwm ^ 1. Further atcbpwm is never NULL, so drop the corresponding
> check.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Thanks. Best regards,
   Nicolas

> ---
>   drivers/pwm/pwm-atmel-tcb.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index aca11493239a..2feee3744b50 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -260,7 +260,8 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   {
>          struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
>          struct atmel_tcb_pwm_device *tcbpwm = &tcbpwmc->pwms[pwm->hwpwm];
> -       struct atmel_tcb_pwm_device *atcbpwm = NULL;
> +       /* companion PWM sharing register values period and div */
> +       struct atmel_tcb_pwm_device *atcbpwm = &tcbpwmc->pwms[pwm->hwpwm ^ 1];
>          int i = 0;
>          int slowclk = 0;
>          unsigned period;
> @@ -305,11 +306,6 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>          duty = div_u64(duty_ns, min);
>          period = div_u64(period_ns, min);
> 
> -       if (pwm->hwpwm == 0)
> -               atcbpwm = &tcbpwmc->pwms[1];
> -       else
> -               atcbpwm = &tcbpwmc->pwms[0];
> -
>          /*
>           * PWM devices provided by the TCB driver are grouped by 2.
>           * PWM devices in a given group must be configured with the
> @@ -318,8 +314,7 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>           * We're checking the period value of the second PWM device
>           * in this group before applying the new config.
>           */
> -       if ((atcbpwm && atcbpwm->duty > 0 &&
> -                       atcbpwm->duty != atcbpwm->period) &&
> +       if ((atcbpwm->duty > 0 && atcbpwm->duty != atcbpwm->period) &&
>                  (atcbpwm->div != i || atcbpwm->period != period)) {
>                  dev_err(pwmchip_parent(chip),
>                          "failed to configure period_ns: PWM group already configured with a different value\n");
> --
> 2.43.0
> 

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] pwm: atmel-tcb: Simplify checking the companion output

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Wed, Jul 10, 2024 at 04:17:47PM +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 two outputs provided by the supported hardware share some settings,
> > so access to the other PWM is required when one of them is configured.
> > 
> > Instead of an explicit if to deterimine the other PWM just use
> > hwpwm ^ 1. Further atcbpwm is never NULL, so drop the corresponding
> > check.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Thanks, I applied both patches with your Ack to my pwm/for-next branch.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Nicolas Ferre]

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