netdev - RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

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: <AM8PR04MB7315470E5A26BB757F503025FF1F0@AM8PR04MB7315.eurprd04.prod.outlook.com>
Date:   Tue, 20 Oct 2020 13:52:44 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Chris Healy <cphealy@...il.com>
CC:     Andrew Lunn <andrew@...n.ch>, Greg Ungerer <gerg@...ux-m68k.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven
 MDIO with polled IO

From: Chris Healy <cphealy@...il.com> Sent: Tuesday, October 20, 2020 9:07 PM
> On Mon, Oct 19, 2020 at 8:02 PM Andy Duan <fugang.duan@....com> wrote:
> >
> > From: Andrew Lunn <andrew@...n.ch> Sent: Tuesday, October 20, 2020
> > 10:40 AM
> > > On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
> > > > Hi Andrew,
> > > >
> > > > Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace
> > > > interrupt driven MDIO with polled IO") breaks the FEC driver on at
> > > > least one of the ColdFire platforms (the 5208). Maybe others, that
> > > > is all I have tested on so far.
> > > >
> > > > Specifically the driver no longer finds any PHY devices when it
> > > > probes the MDIO bus at kernel start time.
> > > >
> > > > I have pinned the problem down to this one specific change in this commit:
> > > >
> > > > > @@ -2143,8 +2142,21 @@ static int fec_enet_mii_init(struct
> > > platform_device *pdev)
> > > > >     if (suppress_preamble)
> > > > >             fep->phy_speed |= BIT(7);
> > > > > +   /* Clear MMFR to avoid to generate MII event by writing MSCR.
> > > > > +    * MII event generation condition:
> > > > > +    * - writing MSCR:
> > > > > +    *      - mmfr[31:0]_not_zero & mscr[7:0]_is_zero &
> > > > > +    *        mscr_reg_data_in[7:0] != 0
> > > > > +    * - writing MMFR:
> > > > > +    *      - mscr[7:0]_not_zero
> > > > > +    */
> > > > > +   writel(0, fep->hwp + FEC_MII_DATA);
> > > >
> > > > At least by removing this I get the old behavior back and
> > > > everything works as it did before.
> > > >
> > > > With that write of the FEC_MII_DATA register in place it seems
> > > > that subsequent MDIO operations return immediately (that is
> > > > FEC_IEVENT is
> > > > set) - even though it is obvious the MDIO transaction has not completed
> yet.
> > > >
> > > > Any ideas?
> > >
> > > Hi Greg
> > >
> > > This has come up before, but the discussion fizzled out without a
> > > final patch fixing the issue. NXP suggested this
> > >
> > > writel(0, fep->hwp + FEC_MII_DATA);
> > >
> > > Without it, some other FEC variants break because they do generate
> > > an interrupt at the wrong time causing all following MDIO transactions to
> fail.
> > >
> > > At the moment, we don't seem to have a clear understanding of the
> > > different FEC versions, and how their MDIO implementations vary.
> > >
> > >           Andrew
> >
> > Andrew, different varants has little different behavior, so the line
> > is required for
> > Imx6/7/7 platforms but should be removed in imx5 and ColdFire.
> 
> Do we know which variants of i.MX6 and i.MX7 do and don't need this?
> I'm successfully running with polling mode using the i.MX6q, i.MX6qp, i.MX7d,
> and Vybrid, all of which benefit from the considerably higher throughput
> achieved with polling.  (In all my use cases I'm working with an Ethernet Switch
> attached via MDIO.)
> 
I think the old version like imx53 and ColdFire don't need this.
Others like imx6/7/8 series required this.

> >
> > As we discuss one solution to resolve the issue, but it bring 30ms latency for
> kernel boot.
> >
> > Now, I want to revert the polling mode to original interrupt mode, do you
> agree ?
> >
> > Andy