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] [day] [month] [year] [list]

Message-ID: <20200428142909.GB22600@lunn.ch>
Date:   Tue, 28 Apr 2020 16:29:09 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Andy Duan <fugang.duan@....com>
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

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

Ah. Now i get it....

The flow is...

Write FEC_MII_SPEED register
Clear event register

A little latter

event bit set because of FEC_MII_SPEED

A little later
Setup read
fec_enet_mdio_wait()
exit immediately, because of pending event from FEC_MII_SPEED
Clear FEC_MII_SPEED event
Read data register too early

A little later

event bit set because read complete

A little later
Setup read
fec_enet_mdio_wait()
exit immediately, because of pending event from read complete
Clear previous read complete event
Read data register too early

A little later

event bit set because read complete

And the cycle continues...

I will make a formal patch from your email.

  Andrew