| Message ID | 20200919193306.1023-1-simon@simonsouth.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] pwm: rockchip: Keep enabled PWMs running while probing | expand |
Hello, On Sat, Sep 19, 2020 at 03:33:06PM -0400, Simon South wrote: > Following commit cfc4c189bc70 ("pwm: Read initial hardware state at > request time") the Rockchip PWM driver can no longer assume a device's > pwm_state structure has been populated after a call to pwmchip_add(). > Consequently, the test in rockchip_pwm_probe() intended to prevent the > driver from stopping PWM devices already enabled by the bootloader no > longer functions reliably and this can lead to the kernel hanging > during startup, particularly on devices like the Pinebook Pro that use > a PWM-controlled backlight for their display. > > Avoid this by querying the device directly at probe time to determine > whether or not it is enabled. > > Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time") > Signed-off-by: Simon South <simon@simonsouth.net> > --- > drivers/pwm/pwm-rockchip.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index eb8c9cb645a6..098e94335cb5 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -288,6 +288,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > const struct of_device_id *id; > struct rockchip_pwm_chip *pc; > struct resource *r; > + u32 enable_conf, ctrl; > int ret, count; > > id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev); > @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > } > > /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > - if (!pwm_is_enabled(pc->chip.pwms)) > + 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); In my book a pwm driver should never call pwm_get_state() (or its wrappers). Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> I looked at the other drivers for similar problems. I found a few issues, but they are all independently of cfc4c189bc70: - pwm-lpc18xx-sct.c does some allocation that the .request callback depends on. pwm-sti.c does allocation that the irq handler depends on quite late. - some drivers modify the hardware after pwmchip_add() [pwm-fsl-ftm.c, pwm-hibvt.c, pwm-lpc18xx-sct.c, pwm-lpc32xx.c, pwm-mtk-disp.c, pwm-mxs.c] - pwm-lpss.c, pwm-pca9685.c, pwm-tiehrpwm.c and cpwm-tiecap.c do some pm_runtime stuff which should better be done before pwmchip_add()? Best regards Uwe
Am Samstag, 19. September 2020, 21:33:06 CEST schrieb Simon South: > Following commit cfc4c189bc70 ("pwm: Read initial hardware state at > request time") the Rockchip PWM driver can no longer assume a device's > pwm_state structure has been populated after a call to pwmchip_add(). > Consequently, the test in rockchip_pwm_probe() intended to prevent the > driver from stopping PWM devices already enabled by the bootloader no > longer functions reliably and this can lead to the kernel hanging > during startup, particularly on devices like the Pinebook Pro that use > a PWM-controlled backlight for their display. > > Avoid this by querying the device directly at probe time to determine > whether or not it is enabled. > > Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time") > Signed-off-by: Simon South <simon@simonsouth.net> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/pwm/pwm-rockchip.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index eb8c9cb645a6..098e94335cb5 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -288,6 +288,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > const struct of_device_id *id; > struct rockchip_pwm_chip *pc; > struct resource *r; > + u32 enable_conf, ctrl; > int ret, count; > > id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev); > @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > } > > /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > - if (!pwm_is_enabled(pc->chip.pwms)) > + 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; >
On Sat, Sep 19, 2020 at 03:33:06PM -0400, Simon South wrote: > Following commit cfc4c189bc70 ("pwm: Read initial hardware state at > request time") the Rockchip PWM driver can no longer assume a device's > pwm_state structure has been populated after a call to pwmchip_add(). > Consequently, the test in rockchip_pwm_probe() intended to prevent the > driver from stopping PWM devices already enabled by the bootloader no > longer functions reliably and this can lead to the kernel hanging > during startup, particularly on devices like the Pinebook Pro that use > a PWM-controlled backlight for their display. > > Avoid this by querying the device directly at probe time to determine > whether or not it is enabled. > > Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time") > Signed-off-by: Simon South <simon@simonsouth.net> > --- > drivers/pwm/pwm-rockchip.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Applied, thanks. Thierry
On Saturday, September 19, 2020 12:33:06 PM PST Simon South wrote: > Following commit cfc4c189bc70 ("pwm: Read initial hardware state at > request time") the Rockchip PWM driver can no longer assume a device's > pwm_state structure has been populated after a call to pwmchip_add(). > Consequently, the test in rockchip_pwm_probe() intended to prevent the > > @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev) ... ret = pwmchip_add(&pc->chip); ... > } > > /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > - if (!pwm_is_enabled(pc->chip.pwms)) > + 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); > I came across this while trying to get a PBP working better. It seems like the issue is the driver was calling pwm_is_enabled() without first requesting the pwm with pwm_get(). Which wouldn't even be possible normally, how would one get the pwm_chip to call pwm_is_enabled on, but the driver already has the pointer. Anyway, it seems like this solution has a race. Isn't the pwm live and requestable as soon as pwmchip_add() returns? Which would mean that disabling the clock here could race with other code requesting and enabling the pwm. Seems like it would be safer to check the initial state and turn off the clock before calling pwmchip_add.
Trent Piepho <tpiepho@gmail.com> writes: > Anyway, it seems like this solution has a race. Isn't the pwm live > and requestable as soon as pwmchip_add() returns? Which would mean > that disabling the clock here could race with other code requesting > and enabling the pwm. Yes, I think that's true. Glad you caught this. > Seems like it would be safer to check the initial state and turn off > the clock before calling pwmchip_add. Yes. I tested this and it works, but on further consideration it seems to me the code is actually doing things backwards: Instead of enabling every PWM's clock and then turning off the clocks for PWMs that are not themselves enabled, it should enable only the clocks for PWMs that appear to be in use and leave the remaining clocks in their default (presumably disabled) state. This should produce the same end result but without the driver enabling clocks indiscriminately and without creating a race condition. I'll follow up with a patch for review that implements this change. I've tested it as best I can on my own Pinebook Pro; everything works as it did previously, with the display backlight on, no kernel hang and no other apparent side effects. > It seems like the issue is the driver was calling pwm_is_enabled() > without first requesting the pwm with pwm_get(). This used to work because pwmchip_add() happened to invoke rockchip_pwm_get_state(), which populates the PWM's pwm_state structure from which pwm_is_enabled() reads. And I think that's why the code was written the way it was originally: By waiting until pwmchip_add() returned the PWM's state could be queried in a convenient manner, without resorting to peeking at the hardware's registers. Commit cfc4c189bc70 ("pwm: Read initial hardware state at request time") changed this and pwmchip_add() no longer has the side effect of populating the state structure, so the call to pwm_is_enabled() no longer worked reliably. That's what I fixed with the patch you're replying to, though now I wish I'd seen the larger issue. As to why this code is needed in the first place (as I wondered recently while working on it again), it seems to be a somewhat hacky way of initializing the enable_count reference counter (see drivers/clk/clk.c) of the already running clock to 1 instead of 0. This is necessary because on SoCs like the RK3399 that use only a single clock for each PWM, the driver treats the "APB" and "bus" clocks as referring to the same physical device ("pc->pclk = pc->clk"), and without it functions like rockchip_pwm_get_state() that enable the APB clock on entry and disable it on exit would end up halting the PWM entirely.
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index eb8c9cb645a6..098e94335cb5 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -288,6 +288,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) const struct of_device_id *id; struct rockchip_pwm_chip *pc; struct resource *r; + u32 enable_conf, ctrl; int ret, count; id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev); @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev) } /* Keep the PWM clk enabled if the PWM appears to be up and running. */ - if (!pwm_is_enabled(pc->chip.pwms)) + 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;
Following commit cfc4c189bc70 ("pwm: Read initial hardware state at request time") the Rockchip PWM driver can no longer assume a device's pwm_state structure has been populated after a call to pwmchip_add(). Consequently, the test in rockchip_pwm_probe() intended to prevent the driver from stopping PWM devices already enabled by the bootloader no longer functions reliably and this can lead to the kernel hanging during startup, particularly on devices like the Pinebook Pro that use a PWM-controlled backlight for their display. Avoid this by querying the device directly at probe time to determine whether or not it is enabled. Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time") Signed-off-by: Simon South <simon@simonsouth.net> --- drivers/pwm/pwm-rockchip.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)