[PATCH] crypto: picoxcell: add missed tasklet_kill

[PATCH] crypto: picoxcell: add missed tasklet_kill

[Chuhong Yuan]

This driver forgets to kill tasklet when probe fails and remove.
Add the calls to fix it.

Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
 drivers/crypto/picoxcell_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/picoxcell_crypto.c b/drivers/crypto/picoxcell_crypto.c
index 3cbefb41b099..8d7c6bb2876e 100644
--- a/drivers/crypto/picoxcell_crypto.c
+++ b/drivers/crypto/picoxcell_crypto.c
@@ -1755,6 +1755,7 @@ static int spacc_probe(struct platform_device *pdev)
 	if (!ret)
 		return 0;
 
+	tasklet_kill(&engine->complete);
 	del_timer_sync(&engine->packet_timeout);
 	device_remove_file(&pdev->dev, &dev_attr_stat_irq_thresh);
 err_clk_disable:
@@ -1771,6 +1772,7 @@ static int spacc_remove(struct platform_device *pdev)
 	struct spacc_alg *alg, *next;
 	struct spacc_engine *engine = platform_get_drvdata(pdev);
 
+	tasklet_kill(&engine->complete);
 	del_timer_sync(&engine->packet_timeout);
 	device_remove_file(&pdev->dev, &dev_attr_stat_irq_thresh);
 
-- 
2.24.0

Re: [PATCH] crypto: picoxcell: add missed tasklet_kill

[Herbert Xu]

On Fri, Nov 15, 2019 at 10:31:16AM +0800, Chuhong Yuan wrote:
> This driver forgets to kill tasklet when probe fails and remove.
> Add the calls to fix it.
> 
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>

Yes this driver does look buggy but I think your patch isn't
enough.

> diff --git a/drivers/crypto/picoxcell_crypto.c b/drivers/crypto/picoxcell_crypto.c
> index 3cbefb41b099..8d7c6bb2876e 100644
> --- a/drivers/crypto/picoxcell_crypto.c
> +++ b/drivers/crypto/picoxcell_crypto.c
> @@ -1755,6 +1755,7 @@ static int spacc_probe(struct platform_device *pdev)
>  	if (!ret)
>  		return 0;
>  
> +	tasklet_kill(&engine->complete);

The tasklet is schedule by the IRQ handler so you should not kill
it until the IRQ handler has been unregistered.

This driver is also buggy because it registers the IRQ handler
before initialising the tasklet.  You must always be prepared for
spurious IRQs.  IOW, as soon as you register the IRQ handler you
must be prepared for it to be called.

> @@ -1771,6 +1772,7 @@ static int spacc_remove(struct platform_device *pdev)
>  	struct spacc_alg *alg, *next;
>  	struct spacc_engine *engine = platform_get_drvdata(pdev);
>  
> +	tasklet_kill(&engine->complete);

Ditto.

However, the IRQ handler is registered through devm which makes it
hard to kill the tasklet after unregistering it.  We should probably
convert it to a normal request_irq so we can control how it's
unregistered.

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: picoxcell: add missed tasklet_kill

[Marc Gonzalez]

On 22/11/2019 09:55, Herbert Xu wrote:

> On Fri, Nov 15, 2019 at 10:31:16AM +0800, Chuhong Yuan wrote:
>> This driver forgets to kill tasklet when probe fails and remove.
>> Add the calls to fix it.
>>
>> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> 
> Yes this driver does look buggy but I think your patch isn't
> enough.
> 
>> diff --git a/drivers/crypto/picoxcell_crypto.c b/drivers/crypto/picoxcell_crypto.c
>> index 3cbefb41b099..8d7c6bb2876e 100644
>> --- a/drivers/crypto/picoxcell_crypto.c
>> +++ b/drivers/crypto/picoxcell_crypto.c
>> @@ -1755,6 +1755,7 @@ static int spacc_probe(struct platform_device *pdev)
>>  	if (!ret)
>>  		return 0;
>>  
>> +	tasklet_kill(&engine->complete);
> 
> The tasklet is schedule by the IRQ handler so you should not kill
> it until the IRQ handler has been unregistered.
> 
> This driver is also buggy because it registers the IRQ handler
> before initialising the tasklet.  You must always be prepared for
> spurious IRQs.  IOW, as soon as you register the IRQ handler you
> must be prepared for it to be called.
> 
>> @@ -1771,6 +1772,7 @@ static int spacc_remove(struct platform_device *pdev)
>>  	struct spacc_alg *alg, *next;
>>  	struct spacc_engine *engine = platform_get_drvdata(pdev);
>>  
>> +	tasklet_kill(&engine->complete);
> 
> Ditto.
> 
> However, the IRQ handler is registered through devm which makes it
> hard to kill the tasklet after unregistering it.  We should probably
> convert it to a normal request_irq so we can control how it's
> unregistered.

Or inversely, registering the tasklet_kill() through devm, so that it
is called *after* the ISR unregistration.

Regards.

Re: [PATCH] crypto: picoxcell: add missed tasklet_kill

[Herbert Xu]

On Fri, Nov 22, 2019 at 05:36:35PM +0100, Marc Gonzalez wrote:
>
> > However, the IRQ handler is registered through devm which makes it
> > hard to kill the tasklet after unregistering it.  We should probably
> > convert it to a normal request_irq so we can control how it's
> > unregistered.
> 
> Or inversely, registering the tasklet_kill() through devm, so that it
> is called *after* the ISR unregistration.

Good Point.  Chuhong, could you please try this approach to see
if it gives us better code?

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