| Message ID | 20201130004419.1714-1-simon@simonsouth.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: rockchip: Eliminate potential race condition when probing | expand |
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
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.
On 2020-12-10 21:00, Trent Piepho wrote: > 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. FWIW I think it becomes clear enough if you read the DT binding in parallel with the code - if devm_clk_get(&pdev->dev, "pwm") fails, the driver falls back to assuming the RK3399-or-earlier case of a single unnamed clock, so "Can't get bus clk" is referring specifically to the devm_clk_get(&pdev->dev, NULL) call where that clock *is* also the APB clock. Possibly the driver could do with a slightly clearer structure here, but compatibility fallbacks are inevitably messy to some degree. Robin. > 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-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip >
Thierry, Trent and Robin, thanks for reviewing this and for your comments. I think I understand a lot better now how this code needs to work. Trent Piepho <tpiepho@gmail.com> writes: > 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. I'll follow up with a revised set of patches that implement this. (I've split it into multiple changes as they seem logically distinct and I find the progression easier to see this way.)
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; + if (enabled) { + ret = clk_enable(pc->clk); + if (ret) { + dev_err(&pdev->dev, "Can't re-enable bus clk: %d\n", + ret); + goto err_clk_prepared; + } + } + ret = clk_prepare(pc->pclk); if (ret) { dev_err(&pdev->dev, "Can't prepare APB clk: %d\n", ret); - goto err_clk; + goto err_clk_enabled; } platform_set_drvdata(pdev, pc); - pc->data = id->data; pc->chip.dev = &pdev->dev; pc->chip.ops = &rockchip_pwm_ops; pc->chip.base = -1; @@ -355,21 +375,18 @@ static int rockchip_pwm_probe(struct platform_device *pdev) if (ret < 0) { clk_unprepare(pc->clk); dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); - goto err_pclk; + goto err_pclk_prepared; } - /* 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) - clk_disable(pc->clk); - return 0; -err_pclk: +err_pclk_prepared: clk_unprepare(pc->pclk); -err_clk: - clk_disable_unprepare(pc->clk); +err_clk_enabled: + if (enabled) + clk_disable(pc->clk); +err_clk_prepared: + clk_unprepare(pc->clk); return ret; }
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(-)