| Message ID | fcfb96fd2fd860ae3f08cb563c74eb864cdab41a.1608407584.git.simon@simonsouth.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: rockchip: Eliminate potential race condition when probing | expand |
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
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 --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)
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(-)