Re: [PATCH] pwm: rockchip: Eliminate potential race condition when probing

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]

On Sun, Nov 29, 2020 at 07:44:19PM -0500, Simon South wrote:
> Commit 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running
> PWMs") introduced a potential race condition in rockchip_pwm_probe() by
> having it disable the clock of a PWM already registered through a call to
> pwmchip_add().
> 
> Eliminate this possibility by calling clk_enable() for a probed PWM's clock
> only when it appears the PWM itself has already been enabled (by a
> bootloader, presumably), instead of always enabling the clock and then
> disabling it after registration for non-enabled PWMs.
> 
> 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 | 45 ++++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 77c23a2c6d71..7efba1d0adb4 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);
> @@ -299,6 +300,8 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	if (!pc)
>  		return -ENOMEM;
>  
> +	pc->data = id->data;
> +
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pc->base = devm_ioremap_resource(&pdev->dev, r);
>  	if (IS_ERR(pc->base))
> @@ -326,21 +329,38 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(pc->clk);
> +	ret = clk_prepare(pc->clk);
>  	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
>  		return ret;
>  	}
>  
> +	/*
> +	 * If it appears the PWM has already been enabled, perhaps by a
> +	 * bootloader, re-enable its clock to increment the clock's enable
> +	 * counter and ensure it is kept running (particularly in the case
> +	 * where there is no separate APB clock).
> +	 */
> +	enable_conf = pc->data->enable_conf;
> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> +	enabled = (ctrl & enable_conf) == enable_conf;

Given that we don't enable the bus clock before this, is it even safe to
access registers on the bus if the clock is disabled? I've seen a lot of
cases where accesses to an unclocked bus either lead to silent hangs or
very noisy crashes, and I would expect something like that (or something
in between) to happen on Rockchip SoCs.

Have you tested this for cases where the bus clock is initially
disabled?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]