pwm: rockchip: Eliminate potential race condition when probing

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 <[email protected]>

Suggested-by Trent Piepho <[email protected]>

Signed-off-by Simon South <[email protected]>

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.

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.

Quoted 2 lines

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?

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 <[email protected]>

---
 drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

drivers/pwm/pwm-rockchip.c +0 -2

index f286a498b82c..b9faef3e9954 100644
@@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pc->clk);
-	if (ret) {

drivers/pwm/pwm-rockchip.c +6 -1

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

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

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

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

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 <[email protected]>

---
  drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
  1 file changed, 13 insertions(+), 15 deletions(-)

drivers/pwm/pwm-rockchip.c +6 -13

index f286a498b82c..b9faef3e9954 100644
@@ -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;
+	}
+

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

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.