| Message ID | 20220520172100.773730-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: atmel-ecc - Remove duplicated error reporting in .remove() | expand |
Hello, On Fri, May 20, 2022 at 07:21:00PM +0200, Uwe Kleine-König wrote: > 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 atmel_ecc_remove() already emits an error message on failure and the > additional error message by the i2c core doesn't add any useful > information, change the return value to zero to suppress this message. > > Also make the error message a bit more drastical because when the device > is still busy on remove, it's likely that it will access freed memory > soon. > > This patch is a preparation for making i2c remove callbacks return void. I want to tackle this (i.e. diff --git a/include/linux/i2c.h b/include/linux/i2c.h index fbda5ada2afc..066b541a0d5d 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. ) directly after the next merge window. That is (depending on Linus's counting capabilities) after v5.20-rc1. So I ask you to either take this crypto patch before (my preferred option), or accept that I send it as part of a bigger series that eventually contains the above hunk and will probably be merged via the i2c tree. Best regards Uwe
On 5/20/22 20:21, Uwe Kleine-König wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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 atmel_ecc_remove() already emits an error message on failure and the > additional error message by the i2c core doesn't add any useful > information, change the return value to zero to suppress this message. > > Also make the error message a bit more drastical because when the device > is still busy on remove, it's likely that it will access freed memory > soon. > > This patch is a preparation for making i2c remove callbacks return void. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/crypto/atmel-ecc.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c > index 333fbefbbccb..6ba38275de8c 100644 > --- a/drivers/crypto/atmel-ecc.c > +++ b/drivers/crypto/atmel-ecc.c > @@ -349,8 +349,16 @@ static int atmel_ecc_remove(struct i2c_client *client) > > /* Return EBUSY if i2c client already allocated. */ > if (atomic_read(&i2c_priv->tfm_count)) { > - dev_err(&client->dev, "Device is busy\n"); > - return -EBUSY; > + /* > + * After we return here, the memory backing the device is freed. > + * That happens no matter what the return value of this function > + * is because in the Linux device model there is no error > + * handling for unbinding a driver. > + * If there is still some action pending, it probably involves > + * accessing the freed memory. > + */ > + dev_emerg(&client->dev, "Device is busy, expect memory corruption.\n"); > + return 0; > } > > crypto_unregister_kpp(&atmel_ecdh_nist_p256); > > base-commit: 3123109284176b1532874591f7c81f3837bbdc17 > -- > 2.35.1 >
Hello On Wed, Jun 08, 2022 at 04:33:48AM +0000, Tudor.Ambarus@microchip.com wrote: > On 5/20/22 20:21, Uwe Kleine-König wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > 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 atmel_ecc_remove() already emits an error message on failure and the > > additional error message by the i2c core doesn't add any useful > > information, change the return value to zero to suppress this message. > > > > Also make the error message a bit more drastical because when the device > > is still busy on remove, it's likely that it will access freed memory > > soon. > > > > This patch is a preparation for making i2c remove callbacks return void. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> In the past patches were picked up by Herbert. I assume your R-b tag was the missing bit to make him pick up this patch? To make a bit more sure that will happen, I added him and davem to Cc. Best regards Uwe
On 6/8/22 10:04, Uwe Kleine-König wrote: > Hello > Hi, > On Wed, Jun 08, 2022 at 04:33:48AM +0000, Tudor.Ambarus@microchip.com wrote: >> On 5/20/22 20:21, Uwe Kleine-König wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> 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 atmel_ecc_remove() already emits an error message on failure and the >>> additional error message by the i2c core doesn't add any useful >>> information, change the return value to zero to suppress this message. >>> >>> Also make the error message a bit more drastical because when the device >>> is still busy on remove, it's likely that it will access freed memory >>> soon. >>> >>> This patch is a preparation for making i2c remove callbacks return void. >>> >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > In the past patches were picked up by Herbert. I assume your R-b tag was he still does. > the missing bit to make him pick up this patch? To make a bit more sure probably not. > that will happen, I added him and davem to Cc. Herbert usually queues patches in a two week time frame. Let's wait for him. Cheers, ta
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > 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 atmel_ecc_remove() already emits an error message on failure and the > additional error message by the i2c core doesn't add any useful > information, change the return value to zero to suppress this message. > > Also make the error message a bit more drastical because when the device > is still busy on remove, it's likely that it will access freed memory > soon. > > This patch is a preparation for making i2c remove callbacks return void. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/crypto/atmel-ecc.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) Patch applied. Thanks.
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c index 333fbefbbccb..6ba38275de8c 100644 --- a/drivers/crypto/atmel-ecc.c +++ b/drivers/crypto/atmel-ecc.c @@ -349,8 +349,16 @@ static int atmel_ecc_remove(struct i2c_client *client) /* Return EBUSY if i2c client already allocated. */ if (atomic_read(&i2c_priv->tfm_count)) { - dev_err(&client->dev, "Device is busy\n"); - return -EBUSY; + /* + * After we return here, the memory backing the device is freed. + * That happens no matter what the return value of this function + * is because in the Linux device model there is no error + * handling for unbinding a driver. + * If there is still some action pending, it probably involves + * accessing the freed memory. + */ + dev_emerg(&client->dev, "Device is busy, expect memory corruption.\n"); + return 0; } crypto_unregister_kpp(&atmel_ecdh_nist_p256);
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 atmel_ecc_remove() already emits an error message on failure and the additional error message by the i2c core doesn't add any useful information, change the return value to zero to suppress this message. Also make the error message a bit more drastical because when the device is still busy on remove, it's likely that it will access freed memory soon. This patch is a preparation for making i2c remove callbacks return void. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/crypto/atmel-ecc.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) base-commit: 3123109284176b1532874591f7c81f3837bbdc17