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

[Trent Piepho <tpiepho@gmail.com>]

On Thursday, December 10, 2020 9:48:30 AM PST Thierry Reding wrote:
> On Sun, Nov 29, 2020 at 07:44:19PM -0500, Simon South wrote:
> > @@ -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.

I would also assume register access with the clock disabled would hang or
otherwise fail.  There are possibly two clocks, one called "bus clock" and
the other "APB clock".  APB being Advanced Peripheral Bus.  Not the greatest
choice of names.  I assume the APB clock is needed for register access and
the "bus clock" is used to generate the PWM signal and does not need to be
enabled for register access.  Unfortunately the RK3399 docs do not have a
clock diagram for the PWM or include details such as these.

There is a low power mode bit in the control register that disables the PWM
signal's clock.  And which clock does that disabled, the "ABP clock" or the
"bus clock"?  I quote §18.6.4, "the APB bus clock … is gated…"  It's like
they're being intentional ambiguous.

Anyway, from the existing code, it seems clear that pc->pclk needs to be
enabled for register access and pc->clk to generate a signal.  The call to
clk_prepare(pc->pclk) should become clk_prepare_enable(pc->pclk) and moved
to before the enabled_conf check.  Then clk_disable(pc->pclk) afterward.
The existing code will disable pclk even if the PWM is enabled, so unless
that is also a bug, it should be ok to disable pc->pclk after enabling
pc->clk.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel