diff mbox series

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

Message ID fcfb96fd2fd860ae3f08cb563c74eb864cdab41a.1608407584.git.simon@simonsouth.net (mailing list archive)
State New
Headers show
Series pwm: rockchip: Eliminate potential race condition when probing | expand

Commit Message

Simon South Dec. 19, 2020, 8:44 p.m. UTC
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(-)

Comments

Uwe Kleine-König Dec. 21, 2020, 8:50 a.m. UTC | #1
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
Simon South Dec. 22, 2020, 4:26 p.m. UTC | #2
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.
diff mbox series

Patch

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)