crypto: atmel-ecc - Remove duplicated error reporting in .remove()

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

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
>

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