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