netdev - Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

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: <20241018-black-dormouse-of-exercise-f7fed0-mkl@pengutronix.de>
Date: Fri, 18 Oct 2024 11:16:46 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Frank Li <Frank.li@....com>
Cc: Wei Fang <wei.fang@....com>, Shenwei Wang <shenwei.wang@....com>, 
	Clark Wang <xiaoning.wang@....com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Richard Cochran <richardcochran@...il.com>, 
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update
 quirk: bring IRQs in correct order

On 17.10.2024 11:30:45, Frank Li wrote:
> On Thu, Oct 17, 2024 at 04:21:33PM +0200, Marc Kleine-Budde wrote:
> > On 17.10.2024 10:03:51, Frank Li wrote:
> > > > > > Yes, that is IMHO the correct description of the IP core, but the
> > > > > > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > > > > > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > > > > > DTS.
> > > > > >
> > > > >
> > > > > I don't think it is a correct behavior for old drivers to use new DTBs or new
> > > > > drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> > > > > question, let's see how Frank responded.
> > > >
> > > > DTBs should be considered stable ABI.
> > > >
> > >
> > > ABI defined at binding doc.
> > >   interrupt-names:
> > >     oneOf:
> > >       - items:
> > >           - const: int0
> > >       - items:
> > >           - const: int0
> > >           - const: pps
> > >       - items:
> > >           - const: int0
> > >           - const: int1
> > >           - const: int2
> > >       - items:
> > >           - const: int0
> > >           - const: int1
> > >           - const: int2
> > >           - const: pps
> > >
> > > DTB should align binding doc. There are not 'descriptions' at 'interrupt',
> > > which should match 'interrupt-names'. So IMX8MP dts have not match ABI,
> > > which defined by binding doc. So it is DTS implement wrong.
> >
> > I follow your conclusion. But keep in mind, fixing the DTB would break
> > compatibility. The wrong DTS looks like this:
> >
> > - const: int1
> > - const: int2
> > - const: int0
> > - const: pps
> >
> > Currently we have broken DTS on the i.MX8M* and the
> > FEC_QUIRK_WAKEUP_FROM_INT2 that "fixes" this.
> >
> > This patch uses this quirk to correct the IRQ <-> queue assignment in
> > the driver.
> 
> This current code
> 
> for (i = 0; i < irq_cnt; i++) {
>                 snprintf(irq_name, sizeof(irq_name), "int%d", i);
>                 irq = platform_get_irq_byname_optional(pdev, irq_name);
> 		      ^^^^^^^^^^^^^^^^^^^^^
> 
> You just need add interrupt-names at imx8mp dts and reorder it to pass
> DTB check.

ACK

> 
>                 if (irq < 0)
>                         irq = platform_get_irq(pdev, i);
>                 if (irq < 0) {
>                         ret = irq;
>                         goto failed_irq;
>                 }
>                 ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
>                                        0, pdev->name, ndev);
>                 if (ret)
>                         goto failed_irq;
> 
>                 fep->irq[i] = irq;
>         }
> 
> All irq handle by the same fec_enet_interrupt().  Change dts irq orders
> doesn't broken compatiblity.

I'm sorry, but this is not 100% correct. Changing the _order_ of IRQs
does break compatibility. New DT (with changed IRQ order) with old
driver breaks wakeup functionality.

Have a look at b7cdc9658ac8 ("net: fec: add WoL support for i.MX8MQ"),
but keep in mind the patch description is not 100% correct:

| By default FEC driver treat irq[0] (i.e. int0 described in dt-binding)
| as wakeup interrupt, but this situation changed on i.MX8M serials, SoC
| integration guys mix wakeup interrupt signal into int2 interrupt line.
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This statement is wrong. The SoC integration is correct, the DT is
wrong.

| This patch introduces FEC_QUIRK_WAKEUP_FROM_INT2 to indicate int2 as
| wakeup interrupt for i.MX8MQ.

> "pre-equeue" irq is new features. You can enable this feature only
> when "interrupt-names" exist in future.

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)