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

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

[Simon South]

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

[PATCH v3 1/7] pwm: rockchip: Enable APB clock during register access while probing

[Simon South]

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).

Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")
Reported-by: Thierry Reding <thierry.reding@gmail.com>
Suggested-by: Trent Piepho <tpiepho@gmail.com>
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 77c23a2c6d71..d2058138ce1e 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -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

Re: [PATCH v3 1/7] pwm: rockchip: Enable APB clock during register access while probing

[Kever Yang]

[-- Attachment #1: Type: text/plain, Size: 2356 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写道：

> 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).
>
> Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while
> probing")
> Reported-by: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Trent Piepho <tpiepho@gmail.com>
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 77c23a2c6d71..d2058138ce1e 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -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
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>
>

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

Re: [PATCH v3 1/7] pwm: rockchip: Enable APB clock during register access while probing

[Uwe Kleine-König]

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

Hello Simon,

On Wed, Dec 23, 2020 at 11:01:03AM -0500, Simon South wrote:
> 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).
> 
> Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")
> Reported-by: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Trent Piepho <tpiepho@gmail.com>
> Signed-off-by: Simon South <simon@simonsouth.net>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
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 --]

[PATCH v3 2/7] pwm: rockchip: rockchip_pwm_probe(): Remove superfluous clk_unprepare()

[Simon South]

If 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().

Fixes: 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs")
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index d2058138ce1e..0c940c7508ea 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -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;
 	}
-- 
2.29.2

Re: [PATCH v3 2/7] pwm: rockchip: rockchip_pwm_probe(): Remove superfluous clk_unprepare()

[Uwe Kleine-König]

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

Hello Simon,

On Wed, Dec 23, 2020 at 11:01:04AM -0500, Simon South wrote:
> If 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.

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

-- 
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 --]

[PATCH v3 3/7] pwm: rockchip: Replace "bus clk" with "PWM clk"

[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).

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 0c940c7508ea..3b1aa5daafff 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -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;
 	}
 
-- 
2.29.2

Re: [PATCH v3 3/7] pwm: rockchip: Replace "bus clk" with "PWM clk"

[Kever Yang]

[-- Attachment #1: Type: text/plain, Size: 1930 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写道：

> 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 <robin.murphy@arm.com>
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 0c940c7508ea..3b1aa5daafff 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -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;
>         }
>
> --
> 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: 2947 bytes --]

Re: [PATCH v3 3/7] pwm: rockchip: Replace "bus clk" with "PWM clk"

[Uwe Kleine-König]

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

Hello Simon,

On Wed, Dec 23, 2020 at 11:01:05AM -0500, Simon South wrote:
> 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

-- 
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 --]

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

[Simon South]

Commit 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.

Fixes: 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs")
Reported-by: Trent Piepho <tpiepho@gmail.com>
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 3b1aa5daafff..d904a5d24885 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -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);
 
-- 
2.29.2

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

[Uwe Kleine-König]

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

Hello,

On Wed, Dec 23, 2020 at 11:01:06AM -0500, Simon South wrote:
> Commit 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.
> 
> Fixes: 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs")
> Reported-by: Trent Piepho <tpiepho@gmail.com>
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 3b1aa5daafff..d904a5d24885 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -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);

The outer parenthesis pair isn't necessary, please drop it.

> +	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;
> +	}
> +

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

-- 
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 --]

[PATCH v3 5/7] pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target

[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.

Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index d904a5d24885..80f5e69d9b8a 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -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)
-- 
2.29.2

Re: [PATCH v3 5/7] pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target

[Kever Yang]

[-- Attachment #1: Type: text/plain, Size: 1709 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写道：

> 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 <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index d904a5d24885..80f5e69d9b8a 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -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)
> --
> 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: 2534 bytes --]

Re: [PATCH v3 5/7] pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target

[Uwe Kleine-König]

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

On Wed, Dec 23, 2020 at 11:01:07AM -0500, Simon South wrote:
> 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

-- 
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 --]

[PATCH v3 6/7] pwm: rockchip: Enable PWM clock of probed device only if running

[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 <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 80f5e69d9b8a..02da7370db70 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -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);
 
-- 
2.29.2

Re: [PATCH v3 6/7] pwm: rockchip: Enable PWM clock of probed device only if running

[Kever Yang]

[-- Attachment #1: Type: text/plain, Size: 2544 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写道：

> 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 <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 80f5e69d9b8a..02da7370db70 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -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);
>
> --
> 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: 3607 bytes --]

Re: [PATCH v3 6/7] pwm: rockchip: Enable PWM clock of probed device only if running

[Uwe Kleine-König]

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

On Wed, Dec 23, 2020 at 11:01:08AM -0500, Simon South wrote:
> 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 <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 80f5e69d9b8a..02da7370db70 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -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?

>  	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;
> +		}
> +	}

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

-- 
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 --]

Re: [PATCH v3 6/7] pwm: rockchip: Enable PWM clock of probed device only if running

[Simon South]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 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.

-- 
Simon South
simon@simonsouth.net

[PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate()

[Simon South]

The 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.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 02da7370db70..44425eeb4e81 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -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;
-- 
2.29.2

Re: [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate()

[Uwe Kleine-König]

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

On Wed, Dec 23, 2020 at 11:01:09AM -0500, Simon South wrote:
> The 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.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Simon South <simon@simonsouth.net>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

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 --]

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

[Kever Yang]

[-- 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 --]

Re: [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing【请注意，邮件由kever.yang@gmail.com代发】

[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 <david.wu@rock-chips.com>

在 2020/12/25 下午3:10, Kever Yang 写道:
> + David and Steven,
> 
> Hi Steven,
>      please help to review this patch set.
> 
> Thanks
> - Kever
> Simon South <simon@simonsouth.net <mailto: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 <mailto: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
>     <mailto:Linux-rockchip@lists.infradead.org>
>     http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

Re: [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing【请注意，邮件由kever.yang@gmail.com代发】

[Uwe Kleine-König]

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

Hi David,

On Tue, Jan 05, 2021 at 07:26:16PM +0800, David Wu wrote:
> 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 <david.wu@rock-chips.com>

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

-- 
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 --]