Re: [PATCH] Input: iqs62x-keys - Suppress duplicated error message in .remove()

[Jeff LaBundy <jeff@labundy.com>]

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