Re: [PATCH v2 0/3] pwm: rockchip: Eliminate potential race condition when probing

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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