netdev - Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241119-spectral-brave-millipede-9a2bd9-mkl@pengutronix.de>
Date: Tue, 19 Nov 2024 12:28:09 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-can@...r.kernel.org, 
	netdev@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	imx@...ts.linux.dev, NXP Linux Team <s32@....com>, 
	Christophe Lizzi <clizzi@...hat.com>, Alberto Ruiz <aruizrui@...hat.com>, 
	Enric Balletbo <eballetb@...hat.com>
Subject: Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt
 lines

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   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)