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: <HE1PR0402MB27457CCE2807853856117D76FFAC0@HE1PR0402MB2745.eurprd04.prod.outlook.com>
Date:   Tue, 28 Apr 2020 13:50:59 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Leonard Crestez <leonard.crestez@....com>,
        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 9:35 PM
> > 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);
> 
> Hi Andy
> 
> Thanks for digging into the internal of the FEC. Just to make sure i understand
> this correctly:
> 
> In fec_enet_mii_init() we have:
> 
>         holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000)
> - 1;
> 
>         fep->phy_speed = mii_speed << 1 | holdtime << 8;
> 
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
>         /* Clear any pending transaction complete indication */
>         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> 
> You are saying this write to the FEC_MII_SPEED register can on some SoCs
> trigger an FEC_ENET_MII event. And because it does not happen immediately,
> it happens after the clear which is performed here?

Correct.
Before write FEC_MII_SPEED register, FEC_MII_DATA register is not zero, and
the current value of FEC_MII_SPEED register is zero, once write non zero value
to FEC_MII_SPEED register, it trigger MII event.

> Sometime later we then go into fec_enet_mdio_wait(), the event is still
> pending, so we read the FEC_MII_DATA register too early?

Correct.
The first mdio operation is mdio read, read FEC_MII_DATA register is too early,
it get invalid value. 
> 
> But this does not fully explain the problem. This should only affect the first
> MDIO transaction, because as we exit fec_enet_mdio_wait() the event is
> cleared. But Leonard reported that all reads return 0, not just the first.

Of course, it impact subsequent mdio read/write operations.
After you clear MII event that is pending before.
Then, after mdio read data back, MII event is set again.

cpu instruction is much faster than mdio read/write operation.

> 
>     Andrew