pwm: rockchip: Eliminate potential race condition when probing

This patch series eliminates the race condition Trent Piepho identified[0] in the Rockchip PWM driver's rockchip_pwm_probe() function, by moving code that checks whether a device is enabled ahead of the code that registers it via pwmchip_add().

It includes several other small fixes and improvements to the driver as well: It also

- 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;

- Removes a superfluous call to clk_unprepare() that could produce warnings from the kernel;

- Clarifies error messages from the driver by replacing "bus clk" with "PWM clk"; and

- Ensures the driver enables a clock before querying its rate with clk_get_rate(), as stated as a requirement in that function's documentation.

This version of the series incorporates Uwe Kleine-König's feedback on v3 and includes these changes:

- Patch 1's commit message has been edited slightly to improve readability, and the error message updated by the patch now reads "prepare enable" rather than just "enable" for consistency with the error message above it in the code.

- Patch 3's commit message now mentions consistency with the device tree.

- Patch 4 has been simplified and now just moves the device-enabled check ahead of the call to pwmchip_add(). It no longers changes any error-handling behaviour, and an extraneous pair of parentheses has been removed.

Patches 2 and 5 are unchanged from v3, while the remaining 2 patches present in v3 (which removed goto targets from and reordered some operations in rockchip_pwm_probe()) have been dropped.

I've tested these changes on my Pinebook Pro (RK3399 with a PWM-driven backlight enabled by U-Boot) and ROCK64 (RK3328) and they appear to work fine.

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

--
Simon South
[email protected]

Simon South (5):
pwm: rockchip: Enable APB clock during register access while probing
pwm: rockchip: rockchip_pwm_probe(): Remove superfluous
clk_unprepare()
pwm: rockchip: Replace "bus clk" with "PWM clk"
pwm: rockchip: Eliminate potential race condition when probing
pwm: rockchip: Enable clock before calling clk_get_rate()

 drivers/pwm/pwm-rockchip.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

2.30.0


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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 enabled, but did not also change the function so it first enables 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).

---
 drivers/pwm/pwm-rockchip.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

index 389a5e140412..e6929bc73968 100644
@@ -330,9 +330,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 prepare enable APB clk: %d\n", ret);
 		goto err_clk;
 	}
 

@@ -362,10 +362,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.30.0


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

If rockchip_pwm_probe() fails to register a PWM device it calls clk_unprepare() for the device's PWM clock, without having first disabled the clock and before jumping to an error handler that also unprepares it. This is likely to produce warnings from the kernel about the clock being unprepared when it is still enabled, and then being unprepared when it has already been unprepared.

Prevent these warnings by removing this unnecessary call to clk_unprepare().

---
 drivers/pwm/pwm-rockchip.c | 1 -
 1 file changed, 1 deletion(-)

index e6929bc73968..90f969f9f5e2 100644
@@ -351,7 +351,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 
 	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;
 	}

2.30.0


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Clarify the Rockchip PWM driver's error messages by referring to the clock that operates a PWM device as the "PWM" clock, matching its name in the device tree, rather than the "bus" clock (which is especially misleading in the case of devices that also use a separate clock for bus access).

---
 drivers/pwm/pwm-rockchip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 90f969f9f5e2..b5bab427b5de 100644
@@ -307,7 +307,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		pc->clk = devm_clk_get(&pdev->dev, NULL);
 		if (IS_ERR(pc->clk))
 			return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk),
-					     "Can't get bus clk\n");
+					     "Can't get PWM clk\n");
 	}
 
 	count = of_count_phandle_with_args(pdev->dev.of_node,

@@ -326,7 +326,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 
 	ret = clk_prepare_enable(pc->clk);
 	if (ret) {
-		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
+		dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret);
 		return ret;
 	}
 

2.30.0


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Commit 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs") introduced a potential race condition in rockchip_pwm_probe(): A consumer could enable an inactive PWM, or disable a running one, between rockchip_pwm_probe() registering the device via pwmchip_add() and checking whether it is enabled (to determine whether it was started by a bootloader). This could result in a device's PWM clock being either enabled once more than necessary, potentially causing it to continue running when no longer needed, or disabled once more than necessary, producing a warning from the kernel.

Eliminate these possibilities by modifying rockchip_pwm_probe() so it checks whether a device is enabled before registering it rather than after.

---
 drivers/pwm/pwm-rockchip.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index b5bab427b5de..228147e9bc6e 100644
@@ -288,6 +288,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	const struct of_device_id *id;
 	struct rockchip_pwm_chip *pc;
 	u32 enable_conf, ctrl;
+	bool enabled;
 	int ret, count;
 
 	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);

@@ -349,6 +350,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		pc->chip.of_pwm_n_cells = 3;
 	}
 
+	enable_conf = pc->data->enable_conf;
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	enabled = (ctrl & enable_conf) == enable_conf;
+
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);

@@ -356,9 +361,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	}
 
 	/* 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)
+	if (!enabled)
 		clk_disable(pc->clk);
 
 	clk_disable(pc->pclk);

2.30.0


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

The documentation for clk_get_rate() in include/linux/clk.h states the function's result is valid only for a clock source that has been enabled. However, the Rockchip PWM driver uses this function in two places to query the rate of a clock without first ensuring it is enabled.

Fix this by modifying rockchip_pwm_get_state() and rockchip_pwm_apply() so they enable a device's PWM clock before querying its rate (in the latter case, the querying is actually done in rockchip_pwm_config()) and disable the clock again before returning.

---
 drivers/pwm/pwm-rockchip.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 228147e9bc6e..6ad7d0a50aed 100644
@@ -72,6 +72,10 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 	if (ret)
 		return;
 
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return;
+
 	clk_rate = clk_get_rate(pc->clk);
 
 	tmp = readl_relaxed(pc->base + pc->data->regs.period);

@@ -90,6 +94,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 	else
 		state->polarity = PWM_POLARITY_NORMAL;
 
+	clk_disable(pc->clk);
 	clk_disable(pc->pclk);
 }
 

@@ -189,6 +194,10 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (ret)
 		return ret;
 
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return ret;
+
 	pwm_get_state(pwm, &curstate);
 	enabled = curstate.enabled;
 

@@ -208,6 +217,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 out:
+	clk_disable(pc->clk);
 	clk_disable(pc->pclk);
 
 	return ret;

2.30.0


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Hello Simon,

I already reviewed this patch in v3 and gave my

You make it a bit easier if you add the received tags for new iterations.

Best regards
Uwe

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

_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip