pwm: rockchip: Eliminate potential race condition when probing
Subsystems (2)
v3 cover pwm: rockchip: Eliminate potential race condition when probing
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
[email protected]
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(-)
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v3 1/7 pwm: rockchip: Enable APB clock during register access while probing
Commit 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing") modified rockchip_pwm_probe() to access a PWM device's registers directly to check whether or not the device is running, but did not also change the function to first enable the device's APB clock to be certain the device can respond. This risks hanging the kernel on systems with PWM devices that use more than a single clock.
Avoid this by enabling the device's APB clock before accessing its registers (and disabling the clock when register access is complete).
--- drivers/pwm/pwm-rockchip.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/pwm/pwm-rockchip.c +5 -3
@@ -332,9 +332,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev)return ret; } - ret =clk_prepare (pc->pclk); + ret =clk_prepare_enable (pc->pclk);if (ret) { -dev_err (&pdev->dev,"Can't prepare APB clk: %d\n" , ret); +dev_err (&pdev->dev,"Can't enable APB clk: %d\n" , ret); goto err_clk; }
@@ -364,10 +364,12 @@ static int rockchip_pwm_probe(struct platform_device *pdev)if ((ctrl & enable_conf) != enable_conf)clk_disable (pc->clk); +clk_disable (pc->pclk); +return 0 ; err_pclk: -clk_unprepare (pc->pclk); +clk_disable_unprepare (pc->pclk); err_clk:clk_disable_unprepare (pc->clk);
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 1/7] pwm: rockchip: Enable APB clock during register access while probing
+ David and Steven,
Hi Steven,
please help to review this patch set.
Thanks
- Kever
Quoted 63 lines · Simon South <[email protected]> 于2020年12月24日周四 上午12:01写道：
Commit
457f74abbed0("pwm: rockchip: Keep enabled PWMs running while probing") modifiedrockchip_pwm_probe()to access a PWM device's registers directly to check whether or not the device is running, but did not also change the function to first enable the device's APB clock to be certain the device can respond. This risks hanging the kernel on systems with PWM devices that use more than a single clock.Avoid this by enabling the device's APB clock before accessing its registers (and disabling the clock when register access is complete).
Fixes 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running whileprobing")
Reported-by Thierry Reding <[email protected]>Suggested-by Trent Piepho <[email protected]>Signed-off-by Simon South <[email protected]>--- drivers/pwm/pwm-rockchip.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/pwm/pwm-rockchip.c +5
-4@@ -332,9 +332,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev)return ret; } - ret =clk_prepare (pc->pclk); + ret =clk_prepare_enable (pc->pclk);if (ret) { -dev_err (&pdev->dev,"Can't prepare APB clk: %d\n" , ret); +dev_err (&pdev->dev,"Can't enable APB clk: %d\n" , ret); goto err_clk; }@@ -364,10 +364,12 @@ static int rockchip_pwm_probe(struct platform_device *pdev)if ((ctrl & enable_conf) != enable_conf)clk_disable (pc->clk); +clk_disable (pc->pclk); +return 0 ; err_pclk: -clk_unprepare (pc->pclk); +clk_disable_unprepare (pc->pclk); err_clk:clk_disable_unprepare (pc->clk); --2.29.2
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 1/7] pwm: rockchip: Enable APB clock during register access while probing
In reply to Simon SouthHello Simon,
Quoted 7 lines · Simon South
Commit
457f74abbed0("pwm: rockchip: Keep enabled PWMs running while probing") modifiedrockchip_pwm_probe()to access a PWM device's registers directly to check whether or not the device is running, but did not also change the function to first enable the device's APB clock to be certain the device can respond. This risks hanging the kernel on systems with PWM devices that use more than a single clock.
Quoted 7 lines
Avoid this by enabling the device's APB clock before accessing its registers (and disabling the clock when register access is complete).
Fixes 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")Reported-by Thierry Reding <[email protected]>Suggested-by Trent Piepho <[email protected]>Signed-off-by Simon South <[email protected]>
Thanks
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v3 2/7 pwm: rockchip: rockchip_pwm_probe(): Remove superfluous clk_unprepare()
In reply to Simon SouthIf rockchip_pwm_probe() fails to register a PWM device it calls clk_unprepare() for the device's PWM clock, without having first disabled the clock and before jumping to an error handler that also unprepares it. This is likely to produce warnings from the kernel about the clock being unprepared when it is still enabled, and then being unprepared when it has already been unprepared.
Prevent these warnings by removing this unnecessary call to clk_unprepare().
--- drivers/pwm/pwm-rockchip.c | 1 - 1 file changed, 1 deletion(-)
drivers/pwm/pwm-rockchip.c +0 -1
@@ -353,7 +353,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) ret =pwmchip_add (&pc->chip);if (ret <0 ) { -clk_unprepare (pc->clk);dev_err (&pdev->dev,"pwmchip_add() failed: %d\n" , ret); goto err_pclk; }
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 2/7] pwm: rockchip: rockchip_pwm_probe(): Remove superfluous clk_unprepare()
Hello Simon,
Quoted 4 lines · Simon South
If
rockchip_pwm_probe()fails to register a PWM device it callsclk_unprepare()for the device's PWM clock, without having first disabled the clock and before jumping to an error handler that also unprepares it.
The "without having first disabled the clock" part is wrong without the first patch. I suggest to swap the order of the first and second patch.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v3 3/7 pwm: rockchip: Replace "bus clk" with "PWM clk"
In reply to Simon SouthClarify the Rockchip PWM driver's error messages by referring to the clock that operates a PWM device as the "PWM" clock, rather than the "bus" clock (which is especially misleading in the case of devices that also use a separate clock for bus access).
--- drivers/pwm/pwm-rockchip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pwm/pwm-rockchip.c +2 -2
@@ -309,7 +309,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) pc->clk =devm_clk_get (&pdev->dev,NULL );if (IS_ERR (pc->clk))return dev_err_probe (&pdev->dev,PTR_ERR (pc->clk), -"Can't get bus clk\n" ); +"Can't get PWM clk\n" ); } count = of_count_phandle_with_args(pdev->dev.of_node,
@@ -328,7 +328,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) ret =clk_prepare_enable (pc->clk);if (ret) { -dev_err (&pdev->dev,"Can't prepare enable bus clk: %d\n" , ret); +dev_err (&pdev->dev,"Can't prepare enable PWM clk: %d\n" , ret);return ret; }
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 3/7] pwm: rockchip: Replace "bus clk" with "PWM clk"
+ David and Steven,
Hi Steven,
please help to review this patch set.
Thanks
- Kever
Quoted 49 lines · Simon South <[email protected]> 于2020年12月24日周四 上午12:01写道：
Clarify the Rockchip PWM driver's error messages by referring to the clock that operates a PWM device as the "PWM" clock, rather than the "bus" clock (which is especially misleading in the case of devices that also use a separate clock for bus access).
Suggested-by Robin Murphy <[email protected]>Signed-off-by Simon South <[email protected]>--- drivers/pwm/pwm-rockchip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/pwm/pwm-rockchip.c +1
-1@@ -309,7 +309,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) pc->clk =devm_clk_get (&pdev->dev,NULL );if (IS_ERR (pc->clk))return dev_err_probe (&pdev->dev,PTR_ERR (pc->clk), -"Can't get bus clk\n" ); +"Can't get PWM clk\n" ); }count = of_count_phandle_with_args(pdev->dev.of_node,drivers/pwm/pwm-rockchip.c +1
-1@@ -328,7 +328,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) ret =clk_prepare_enable (pc->clk);if (ret) { -dev_err (&pdev->dev,"Can't prepare enable bus clk: %d\n" , ret); +dev_err (&pdev->dev,"Can't prepare enable PWM clk: %d\n" , ret);return ret; }--
2.29.2_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 3/7] pwm: rockchip: Replace "bus clk" with "PWM clk"
In reply to Simon SouthHello Simon,
Quoted 4 lines · Simon South
Clarify the Rockchip PWM driver's error messages by referring to the clock that operates a PWM device as the "PWM" clock, rather than the "bus" clock (which is especially misleading in the case of devices that also use a separate clock for bus access).
Maybe mention that this is the clock that is named "pwm" in the devicetree and so matches to that, too.
Other than that the patch is fine.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v3 4/7 pwm: rockchip: Eliminate potential race condition when probing
In reply to Simon SouthCommit 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs") introduced a potential race condition in rockchip_pwm_probe(): A consumer could enable an inactive PWM, or disable a running one, between rockchip_pwm_probe() registering the device via pwmchip_add() and checking whether it is enabled (to determine whether it was started by a bootloader). This could result in a device's PWM clock being either enabled once more than necessary, potentially causing it to continue running when no longer needed, or disabled once more than necessary, producing a warning from the kernel.
Eliminate these possibilities by modifying rockchip_pwm_probe() so it checks whether a device is enabled before registering it rather than after.
Also update the code that handles errors from pwmchip_add() to account for the fact a device's PWM clock may now be disabled and that its APB clock certainly is, and eliminate the "err_pclk" goto target as it is no longer of use.
--- drivers/pwm/pwm-rockchip.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
drivers/pwm/pwm-rockchip.c +13 -9
@@ -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);
@@ -351,24 +352,27 @@ static int rockchip_pwm_probe(struct platform_device *pdev) pc->chip.of_pwm_n_cells =3 ; } - ret =pwmchip_add (&pc->chip); -if (ret <0 ) { -dev_err (&pdev->dev,"pwmchip_add() failed: %d\n" , ret); - goto err_pclk; - } -/* 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) + enabled = ((ctrl & enable_conf) == enable_conf); +if (!enabled)clk_disable (pc->clk);clk_disable (pc->pclk); + ret =pwmchip_add (&pc->chip); +if (ret <0 ) { +dev_err (&pdev->dev,"pwmchip_add() failed: %d\n" , ret); +if (enabled) +clk_disable (pc->clk); +clk_unprepare (pc->clk); +clk_unprepare (pc->pclk); +return ret; + } +return 0 ; -err_pclk: -clk_disable_unprepare (pc->pclk); err_clk:clk_disable_unprepare (pc->clk);
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hello,
Quoted 42 lines · Simon South
Commit
48cf973cae33("pwm: rockchip: Avoid glitches on already running PWMs") introduced a potential race condition inrockchip_pwm_probe(): A consumer could enable an inactive PWM, or disable a running one, betweenrockchip_pwm_probe()registering the device viapwmchip_add()and checking whether it is enabled (to determine whether it was started by a bootloader). This could result in a device's PWM clock being either enabled once more than necessary, potentially causing it to continue running when no longer needed, or disabled once more than necessary, producing a warning from the kernel.Eliminate these possibilities by modifying
rockchip_pwm_probe()so it checks whether a device is enabled before registering it rather than after.Also update the code that handles errors from
pwmchip_add()to account for the fact a device's PWM clock may now be disabled and that its APB clock certainly is, and eliminate the "err_pclk" goto target as it is no longer of use.Fixes 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs")Reported-by Trent Piepho <[email protected]>Signed-off-by Simon South <[email protected]>--- drivers/pwm/pwm-rockchip.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)drivers/pwm/pwm-rockchip.c +1
-1@@ -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);@@ -351,24 +352,27 @@ static int rockchip_pwm_probe(struct platform_device *pdev) pc->chip.of_pwm_n_cells =3 ; } - ret =pwmchip_add (&pc->chip);
drivers/pwm/pwm-rockchip.c static int rockchip_pwm_probe(struct platform_device *pdev) · line 358
Quoted 10 lines
drivers/pwm/pwm-rockchip.c +1
-6-if (ret <0 ) { -dev_err (&pdev->dev,"pwmchip_add() failed: %d\n" , ret); - goto err_pclk; - } -/* 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) + enabled = ((ctrl & enable_conf) == enable_conf);
The outer parenthesis pair isn't necessary, please drop it.
Quoted 5 lines
drivers/pwm/pwm-rockchip.c +1
-0+if (!enabled)clk_disable (pc->clk);clk_disable (pc->pclk);
drivers/pwm/pwm-rockchip.c static int rockchip_pwm_probe(struct platform_device *pdev) · line 373
Quoted 10 lines
drivers/pwm/pwm-rockchip.c +10
-0+ ret =pwmchip_add (&pc->chip); +if (ret <0 ) { +dev_err (&pdev->dev,"pwmchip_add() failed: %d\n" , ret); +if (enabled) +clk_disable (pc->clk); +clk_unprepare (pc->clk); +clk_unprepare (pc->pclk); +return ret; + } +
If you do the pwmchip_add before the clock disable, you can continue to only have a single and simpler error path. The critical part is to check for the enabled hardware before.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v3 5/7 pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target
In reply to Simon SouthEliminate the remaining goto target in rockchip_pwm_probe() by moving the code that follows it to the point where it is invoked.
The target no longer serves any purpose as the error-handling portions of this function no longer share any recovery steps.
--- drivers/pwm/pwm-rockchip.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/pwm/pwm-rockchip.c +2 -6
@@ -336,7 +336,8 @@ static int rockchip_pwm_probe(struct platform_device *pdev) ret =clk_prepare_enable (pc->pclk);if (ret) {dev_err (&pdev->dev,"Can't enable APB clk: %d\n" , ret); - goto err_clk; +clk_disable_unprepare (pc->clk); +return ret; }platform_set_drvdata (pdev, pc);
@@ -372,11 +373,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) }return 0 ; - -err_clk: -clk_disable_unprepare (pc->clk); - -return ret; }static int rockchip_pwm_remove (struct platform_device *pdev)
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 5/7] pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target
+ David and Steven,
Hi Steven,
please help to review this patch set.
Thanks
- Kever
Quoted 51 lines · Simon South <[email protected]> 于2020年12月24日周四 上午12:01写道：
Eliminate the remaining goto target in
rockchip_pwm_probe()by moving the code that follows it to the point where it is invoked.The target no longer serves any purpose as the error-handling portions of this function no longer share any recovery steps.
Signed-off-by Simon South <[email protected]>--- drivers/pwm/pwm-rockchip.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)drivers/pwm/pwm-rockchip.c +2
-1@@ -336,7 +336,8 @@ static int rockchip_pwm_probe(struct platform_device *pdev) ret =clk_prepare_enable (pc->pclk);if (ret) {dev_err (&pdev->dev,"Can't enable APB clk: %d\n" , ret); - goto err_clk; +clk_disable_unprepare (pc->clk); +return ret; }platform_set_drvdata(pdev, pc);drivers/pwm/pwm-rockchip.c +0
-5@@ -372,11 +373,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) }return 0 ; - -err_clk: -clk_disable_unprepare (pc->clk); - -return ret; }static int
rockchip_pwm_remove(struct platform_device *pdev)
--
2.29.2_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 5/7] pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target
In reply to Simon SouthQuoted 5 lines · Simon South
Eliminate the remaining goto target in
rockchip_pwm_probe()by moving the code that follows it to the point where it is invoked.The target no longer serves any purpose as the error-handling portions of this function no longer share any recovery steps.
If you adapt patch 4 as I suggested, this patch becomes obsolete.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v3 6/7 pwm: rockchip: Enable PWM clock of probed device only if running
In reply to Simon SouthCurrently rockchip_pwm_probe() enables the PWM clock of every device it matches, then disables the clock unless the device itself appears to have been enabled (by a bootloader, presumably) before the kernel started.
Simplify this by enabling (rather, keeping enabled) the PWM clock of only devices that are already running, as enabling the clock for any other device during probing is unnecessary.
--- drivers/pwm/pwm-rockchip.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
drivers/pwm/pwm-rockchip.c +13 -8
@@ -327,12 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)return ret; } - ret =clk_prepare_enable (pc->clk); -if (ret) { -dev_err (&pdev->dev,"Can't prepare enable PWM clk: %d\n" , ret); -return ret; - } - ret =clk_prepare_enable (pc->pclk);if (ret) {dev_err (&pdev->dev,"Can't enable APB clk: %d\n" , ret);
@@ -357,8 +351,19 @@ static int rockchip_pwm_probe(struct platform_device *pdev) enable_conf = pc->data->enable_conf; ctrl =readl_relaxed (pc->base + pc->data->regs.ctrl); enabled = ((ctrl & enable_conf) == enable_conf); -if (!enabled) -clk_disable (pc->clk); +if (enabled) { + ret =clk_prepare_enable (pc->clk); +if (ret) { +dev_err (&pdev->dev,"Can't enable PWM clk: %d\n" , ret); +return ret; + } + }else { + ret =clk_prepare (pc->clk); +if (ret) { +dev_err (&pdev->dev,"Can't prepare PWM clk: %d\n" , ret); +return ret; + } + }clk_disable (pc->pclk);
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 6/7] pwm: rockchip: Enable PWM clock of probed device only if running
+ David and Steven,
Hi Steven,
please help to review this patch set.
Thanks
- Kever
Quoted 69 lines · Simon South <[email protected]> 于2020年12月24日周四 上午12:01写道：
Currently
rockchip_pwm_probe()enables the PWM clock of every device it matches, then disables the clock unless the device itself appears to have been enabled (by a bootloader, presumably) before the kernel started.Simplify this by enabling (rather, keeping enabled) the PWM clock of only devices that are already running, as enabling the clock for any other device during probing is unnecessary.
Signed-off-by Simon South <[email protected]>--- drivers/pwm/pwm-rockchip.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)drivers/pwm/pwm-rockchip.c +0
-6@@ -327,12 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)return ret; } - ret =clk_prepare_enable (pc->clk); -if (ret) { -dev_err (&pdev->dev,"Can't prepare enable PWM clk: %d\n" , ret); -return ret; - } - ret =clk_prepare_enable (pc->pclk);if (ret) { dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);drivers/pwm/pwm-rockchip.c +13
-2@@ -357,8 +351,19 @@ static int rockchip_pwm_probe(struct platform_device *pdev) enable_conf = pc->data->enable_conf; ctrl =readl_relaxed (pc->base + pc->data->regs.ctrl); enabled = ((ctrl & enable_conf) == enable_conf); -if (!enabled) -clk_disable (pc->clk); +if (enabled) { + ret =clk_prepare_enable (pc->clk); +if (ret) { +dev_err (&pdev->dev,"Can't enable PWM clk: %d\n" , ret); +return ret; + } + }else { + ret =clk_prepare (pc->clk); +if (ret) { +dev_err (&pdev->dev,"Can't prepare PWM clk: %d\n" , ret); +return ret; + } + }clk_disable(pc->pclk);--
2.29.2_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 6/7] pwm: rockchip: Enable PWM clock of probed device only if running
In reply to Simon SouthQuoted 18 lines · Simon South
Currently
rockchip_pwm_probe()enables the PWM clock of every device it matches, then disables the clock unless the device itself appears to have been enabled (by a bootloader, presumably) before the kernel started.Simplify this by enabling (rather, keeping enabled) the PWM clock of only devices that are already running, as enabling the clock for any other device during probing is unnecessary.
Signed-off-by Simon South <[email protected]>--- drivers/pwm/pwm-rockchip.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)drivers/pwm/pwm-rockchip.c +0
-0
drivers/pwm/pwm-rockchip.c static int rockchip_pwm_probe(struct platform_device *pdev) · line 335
Quoted 9 lines
drivers/pwm/pwm-rockchip.c +0
-6@@ -327,12 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)return ret; } - ret =clk_prepare_enable (pc->clk); -if (ret) { -dev_err (&pdev->dev,"Can't prepare enable PWM clk: %d\n" , ret); -return ret; - } -
This undoes stuff that you introduced in patch 1. Don't you reintroduce the problem that was fixed by this patch?
Quoted 12 lines
drivers/pwm/pwm-rockchip.c +3
-2ret =clk_prepare_enable (pc->pclk);if (ret) {dev_err (&pdev->dev,"Can't enable APB clk: %d\n" , ret);@@ -357,8 +351,19 @@ static int rockchip_pwm_probe(struct platform_device *pdev) enable_conf = pc->data->enable_conf; ctrl =readl_relaxed (pc->base + pc->data->regs.ctrl); enabled = ((ctrl & enable_conf) == enable_conf); -if (!enabled) -clk_disable (pc->clk); +if (enabled) { + ret =clk_prepare_enable (pc->clk); +if (ret) {
drivers/pwm/pwm-rockchip.c static int rockchip_pwm_probe(struct platform_device *pdev) · line 366
Quoted 10 lines
drivers/pwm/pwm-rockchip.c +10
-0+dev_err (&pdev->dev,"Can't enable PWM clk: %d\n" , ret); +return ret; + } + }else { + ret =clk_prepare (pc->clk); +if (ret) { +dev_err (&pdev->dev,"Can't prepare PWM clk: %d\n" , ret); +return ret; + } + }
I don't see the advantage of this patch. In my eyes the code complication doesn't justify the gain (i.e. prevent the PWM clock being enabled for a few instructions).
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 6/7] pwm: rockchip: Enable PWM clock of probed device only if running
Quoted 3 lines · Uwe Kleine-König
I don't see the advantage of this patch. In my eyes the code complication doesn't justify the gain (i.e. prevent the PWM clock being enabled for a few instructions).
I was starting to feel the same way after I sent out this series, so let's just drop this change.
Thanks for the feedback, Uwe. I'll get a v4 out shortly.
Signature · 7 lines
Simon South [email protected] _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v3 7/7 pwm: rockchip: Enable clock before calling clk_get_rate()
In reply to Simon SouthThe documentation for clk_get_rate() in include/linux/clk.h states the function's result is valid only for a clock source that has been enabled. However, the Rockchip PWM driver uses this function in two places to query the rate of a clock without first ensuring it is enabled.
Fix this by modifying rockchip_pwm_get_state() and rockchip_pwm_apply() so they enable a device's PWM clock before querying its rate (in the latter case, the querying is actually done in rockchip_pwm_config()) and disable the clock again before returning.
--- drivers/pwm/pwm-rockchip.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/pwm/pwm-rockchip.c +10 -0
@@ -72,6 +72,10 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,if (ret)return ; + ret =clk_enable (pc->clk); +if (ret) +return ; + clk_rate =clk_get_rate (pc->clk); tmp =readl_relaxed (pc->base + pc->data->regs.period);
@@ -90,6 +94,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,else state->polarity =PWM_POLARITY_NORMAL ; +clk_disable (pc->clk);clk_disable (pc->pclk); }
@@ -189,6 +194,10 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,if (ret)return ret; + ret =clk_enable (pc->clk); +if (ret) +return ret; +pwm_get_state (pwm, &curstate); enabled = curstate.enabled;
@@ -208,6 +217,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, } out: +clk_disable (pc->clk);clk_disable (pc->pclk);return ret;
Signature · 7 lines
2.29.2 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate()
Quoted 5 lines · Simon South
The documentation for
clk_get_rate()ininclude/linux/clk.hstates the function's result is valid only for a clock source that has been enabled. However, the Rockchip PWM driver uses this function in two places to query the rate of a clock without first ensuring it is enabled.
Quoted 7 lines
Fix this by modifying
rockchip_pwm_get_state()androckchip_pwm_apply()so they enable a device's PWM clock before querying its rate (in the latter case, the querying is actually done inrockchip_pwm_config()) and disable the clock again before returning.Reported-by Uwe Kleine-König <[email protected]>Signed-off-by Simon South <[email protected]>
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
+ David and Steven,
Hi Steven,
please help to review this patch set.
Thanks
- Kever
Quoted 80 lines · Simon South <[email protected]> 于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 viapwmchip_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
[email protected]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 callingclk_get_rate()drivers/pwm/pwm-rockchip.c | 64 ++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 24 deletions(-)--
2.29.2_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing【请注意，邮件由[email protected]代发】
Hi Simon,
This series of patches are okay for me, simplify to read pwm register to check pwm whether it is enabled. So,
Quoted 84 lines · Kever Yang
+ David and Steven,
Hi Steven,
please help to review this patch set.Thanks
- Kever
Simon South <[email protected] <mailto:[email protected]>> 于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 viapwmchip_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
[email protected] <mailto:[email protected]>Simon South (7):
pwm: rockchip: Enable APB clock during register access while probing
pwm: rockchip:rockchip_pwm_probe(): Remove superfluousclk_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 callingclk_get_rate()drivers/pwm/pwm-rockchip.c | 64 ++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 24 deletions(-)
--
2.29.2
Quoted 6 lines
_______________________________________________
Linux-rockchip mailing list
[email protected]
<mailto:[email protected]>
http://lists.infradead.org/mailman/listinfo/linux-rockchip
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing【请注意，邮件由[email protected]代发】
Hi David,
Quoted 6 lines · David Wu
Hi Simon,
This series of patches are okay for me, simplify to read pwm register to check pwm whether it is enabled. So,
Revieded-by David Wu <[email protected]>
You might want to reply to each patch individually and use "Reviewed-by" instead of "Revieded-by" for the patch application tools (like patchwork and b4) to properly account for your effort checking the patches.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip