[PATCHv2] crypto: eip93 - fix IRQ teardown ordering in remove path

[PATCHv2] crypto: eip93 - fix IRQ teardown ordering in remove path

[Rosen Penev]

Replace the devm-managed threaded IRQ with request_irq() so free_irq()
can run in eip93_cleanup() instead of after the remove callback has
returned. Register the IRQ only after tasklet_init() and the ring
locks are set up, so the handler can never schedule an uninitialized
tasklet.

The original probe requested the IRQ before eip93_desc_init() and
tasklet_init(), and eip93_cleanup() killed the tasklet before masking
the device interrupts. An IRQ arriving in either window could schedule
a tasklet that was not yet initialized or had already been killed.

Reorder eip93_cleanup() to:
  1. mask and clear the device interrupts
  2. free_irq(), which synchronizes and removes the handler
  3. tasklet_kill(), which drains anything scheduled before the mask

The tasklet re-enables the EIP93_INT_RDR_THRESH interrupt as its last
action once the result ring is drained, so mask the device interrupts a
second time after tasklet_kill() to keep the hardware quiet while it is
powered off.

The driver implements ONESHOT semantics itself: the hardirq handler
masks the interrupt source before scheduling the tasklet. Use
request_irq() so the line is unmasked for the tasklet and the
IRQF_ONESHOT WARN_ON_ONCE is avoided.

Add an err_free_tasklet path in probe so a request_irq() failure
releases the tasklet and descriptor resources instead of leaking them.

Fixes: 9739f5f93b78 ("crypto: eip93 - Add Inside Secure SafeXcel EIP-93 crypto engine support")
Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: extra clear/disable in _remove.
 .../crypto/inside-secure/eip93/eip93-main.c   | 31 ++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/inside-secure/eip93/eip93-main.c b/drivers/crypto/inside-secure/eip93/eip93-main.c
index e62785952b0d..cc504590fd0f 100644
--- a/drivers/crypto/inside-secure/eip93/eip93-main.c
+++ b/drivers/crypto/inside-secure/eip93/eip93-main.c
@@ -395,9 +395,21 @@ static int eip93_desc_init(struct eip93_device *eip93)
 
 static void eip93_cleanup(struct eip93_device *eip93)
 {
+	/* Stop HW from asserting IRQ first */
+	eip93_irq_clear(eip93, EIP93_INT_ALL);
+	eip93_irq_disable(eip93, EIP93_INT_ALL);
+
+	/* Synchronize and unregister the IRQ handler */
+	free_irq(eip93->irq, eip93);
+
+	/* Drain any tasklet scheduled before the IRQ was disabled */
 	tasklet_kill(&eip93->ring->done_task);
 
-	/* Clear/ack all interrupts before disable all */
+	/*
+	 * The tasklet re-enables the RDR interrupt when it drains the
+	 * result ring, so mask the device interrupts again before the
+	 * hardware is switched off.
+	 */
 	eip93_irq_clear(eip93, EIP93_INT_ALL);
 	eip93_irq_disable(eip93, EIP93_INT_ALL);
 
@@ -430,12 +442,6 @@ static int eip93_crypto_probe(struct platform_device *pdev)
 	if (eip93->irq < 0)
 		return eip93->irq;
 
-	ret = devm_request_threaded_irq(eip93->dev, eip93->irq, eip93_irq_handler,
-					NULL, IRQF_ONESHOT,
-					dev_name(eip93->dev), eip93);
-	if (ret)
-		return ret;
-
 	ret = eip93_desc_init(eip93);
 	if (ret)
 		return ret;
@@ -448,6 +454,11 @@ static int eip93_crypto_probe(struct platform_device *pdev)
 	spin_lock_init(&eip93->ring->idr_lock);
 	idr_init(&eip93->ring->crypto_async_idr);
 
+	ret = request_irq(eip93->irq, eip93_irq_handler, 0,
+			  dev_name(eip93->dev), eip93);
+	if (ret)
+		goto err_free_tasklet;
+
 	algo_flags = readl(eip93->base + EIP93_REG_PE_OPTION_1);
 
 	eip93_initialize(eip93, algo_flags);
@@ -472,6 +483,12 @@ static int eip93_crypto_probe(struct platform_device *pdev)
 		 readl(eip93->base + EIP93_REG_PE_OPTION_0));
 
 	return 0;
+
+err_free_tasklet:
+	idr_destroy(&eip93->ring->crypto_async_idr);
+	tasklet_kill(&eip93->ring->done_task);
+	eip93_desc_free(eip93);
+	return ret;
 }
 
 static void eip93_crypto_remove(struct platform_device *pdev)
-- 
2.55.0

Re: [PATCHv2] crypto: eip93 - fix IRQ teardown ordering in remove path

[Herbert Xu]

On Mon, Sep 21, 2026 at 04:11:21PM -0700, Rosen Penev wrote:
>
> diff --git a/drivers/crypto/inside-secure/eip93/eip93-main.c b/drivers/crypto/inside-secure/eip93/eip93-main.c
> index e62785952b0d..cc504590fd0f 100644
> --- a/drivers/crypto/inside-secure/eip93/eip93-main.c
> +++ b/drivers/crypto/inside-secure/eip93/eip93-main.c
> @@ -395,9 +395,21 @@ static int eip93_desc_init(struct eip93_device *eip93)
>  
>  static void eip93_cleanup(struct eip93_device *eip93)
>  {
> +	/* Stop HW from asserting IRQ first */
> +	eip93_irq_clear(eip93, EIP93_INT_ALL);
> +	eip93_irq_disable(eip93, EIP93_INT_ALL);

What's the point of this since the tasklet can undo it at any time?
IOW why not just get rid of these two lines and only do them after
the tasklet_kill call?

> +
> +	/* Synchronize and unregister the IRQ handler */
> +	free_irq(eip93->irq, eip93);
> +
> +	/* Drain any tasklet scheduled before the IRQ was disabled */
>  	tasklet_kill(&eip93->ring->done_task);
>  
> -	/* Clear/ack all interrupts before disable all */
> +	/*
> +	 * The tasklet re-enables the RDR interrupt when it drains the
> +	 * result ring, so mask the device interrupts again before the
> +	 * hardware is switched off.
> +	 */
>  	eip93_irq_clear(eip93, EIP93_INT_ALL);
>  	eip93_irq_disable(eip93, EIP93_INT_ALL);
>  
> @@ -430,12 +442,6 @@ static int eip93_crypto_probe(struct platform_device *pdev)
>  	if (eip93->irq < 0)
>  		return eip93->irq;
>  
> -	ret = devm_request_threaded_irq(eip93->dev, eip93->irq, eip93_irq_handler,
> -					NULL, IRQF_ONESHOT,
> -					dev_name(eip93->dev), eip93);
> -	if (ret)
> -		return ret;
> -
>  	ret = eip93_desc_init(eip93);
>  	if (ret)
>  		return ret;
> @@ -448,6 +454,11 @@ static int eip93_crypto_probe(struct platform_device *pdev)
>  	spin_lock_init(&eip93->ring->idr_lock);
>  	idr_init(&eip93->ring->crypto_async_idr);
>  
> +	ret = request_irq(eip93->irq, eip93_irq_handler, 0,
> +			  dev_name(eip93->dev), eip93);
> +	if (ret)
> +		goto err_free_tasklet;

Going from a threaded handler to a legacy handler seems to be
a step backward, even if this driver didn't make use of the
threaded functionality.

What about getting rid of the tasklet altogether and just using
the threaded IRQ handler directly, that too would kill the race
condition, right?

> @@ -472,6 +483,12 @@ static int eip93_crypto_probe(struct platform_device *pdev)
>  		 readl(eip93->base + EIP93_REG_PE_OPTION_0));
>  
>  	return 0;
> +
> +err_free_tasklet:
> +	idr_destroy(&eip93->ring->crypto_async_idr);
> +	tasklet_kill(&eip93->ring->done_task);

This seems weird.  You can only get here if request_irq fails,
how can you schedule a tasklet in that case?

Cheers,
-- 
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