[PATCH 0/5] reset: cleanup and simplify with devm and scoped

[PATCH 0/5] reset: cleanup and simplify with devm and scoped

[Krzysztof Kozlowski]

Hi,

Two simple fixes, which do not cover real scenario (memory allocation
failure during probe), thus not marking CC-stable. Rest of patchset is
simplifying of code.

Best regards,
Krzysztof

---
Krzysztof Kozlowski (5):
      reset: berlin: fix OF node leak in probe() error path
      reset: k210: fix OF node leak in probe() error path
      reset: simplify locking with guard()
      reset: lpc18xx: simplify with dev_err_probe()
      reset: lpc18xx: simplify with devm_clk_get_enabled()

 drivers/reset/core.c          | 15 ++++-----------
 drivers/reset/reset-berlin.c  |  3 ++-
 drivers/reset/reset-k210.c    |  3 ++-
 drivers/reset/reset-lpc18xx.c | 43 ++++++++++---------------------------------
 4 files changed, 18 insertions(+), 46 deletions(-)
---
base-commit: e706b1fe2384d38e6e9edfb6d9e11e26873c24c7
change-id: 20240825-reset-cleanup-scoped-64bb3cb6157f

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

[PATCH 1/5] reset: berlin: fix OF node leak in probe() error path

[Krzysztof Kozlowski]

Driver is leaking OF node reference on memory allocation failure.
Acquire the OF node reference after memory allocation to fix this and
keep it simple.

Fixes: aed6f3cadc86 ("reset: berlin: convert to a platform driver")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/reset/reset-berlin.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c
index 2537ec05ecee..578fe867080c 100644
--- a/drivers/reset/reset-berlin.c
+++ b/drivers/reset/reset-berlin.c
@@ -68,13 +68,14 @@ static int berlin_reset_xlate(struct reset_controller_dev *rcdev,
 
 static int berlin2_reset_probe(struct platform_device *pdev)
 {
-	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
+	struct device_node *parent_np;
 	struct berlin_reset_priv *priv;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	parent_np = of_get_parent(pdev->dev.of_node);
 	priv->regmap = syscon_node_to_regmap(parent_np);
 	of_node_put(parent_np);
 	if (IS_ERR(priv->regmap))

-- 
2.43.0

Re: [PATCH 1/5] reset: berlin: fix OF node leak in probe() error path

[Damien Le Moal]

On 8/25/24 11:14 PM, Krzysztof Kozlowski wrote:
> Driver is leaking OF node reference on memory allocation failure.
> Acquire the OF node reference after memory allocation to fix this and
> keep it simple.
> 
> Fixes: aed6f3cadc86 ("reset: berlin: convert to a platform driver")
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/5] reset: k210: fix OF node leak in probe() error path

[Krzysztof Kozlowski]

Driver is leaking OF node reference on memory allocation failure.
Acquire the OF node reference after memory allocation to fix this and
keep it simple.

Fixes: 5a2308da9f60 ("riscv: Add Canaan Kendryte K210 reset controller")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/reset/reset-k210.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-k210.c b/drivers/reset/reset-k210.c
index b62a2fd44e4e..e77e4cca377d 100644
--- a/drivers/reset/reset-k210.c
+++ b/drivers/reset/reset-k210.c
@@ -90,7 +90,7 @@ static const struct reset_control_ops k210_rst_ops = {
 static int k210_rst_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *parent_np = of_get_parent(dev->of_node);
+	struct device_node *parent_np;
 	struct k210_rst *ksr;
 
 	dev_info(dev, "K210 reset controller\n");
@@ -99,6 +99,7 @@ static int k210_rst_probe(struct platform_device *pdev)
 	if (!ksr)
 		return -ENOMEM;
 
+	parent_np = of_get_parent(dev->of_node);
 	ksr->map = syscon_node_to_regmap(parent_np);
 	of_node_put(parent_np);
 	if (IS_ERR(ksr->map))

-- 
2.43.0

Re: [PATCH 2/5] reset: k210: fix OF node leak in probe() error path

[Damien Le Moal]

On 8/25/24 11:14 PM, Krzysztof Kozlowski wrote:
> Driver is leaking OF node reference on memory allocation failure.
> Acquire the OF node reference after memory allocation to fix this and
> keep it simple.
> 
> Fixes: 5a2308da9f60 ("riscv: Add Canaan Kendryte K210 reset controller")
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 3/5] reset: simplify locking with guard()

[Krzysztof Kozlowski]

Simplify error handling (less gotos) over locks with guard().

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/reset/core.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index dba74e857be6..c9074810306c 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -916,20 +916,18 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	 */
 	lockdep_assert_not_held(&reset_list_mutex);
 
-	mutex_lock(&reset_gpio_lookup_mutex);
+	guard(mutex)(&reset_gpio_lookup_mutex);
 
 	list_for_each_entry(rgpio_dev, &reset_gpio_lookup_list, list) {
 		if (args->np == rgpio_dev->of_args.np) {
 			if (of_phandle_args_equal(args, &rgpio_dev->of_args))
-				goto out; /* Already on the list, done */
+				return 0; /* Already on the list, done */
 		}
 	}
 
 	id = ida_alloc(&reset_gpio_ida, GFP_KERNEL);
-	if (id < 0) {
-		ret = id;
-		goto err_unlock;
-	}
+	if (id < 0)
+		return id;
 
 	/* Not freed on success, because it is persisent subsystem data. */
 	rgpio_dev = kzalloc(sizeof(*rgpio_dev), GFP_KERNEL);
@@ -959,9 +957,6 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 
 	list_add(&rgpio_dev->list, &reset_gpio_lookup_list);
 
-out:
-	mutex_unlock(&reset_gpio_lookup_mutex);
-
 	return 0;
 
 err_put:
@@ -970,8 +965,6 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	kfree(rgpio_dev);
 err_ida_free:
 	ida_free(&reset_gpio_ida, id);
-err_unlock:
-	mutex_unlock(&reset_gpio_lookup_mutex);
 
 	return ret;
 }

-- 
2.43.0

Re: [PATCH 3/5] reset: simplify locking with guard()

[Philipp Zabel]

On So, 2024-08-25 at 16:14 +0200, Krzysztof Kozlowski wrote:
> Simplify error handling (less gotos) over locks with guard().
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

[PATCH 4/5] reset: lpc18xx: simplify with dev_err_probe()

[Krzysztof Kozlowski]

Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
the code also simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/reset/reset-lpc18xx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
index 28fb85772b3e..e7896e3f1851 100644
--- a/drivers/reset/reset-lpc18xx.c
+++ b/drivers/reset/reset-lpc18xx.c
@@ -151,16 +151,14 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
 		return PTR_ERR(rc->base);
 
 	rc->clk_reg = devm_clk_get(&pdev->dev, "reg");
-	if (IS_ERR(rc->clk_reg)) {
-		dev_err(&pdev->dev, "reg clock not found\n");
-		return PTR_ERR(rc->clk_reg);
-	}
+	if (IS_ERR(rc->clk_reg))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rc->clk_reg),
+				     "reg clock not found\n");
 
 	rc->clk_delay = devm_clk_get(&pdev->dev, "delay");
-	if (IS_ERR(rc->clk_delay)) {
-		dev_err(&pdev->dev, "delay clock not found\n");
-		return PTR_ERR(rc->clk_delay);
-	}
+	if (IS_ERR(rc->clk_delay))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rc->clk_delay),
+				     "delay clock not found\n");
 
 	ret = clk_prepare_enable(rc->clk_reg);
 	if (ret) {

-- 
2.43.0

Re: [PATCH 4/5] reset: lpc18xx: simplify with dev_err_probe()

[Vladimir Zapolskiy]

On 8/25/24 17:14, Krzysztof Kozlowski wrote:
> Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
> the code also simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Acked-by: Vladimir Zapolskiy <vz@mleia.com>
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

--
Best wishes,
Vladimir

Re: [PATCH 4/5] reset: lpc18xx: simplify with dev_err_probe()

[Damien Le Moal]

On 8/25/24 11:14 PM, Krzysztof Kozlowski wrote:
> Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
> the code also simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 5/5] reset: lpc18xx: simplify with devm_clk_get_enabled()

[Krzysztof Kozlowski]

Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
make code simpler.  Change to dev_err_probe() in handling
reset_controller_register() error to make it even simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/reset/reset-lpc18xx.c | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
index e7896e3f1851..e42b2f24a93d 100644
--- a/drivers/reset/reset-lpc18xx.c
+++ b/drivers/reset/reset-lpc18xx.c
@@ -150,28 +150,16 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
 	if (IS_ERR(rc->base))
 		return PTR_ERR(rc->base);
 
-	rc->clk_reg = devm_clk_get(&pdev->dev, "reg");
+	rc->clk_reg = devm_clk_get_enabled(&pdev->dev, "reg");
 	if (IS_ERR(rc->clk_reg))
 		return dev_err_probe(&pdev->dev, PTR_ERR(rc->clk_reg),
 				     "reg clock not found\n");
 
-	rc->clk_delay = devm_clk_get(&pdev->dev, "delay");
+	rc->clk_delay = devm_clk_get_enabled(&pdev->dev, "delay");
 	if (IS_ERR(rc->clk_delay))
 		return dev_err_probe(&pdev->dev, PTR_ERR(rc->clk_delay),
 				     "delay clock not found\n");
 
-	ret = clk_prepare_enable(rc->clk_reg);
-	if (ret) {
-		dev_err(&pdev->dev, "unable to enable reg clock\n");
-		return ret;
-	}
-
-	ret = clk_prepare_enable(rc->clk_delay);
-	if (ret) {
-		dev_err(&pdev->dev, "unable to enable delay clock\n");
-		goto dis_clk_reg;
-	}
-
 	fcclk = clk_get_rate(rc->clk_reg) / USEC_PER_SEC;
 	firc = clk_get_rate(rc->clk_delay) / USEC_PER_SEC;
 	if (fcclk == 0 || firc == 0)
@@ -187,10 +175,8 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
 	rc->rcdev.of_node = pdev->dev.of_node;
 
 	ret = reset_controller_register(&rc->rcdev);
-	if (ret) {
-		dev_err(&pdev->dev, "unable to register device\n");
-		goto dis_clks;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "unable to register device\n");
 
 	rc->restart_nb.priority = 192,
 	rc->restart_nb.notifier_call = lpc18xx_rgu_restart,
@@ -199,13 +185,6 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "failed to register restart handler\n");
 
 	return 0;
-
-dis_clks:
-	clk_disable_unprepare(rc->clk_delay);
-dis_clk_reg:
-	clk_disable_unprepare(rc->clk_reg);
-
-	return ret;
 }
 
 static const struct of_device_id lpc18xx_rgu_match[] = {

-- 
2.43.0

Re: [PATCH 5/5] reset: lpc18xx: simplify with devm_clk_get_enabled()

[Vladimir Zapolskiy]

On 8/25/24 17:14, Krzysztof Kozlowski wrote:
> Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
> make code simpler.  Change to dev_err_probe() in handling
> reset_controller_register() error to make it even simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Acked-by: Vladimir Zapolskiy <vz@mleia.com>
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

--
Best wishes,
Vladimir

Re: [PATCH 5/5] reset: lpc18xx: simplify with devm_clk_get_enabled()

[Damien Le Moal]

On 8/25/24 11:14 PM, Krzysztof Kozlowski wrote:
> Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
> make code simpler.  Change to dev_err_probe() in handling

s/code/the code

> reset_controller_register() error to make it even simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Other than this nit, looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/5] reset: cleanup and simplify with devm and scoped

[Damien Le Moal]

On 8/25/24 11:14 PM, Krzysztof Kozlowski wrote:
> Hi,
> 
> Two simple fixes, which do not cover real scenario (memory allocation
> failure during probe), thus not marking CC-stable. Rest of patchset is
> simplifying of code.

Patches that have a Fixes tag will likely be picked up for stable by Sasha's
bot anyway... Not a problem for these patches I think.

> 
> Best regards,
> Krzysztof
> 
> ---
> Krzysztof Kozlowski (5):
>       reset: berlin: fix OF node leak in probe() error path
>       reset: k210: fix OF node leak in probe() error path
>       reset: simplify locking with guard()
>       reset: lpc18xx: simplify with dev_err_probe()
>       reset: lpc18xx: simplify with devm_clk_get_enabled()
> 
>  drivers/reset/core.c          | 15 ++++-----------
>  drivers/reset/reset-berlin.c  |  3 ++-
>  drivers/reset/reset-k210.c    |  3 ++-
>  drivers/reset/reset-lpc18xx.c | 43 ++++++++++---------------------------------
>  4 files changed, 18 insertions(+), 46 deletions(-)
> ---
> base-commit: e706b1fe2384d38e6e9edfb6d9e11e26873c24c7
> change-id: 20240825-reset-cleanup-scoped-64bb3cb6157f
> 
> Best regards,

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/5] reset: cleanup and simplify with devm and scoped

[Philipp Zabel]

On So, 2024-08-25 at 16:14 +0200, Krzysztof Kozlowski wrote:
> Hi,
> 
> Two simple fixes, which do not cover real scenario (memory allocation
> failure during probe), thus not marking CC-stable. Rest of patchset is
> simplifying of code.

Thank you, applied to reset/next with Damien's suggested change.

regards
Philipp