On 19.11.2024 20:26:26, Vincent Mailhol wrote:
> On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
> > On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
> >> On 19/11/2024 at 17:10, Ciprian Costea wrote:
>
> (...)
>
> >>> + if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> >>> + err = request_irq(priv->irq_secondary_mb,
> >>> + flexcan_irq, IRQF_SHARED, dev->name, dev);
> >>> + if (err)
> >>> + goto out_free_irq_err;
> >>> + }
> >>
> >> Is the logic here correct?
> >>
> >> request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
> >>
> >> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
> >>
> >> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
> >> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
> >> not initialized.
> >>
> >> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
> >>
> >> (and I can see that currently there is no such device with
> >> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
> >> who knows if such device will be introduced in the future?)
> >>
> >
> > Hello Vincent,
> >
> > Thanks for your review. Indeed this seems to be an incorrect logic since
> > I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
> > and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
> >
> > I will change the impacted section to:
> > if (err) {
> > if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
> > goto out_free_irq_err;
> > else
> > goto out_free_irq;
> > }
>
> This is better. Alternatively, you could move the check into the label:
+1
> out_free_irq_err:
> if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
> free_irq(priv->irq_err, dev);
>
> But this is not a strong preference, I let you pick the one which you
> prefer.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
