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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx
>
> 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@xxxxxxxxxxxxxx
> 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
