On Mon, 2022-04-25 at 21:11 +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Mar 31, 2022 at 03:22:31PM +0200, Uwe Kleine-König wrote:
> > On Tue, Nov 16, 2021 at 06:30:39PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Nov 16, 2021 at 05:55:35PM +0200, Jarkko Sakkinen wrote:
> > > > On Sat, 2021-11-13 at 22:53 +0100, Uwe Kleine-König wrote:
> > > > > Hello,
> > > > >
> > > > > On Sat, Nov 13, 2021 at 12:53:32PM +0200, Jarkko Sakkinen wrote:
> > > > > > On Fri, 2021-11-12 at 23:53 +0100, Uwe Kleine-König wrote:
> > > > > > > tpm_cr50_i2c_remove() is only called after tpm_cr50_i2c_probe() returned
> > > > > > > successfully. As i2c_get_clientdata() returns driver data for the
> > > > > > > client's device and this was set in tpmm_chip_alloc() it won't return
> > > > > > > NULL.
> > > > > >
> > > > > > This does not make the check obsolete, e.g. it would catch a programming
> > > > > > error elsewhere.
> > > > > >
> > > > > > > Simplify accordingly to prepare changing the prototype of the i2c remove
> > > > > > > callback to return void. Notice that already today returning an error
> > > > > > > code from the remove callback doesn't prevent removal.
> > > > > >
> > > > > > I don't understand what you are trying to say.
> > > > >
> > > > > The eventual goal is the following change:
> > > > >
> > > > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > > > > index 16119ac1aa97..c7069ebf5a66 100644
> > > > > --- a/include/linux/i2c.h
> > > > > +++ b/include/linux/i2c.h
> > > > > @@ -273,7 +273,7 @@ struct i2c_driver {
> > > > >
> > > > > /* Standard driver model interfaces */
> > > > > int (*probe)(struct i2c_client *client, const struct i2c_device_id *id);
> > > > > - int (*remove)(struct i2c_client *client);
> > > > > + void (*remove)(struct i2c_client *client);
> > > > >
> > > > > /* New driver model interface to aid the seamless removal of the
> > > > > * current probe()'s, more commonly unused than used second parameter.
> > > > >
> > > > > To prepare that I want to change all remove callbacks to unconditionally
> > > > > return 0.
> > > > >
> > > > > The motivation for the above change is that returning an error from an
> > > > > i2c (or spi or platform) remove callback doesn't prevent the device from
> > > > > being removed. So the ability to return an int leads to wrong
> > > > > expectations by driver authors.
> > > > >
> > > > > The only effect a non-zero return code has, is an error message from the
> > > > > i2c core. So if you object to my suggested change, the minimal change I
> > > > > want to convince you of is to replace
> > > > >
> > > > > return -ENODEV;
> > > > >
> > > > > by
> > > > >
> > > > > return 0;
> > > > >
> > > > > .
> > > >
> > > > Please then include it to a patch set, where this happens.
> > >
> > > My plan is to do all the preparation before submitting the change to
> > > struct i2c_driver such that in the end coordination is only needed for a
> > > single patch. (As this patch should be easy to review and without side
> > > effects it should only drop "return 0;" (or replace them by "return;",
> > > depending on context) to make this easy to review/verify.
> > >
> > > Note that the suggested change has already a benefit today because in
> > > the error case (and without the change) you get two error messages.
> > > Returning 0 suppresses the generic (and so less useful) one.
> >
> > Either this was not convincing or this patch fell through the cracks.
> > Whatever it was, nobody replied and the patch isn't applied either.
> >
> > Would you please (re)consider this patch?
>
> More than three weeks later still no feedback :-\
>
> Please consider applying the patch.
>
> Best regards
> Uwe
Even if chip is expected not to be NULL, a sanity check costs nothing.
As already said, this should be reviewed in the context of the callback
change.
Even then, the change should rather be:
if (!chip) {
dev_err(dev, "Could not get client data at remove\n");
return;
}
BR, Jarkko
