[PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Uwe Kleine-König]

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.

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.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/char/tpm/tpm_tis_i2c_cr50.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index c89278103703..622cdf622ddc 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -751,12 +751,6 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 static int tpm_cr50_i2c_remove(struct i2c_client *client)
 {
 	struct tpm_chip *chip = i2c_get_clientdata(client);
-	struct device *dev = &client->dev;
-
-	if (!chip) {
-		dev_err(dev, "Could not get client data at remove\n");
-		return -ENODEV;
-	}
 
 	tpm_chip_unregister(chip);
 	tpm_cr50_release_locality(chip, true);
-- 
2.30.2

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Jarkko Sakkinen]

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.

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/char/tpm/tpm_tis_i2c_cr50.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> index c89278103703..622cdf622ddc 100644
> --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> @@ -751,12 +751,6 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
>  static int tpm_cr50_i2c_remove(struct i2c_client *client)
>  {
>         struct tpm_chip *chip = i2c_get_clientdata(client);
> -       struct device *dev = &client->dev;
> -
> -       if (!chip) {
> -               dev_err(dev, "Could not get client data at remove\n");
> -               return -ENODEV;
> -       }
>  
>         tpm_chip_unregister(chip);
>         tpm_cr50_release_locality(chip, true);


/Jarkko

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2846 bytes --]

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;

.

Best regards
Uwe

> > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > index c89278103703..622cdf622ddc 100644
> > --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > @@ -751,12 +751,6 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
> >  static int tpm_cr50_i2c_remove(struct i2c_client *client)
> >  {
> >         struct tpm_chip *chip = i2c_get_clientdata(client);
> > -       struct device *dev = &client->dev;
> > -
> > -       if (!chip) {
> > -               dev_err(dev, "Could not get client data at remove\n");
> > -               return -ENODEV;
> > -       }
> >  
> >         tpm_chip_unregister(chip);
> >         tpm_cr50_release_locality(chip, true);

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Jarkko Sakkinen]

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;
> 
> .
> 
> Best regards
> Uwe

Please then include it to a patch set, where this happens.

/Jarkko

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3482 bytes --]

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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3797 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Jarkko Sakkinen]

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

[PATCH] char: tpm: cr50_i2c: Suppress duplicated error message in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 7254 bytes --]

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@pengutronix.de>
---

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] char: tpm: cr50_i2c: Suppress duplicated error message in .remove()

[Jarkko Sakkinen]

On Tue, Apr 26, 2022 at 10:06:02AM +0200, Uwe Kleine-König wrote:
> -		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;

Just change the return value 0 and log-level, message can be as it is.

BR, Jarkko

Re: [PATCH] char: tpm: cr50_i2c: Suppress duplicated error message in .remove()

[Jarkko Sakkinen]

On Wed, May 04, 2022 at 06:53:17AM +0300, Jarkko Sakkinen wrote:
> On Tue, Apr 26, 2022 at 10:06:02AM +0200, Uwe Kleine-König wrote:
> > -		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;
> 
> Just change the return value 0 and log-level, message can be as it is.

Ignore, I applied the patch.

BR, Jarkko

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Jason Gunthorpe]

On Tue, Apr 26, 2022 at 07:35:29AM +0300, Jarkko Sakkinen wrote:

> 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;
>        } 

If it can't happen by design it should be deleted entirely, or be
turned into a WARN_ON:

if (WARN_ON(!chip))
   return;

But I find this largely unnecessary as a null chip will reliably oops
later on in the same function.

Jason

Re: [PATCH] char: tpm: cr50_i2c: Drop if with an always false condition

[Jarkko Sakkinen]

On Tue, Apr 26, 2022 at 08:35:41AM -0300, Jason Gunthorpe wrote:
> On Tue, Apr 26, 2022 at 07:35:29AM +0300, Jarkko Sakkinen wrote:
> 
> > 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;
> >        } 
> 
> If it can't happen by design it should be deleted entirely, or be
> turned into a WARN_ON:
> 
> if (WARN_ON(!chip))
>    return;
> 
> But I find this largely unnecessary as a null chip will reliably oops
> later on in the same function.
> 
> Jason

Fine, I applied the patch.

BR, Jarkko