Re: [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing

From: Kever Yang <kever.yang@rock-chips.com>
To: Simon South <simon@simonsouth.net>
Cc: tpiepho@gmail.com, thierry.reding@gmail.com,
	u.kleine-koenig@pengutronix.de,
	Robin Murphy <robin.murphy@arm.com>,
	lee.jones@linaro.org, Heiko Stuebner <heiko@sntech.de>,
	bbrezillon@kernel.org, linux-pwm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org,
	David Wu <david.wu@rock-chips.com>,
	steven.liu@rock-chips.com
Subject: Re: [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing
Date: Fri, 25 Dec 2020 15:10:44 +0800	[thread overview]
Message-ID: <CAKUh=Ry7=WUmN5OYv5g_E1EHk8jNE-QcOtw_BrMbH+_aP68w8Q@mail.gmail.com> (raw)
In-Reply-To: <cover.1608735481.git.simon@simonsouth.net>

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

+ David and Steven,

Hi Steven,
    please help to review this patch set.

Thanks
- Kever
Simon South <simon@simonsouth.net> 于2020年12月24日周四 上午12:01写道：

> 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 checks whether a device is enabled ahead
> of the code that registers it via pwmchip_add().
>
> It has grown to include a number of other small fixes and improvements
> to the driver. It now also
>
> - 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;
>
> - Removes a superfluous call to clk_unprepare() that could result in
>   warnings from the kernel;
>
> - Clarifies the driver's error messages by replacing "bus clk" with
>   "PWM clk";
>
> - Removes the now-unneeded goto targets from rockchip_pwm_probe();
>
> - Tries to improve rockchip_pwm_probe() by having it enable the signal
>   clock of only PWM devices that are already running; and
>
> - Ensures the driver enables a clock before querying its rate with
>   clk_get_rate(), as stated as a requirement in that function's
>   documentation.
>
> The first patch ("Enable APB clock...") is unchanged from version 2.
>
> New in version 3 are
>
> - Finer patch granularity, with patches 2 and 5 added to clarify
>   changes included with others in v2;
>
> - A rewritten patch 6 ("Enable PWM clock...") with a smaller change
>   and the use of if...else in place of a ternary operator;
>
> - Patches 3 and 7 with fixes suggested by Robin Murphy and Uwe
>   Kleine-König; and
>
> - Rewritten and (hopefully) more accurate commit messages.
>
> I've tested these changes on my (RK3399-based) Pinebook Pro with its
> screen backlight enabled by U-Boot and each one appears to work fine.
>
> I'd (still) 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 (7):
>   pwm: rockchip: Enable APB clock during register access while probing
>   pwm: rockchip: rockchip_pwm_probe(): Remove superfluous
>     clk_unprepare()
>   pwm: rockchip: Replace "bus clk" with "PWM clk"
>   pwm: rockchip: Eliminate potential race condition when probing
>   pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target
>   pwm: rockchip: Enable PWM clock of probed device only if running
>   pwm: rockchip: Enable clock before calling clk_get_rate()
>
>  drivers/pwm/pwm-rockchip.c | 64 ++++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 24 deletions(-)
>
> --
> 2.29.2
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>

[-- Attachment #2: Type: text/html, Size: 4236 bytes --]