[PATCH] crypto: atmel-ecc - remove stale comments in atmel_ecc_remove

[PATCH] crypto: atmel-ecc - remove stale comments in atmel_ecc_remove

[Thorsten Blum]

atmel_ecc_remove() no longer returns -EBUSY since commit 7df7563b16aa
("crypto: atmel-ecc - Remove duplicated error reporting in .remove()")
and is a void function since commit ed5c2f5fd10d ("i2c: Make remove
callback return void").

Remove and update the outdated comments.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/crypto/atmel-ecc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 9c380351d2f9..e6068dc0a0c1 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -347,13 +347,11 @@ static void atmel_ecc_remove(struct i2c_client *client)
 {
 	struct atmel_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
 
-	/* Return EBUSY if i2c client already allocated. */
 	if (atomic_read(&i2c_priv->tfm_count)) {
 		/*
 		 * 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.
+		 * That happens 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.
 		 */

Re: [PATCH] crypto: atmel-ecc - remove stale comments in atmel_ecc_remove

[Herbert Xu]

On Tue, Jun 02, 2026 at 06:52:49PM +0200, Thorsten Blum wrote:
> atmel_ecc_remove() no longer returns -EBUSY since commit 7df7563b16aa
> ("crypto: atmel-ecc - Remove duplicated error reporting in .remove()")
> and is a void function since commit ed5c2f5fd10d ("i2c: Make remove
> callback return void").
> 
> Remove and update the outdated comments.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/crypto/atmel-ecc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index 9c380351d2f9..e6068dc0a0c1 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -347,13 +347,11 @@ static void atmel_ecc_remove(struct i2c_client *client)
>  {
>  	struct atmel_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
>  
> -	/* Return EBUSY if i2c client already allocated. */
>  	if (atomic_read(&i2c_priv->tfm_count)) {
>  		/*
>  		 * 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.
> +		 * That happens 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.
>  		 */

Please fix this properly rather than fiddling with the comments.

Drivers should always fail gracefully if the hardware disappears.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: atmel-ecc - remove stale comments in atmel_ecc_remove

[Thorsten Blum]

On Thu, Jun 11, 2026 at 01:29:52PM +0800, Herbert Xu wrote:
> On Tue, Jun 02, 2026 at 06:52:49PM +0200, Thorsten Blum wrote:
> > atmel_ecc_remove() no longer returns -EBUSY since commit 7df7563b16aa
> > ("crypto: atmel-ecc - Remove duplicated error reporting in .remove()")
> > and is a void function since commit ed5c2f5fd10d ("i2c: Make remove
> > callback return void").
> > 
> > Remove and update the outdated comments.
> > 
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > ---
> >  drivers/crypto/atmel-ecc.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> > index 9c380351d2f9..e6068dc0a0c1 100644
> > --- a/drivers/crypto/atmel-ecc.c
> > +++ b/drivers/crypto/atmel-ecc.c
> > @@ -347,13 +347,11 @@ static void atmel_ecc_remove(struct i2c_client *client)
> >  {
> >  	struct atmel_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
> >  
> > -	/* Return EBUSY if i2c client already allocated. */
> >  	if (atomic_read(&i2c_priv->tfm_count)) {
> >  		/*
> >  		 * 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.
> > +		 * That happens 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.
> >  		 */
> 
> Please fix this properly rather than fiddling with the comments.
> 
> Drivers should always fail gracefully if the hardware disappears.

Yes, I'm working on a fix, but it's not ready yet.

Thanks,
Thorsten

Re: [PATCH] crypto: atmel-ecc - remove stale comments in atmel_ecc_remove

[Lothar Rubusch]

> From linux-crypto-vger  Thu Jun 11 10:55:01 2026
> From: Thorsten Blum <thorsten.blum () linux ! dev>
> Date: Thu, 11 Jun 2026 10:55:01 +0000
> To: linux-crypto-vger
> Subject: Re: [PATCH] crypto: atmel-ecc - remove stale comments in atmel_ecc_remove
> Message-Id: <aiqUBXIybgHXA6uj () linux ! dev>
> X-MARC-Message: https://marc.info/?l=linux-crypto-vger&m=178117527182807
> 
> On Thu, Jun 11, 2026 at 01:29:52PM +0800, Herbert Xu wrote:
> > On Tue, Jun 02, 2026 at 06:52:49PM +0200, Thorsten Blum wrote:
> > > atmel_ecc_remove() no longer returns -EBUSY since commit 7df7563b16aa
> > > ("crypto: atmel-ecc - Remove duplicated error reporting in .remove()")
> > > and is a void function since commit ed5c2f5fd10d ("i2c: Make remove
> > > callback return void").
> > > 
> > > Remove and update the outdated comments.
> > > 
> > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > > ---
> > >  drivers/crypto/atmel-ecc.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> > > index 9c380351d2f9..e6068dc0a0c1 100644
> > > --- a/drivers/crypto/atmel-ecc.c
> > > +++ b/drivers/crypto/atmel-ecc.c
> > > @@ -347,13 +347,11 @@ static void atmel_ecc_remove(struct i2c_client *client)
> > >  {
> > >  	struct atmel_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
> > >  
> > > -	/* Return EBUSY if i2c client already allocated. */
> > >  	if (atomic_read(&i2c_priv->tfm_count)) {
> > >  		/*
> > >  		 * 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.
> > > +		 * That happens 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.
> > >  		 */
> > 
> > Please fix this properly rather than fiddling with the comments.
> > 
> > Drivers should always fail gracefully if the hardware disappears.
> 
> Yes, I'm working on a fix, but it's not ready yet.
> 

Hi guys, since this is going towards some work I already presented here and
still waiting on answer/request for comment from maintainer(s).
https://marc.info/?l=linux-kernel&m=178099821038957&w=2

The issue in the remove() arises when working with devres in combination with
asynch slow bus hardware, as we do here. AFAIK in the remove() are mainly two
options, either give a timeout to solve communication gracefully, then cut; or
wait indefinitely on the device to clear, in case forever.

When we cut off after timeout (first case) and still something arrives, it
would probably access freed memory resources. In the second case, simply
waiting on the device to resolve, might contain the risk of an infinite
waiting at driver removal. The other alternative would be to manage kmallocs
manually, i.e. to move away from devres (probably not what we want).
Currently, the driver just simply cuts off and has this problematic situation
very well spotted by the original author and commented.

Further, related to this situation in the remove() is using the global driver
data, which then might be overriden, and thus leak, when still around, and
this connects to dealing with synchronizing adding to the i2c_clientList and
algo registration, both happening in probe().

I tried to address all three issues. That's why the patch ended with such a
lengthy comment. The patch is reviewed by sashiko complaining only the above
dilemma.
https://sashiko.dev/#/patchset/20260609092927.47222-1-l.rubusch%40gmail.com

I hope I did not interfere too much with Thorstens fixes here. Since I assumed
you were active on rather different topics. Pls, let me know if so. I just
want to see this issue out of my way for the refac patch series.

Best,
L