[PATCH 0/2] Changes for pwmchip_remove()

[PATCH 0/2] Changes for pwmchip_remove()

[Uwe Kleine-König]

pwmchip_remove currently can (theoretically) return an error code and
most drivers check for that and pass an error to the upper layer just to
be ignored there. Before device links were used this resulted in
half-registered drivers that failed to free all allocated resources.
Today it just makes these remove functions more complicated than
necessary.

This series simplifies pwmchip_remove() and prepares a first driver to
eventually make pwmchip_remove() return void.

Best regards
Uwe

Uwe Kleine-König (2):
  pwm: Drop unused error path from pwmchip_remove()
  pwm: imx27: Don't check the return code of pwmchip_remove()

 drivers/pwm/core.c      | 16 ++--------------
 drivers/pwm/pwm-imx27.c |  4 +++-
 2 files changed, 5 insertions(+), 15 deletions(-)

-- 
2.30.2

[PATCH 1/2] pwm: Drop unused error path from pwmchip_remove()

[Uwe Kleine-König]

Since the pwm core uses device links (commit b2c200e3f2fd ("pwm: Add
consumer device link")) it cannot happen any more that there is still a
consumer when a pwmchip goes away. So drop this check.

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

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 7d0266bc5fcb..57b90469a7ad 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -324,22 +324,10 @@ EXPORT_SYMBOL_GPL(pwmchip_add);
  */
 int pwmchip_remove(struct pwm_chip *chip)
 {
-	unsigned int i;
-	int ret = 0;
-
 	pwmchip_sysfs_unexport(chip);
 
 	mutex_lock(&pwm_lock);
 
-	for (i = 0; i < chip->npwm; i++) {
-		struct pwm_device *pwm = &chip->pwms[i];
-
-		if (test_bit(PWMF_REQUESTED, &pwm->flags)) {
-			ret = -EBUSY;
-			goto out;
-		}
-	}
-
 	list_del_init(&chip->list);
 
 	if (IS_ENABLED(CONFIG_OF))
@@ -347,9 +335,9 @@ int pwmchip_remove(struct pwm_chip *chip)
 
 	free_pwms(chip);
 
-out:
 	mutex_unlock(&pwm_lock);
-	return ret;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
-- 
2.30.2

[PATCH 2/2] pwm: imx27: Don't check the return code of pwmchip_remove()

[Uwe Kleine-König]

pwmchip_remove() always returns 0. Don't use the value to make it
possible to eventually change the function to return void. This is a
good thing as pwmchip_remove() is usually called from a remove function
(mostly for platform devices) and their return value is ignored by the
device core anyhow.

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

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index ba695115c160..9c5b336b00bc 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -354,7 +354,9 @@ static int pwm_imx27_remove(struct platform_device *pdev)
 
 	imx = platform_get_drvdata(pdev);
 
-	return pwmchip_remove(&imx->chip);
+	pwmchip_remove(&imx->chip);
+
+	return 0;
 }
 
 static struct platform_driver imx_pwm_driver = {
-- 
2.30.2

Re: [PATCH 1/2] pwm: Drop unused error path from pwmchip_remove()

[Thierry Reding]

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

On Wed, Mar 24, 2021 at 04:20:57PM +0100, Uwe Kleine-König wrote:
> Since the pwm core uses device links (commit b2c200e3f2fd ("pwm: Add
> consumer device link")) it cannot happen any more that there is still a
> consumer when a pwmchip goes away. So drop this check.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)

Can't this still happen when a consumer forgets to pwm_put() the PWM?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] pwm: imx27: Don't check the return code of pwmchip_remove()

[Thierry Reding]

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

On Wed, Mar 24, 2021 at 04:20:58PM +0100, Uwe Kleine-König wrote:
> pwmchip_remove() always returns 0. Don't use the value to make it
> possible to eventually change the function to return void. This is a
> good thing as pwmchip_remove() is usually called from a remove function
> (mostly for platform devices) and their return value is ignored by the
> device core anyhow.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx27.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

It's true that there's not much we can do upon failure, so failing
doesn't make much sense and therefore returning an error doesn't make
sense. So how about we WARN on the -EBUSY case instead of returning an
error? That should have an even higher impact than an error that's being
silently ignored.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] pwm: Drop unused error path from pwmchip_remove()

[Uwe Kleine-König]

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

On Fri, Apr 09, 2021 at 01:57:46PM +0200, Thierry Reding wrote:
> On Wed, Mar 24, 2021 at 04:20:57PM +0100, Uwe Kleine-König wrote:
> > Since the pwm core uses device links (commit b2c200e3f2fd ("pwm: Add
> > consumer device link")) it cannot happen any more that there is still a
> > consumer when a pwmchip goes away. So drop this check.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/core.c | 16 ++--------------
> >  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> Can't this still happen when a consumer forgets to pwm_put() the PWM?

The change is still good, and a more correct change log would be:

	Since the pwm core uses device links (commit b2c200e3f2fd ("pwm: Add
	consumer device link")) each consumer driver that requested the PWMs is
	already gone. If they called pwm_put() (as they should) the
	PWMF_REQUESTED bit is not set. If they failed (which is a bug) the
	PWMF_REQUESTED bit is still set, but the driver that cared is still
	gone, so nothing bad happens if the pwmchip goes away anyhow.

	So the check can be dropped.

Does this sound better?

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 2/2] pwm: imx27: Don't check the return code of pwmchip_remove()

[Uwe Kleine-König]

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

Hello Thierry,

On Fri, Apr 09, 2021 at 02:00:20PM +0200, Thierry Reding wrote:
> On Wed, Mar 24, 2021 at 04:20:58PM +0100, Uwe Kleine-König wrote:
> > pwmchip_remove() always returns 0. Don't use the value to make it
> > possible to eventually change the function to return void. This is a
> > good thing as pwmchip_remove() is usually called from a remove function
> > (mostly for platform devices) and their return value is ignored by the
> > device core anyhow.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-imx27.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> It's true that there's not much we can do upon failure, so failing
> doesn't make much sense and therefore returning an error doesn't make
> sense. So how about we WARN on the -EBUSY case instead of returning an
> error? That should have an even higher impact than an error that's being
> silently ignored.

After patch 1 in this series (or the reworked version from
https://lore.kernel.org/r/20210423165902.2439564-1-u.kleine-koenig@pengutronix.de)
pwmchip_remove() really always returns 0, so adding a WARN in the imx27
(or any other) driver doesn't make much sense.

It might make sense to add such a WARN in pwmchip_remove(), is it that
what you intended to suggest?

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 2/2] pwm: imx27: Don't check the return code of pwmchip_remove()

[Uwe Kleine-König]

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

Hello Thierry,

On Tue, May 25, 2021 at 09:59:05PM +0200, Uwe Kleine-König wrote:
> On Fri, Apr 09, 2021 at 02:00:20PM +0200, Thierry Reding wrote:
> > On Wed, Mar 24, 2021 at 04:20:58PM +0100, Uwe Kleine-König wrote:
> > > pwmchip_remove() always returns 0. Don't use the value to make it
> > > possible to eventually change the function to return void. This is a
> > > good thing as pwmchip_remove() is usually called from a remove function
> > > (mostly for platform devices) and their return value is ignored by the
> > > device core anyhow.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/pwm/pwm-imx27.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > It's true that there's not much we can do upon failure, so failing
> > doesn't make much sense and therefore returning an error doesn't make
> > sense. So how about we WARN on the -EBUSY case instead of returning an
> > error? That should have an even higher impact than an error that's being
> > silently ignored.
> 
> After patch 1 in this series (or the reworked version from
> https://lore.kernel.org/r/20210423165902.2439564-1-u.kleine-koenig@pengutronix.de)
> pwmchip_remove() really always returns 0, so adding a WARN in the imx27
> (or any other) driver doesn't make much sense.
> 
> It might make sense to add such a WARN in pwmchip_remove(), is it that
> what you intended to suggest?

You never replied to this question. Given that today pwmchip_remove()
indeed always returns 0, your concern here should not stop application
of the change to the pwm-imx27 driver.

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