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

[Rosen Penev]

On Sat, Oct 3, 2026 at 3:11 PM Paul Louvel <paul.louvel@xxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > ---
> >  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().
Actually in this case I'm removing an extra error message.
devm_platform_ioremap_resource prints its own.
>
> >       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
>