Re: [PATCH v3] crypto: talitos: fix probe IRQ ordering

["Paul Louvel" <paul.louvel@bootlin.com>]

On Sat Oct 3, 2026 at 11:23 PM CEST, Rosen Penev wrote:
> The talitos interrupt handlers schedule priv->done_task[] via
> tasklet_schedule().  In probe(), talitos_probe_irq() ran before
> tasklet_init(), so an interrupt arriving during that window (a shared
> IRQ, or a completion pending from an earlier transmission) would
> schedule an uninitialized tasklet.
>
> Resolve the IRQ numbers before the tasklet_init() calls so the
> done_task[] selection can see the secondary IRQ, and only request the
> IRQs after the channel fifos are allocated and the device is
> initialized.  Every structure the handlers touch is then fully set up
> before interrupts are enabled.  This matches remove(), which frees the
> IRQs before killing the tasklets.

LLM tends to be very verbose on commit messages and describe what the git diff
could already tell us.
The second paragraph could be resumed to:

"Request IRQs after the necessary data structures are initialized, so that
interrupts do not schedule uninitialized tasklets."

>
> Assisted-by: LLM

There are some available skills to improve AI wording, or you can make one
yourself tell it to avoid verbosity.

> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  v3: reshuffle again to fix IRQs.
>  v2: reshuffle code to avoid NULL derefs
>  drivers/crypto/talitos.c | 48 ++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
> index 41a87d7c30a0..70f7ad9e09f9 100644
> --- a/drivers/crypto/talitos.c
> +++ b/drivers/crypto/talitos.c
> @@ -3353,21 +3353,13 @@ static int talitos_probe_irq(struct platform_device *ofdev)
>  	int err;
>  	bool is_sec1 = has_ftr_sec1(priv);
>  
> -	priv->irq[0] = platform_get_irq(ofdev, 0);
> -	if (priv->irq[0] < 0)
> -		return priv->irq[0];
> -
>  	if (is_sec1) {
>  		err = request_irq(priv->irq[0], talitos1_interrupt_4ch, 0,
>  				  dev_driver_string(dev), priv);
>  		goto primary_out;
>  	}
>  
> -	priv->irq[1] = platform_get_irq_optional(ofdev, 1);
> -	if (priv->irq[1] == -EPROBE_DEFER)
> -		return priv->irq[1];
> -
> -	/* get the primary irq line */
> +	/* single (or primary) irq line */
>  	if (priv->irq[1] < 0) {
>  		err = request_irq(priv->irq[0], talitos2_interrupt_4ch, 0,
>  				  dev_driver_string(dev), priv);
> @@ -3379,13 +3371,11 @@ static int talitos_probe_irq(struct platform_device *ofdev)
>  	if (err)
>  		goto primary_out;
>  
> -	/* get the secondary irq line */
> +	/* secondary irq line */
>  	err = request_irq(priv->irq[1], talitos2_interrupt_ch1_3, 0,
>  			  dev_driver_string(dev), priv);
> -	if (err) {
> +	if (err)
>  		dev_err(dev, "failed to request secondary irq\n");
> -		priv->irq[1] = 0;
> -	}
>  
>  	return err;
>  
> @@ -3404,12 +3394,27 @@ static int talitos_probe(struct platform_device *ofdev)
>  	struct device_node *np = ofdev->dev.of_node;
>  	struct talitos_private *priv;
>  	unsigned int num_channels;
> +	void __iomem *reg;
>  	int i, err;
>  	int stride;
> +	int irq0;
> +	int irq1;
>  
>  	if (of_property_read_u32(np, "fsl,num-channels", &num_channels))
>  		return -EINVAL;
>  
> +	irq0 = platform_get_irq(ofdev, 0);
> +	if (irq0 < 0)
> +		return irq0;
> +
> +	irq1 = platform_get_irq_optional(ofdev, 1);
> +	if (irq1 == -EPROBE_DEFER)
> +		return irq1;
> +
> +	reg = devm_platform_ioremap_resource(ofdev, 0);
> +	if (IS_ERR(reg))
> +		return PTR_ERR(reg);
> +

You are dropping a previous error message here, use dev_err_probe().

>  	priv = devm_kzalloc(dev, struct_size(priv, chan, num_channels), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> @@ -3425,12 +3430,7 @@ static int talitos_probe(struct platform_device *ofdev)
>  
>  	spin_lock_init(&priv->reg_lock);
>  
> -	priv->reg = devm_platform_ioremap_resource(ofdev, 0);
> -	if (IS_ERR(priv->reg)) {
> -		dev_err(dev, "failed to of_iomap\n");
> -		err = PTR_ERR(priv->reg);
> -		goto err_out;
> -	}
> +	priv->reg = reg;
>  
>  	/* get SEC version capabilities from device tree */
>  	of_property_read_u32(np, "fsl,channel-fifo-len", &priv->chfifo_len);
> @@ -3481,9 +3481,8 @@ static int talitos_probe(struct platform_device *ofdev)
>  		stride = TALITOS2_CH_STRIDE;
>  	}
>  
> -	err = talitos_probe_irq(ofdev);
> -	if (err)
> -		goto err_out;
> +	priv->irq[0] = irq0;
> +	priv->irq[1] = irq1;
>  
>  	if (has_ftr_sec1(priv)) {
>  		if (priv->num_channels == 1)
> @@ -3540,6 +3539,11 @@ static int talitos_probe(struct platform_device *ofdev)
>  		goto err_out;
>  	}
>  
> +	/* enable interrupts once the channel fifos and tasklets are set up */
> +	err = talitos_probe_irq(ofdev);
> +	if (err)
> +		goto err_out;
> +
>  	/* register the RNG, if available */
>  	if (hw_supports(dev, DESC_HDR_SEL0_RNG)) {
>  		err = talitos_register_rng(dev);

I guess to be 100% correct, IRQs could be masked at the beginning of probe
and unmasked at the end.

Thanks,
-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com