diff mbox series

Input: iqs62x-keys - Suppress duplicated error message in .remove()

Message ID 20230318225110.261439-1-u.kleine-koenig@pengutronix.de (mailing list archive)
State Accepted
Commit 6906f5060d39f95aa0be30e998e0cc1179ba3d04
Headers show
Series Input: iqs62x-keys - Suppress duplicated error message in .remove() | expand

Commit Message

Uwe Kleine-König March 18, 2023, 10:51 p.m. UTC 
If a platform driver's remove callback returns non-zero the driver core
emits an error message. In such a case however iqs62x_keys_remove()
already issued a (better) message. So return zero to suppress the
generic message.

This patch has no other side effects as platform_remove() ignores the
return value of .remove() after the warning.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/input/keyboard/iqs62x-keys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Comments

Dmitry Torokhov March 20, 2023, 4:28 a.m. UTC | #1 
On Sat, Mar 18, 2023 at 11:51:10PM +0100, Uwe Kleine-König wrote:
> If a platform driver's remove callback returns non-zero the driver core
> emits an error message. In such a case however iqs62x_keys_remove()
> already issued a (better) message. So return zero to suppress the
> generic message.
> 
> This patch has no other side effects as platform_remove() ignores the
> return value of .remove() after the warning.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied, thank you.
Jeff LaBundy March 27, 2023, 11:27 p.m. UTC | #2 
Hi Uwe,

On Sat, Mar 18, 2023 at 11:51:10PM +0100, Uwe Kleine-König wrote:
> If a platform driver's remove callback returns non-zero the driver core
> emits an error message. In such a case however iqs62x_keys_remove()
> already issued a (better) message. So return zero to suppress the
> generic message.
> 
> This patch has no other side effects as platform_remove() ignores the
> return value of .remove() after the warning.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I was traveling all last week, and therefore unable to voice my opposition
in time. However, I figured I would still provide my feedback in case this
change may be proposed for other cases.

> ---
>  drivers/input/keyboard/iqs62x-keys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c
> index db793a550c25..02ceebad7bda 100644
> --- a/drivers/input/keyboard/iqs62x-keys.c
> +++ b/drivers/input/keyboard/iqs62x-keys.c
> @@ -320,7 +320,7 @@ static int iqs62x_keys_remove(struct platform_device *pdev)
>  	if (ret)
>  		dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret);
>  
> -	return ret;
> +	return 0;

In my opinion, we should never silence a function's return value, especially
in service of what is ultimately innocuous and cosmetic in nature. While this
specific example is harmless today, the caller can change and hence should be
the only instance who decides whether the return value is important.

If having both fine and subsequently coarse print statements is unacceptable,
I would have preferred to drop this driver's print statement and continue to
return ret. Or at the very least, include a comment as to why we deliberately
ignore the return value.

However, it's quite common for drivers to print a detailed message from probe
followed by the core printing "failed to probe," so I don't see why the remove
case cannot be the same. At any rate, this is just my $.02.

>  }
>  
>  static struct platform_driver iqs62x_keys_platform_driver = {
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
> -- 
> 2.39.2
> 

Kind regards,
Jeff LaBundy
Uwe Kleine-König March 28, 2023, 6:08 a.m. UTC | #3 
Hello Jeff,

On Mon, Mar 27, 2023 at 06:27:24PM -0500, Jeff LaBundy wrote:
> On Sat, Mar 18, 2023 at 11:51:10PM +0100, Uwe Kleine-König wrote:
> > If a platform driver's remove callback returns non-zero the driver core
> > emits an error message. In such a case however iqs62x_keys_remove()
> > already issued a (better) message. So return zero to suppress the
> > generic message.
> > 
> > This patch has no other side effects as platform_remove() ignores the
> > return value of .remove() after the warning.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I was traveling all last week, and therefore unable to voice my opposition
> in time. However, I figured I would still provide my feedback in case this
> change may be proposed for other cases.

It is.

> > ---
> >  drivers/input/keyboard/iqs62x-keys.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c
> > index db793a550c25..02ceebad7bda 100644
> > --- a/drivers/input/keyboard/iqs62x-keys.c
> > +++ b/drivers/input/keyboard/iqs62x-keys.c
> > @@ -320,7 +320,7 @@ static int iqs62x_keys_remove(struct platform_device *pdev)
> >  	if (ret)
> >  		dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret);
> >  
> > -	return ret;
> > +	return 0;
> 
> In my opinion, we should never silence a function's return value, especially
> in service of what is ultimately innocuous and cosmetic in nature. While this
> specific example is harmless today, the caller can change and hence should be
> the only instance who decides whether the return value is important.

The caller will change. Today the caller (i.e. platform_remove()) looks
as follows:

	... if (drv->remove) {
                int ret = drv->remove(dev);

                if (ret)
                        dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
	}

(so ret isn't used later any more). And I eventually will do

 struct platform_driver {
 ...
-	int (*remove)(struct platform_device *);
+	void (*remove)(struct platform_device *);
 ...
 }

and change platform_remove() to just:

	if (drv->remove)
		drv->remove(dev);

The change in question is a preparation for that.

The reason I tackle that is that .remove() returning an int seduces
driver authors to exit early in .remove() in the expectation that there
is error handling in the core (which there isn't).

See

	https://lore.kernel.org/linux-spi/20230317084232.142257-3-u.kleine-koenig@pengutronix.de

for such an issue.

> If having both fine and subsequently coarse print statements is unacceptable,
> I would have preferred to drop this driver's print statement and continue to
> return ret. Or at the very least, include a comment as to why we deliberately
> ignore the return value.

I have a patch series in the queue that will convert all drivers in
drivers/input to .remove_new(). (See
https://lore.kernel.org/linux-media/20230326143224.572654-9-u.kleine-koenig@pengutronix.de
for an example of such a conversion.) If we add such a comment now, I
will probably miss to adapt it then.

So I'm still convinced the patch I did is the right thing to do.

Best regards
Uwe
Jeff LaBundy March 29, 2023, 2 a.m. UTC | #4 
Hi Uwe,

Thank you for this additional background information.

On Tue, Mar 28, 2023 at 08:08:29AM +0200, Uwe Kleine-König wrote:
> Hello Jeff,
> 
> On Mon, Mar 27, 2023 at 06:27:24PM -0500, Jeff LaBundy wrote:
> > On Sat, Mar 18, 2023 at 11:51:10PM +0100, Uwe Kleine-König wrote:
> > > If a platform driver's remove callback returns non-zero the driver core
> > > emits an error message. In such a case however iqs62x_keys_remove()
> > > already issued a (better) message. So return zero to suppress the
> > > generic message.
> > > 
> > > This patch has no other side effects as platform_remove() ignores the
> > > return value of .remove() after the warning.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > I was traveling all last week, and therefore unable to voice my opposition
> > in time. However, I figured I would still provide my feedback in case this
> > change may be proposed for other cases.
> 
> It is.
> 
> > > ---
> > >  drivers/input/keyboard/iqs62x-keys.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c
> > > index db793a550c25..02ceebad7bda 100644
> > > --- a/drivers/input/keyboard/iqs62x-keys.c
> > > +++ b/drivers/input/keyboard/iqs62x-keys.c
> > > @@ -320,7 +320,7 @@ static int iqs62x_keys_remove(struct platform_device *pdev)
> > >  	if (ret)
> > >  		dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret);
> > >  
> > > -	return ret;
> > > +	return 0;
> > 
> > In my opinion, we should never silence a function's return value, especially
> > in service of what is ultimately innocuous and cosmetic in nature. While this
> > specific example is harmless today, the caller can change and hence should be
> > the only instance who decides whether the return value is important.
> 
> The caller will change. Today the caller (i.e. platform_remove()) looks
> as follows:
> 
> 	... if (drv->remove) {
>                 int ret = drv->remove(dev);
> 
>                 if (ret)
>                         dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
> 	}
> 
> (so ret isn't used later any more). And I eventually will do
> 
>  struct platform_driver {
>  ...
> -	int (*remove)(struct platform_device *);
> +	void (*remove)(struct platform_device *);
>  ...
>  }
> 
> and change platform_remove() to just:
> 
> 	if (drv->remove)
> 		drv->remove(dev);
> 
> The change in question is a preparation for that.

In that case, this change seems perfectly reasonable; although your
ultimate intention would have been useful to include in the commit
message. Of course, I could have also bothered to read the statement
in platform_remove() and it would have been obvious ;)

> 
> The reason I tackle that is that .remove() returning an int seduces
> driver authors to exit early in .remove() in the expectation that there
> is error handling in the core (which there isn't).
> 
> See
> 
> 	https://lore.kernel.org/linux-spi/20230317084232.142257-3-u.kleine-koenig@pengutronix.de
> 
> for such an issue.

Fair enough, I would have also been fine with simply converting this
function to void straight away as part of your impending wider change.

> 
> > If having both fine and subsequently coarse print statements is unacceptable,
> > I would have preferred to drop this driver's print statement and continue to
> > return ret. Or at the very least, include a comment as to why we deliberately
> > ignore the return value.
> 
> I have a patch series in the queue that will convert all drivers in
> drivers/input to .remove_new(). (See
> https://lore.kernel.org/linux-media/20230326143224.572654-9-u.kleine-koenig@pengutronix.de
> for an example of such a conversion.) If we add such a comment now, I
> will probably miss to adapt it then.

I don't think a comment is necessary anymore given this is not this
driver's final state. I was moreso concerned that someone later would
identify this as a bug and attempt to change it back.

> 
> So I'm still convinced the patch I did is the right thing to do.

Based on our discussion, I no longer have any objection.

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

Kind regards,
Jeff LaBundy
diff mbox series

Patch

diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c
index db793a550c25..02ceebad7bda 100644
--- a/drivers/input/keyboard/iqs62x-keys.c
+++ b/drivers/input/keyboard/iqs62x-keys.c
@@ -320,7 +320,7 @@  static int iqs62x_keys_remove(struct platform_device *pdev)
 	if (ret)
 		dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret);
 
-	return ret;
+	return 0;
 }
 
 static struct platform_driver iqs62x_keys_platform_driver = {