| Message ID | cover.1608407584.git.simon@simonsouth.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | pwm: rockchip: Eliminate potential race condition when probing | expand |
Hello Simon, On Sat, Dec 19, 2020 at 03:44:07PM -0500, Simon South wrote: > This patch series aims to eliminate the race condition Trent Piepho > identified[0] in the Rockchip PWM driver's rockchip_pwm_probe() > function, by moving code that disables a PWM device's signal clock > ahead of the code that registers the device via pwmchip_add(). > > It additionally > > - Fixes a potential kernel hang introduced by my earlier commit > 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while > probing") by ensuring a device's APB clock is enabled before its > registers are accessed, and > > - Tries to improve the driver by (re-)enabling the signal clock of > only PWM devices that appear to have been started already by the > bootloader, rather than enabling every device's signal clock and > selectively disabling it later. > > I've tested these changes on my (RK3399-based) Pinebook Pro with its > screen backlight enabled by U-Boot and they appear to work fine. > > I'd be grateful for help with testing on other devices, particularly > those with SoCs like the RK3328 that use separate bus and signal > clocks for their PWM devices. (My ROCK64 uses its PWM-output pins for > other purposes and wasn't of help here.) While looking through your I found another little problem that you might want to address: rockchip_pwm_get_state() calls clk_get_rate(pc->clk). According to the documentation (in include/linux/clk.h) this is only valid for enabled clocks but there are no precautions that pc->clk is enabled. Best regards Uwe
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > While looking through your I found another little problem that you might > want to address: rockchip_pwm_get_state() calls clk_get_rate(pc->clk). > According to the documentation (in include/linux/clk.h) this is only > valid for enabled clocks but there are no precautions that pc->clk is > enabled. Sure. Thanks very much for reviewing all this, Uwe. I'll follow up shortly with a new patch series.
This patch series aims to eliminate the race condition Trent Piepho identified[0] in the Rockchip PWM driver's rockchip_pwm_probe() function, by moving code that disables a PWM device's signal clock ahead of the code that registers the device via pwmchip_add(). It additionally - Fixes a potential kernel hang introduced by my earlier commit 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing") by ensuring a device's APB clock is enabled before its registers are accessed, and - Tries to improve the driver by (re-)enabling the signal clock of only PWM devices that appear to have been started already by the bootloader, rather than enabling every device's signal clock and selectively disabling it later. I've tested these changes on my (RK3399-based) Pinebook Pro with its screen backlight enabled by U-Boot and they appear to work fine. I'd be grateful for help with testing on other devices, particularly those with SoCs like the RK3328 that use separate bus and signal clocks for their PWM devices. (My ROCK64 uses its PWM-output pins for other purposes and wasn't of help here.) [0] https://www.spinics.net/lists/linux-pwm/msg14611.html -- Simon South simon@simonsouth.net Simon South (3): pwm: rockchip: Enable APB clock during register access while probing pwm: rockchip: Eliminate potential race condition when probing pwm: rockchip: Do not start PWMs not already running drivers/pwm/pwm-rockchip.c | 49 +++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 25 deletions(-)