linux-kernel - 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] [day] [month] [year] [list]
Message-ID: <ZxJ+DAn7fRE3Aiqm@lizhi-Precision-Tower-5810>
Date: Fri, 18 Oct 2024 11:26:04 -0400
From: Frank Li <Frank.li@....com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
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 Fri, Oct 18, 2024 at 11:16:46AM +0200, Marc Kleine-Budde wrote:
> 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.

We should fix wrong thing instead of continuing on the wrong path. No
much user use wakeup funcationlity. Actually you enable both int0 and int2
as wakeup source if try to keep compatility.

for example:
	fec->wake_irq = fep->irq[0];
	if (FEC_QUIRK_WAKEUP_FROM_INT2)
		fec->wake_irq2 = fep->irq[2];


...
	if (fep->wake_irq2 > 0) {
                                disable_irq(fep->wake_irq2);
                                enable_irq_wake(fep->wake_irq2);
                        }

I perfer fix dts and remove FEC_QUIRK_WAKEUP_FROM_INT2.


>
> | 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   |