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: <HE1PR0402MB2745B6388B6BF7306629A305FFAC0@HE1PR0402MB2745.eurprd04.prod.outlook.com>
Date:   Tue, 28 Apr 2020 07:50:58 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Andrew Lunn <andrew@...n.ch>,
        Leonard Crestez <leonard.crestez@....com>
CC:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Chris Healy <Chris.Healy@....aero>,
        dl-linux-imx <linux-imx@....com>, Chris Healy <cphealy@...il.com>
Subject: RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven
 MDIO with polled IO

From: Andrew Lunn <andrew@...n.ch> Sent: Tuesday, April 28, 2020 4:14 AM
> Hi Leonard
> 
> > Does not help.
> 
> Thanks for testing it.
> 
> > What does seem to help is inserting prints after the FEC_ENET_MII
> > check but that's probably because it inject a long delay equivalent to
> > the long udelay Andy has mentioned.
> 
> Yes, serial ports are slow...
> 
> > I found that in my case FEC_ENET_MII is already set on entry to
> > fec_enet_mdio_read, doesn't this make fec_enet_mdio_wait pointless?
> > Perhaps the problem is that the MII Interrupt pending bit is not
> > cleared. I can fix the problem like this:
> >
> > diff --git drivers/net/ethernet/freescale/fec_main.c
> > drivers/net/ethernet/freescale/fec_main.c
> > index 1ae075a246a3..f1330071647c 100644
> > --- drivers/net/ethernet/freescale/fec_main.c
> > +++ drivers/net/ethernet/freescale/fec_main.c
> > @@ -1841,10 +1841,19 @@ static int fec_enet_mdio_read(struct mii_bus
> > *bus, int mii_id, int regnum)
> >
> >          ret = pm_runtime_get_sync(dev);
> >          if (ret < 0)
> >                  return ret;
> >
> > +       if (1) {
> > +               u32 ievent;
> > +               ievent = readl(fep->hwp + FEC_IEVENT);
> > +               if (ievent & FEC_ENET_MII) {
> > +                       dev_warn(dev, "found FEC_ENET_MII
> pending\n");
> > +                       writel(FEC_ENET_MII, fep->hwp +
> FEC_IEVENT);
> > +               }
> 
> How often do you see this warning?
> 
> The patch which is causing the regression clears any pending events in
> fec_enet_mii_init() and after each time we wait. So the bit should not be set
> here. If it is set, the question is why?
> 
> The other option is that the hardware is broken. It is setting the event bit way
> too soon, before we can actually read the data from the register.
> 
>         Andrew

Andrew, after investigate the issue, there have one MII event coming later then
clearing MII pending event when writing MSCR register (MII_SPEED).

Check the rtl design by co-working with our IC designer, the 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
	
mmfr[31:0]: current MMFR register value
mscr[7:0]: current MSCR register value
mscr_reg_data_in[7:0]: the value wrote to MSCR


Below patch can fix the block issue:
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2142,6 +2142,15 @@ 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);
        writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);