Returning an error value in an i2c remove callback results in an error
message being emitted by the i2c core, but otherwise it doesn't make a
difference. The device goes away anyhow and the devm cleanups are
called.
As tpm_cr50_i2c_remove() emits an error message already and the
additional error message by the i2c core doesn't add any useful
information, change the return value to zero to suppress this error
message.
Note that if i2c_clientdata is NULL, there is something really fishy.
Assuming no memory corruption happened (then all bets are lost anyhow),
tpm_cr50_i2c_remove() is only called after tpm_cr50_i2c_probe() returned
successfully. So there was a tpm chip registered before and after
tpm_cr50_i2c_remove() its privdata is freed but the associated character
device isn't removed. If after that happened userspace accesses the
character device it's likely that the freed memory is accessed. For that
reason the warning message is made a bit more frightening.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
---
On Tue, Apr 26, 2022 at 07:35:29AM +0300, Jarkko Sakkinen wrote:
> 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.
>
> 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.
I don't agree. This callback change is a very big change affecting most
i2c drivers. A quick heuristic: This affects more than 1200 drivers:
$ git grep 'static struct i2c_driver' | wc -l
1227
If such a patch contains hunks like:
if (!chip) {
dev_err(dev, "Could not get client data at remove\n");
- return -ENODEV;
+ return;
}
this makes the review process needlessly complicated and the change
isn't a noop. So in my eyes changing the return value to zero should be
done in a patch before the callback change.
There is no win on delaying the tpm_tis_i2c_cr50 change as a) there is a
benefit already today (no duplicated error message) and b) keeping this
patch in a series together with the callback patch (and maybe even more
similar patches) results in an effort to keep it updated and the need
for coordination before the callback patch goes in. Also if such a
driver specific patch results in a discussion as this one, this delays
the callback change and then there is a real chance that the callback
change needs to be rebased and reverified.
So this is the chance to apply this patch without timing pressure via
its normal maintainer tree.
As you insist on keeping the if block, here comes a patch suggestion
that allows me to not having to care for a driver specific patch in the
quest to change the i2c remove callback and still keep the "sanity"
check.
Best regards
Uwe
drivers/char/tpm/tpm_tis_i2c_cr50.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index f6c0affbb456..bf608b6af339 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -768,8 +768,8 @@ static int tpm_cr50_i2c_remove(struct i2c_client *client)
struct device *dev = &client->dev;
if (!chip) {
- dev_err(dev, "Could not get client data at remove\n");
- return -ENODEV;
+ dev_crit(dev, "Could not get client data at remove, memory corruption ahead\n");
+ return 0;
}
tpm_chip_unregister(chip);
--
2.35.1
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
