[PATCH] pwm: omap-dmtimer: Add missing put_device() call in pwm_omap_dmtimer_probe()

[PATCH] pwm: omap-dmtimer: Add missing put_device() call in pwm_omap_dmtimer_probe()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 9 Nov 2019 13:09:42 +0100

A coccicheck run provided information like the following.

drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
call of_find_device_by_node on line 255, but without a corresponding
object release within this function.

Generated by: scripts/coccinelle/free/put_device.cocci

Thus add jump targets to fix the exception handling for this
function implementation.

Fixes: b7290cf6ff7869ec12070aa146c370728cab62c2 ("pwm: pwm-omap-dmtimer: Adapt driver to utilize dmtimer pdata ops")
Fixes: 6604c6556db9e41c85f2839f66bd9d617bcf9f87 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/pwm/pwm-omap-dmtimer.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 00772fc53490..958854213786 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -301,12 +301,13 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 put:
 	of_node_put(timer);
 	if (ret < 0)
-		return ret;
+		goto check_timer_pdev;

 	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
 	if (!omap) {
 		pdata->free(dm_timer);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto put_device;
 	}

 	omap->pdata = pdata;
@@ -340,12 +341,19 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register PWM\n");
 		omap->pdata->free(omap->dm_timer);
-		return ret;
+		goto put_device;
 	}

 	platform_set_drvdata(pdev, omap);

 	return 0;
+
+check_timer_pdev:
+	if (timer_pdev)
+put_device:
+		put_device(&timer_pdev->dev);
+
+	return ret;
 }

 static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
--
2.24.0

[PATCH] pwm: omap-dmtimer: Add missing put_device() call in pwm_omap_dmtimer_probe()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 9 Nov 2019 13:09:42 +0100

A coccicheck run provided information like the following.

drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
call of_find_device_by_node on line 255, but without a corresponding
object release within this function.

Generated by: scripts/coccinelle/free/put_device.cocci

Thus add jump targets to fix the exception handling for this
function implementation.

Fixes: b7290cf6ff7869ec12070aa146c370728cab62c2 ("pwm: pwm-omap-dmtimer: Adapt driver to utilize dmtimer pdata ops")
Fixes: 6604c6556db9e41c85f2839f66bd9d617bcf9f87 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/pwm/pwm-omap-dmtimer.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 00772fc53490..958854213786 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -301,12 +301,13 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 put:
 	of_node_put(timer);
 	if (ret < 0)
-		return ret;
+		goto check_timer_pdev;

 	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
 	if (!omap) {
 		pdata->free(dm_timer);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto put_device;
 	}

 	omap->pdata = pdata;
@@ -340,12 +341,19 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register PWM\n");
 		omap->pdata->free(omap->dm_timer);
-		return ret;
+		goto put_device;
 	}

 	platform_set_drvdata(pdev, omap);

 	return 0;
+
+check_timer_pdev:
+	if (timer_pdev)
+put_device:
+		put_device(&timer_pdev->dev);
+
+	return ret;
 }

 static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
--
2.24.0

Re: [PATCH] pwm: omap-dmtimer: Add missing put_device() call in pwm_omap_dmtimer_probe()

[Uwe Kleine-König]

Hello Markus,

On Sat, Nov 09, 2019 at 01:26:50PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 9 Nov 2019 13:09:42 +0100
> 
> A coccicheck run provided information like the following.
> 
> drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
> call of_find_device_by_node on line 255, but without a corresponding
> object release within this function.
> 
> Generated by: scripts/coccinelle/free/put_device.cocci
> 
> Thus add jump targets to fix the exception handling for this
> function implementation.
> 
> Fixes: b7290cf6ff7869ec12070aa146c370728cab62c2 ("pwm: pwm-omap-dmtimer: Adapt driver to utilize dmtimer pdata ops")
> Fixes: 6604c6556db9e41c85f2839f66bd9d617bcf9f87 ("pwm: Add PWM driver for OMAP using dual-mode timers")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index 00772fc53490..958854213786 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -301,12 +301,13 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  put:
>  	of_node_put(timer);
>  	if (ret < 0)
> -		return ret;
> +		goto check_timer_pdev;
> 
>  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
>  	if (!omap) {
>  		pdata->free(dm_timer);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto put_device;
>  	}
> 
>  	omap->pdata = pdata;
> @@ -340,12 +341,19 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to register PWM\n");
>  		omap->pdata->free(omap->dm_timer);
> -		return ret;
> +		goto put_device;
>  	}
> 
>  	platform_set_drvdata(pdev, omap);
> 
>  	return 0;
> +
> +check_timer_pdev:
> +	if (timer_pdev)
> +put_device:
> +		put_device(&timer_pdev->dev);

This is ugly but necessary with the driver as is because the error
handling is interwinded within the normal path through this function.

I would prefer to clean this up first, then this fix gets a bit nicer.
Will send a patch in reply to this mail.

Best regards
Uwe

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

Re: [PATCH] pwm: omap-dmtimer: Add missing put_device() call in pwm_omap_dmtimer_probe()

[Uwe Kleine-König]

Hello Markus,

On Sat, Nov 09, 2019 at 01:26:50PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 9 Nov 2019 13:09:42 +0100
> 
> A coccicheck run provided information like the following.
> 
> drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
> call of_find_device_by_node on line 255, but without a corresponding
> object release within this function.
> 
> Generated by: scripts/coccinelle/free/put_device.cocci
> 
> Thus add jump targets to fix the exception handling for this
> function implementation.
> 
> Fixes: b7290cf6ff7869ec12070aa146c370728cab62c2 ("pwm: pwm-omap-dmtimer: Adapt driver to utilize dmtimer pdata ops")
> Fixes: 6604c6556db9e41c85f2839f66bd9d617bcf9f87 ("pwm: Add PWM driver for OMAP using dual-mode timers")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index 00772fc53490..958854213786 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -301,12 +301,13 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  put:
>  	of_node_put(timer);
>  	if (ret < 0)
> -		return ret;
> +		goto check_timer_pdev;
> 
>  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
>  	if (!omap) {
>  		pdata->free(dm_timer);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto put_device;
>  	}
> 
>  	omap->pdata = pdata;
> @@ -340,12 +341,19 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to register PWM\n");
>  		omap->pdata->free(omap->dm_timer);
> -		return ret;
> +		goto put_device;
>  	}
> 
>  	platform_set_drvdata(pdev, omap);
> 
>  	return 0;
> +
> +check_timer_pdev:
> +	if (timer_pdev)
> +put_device:
> +		put_device(&timer_pdev->dev);

This is ugly but necessary with the driver as is because the error
handling is interwinded within the normal path through this function.

I would prefer to clean this up first, then this fix gets a bit nicer.
Will send a patch in reply to this mail.

Best regards
Uwe

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

[PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Uwe Kleine-König]

In the old code (e.g.) mutex_destroy() was called before
pwmchip_remove(). Between these two calls it is possible that a pwm
callback is used which tries to grab the mutex.

Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-omap-dmtimer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 00772fc53490..bdf94c78655f 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
 {
 	struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&omap->chip);
+	if (ret)
+		return ret;
 
 	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
 		omap->pdata->stop(omap->dm_timer);
@@ -359,7 +364,7 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
 
 	mutex_destroy(&omap->mutex);
 
-	return pwmchip_remove(&omap->chip);
+	return 0;
 }
 
 static const struct of_device_id pwm_omap_dmtimer_of_match[] = {
-- 
2.23.0

[PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Uwe Kleine-König]

In the old code (e.g.) mutex_destroy() was called before
pwmchip_remove(). Between these two calls it is possible that a pwm
callback is used which tries to grab the mutex.

Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-omap-dmtimer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 00772fc53490..bdf94c78655f 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
 {
 	struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&omap->chip);
+	if (ret)
+		return ret;
 
 	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
 		omap->pdata->stop(omap->dm_timer);
@@ -359,7 +364,7 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
 
 	mutex_destroy(&omap->mutex);
 
-	return pwmchip_remove(&omap->chip);
+	return 0;
 }
 
 static const struct of_device_id pwm_omap_dmtimer_of_match[] = {
-- 
2.23.0

[PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Uwe Kleine-König]

Instead of doing error handling in the middle of
pwm_omap_dmtimer_probe() move error handling and freeing the reference
to timer to the end.

This fixes a resource leak as dm_timer wasn't freed when allocating
*omap failed.

Implementation note: The put: label was never reached without a goto and
ret being unequal to 0, so the removed return statement is fine.

Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-omap-dmtimer.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index bdf94c78655f..e36fcad668a6 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -298,15 +298,10 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 		goto put;
 	}
 
-put:
-	of_node_put(timer);
-	if (ret < 0)
-		return ret;
-
 	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
 	if (!omap) {
-		pdata->free(dm_timer);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_alloc_omap;
 	}
 
 	omap->pdata = pdata;
@@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&omap->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register PWM\n");
-		omap->pdata->free(omap->dm_timer);
-		return ret;
+		goto err_pwmchip_add;
 	}
 
+	of_node_put(timer);
+
 	platform_set_drvdata(pdev, omap);
 
 	return 0;
+
+err_pwmchip_add:
+
+	/*
+	 * *omap is allocated using devm_kzalloc,
+	 * so no free necessary here
+	 */
+err_alloc_omap:
+
+	pdata->free(dm_timer);
+put:
+	of_node_put(timer);
+
+	return ret;
 }
 
 static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
-- 
2.23.0

[PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Uwe Kleine-König]

Instead of doing error handling in the middle of
pwm_omap_dmtimer_probe() move error handling and freeing the reference
to timer to the end.

This fixes a resource leak as dm_timer wasn't freed when allocating
*omap failed.

Implementation note: The put: label was never reached without a goto and
ret being unequal to 0, so the removed return statement is fine.

Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-omap-dmtimer.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index bdf94c78655f..e36fcad668a6 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -298,15 +298,10 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 		goto put;
 	}
 
-put:
-	of_node_put(timer);
-	if (ret < 0)
-		return ret;
-
 	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
 	if (!omap) {
-		pdata->free(dm_timer);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_alloc_omap;
 	}
 
 	omap->pdata = pdata;
@@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&omap->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register PWM\n");
-		omap->pdata->free(omap->dm_timer);
-		return ret;
+		goto err_pwmchip_add;
 	}
 
+	of_node_put(timer);
+
 	platform_set_drvdata(pdev, omap);
 
 	return 0;
+
+err_pwmchip_add:
+
+	/*
+	 * *omap is allocated using devm_kzalloc,
+	 * so no free necessary here
+	 */
+err_alloc_omap:
+
+	pdata->free(dm_timer);
+put:
+	of_node_put(timer);
+
+	return ret;
 }
 
 static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
-- 
2.23.0

[PATCH 3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Uwe Kleine-König]

This was found by coccicheck:

	drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
	call of_find_device_by_node on line 255, but without a corresponding
	object release within this function.

Reported-by: Markus Elfring <elfring@users.sourceforge.net>
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-omap-dmtimer.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index e36fcad668a6..88a3c5690fea 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -256,7 +256,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	if (!timer_pdev) {
 		dev_err(&pdev->dev, "Unable to find Timer pdev\n");
 		ret = -ENODEV;
-		goto put;
+		goto err_find_timer_pdev;
 	}
 
 	timer_pdata = dev_get_platdata(&timer_pdev->dev);
@@ -264,7 +264,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 		dev_dbg(&pdev->dev,
 			 "dmtimer pdata structure NULL, deferring probe\n");
 		ret = -EPROBE_DEFER;
-		goto put;
+		goto err_platdata;
 	}
 
 	pdata = timer_pdata->timer_ops;
@@ -283,19 +283,19 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	    !pdata->write_counter) {
 		dev_err(&pdev->dev, "Incomplete dmtimer pdata structure\n");
 		ret = -EINVAL;
-		goto put;
+		goto err_platdata;
 	}
 
 	if (!of_get_property(timer, "ti,timer-pwm", NULL)) {
 		dev_err(&pdev->dev, "Missing ti,timer-pwm capability\n");
 		ret = -ENODEV;
-		goto put;
+		goto err_timer_property;
 	}
 
 	dm_timer = pdata->request_by_node(timer);
 	if (!dm_timer) {
 		ret = -EPROBE_DEFER;
-		goto put;
+		goto err_request_timer;
 	}
 
 	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
@@ -352,7 +352,14 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 err_alloc_omap:
 
 	pdata->free(dm_timer);
-put:
+err_request_timer:
+
+err_timer_property:
+err_platdata:
+
+	put_device(&timer_pdev->dev);
+err_find_timer_pdev:
+
 	of_node_put(timer);
 
 	return ret;
@@ -372,6 +379,8 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
 
 	omap->pdata->free(omap->dm_timer);
 
+	put_device(&omap->dm_timer_pdev->dev);
+
 	mutex_destroy(&omap->mutex);
 
 	return 0;
-- 
2.23.0

[PATCH 3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Uwe Kleine-König]

This was found by coccicheck:

	drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
	call of_find_device_by_node on line 255, but without a corresponding
	object release within this function.

Reported-by: Markus Elfring <elfring@users.sourceforge.net>
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-omap-dmtimer.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index e36fcad668a6..88a3c5690fea 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -256,7 +256,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	if (!timer_pdev) {
 		dev_err(&pdev->dev, "Unable to find Timer pdev\n");
 		ret = -ENODEV;
-		goto put;
+		goto err_find_timer_pdev;
 	}
 
 	timer_pdata = dev_get_platdata(&timer_pdev->dev);
@@ -264,7 +264,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 		dev_dbg(&pdev->dev,
 			 "dmtimer pdata structure NULL, deferring probe\n");
 		ret = -EPROBE_DEFER;
-		goto put;
+		goto err_platdata;
 	}
 
 	pdata = timer_pdata->timer_ops;
@@ -283,19 +283,19 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	    !pdata->write_counter) {
 		dev_err(&pdev->dev, "Incomplete dmtimer pdata structure\n");
 		ret = -EINVAL;
-		goto put;
+		goto err_platdata;
 	}
 
 	if (!of_get_property(timer, "ti,timer-pwm", NULL)) {
 		dev_err(&pdev->dev, "Missing ti,timer-pwm capability\n");
 		ret = -ENODEV;
-		goto put;
+		goto err_timer_property;
 	}
 
 	dm_timer = pdata->request_by_node(timer);
 	if (!dm_timer) {
 		ret = -EPROBE_DEFER;
-		goto put;
+		goto err_request_timer;
 	}
 
 	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
@@ -352,7 +352,14 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 err_alloc_omap:
 
 	pdata->free(dm_timer);
-put:
+err_request_timer:
+
+err_timer_property:
+err_platdata:
+
+	put_device(&timer_pdev->dev);
+err_find_timer_pdev:
+
 	of_node_put(timer);
 
 	return ret;
@@ -372,6 +379,8 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
 
 	omap->pdata->free(omap->dm_timer);
 
+	put_device(&omap->dm_timer_pdev->dev);
+
 	mutex_destroy(&omap->mutex);
 
 	return 0;
-- 
2.23.0

[PATCH 4/4] pwm: omap-dmtimer: Allow compiling with COMPILE_TEST

[Uwe Kleine-König]

The dependency on OMAP_DM_TIMER is only a runtime dependency. Also
OMAP_DM_TIMER cannot be enabled without ARCH_OMAP being enabled.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e3a2518503ed..e49cbdb1ce29 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -328,7 +328,8 @@ config PWM_MXS
 
 config PWM_OMAP_DMTIMER
 	tristate "OMAP Dual-Mode Timer PWM support"
-	depends on OF && ARCH_OMAP && OMAP_DM_TIMER
+	depends on OF
+	depends on OMAP_DM_TIMER || COMPILE_TEST
 	help
 	  Generic PWM framework driver for OMAP Dual-Mode Timer PWM output
 
-- 
2.23.0

[PATCH 4/4] pwm: omap-dmtimer: Allow compiling with COMPILE_TEST

[Uwe Kleine-König]

The dependency on OMAP_DM_TIMER is only a runtime dependency. Also
OMAP_DM_TIMER cannot be enabled without ARCH_OMAP being enabled.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e3a2518503ed..e49cbdb1ce29 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -328,7 +328,8 @@ config PWM_MXS
 
 config PWM_OMAP_DMTIMER
 	tristate "OMAP Dual-Mode Timer PWM support"
-	depends on OF && ARCH_OMAP && OMAP_DM_TIMER
+	depends on OF
+	depends on OMAP_DM_TIMER || COMPILE_TEST
 	help
 	  Generic PWM framework driver for OMAP Dual-Mode Timer PWM output
 
-- 
2.23.0

Re: [PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Uwe Kleine-König]

Hello,

I created a cover letter but failed to send it together with the series
:-|

It said:

| I promised a cleanup patch but as I found a few more issues I have four
| patches now. The third patch replaces Markus' patch with a more complete
| fix that also drops the reference in .remove not only the error path of
| .probe.
| 
| The last patch allows to compile the driver in more configurations, it
| compiled successfully on amd64 and arm.

Best regards
Uwe

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

Re: [PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Uwe Kleine-König]

Hello,

I created a cover letter but failed to send it together with the series
:-|

It said:

| I promised a cleanup patch but as I found a few more issues I have four
| patches now. The third patch replaces Markus' patch with a more complete
| fix that also drops the reference in .remove not only the error path of
| .probe.
| 
| The last patch allows to compile the driver in more configurations, it
| compiled successfully on amd64 and arm.

Best regards
Uwe

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

Re: [0/4] pwm: omap-dmtimer: Software improvements

[Markus Elfring]

> I created a cover letter but failed to send it together with the series
> :-|

Did you omit the address “linux-kernel@vger.kernel.org” from
the recipient list intentionally?

Regards,
Markus

Re: [0/4] pwm: omap-dmtimer: Software improvements

[Markus Elfring]

> I created a cover letter but failed to send it together with the series
> :-|

Did you omit the address “linux-kernel@vger.kernel.org” from
the recipient list intentionally?

Regards,
Markus

Re: [0/4] pwm: omap-dmtimer: Software improvements

[Markus Elfring]

> I created a cover letter but failed to send it together with the series
> :-|

Did you omit the address “linux-kernel@vger.kernel.org” from
the recipient list intentionally?

Regards,
Markus

Re: [0/4] pwm: omap-dmtimer: Software improvements

[Markus Elfring]

> I created a cover letter but failed to send it together with the series
> :-|

Did you omit the address “linux-kernel@vger.kernel.org” from
the recipient list intentionally?

Regards,
Markus

Re: [PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Markus Elfring]

> In the old code (e.g.) mutex_destroy() was called before
> pwmchip_remove(). Between these two calls it is possible that a pwm
> callback is used which tries to grab the mutex.

How do you think about to add a more “imperative mood” for your
change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151


> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
>  {
>  	struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&omap->chip);
> +	if (ret)
> +		return ret;
>
>  	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
>  		omap->pdata->stop(omap->dm_timer);

How do you think about to use the following statement variant?

+	int ret = pwmchip_remove(&omap->chip);

Regards,
Markus

Re: [PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Markus Elfring]

> In the old code (e.g.) mutex_destroy() was called before
> pwmchip_remove(). Between these two calls it is possible that a pwm
> callback is used which tries to grab the mutex.

How do you think about to add a more “imperative mood” for your
change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151


> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
>  {
>  	struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&omap->chip);
> +	if (ret)
> +		return ret;
>
>  	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
>  		omap->pdata->stop(omap->dm_timer);

How do you think about to use the following statement variant?

+	int ret = pwmchip_remove(&omap->chip);

Regards,
Markus

Re: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Markus Elfring]

> Implementation note: The put: label was never reached without a goto and
> ret being unequal to 0, so the removed return statement is fine.

This can look fine (in principle) because the label was repositioned here.
Do you really want to call the function “of_node_put” at two places now?


> +++ b/drivers/pwm/pwm-omap-dmtimer.c
…
>  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
>  	if (!omap) {
> -		pdata->free(dm_timer);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_alloc_omap;
>  	}
…

I suggest to reconsider your label name selection according to
the Linux coding style.


> @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
…
> +err_pwmchip_add:
> +
> +	/*
> +	 * *omap is allocated using devm_kzalloc,
> +	 * so no free necessary here
> +	 */
> +err_alloc_omap:
> +
> +	pdata->free(dm_timer);

Would the use of the label “free_dm_timer” be more appropriate?


> +put:
> +	of_node_put(timer);
…

Can the label “put_node” be nicer?

Regards,
Markus

Re: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Markus Elfring]

> Implementation note: The put: label was never reached without a goto and
> ret being unequal to 0, so the removed return statement is fine.

This can look fine (in principle) because the label was repositioned here.
Do you really want to call the function “of_node_put” at two places now?


> +++ b/drivers/pwm/pwm-omap-dmtimer.c
…
>  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
>  	if (!omap) {
> -		pdata->free(dm_timer);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_alloc_omap;
>  	}
…

I suggest to reconsider your label name selection according to
the Linux coding style.


> @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
…
> +err_pwmchip_add:
> +
> +	/*
> +	 * *omap is allocated using devm_kzalloc,
> +	 * so no free necessary here
> +	 */
> +err_alloc_omap:
> +
> +	pdata->free(dm_timer);

Would the use of the label “free_dm_timer” be more appropriate?


> +put:
> +	of_node_put(timer);
…

Can the label “put_node” be nicer?

Regards,
Markus

Re: [PATCH 3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Markus Elfring]

> This was found by coccicheck:
>
> 	drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
> 	call of_find_device_by_node on line 255, but without a corresponding
> 	object release within this function.

How do you think about to add a wording according to “imperative mood”
for your change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

…
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
…
> @@ -352,7 +352,14 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
…
>  	pdata->free(dm_timer);
> -put:
> +err_request_timer:
> +
> +err_timer_property:
> +err_platdata:
> +
> +	put_device(&timer_pdev->dev);

Would the use of the label “put_device” be more appropriate?


> +err_find_timer_pdev:
> +
>  	of_node_put(timer);
…

Would the use of the label “put_node” be better here?


> @@ -372,6 +379,8 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
>
>  	omap->pdata->free(omap->dm_timer);
>
> +	put_device(&omap->dm_timer_pdev->dev);
> +
>  	mutex_destroy(&omap->mutex);
>
>  	return 0;

I suggest to omit a few blank lines.

Regards,
Markus

Re: [PATCH 3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Markus Elfring]

> This was found by coccicheck:
>
> 	drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
> 	call of_find_device_by_node on line 255, but without a corresponding
> 	object release within this function.

How do you think about to add a wording according to “imperative mood”
for your change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

…
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
…
> @@ -352,7 +352,14 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
…
>  	pdata->free(dm_timer);
> -put:
> +err_request_timer:
> +
> +err_timer_property:
> +err_platdata:
> +
> +	put_device(&timer_pdev->dev);

Would the use of the label “put_device” be more appropriate?


> +err_find_timer_pdev:
> +
>  	of_node_put(timer);
…

Would the use of the label “put_node” be better here?


> @@ -372,6 +379,8 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
>
>  	omap->pdata->free(omap->dm_timer);
>
> +	put_device(&omap->dm_timer_pdev->dev);
> +
>  	mutex_destroy(&omap->mutex);
>
>  	return 0;

I suggest to omit a few blank lines.

Regards,
Markus

Re: [PATCH 4/4] pwm: omap-dmtimer: Allow compiling with COMPILE_TEST

[Markus Elfring]

> The dependency on OMAP_DM_TIMER is only a runtime dependency. Also
> OMAP_DM_TIMER cannot be enabled without ARCH_OMAP being enabled.

Will a more “imperative mood” become relevant also for this change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

Regards,
Markus

Re: [PATCH 4/4] pwm: omap-dmtimer: Allow compiling with COMPILE_TEST

[Markus Elfring]

> The dependency on OMAP_DM_TIMER is only a runtime dependency. Also
> OMAP_DM_TIMER cannot be enabled without ARCH_OMAP being enabled.

Will a more “imperative mood” become relevant also for this change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

Regards,
Markus

Re: [PATCH 4/4] pwm: omap-dmtimer: Allow compiling with COMPILE_TEST

[Markus Elfring]

> The dependency on OMAP_DM_TIMER is only a runtime dependency. Also
> OMAP_DM_TIMER cannot be enabled without ARCH_OMAP being enabled.

Will a more “imperative mood” become relevant also for this change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

Regards,
Markus

Re: [PATCH 4/4] pwm: omap-dmtimer: Allow compiling with COMPILE_TEST

[Markus Elfring]

> The dependency on OMAP_DM_TIMER is only a runtime dependency. Also
> OMAP_DM_TIMER cannot be enabled without ARCH_OMAP being enabled.

Will a more “imperative mood” become relevant also for this change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

Regards,
Markus

Re: [0/4] pwm: omap-dmtimer: Software improvements

[Uwe Kleine-König]

On Mon, Nov 11, 2019 at 02:28:52PM +0100, Markus Elfring wrote:
> > I created a cover letter but failed to send it together with the series
> > :-|
> 
> Did you omit the address “linux-kernel@vger.kernel.org” from
> the recipient list intentionally?

Yes, even though it's documented to Cc: all patches there, there is IMHO
little use. If there is a dedicated mailing list, not adding lkml is
fine for all maintainers I interacted with in the last few years.

The volume on lkml is that high that sending all patches there only adds
noise.

Best regards
Uwe

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

Re: [0/4] pwm: omap-dmtimer: Software improvements

[Uwe Kleine-König]

On Mon, Nov 11, 2019 at 02:28:52PM +0100, Markus Elfring wrote:
> > I created a cover letter but failed to send it together with the series
> > :-|
> 
> Did you omit the address “linux-kernel@vger.kernel.org” from
> the recipient list intentionally?

Yes, even though it's documented to Cc: all patches there, there is IMHO
little use. If there is a dedicated mailing list, not adding lkml is
fine for all maintainers I interacted with in the last few years.

The volume on lkml is that high that sending all patches there only adds
noise.

Best regards
Uwe

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

Re: [PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Uwe Kleine-König]

Hello Markus,

On Mon, Nov 11, 2019 at 02:30:42PM +0100, Markus Elfring wrote:
> > In the old code (e.g.) mutex_destroy() was called before
> > pwmchip_remove(). Between these two calls it is possible that a pwm
> > callback is used which tries to grab the mutex.
> 
> How do you think about to add a more “imperative mood” for your
> change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

I described the old behaviour and like my wording.

> > +++ b/drivers/pwm/pwm-omap-dmtimer.c
> > @@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> >  static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
> >  {
> >  	struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwmchip_remove(&omap->chip);
> > +	if (ret)
> > +		return ret;
> >
> >  	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
> >  		omap->pdata->stop(omap->dm_timer);
> 
> How do you think about to use the following statement variant?
> 
> +	int ret = pwmchip_remove(&omap->chip);

I think that between the declarations and code should be an empty line
and between the assignment to ret and the respective check there
shouldn't be one.

Best regards
Uwe

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

Re: [PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Uwe Kleine-König]

Hello Markus,

On Mon, Nov 11, 2019 at 02:30:42PM +0100, Markus Elfring wrote:
> > In the old code (e.g.) mutex_destroy() was called before
> > pwmchip_remove(). Between these two calls it is possible that a pwm
> > callback is used which tries to grab the mutex.
> 
> How do you think about to add a more “imperative mood” for your
> change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id1f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

I described the old behaviour and like my wording.

> > +++ b/drivers/pwm/pwm-omap-dmtimer.c
> > @@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> >  static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
> >  {
> >  	struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwmchip_remove(&omap->chip);
> > +	if (ret)
> > +		return ret;
> >
> >  	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
> >  		omap->pdata->stop(omap->dm_timer);
> 
> How do you think about to use the following statement variant?
> 
> +	int ret = pwmchip_remove(&omap->chip);

I think that between the declarations and code should be an empty line
and between the assignment to ret and the respective check there
shouldn't be one.

Best regards
Uwe

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

Re: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Uwe Kleine-König]

On Mon, Nov 11, 2019 at 02:32:30PM +0100, Markus Elfring wrote:
> > Implementation note: The put: label was never reached without a goto and
> > ret being unequal to 0, so the removed return statement is fine.
> 
> This can look fine (in principle) because the label was repositioned here.
> Do you really want to call the function “of_node_put” at two places now?

Yes, this is in my eyes more sensible. Either you have the expected path
and the error path interwinded, or you have to duplicate some cleanup.
IMHO the latter variant is the one that is easier to understand and the
one where it's less likely to oversee a needed cleanup.

> > +++ b/drivers/pwm/pwm-omap-dmtimer.c
> …
> >  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
> >  	if (!omap) {
> > -		pdata->free(dm_timer);
> > -		return -ENOMEM;
> > +		ret = -ENOMEM;
> > +		goto err_alloc_omap;
> >  	}
> …
> 
> I suggest to reconsider your label name selection according to
> the Linux coding style.

Documentation/process/coding-style.rst states: "Choose label names which
say what the goto does or why the goto exists." So I'd say my names are
perfectly fine.

> > @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> …
> > +err_pwmchip_add:
> > +
> > +	/*
> > +	 * *omap is allocated using devm_kzalloc,
> > +	 * so no free necessary here
> > +	 */
> > +err_alloc_omap:
> > +
> > +	pdata->free(dm_timer);
> 
> Would the use of the label “free_dm_timer” be more appropriate?

Either you name your labels after what the code at the label does (then
"free_dm_timer" is good) or you name it after why you are here (and then
err_alloc_omap is fine). I prefer the latter style and then the label
name always has to correspond to the action just above it (if any).
That's why I grouped the "err_alloc_omap" label to a comment saying that
*omap doesn't need to be freed.

> > +put:
> > +	of_node_put(timer);
> …
> 
> Can the label “put_node” be nicer?

I agree that the label name is bad. I kept the name here and after the
3rd patch the label names are consistent. 

Best regards
Uwe

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

Re: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Uwe Kleine-König]

On Mon, Nov 11, 2019 at 02:32:30PM +0100, Markus Elfring wrote:
> > Implementation note: The put: label was never reached without a goto and
> > ret being unequal to 0, so the removed return statement is fine.
> 
> This can look fine (in principle) because the label was repositioned here.
> Do you really want to call the function “of_node_put” at two places now?

Yes, this is in my eyes more sensible. Either you have the expected path
and the error path interwinded, or you have to duplicate some cleanup.
IMHO the latter variant is the one that is easier to understand and the
one where it's less likely to oversee a needed cleanup.

> > +++ b/drivers/pwm/pwm-omap-dmtimer.c
> …
> >  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
> >  	if (!omap) {
> > -		pdata->free(dm_timer);
> > -		return -ENOMEM;
> > +		ret = -ENOMEM;
> > +		goto err_alloc_omap;
> >  	}
> …
> 
> I suggest to reconsider your label name selection according to
> the Linux coding style.

Documentation/process/coding-style.rst states: "Choose label names which
say what the goto does or why the goto exists." So I'd say my names are
perfectly fine.

> > @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> …
> > +err_pwmchip_add:
> > +
> > +	/*
> > +	 * *omap is allocated using devm_kzalloc,
> > +	 * so no free necessary here
> > +	 */
> > +err_alloc_omap:
> > +
> > +	pdata->free(dm_timer);
> 
> Would the use of the label “free_dm_timer” be more appropriate?

Either you name your labels after what the code at the label does (then
"free_dm_timer" is good) or you name it after why you are here (and then
err_alloc_omap is fine). I prefer the latter style and then the label
name always has to correspond to the action just above it (if any).
That's why I grouped the "err_alloc_omap" label to a comment saying that
*omap doesn't need to be freed.

> > +put:
> > +	of_node_put(timer);
> …
> 
> Can the label “put_node” be nicer?

I agree that the label name is bad. I kept the name here and after the
3rd patch the label names are consistent. 

Best regards
Uwe

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

Re: [PATCH 3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Uwe Kleine-König]

On Mon, Nov 11, 2019 at 02:41:58PM +0100, Markus Elfring wrote:
> > This was found by coccicheck:
> >
> > 	drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
> > 	call of_find_device_by_node on line 255, but without a corresponding
> > 	object release within this function.
> 
> How do you think about to add a wording according to “imperative mood”
> for your change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

Are you a bot?

> > +++ b/drivers/pwm/pwm-omap-dmtimer.c
> …
> > @@ -352,7 +352,14 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> …
> >  	pdata->free(dm_timer);
> > -put:
> > +err_request_timer:
> > +
> > +err_timer_property:
> > +err_platdata:
> > +
> > +	put_device(&timer_pdev->dev);
> 
> Would the use of the label “put_device” be more appropriate?
> 
> 
> > +err_find_timer_pdev:
> > +
> >  	of_node_put(timer);
> …
> 
> Would the use of the label “put_node” be better here?
> 
> 
> > @@ -372,6 +379,8 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
> >
> >  	omap->pdata->free(omap->dm_timer);
> >
> > +	put_device(&omap->dm_timer_pdev->dev);
> > +
> >  	mutex_destroy(&omap->mutex);
> >
> >  	return 0;
> 
> I suggest to omit a few blank lines.

And I like it the way it is.

Best regards
Uwe

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

Re: [PATCH 3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Uwe Kleine-König]

On Mon, Nov 11, 2019 at 02:41:58PM +0100, Markus Elfring wrote:
> > This was found by coccicheck:
> >
> > 	drivers/pwm/pwm-omap-dmtimer.c:304:2-8: ERROR: missing put_device;
> > 	call of_find_device_by_node on line 255, but without a corresponding
> > 	object release within this function.
> 
> How do you think about to add a wording according to “imperative mood”
> for your change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id1f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151

Are you a bot?

> > +++ b/drivers/pwm/pwm-omap-dmtimer.c
> …
> > @@ -352,7 +352,14 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> …
> >  	pdata->free(dm_timer);
> > -put:
> > +err_request_timer:
> > +
> > +err_timer_property:
> > +err_platdata:
> > +
> > +	put_device(&timer_pdev->dev);
> 
> Would the use of the label “put_device” be more appropriate?
> 
> 
> > +err_find_timer_pdev:
> > +
> >  	of_node_put(timer);
> …
> 
> Would the use of the label “put_node” be better here?
> 
> 
> > @@ -372,6 +379,8 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
> >
> >  	omap->pdata->free(omap->dm_timer);
> >
> > +	put_device(&omap->dm_timer_pdev->dev);
> > +
> >  	mutex_destroy(&omap->mutex);
> >
> >  	return 0;
> 
> I suggest to omit a few blank lines.

And I like it the way it is.

Best regards
Uwe

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

Re: [1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Markus Elfring]

>>> In the old code (e.g.) mutex_destroy() was called before
>>> pwmchip_remove(). Between these two calls it is possible that a pwm
>>> callback is used which tries to grab the mutex.
>>
>> How do you think about to add a more “imperative mood” for your
>> change description?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151
>
> I described the old behaviour and like my wording.

I find that the first paragraph contains useful information.
Would you like to specify any corresponding actions then
at this place?

Regards,
Markus

Re: [1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional

[Markus Elfring]

>>> In the old code (e.g.) mutex_destroy() was called before
>>> pwmchip_remove(). Between these two calls it is possible that a pwm
>>> callback is used which tries to grab the mutex.
>>
>> How do you think about to add a more “imperative mood” for your
>> change description?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151
>
> I described the old behaviour and like my wording.

I find that the first paragraph contains useful information.
Would you like to specify any corresponding actions then
at this place?

Regards,
Markus

Re: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Markus Elfring]

>> Do you really want to call the function “of_node_put” at two places now?
>
> Yes, this is in my eyes more sensible.

Thanks for this explanation.


> Either you have the expected path and the error path interwinded,
> and the error path interwinded,

This is also reasonable then.
This design approach provides the possibility to release a few resources
earlier before using additional functionality.


> or you have to duplicate some cleanup.

* This can be required.

* I imagine that specific software infrastructures can help to avoid
  such duplication, can't they?


> IMHO the latter variant is the one that is easier to understand and the
> one where it's less likely to oversee a needed cleanup.

I am curious on how the clarification will be continued.


>>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>> …
>>>  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
>>>  	if (!omap) {
>>> -		pdata->free(dm_timer);
>>> -		return -ENOMEM;
>>> +		ret = -ENOMEM;
>>> +		goto err_alloc_omap;
>>>  	}
>> …
>>
>> I suggest to reconsider your label name selection according to
>> the Linux coding style.
>
> Documentation/process/coding-style.rst states: "Choose label names which
> say what the goto does or why the goto exists." So I'd say my names are
> perfectly fine.

The guidance from the example after this quotation might be still too terse
so far, isn't it?


>>> @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>> …
>>> +err_pwmchip_add:
>>> +
>>> +	/*
>>> +	 * *omap is allocated using devm_kzalloc,
>>> +	 * so no free necessary here
>>> +	 */
>>> +err_alloc_omap:
>>> +
>>> +	pdata->free(dm_timer);
>>
>> Would the use of the label “free_dm_timer” be more appropriate?
>
> Either you name your labels after what the code at the label does
> (then "free_dm_timer" is good)

I got used to this approach.


> you name it after why you are here (and then err_alloc_omap is fine).

This choice can trigger special software design consequences.


> I prefer the latter style and then the label
> name always has to correspond to the action just above it (if any).
> That's why I grouped the "err_alloc_omap" label to a comment saying that
> *omap doesn't need to be freed.

I am also curious if any other contributors would like to share more
views around this choice.


>>> +put:
>>> +	of_node_put(timer);
>> …
>>
>> Can the label “put_node” be nicer?
>
> I agree that the label name is bad.

I find your agreement on this aspect interesting then.


> I kept the name here and after the 3rd patch the label names are consistent.

Can such an evolution be questionable?

Regards,
Markus

Re: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling

[Markus Elfring]

>> Do you really want to call the function “of_node_put” at two places now?
>
> Yes, this is in my eyes more sensible.

Thanks for this explanation.


> Either you have the expected path and the error path interwinded,
> and the error path interwinded,

This is also reasonable then.
This design approach provides the possibility to release a few resources
earlier before using additional functionality.


> or you have to duplicate some cleanup.

* This can be required.

* I imagine that specific software infrastructures can help to avoid
  such duplication, can't they?


> IMHO the latter variant is the one that is easier to understand and the
> one where it's less likely to oversee a needed cleanup.

I am curious on how the clarification will be continued.


>>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>> …
>>>  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
>>>  	if (!omap) {
>>> -		pdata->free(dm_timer);
>>> -		return -ENOMEM;
>>> +		ret = -ENOMEM;
>>> +		goto err_alloc_omap;
>>>  	}
>> …
>>
>> I suggest to reconsider your label name selection according to
>> the Linux coding style.
>
> Documentation/process/coding-style.rst states: "Choose label names which
> say what the goto does or why the goto exists." So I'd say my names are
> perfectly fine.

The guidance from the example after this quotation might be still too terse
so far, isn't it?


>>> @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>> …
>>> +err_pwmchip_add:
>>> +
>>> +	/*
>>> +	 * *omap is allocated using devm_kzalloc,
>>> +	 * so no free necessary here
>>> +	 */
>>> +err_alloc_omap:
>>> +
>>> +	pdata->free(dm_timer);
>>
>> Would the use of the label “free_dm_timer” be more appropriate?
>
> Either you name your labels after what the code at the label does
> (then "free_dm_timer" is good)

I got used to this approach.


> you name it after why you are here (and then err_alloc_omap is fine).

This choice can trigger special software design consequences.


> I prefer the latter style and then the label
> name always has to correspond to the action just above it (if any).
> That's why I grouped the "err_alloc_omap" label to a comment saying that
> *omap doesn't need to be freed.

I am also curious if any other contributors would like to share more
views around this choice.


>>> +put:
>>> +	of_node_put(timer);
>> …
>>
>> Can the label “put_node” be nicer?
>
> I agree that the label name is bad.

I find your agreement on this aspect interesting then.


> I kept the name here and after the 3rd patch the label names are consistent.

Can such an evolution be questionable?

Regards,
Markus

Re: [3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Markus Elfring]

>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151
>
> Are you a bot?

I hope not.

But I got used to the need to point specific suggestions out several times.
Would you like to mention any actions in the commit message explicitly?

Regards,
Markus

Re: [3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Markus Elfring]

>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151
>
> Are you a bot?

I hope not.

But I got used to the need to point specific suggestions out several times.
Would you like to mention any actions in the commit message explicitly?

Regards,
Markus

Re: [3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Uwe Kleine-König]

Hello,

On Mon, Nov 11, 2019 at 10:38:38PM +0100, Markus Elfring wrote:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151
> >
> > Are you a bot?
> 
> I hope not.
> 
> But I got used to the need to point specific suggestions out several times.

And are you also used to people ignore at least n-1 of n of these
repetitions? I don't feel that several near-identical mails that just
include a link to some documentation is very constructive. Also I got
some of your mails twice which doesn't improve the reception of your
comments.

> Would you like to mention any actions in the commit message explicitly?

I don't understand your question, but I assume the answer is "No, I want
to keep the commit log as is".

Best regards
Uwe

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

Re: [3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Uwe Kleine-König]

Hello,

On Mon, Nov 11, 2019 at 10:38:38PM +0100, Markus Elfring wrote:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id1f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151
> >
> > Are you a bot?
> 
> I hope not.
> 
> But I got used to the need to point specific suggestions out several times.

And are you also used to people ignore at least n-1 of n of these
repetitions? I don't feel that several near-identical mails that just
include a link to some documentation is very constructive. Also I got
some of your mails twice which doesn't improve the reception of your
comments.

> Would you like to mention any actions in the commit message explicitly?

I don't understand your question, but I assume the answer is "No, I want
to keep the commit log as is".

Best regards
Uwe

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

Re: [3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Markus Elfring]

>> Would you like to mention any actions in the commit message explicitly?
>
> I don't understand your question,

I hope that remaining communication difficulties will be resolved better.


> but I assume the answer is "No, I want to keep the commit log as is".

I suggest to take another look at the relevance of the corresponding
Linux development documentation then.

Regards,
Markus

Re: [3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node()

[Markus Elfring]

>> Would you like to mention any actions in the commit message explicitly?
>
> I don't understand your question,

I hope that remaining communication difficulties will be resolved better.


> but I assume the answer is "No, I want to keep the commit log as is".

I suggest to take another look at the relevance of the corresponding
Linux development documentation then.

Regards,
Markus