[PATCH v2 0/3] pwm: rockchip: Eliminate potential race condition when probing

[PATCH v2 0/3] pwm: rockchip: Eliminate potential race condition when probing

[Simon South]

This patch series aims to eliminate the race condition Trent Piepho
identified[0] in the Rockchip PWM driver's rockchip_pwm_probe()
function, by moving code that disables a PWM device's signal clock
ahead of the code that registers the device via pwmchip_add().

It additionally

- Fixes a potential kernel hang introduced by my earlier commit
  457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while
  probing") by ensuring a device's APB clock is enabled before its
  registers are accessed, and

- Tries to improve the driver by (re-)enabling the signal clock of
  only PWM devices that appear to have been started already by the
  bootloader, rather than enabling every device's signal clock and
  selectively disabling it later.

I've tested these changes on my (RK3399-based) Pinebook Pro with its
screen backlight enabled by U-Boot and they appear to work fine.

I'd be grateful for help with testing on other devices, particularly
those with SoCs like the RK3328 that use separate bus and signal
clocks for their PWM devices. (My ROCK64 uses its PWM-output pins for
other purposes and wasn't of help here.)

[0] https://www.spinics.net/lists/linux-pwm/msg14611.html

--
Simon South
simon@simonsouth.net


Simon South (3):
  pwm: rockchip: Enable APB clock during register access while probing
  pwm: rockchip: Eliminate potential race condition when probing
  pwm: rockchip: Do not start PWMs not already running

 drivers/pwm/pwm-rockchip.c | 49 +++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

-- 
2.29.2

[PATCH v2 1/3] pwm: rockchip: Enable APB clock during register access while probing

[Simon South]

Commit 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while
probing") modified rockchip_pwm_probe() to access a PWM device's registers
directly to check whether or not the device is running, but did not also
change the function to first enable the device's APB clock to be certain
the device can respond. This risks hanging the kernel on systems with PWM
devices that use more than a single clock.

Avoid this by enabling the device's APB clock before accessing its
registers (and disabling the clock when register access is complete).

Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")
Reported-by: Thierry Reding <thierry.reding@gmail.com>
Suggested-by: Trent Piepho <tpiepho@gmail.com>
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 77c23a2c6d71..d2058138ce1e 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -332,9 +332,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = clk_prepare(pc->pclk);
+	ret = clk_prepare_enable(pc->pclk);
 	if (ret) {
-		dev_err(&pdev->dev, "Can't prepare APB clk: %d\n", ret);
+		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
 		goto err_clk;
 	}
 
@@ -364,10 +364,12 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if ((ctrl & enable_conf) != enable_conf)
 		clk_disable(pc->clk);
 
+	clk_disable(pc->pclk);
+
 	return 0;
 
 err_pclk:
-	clk_unprepare(pc->pclk);
+	clk_disable_unprepare(pc->pclk);
 err_clk:
 	clk_disable_unprepare(pc->clk);
 
-- 
2.29.2

[PATCH v2 2/3] pwm: rockchip: Eliminate potential race condition when probing

[Simon South]

Commit 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running
PWMs") introduced a potential race condition in rockchip_pwm_probe() as the
function could disable the clock of a PWM device after having registered it
through a call to pwmchip_add().

Eliminate this possibility by moving the code that disables the clock of a
non-enabled PWM ahead of the code that registers the device.

Also refactor the code slightly to eliminate goto targets as the error
handlers no longer share any recovery steps.

Fixes: 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs")
Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")
Reported-by: Trent Piepho <tpiepho@gmail.com>
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index d2058138ce1e..f286a498b82c 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -289,6 +289,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	struct rockchip_pwm_chip *pc;
 	struct resource *r;
 	u32 enable_conf, ctrl;
+	bool enabled;
 	int ret, count;
 
 	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
@@ -335,7 +336,8 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(pc->pclk);
 	if (ret) {
 		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
-		goto err_clk;
+		clk_disable_unprepare(pc->clk);
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, pc);
@@ -351,29 +353,26 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		pc->chip.of_pwm_n_cells = 3;
 	}
 
-	ret = pwmchip_add(&pc->chip);
-	if (ret < 0) {
-		clk_unprepare(pc->clk);
-		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
-		goto err_pclk;
-	}
-
 	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
 	enable_conf = pc->data->enable_conf;
 	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
-	if ((ctrl & enable_conf) != enable_conf)
+	enabled = ((ctrl & enable_conf) == enable_conf);
+	if (!enabled)
 		clk_disable(pc->clk);
 
 	clk_disable(pc->pclk);
 
-	return 0;
-
-err_pclk:
-	clk_disable_unprepare(pc->pclk);
-err_clk:
-	clk_disable_unprepare(pc->clk);
+	ret = pwmchip_add(&pc->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		if (enabled)
+			clk_disable(pc->clk);
+		clk_unprepare(pc->clk);
+		clk_unprepare(pc->pclk);
+		return ret;
+	}
 
-	return ret;
+	return 0;
 }
 
 static int rockchip_pwm_remove(struct platform_device *pdev)
-- 
2.29.2

[PATCH v2 3/3] pwm: rockchip: Do not start PWMs not already running

[Simon South]

Currently the Rockchip PWM driver enables the signal ("bus") clock for
every PWM device it finds during probing, then disables it for any device
that was not already enabled (such as by a bootloader) when the kernel
started.

Instead of starting PWMs unnecessarily, check first to see whether a device
has already been enabled and if not, do not enable its signal clock.

Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index f286a498b82c..b9faef3e9954 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pc->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(pc->pclk);
-	if (ret) {
-		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
-		clk_disable_unprepare(pc->clk);
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, pc);
 
 	pc->data = id->data;
@@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		pc->chip.of_pwm_n_cells = 3;
 	}
 
+	ret = clk_prepare_enable(pc->pclk);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
+		return ret;
+	}
+
 	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
 	enable_conf = pc->data->enable_conf;
 	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
 	enabled = ((ctrl & enable_conf) == enable_conf);
-	if (!enabled)
-		clk_disable(pc->clk);
+
+	ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
+		clk_disable_unprepare(pc->pclk);
+		return ret;
+	}
 
 	clk_disable(pc->pclk);
 
-- 
2.29.2

Re: [PATCH v2 1/3] pwm: rockchip: Enable APB clock during register access while probing

[Uwe Kleine-König]

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

Hello,

On Sat, Dec 19, 2020 at 03:44:08PM -0500, Simon South wrote:
> Commit 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while
> probing") modified rockchip_pwm_probe() to access a PWM device's registers
> directly to check whether or not the device is running, but did not also
> change the function to first enable the device's APB clock to be certain
> the device can respond. This risks hanging the kernel on systems with PWM
> devices that use more than a single clock.
> 
> Avoid this by enabling the device's APB clock before accessing its
> registers (and disabling the clock when register access is complete).
> 
> Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")
> Reported-by: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Trent Piepho <tpiepho@gmail.com>
> Signed-off-by: Simon South <simon@simonsouth.net>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 2/3] pwm: rockchip: Eliminate potential race condition when probing

[Uwe Kleine-König]

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

Hello,

On Sat, Dec 19, 2020 at 03:44:09PM -0500, Simon South wrote:
> Commit 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running
> PWMs") introduced a potential race condition in rockchip_pwm_probe() as the
> function could disable the clock of a PWM device after having registered it
> through a call to pwmchip_add().
> 
> Eliminate this possibility by moving the code that disables the clock of a
> non-enabled PWM ahead of the code that registers the device.

OK, I think I understood the problem. The code in the probe function
looks as follows (simplified):

	pwmchip_add(...);

	if (pwm_is_not_running):
		clk_disable(pc->clk);

The intention is that if probe is called when the PWM is already running
it should not stop (which is good).

However calling pwmchip_add allows for consumers to modify the state and
the check after pwmchip_add then checks the modified state. The result
(if the race happens) is that either the clk is enabled once too much
(if the consumer enabled the PWM) or it disables the clk twice after
enabling only once (if the consumer stopped the running hardware).

So the effect is that either the clk isn't stopped even though it is
unused, or we might hit:

	if (WARN(core->enable_count == 0, "%s already disabled\n", core->name))
		return;

in drivers/clk/clk.c, right?

I wonder if the commit log should be more detailed about this, after
reading it I thought the effect of the bug would be that the PWM stops
even though it should oscillate.

> Also refactor the code slightly to eliminate goto targets as the error
> handlers no longer share any recovery steps.

This however makes it hard to review the patch. Maybe this refactoring
can be split out?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 3/3] pwm: rockchip: Do not start PWMs not already running

[Uwe Kleine-König]

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

On Sat, Dec 19, 2020 at 03:44:10PM -0500, Simon South wrote:
> Currently the Rockchip PWM driver enables the signal ("bus") clock for
> every PWM device it finds during probing, then disables it for any device
> that was not already enabled (such as by a bootloader) when the kernel
> started.
> 
> Instead of starting PWMs unnecessarily, check first to see whether a device

"starting PWM" here means enabling their clocks, right? I wouldn't
expect that this has any effect on the output, am I right?

> has already been enabled and if not, do not enable its signal clock.
> 
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index f286a498b82c..b9faef3e9954 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(pc->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(pc->pclk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> -		clk_disable_unprepare(pc->clk);
> -		return ret;
> -	}

Just for my understanding: That you moved clk_prepare_enable(pc->pclk)
further down is not strictly necessary for your change, right?

> -
>  	platform_set_drvdata(pdev, pc);
>  
>  	pc->data = id->data;
> @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		pc->chip.of_pwm_n_cells = 3;
>  	}
>  
> +	ret = clk_prepare_enable(pc->pclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> +		return ret;
> +	}
> +
>  	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
>  	enable_conf = pc->data->enable_conf;
>  	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
>  	enabled = ((ctrl & enable_conf) == enable_conf);
> -	if (!enabled)
> -		clk_disable(pc->clk);
> +
> +	ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
> +		clk_disable_unprepare(pc->pclk);
> +		return ret;
> +	}

I'm not a big fan of this ?: construct. I'd prefer

	ret = clk_prepare(pc->clk);
	if (ret)
		...

	/* Keep the PWM clk enabled ... */
	enabled = ...
	if (enabled) {
		ret = clk_enable(pc->clk);
		if (ret)
			...
	}

even though it is less compact. A small benefit is that the error
message can be more accurate. (You wrote "Can't prepare bus clk" while
the problem might well be in the enable part, but mentioning "enable"
might also be misleading for the enabled = false case.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 0/3] pwm: rockchip: Eliminate potential race condition when probing

[Uwe Kleine-König]

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

Hello Simon,

On Sat, Dec 19, 2020 at 03:44:07PM -0500, Simon South wrote:
> This patch series aims to eliminate the race condition Trent Piepho
> identified[0] in the Rockchip PWM driver's rockchip_pwm_probe()
> function, by moving code that disables a PWM device's signal clock
> ahead of the code that registers the device via pwmchip_add().
> 
> It additionally
> 
> - Fixes a potential kernel hang introduced by my earlier commit
>   457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while
>   probing") by ensuring a device's APB clock is enabled before its
>   registers are accessed, and
> 
> - Tries to improve the driver by (re-)enabling the signal clock of
>   only PWM devices that appear to have been started already by the
>   bootloader, rather than enabling every device's signal clock and
>   selectively disabling it later.
> 
> I've tested these changes on my (RK3399-based) Pinebook Pro with its
> screen backlight enabled by U-Boot and they appear to work fine.
> 
> I'd be grateful for help with testing on other devices, particularly
> those with SoCs like the RK3328 that use separate bus and signal
> clocks for their PWM devices. (My ROCK64 uses its PWM-output pins for
> other purposes and wasn't of help here.)

While looking through your I found another little problem that you might
want to address: rockchip_pwm_get_state() calls clk_get_rate(pc->clk).
According to the documentation (in include/linux/clk.h) this is only
valid for enabled clocks but there are no precautions that pc->clk is
enabled.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 2/3] pwm: rockchip: Eliminate potential race condition when probing

[Simon South]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> I wonder if the commit log should be more detailed about this, after
> reading it I thought the effect of the bug would be that the PWM stops
> even though it should oscillate.

Your understanding is correct; this was the result of confusion on my
part. I'll write a new commit message.

>> Also refactor the code slightly to eliminate goto targets as the
>> error handlers no longer share any recovery steps.
>
> This however makes it hard to review the patch. Maybe this refactoring
> can be split out?

I'll give this another shot, too.

-- 
Simon South
simon@simonsouth.net

Re: [PATCH v2 3/3] pwm: rockchip: Do not start PWMs not already running

[Simon South]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> "starting PWM" here means enabling their clocks, right? I wouldn't
> expect that this has any effect on the output, am I right?

Right, it does not; another misunderstanding on my part. I'll fix the
commit message.

> Just for my understanding: That you moved clk_prepare_enable(pc->pclk)
> further down is not strictly necessary for your change, right?

That's right. My thought was to keep the code that enables this clock
close to the code that relies on it, and to minimize the duration the
clock is enabled.

Would it be better to leave the code where it is?

> I'm not a big fan of this ?: construct...

Neither was I, actually, but I guessed the shorter version would be
preferred. I'm happy to change it back to match what you described.

-- 
Simon South
simon@simonsouth.net

Re: [PATCH v2 0/3] pwm: rockchip: Eliminate potential race condition when probing

[Simon South]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> While looking through your I found another little problem that you might
> want to address: rockchip_pwm_get_state() calls clk_get_rate(pc->clk).
> According to the documentation (in include/linux/clk.h) this is only
> valid for enabled clocks but there are no precautions that pc->clk is
> enabled.

Sure.

Thanks very much for reviewing all this, Uwe. I'll follow up shortly
with a new patch series.

-- 
Simon South
simon@simonsouth.net

Re: [PATCH v2 3/3] pwm: rockchip: Do not start PWMs not already running

[Robin Murphy]

On 2020-12-19 20:44, Simon South wrote:
> Currently the Rockchip PWM driver enables the signal ("bus") clock for
> every PWM device it finds during probing, then disables it for any device
> that was not already enabled (such as by a bootloader) when the kernel
> started.
> 
> Instead of starting PWMs unnecessarily, check first to see whether a device
> has already been enabled and if not, do not enable its signal clock.
> 
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>   drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index f286a498b82c..b9faef3e9954 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	ret = clk_prepare_enable(pc->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(pc->pclk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> -		clk_disable_unprepare(pc->clk);
> -		return ret;
> -	}
> -
>   	platform_set_drvdata(pdev, pc);
>   
>   	pc->data = id->data;
> @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>   		pc->chip.of_pwm_n_cells = 3;
>   	}
>   
> +	ret = clk_prepare_enable(pc->pclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> +		return ret;
> +	}
> +
>   	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
>   	enable_conf = pc->data->enable_conf;
>   	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
>   	enabled = ((ctrl & enable_conf) == enable_conf);
> -	if (!enabled)
> -		clk_disable(pc->clk);
> +
> +	ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);

Since you're touching it, I guess it might be a good idea to update this 
message to say "PWM clk" for clarity.

I suspect there might also have been some historical confounding in the 
fact that when there is only one clock (pclk_pwm), it's merely a gate 
whose parent is pclk_bus, which serves all the peripherals in the 
usefully-named PD_BUS domain...

Robin.

> +		clk_disable_unprepare(pc->pclk);
> +		return ret;
> +	}
>   
>   	clk_disable(pc->pclk);
>   
> 

Re: [PATCH v2 3/3] pwm: rockchip: Do not start PWMs not already running

[Simon South]

Robin Murphy <robin.murphy@arm.com> writes:
> Since you're touching it, I guess it might be a good idea to update
> this message to say "PWM clk" for clarity.

Sure, I'll be happy to remove another source of confusion.

-- 
Simon South
simon@simonsouth.net