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.
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.
> 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.
So I'm still convinced the patch I did is the right thing to do.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
