netdev - Re: [PATCH v7 06/11] net: pch_gbe: Only enable MAC when PHY link is active

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]

Date:   Wed, 27 Jun 2018 10:54:28 -0700
From:   Paul Burton <paul.burton@...s.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v7 06/11] net: pch_gbe: Only enable MAC when PHY link is
 active

Hi Andrew,

On Wed, Jun 27, 2018 at 07:30:14PM +0200, Andrew Lunn wrote:
> On Tue, Jun 26, 2018 at 05:06:07PM -0700, Paul Burton wrote:
> > When using a PHY connected via RGMII, as the pch_gbe driver presumes is
> > the case, the RX clock is provided by the PHY to the MAC. Various PHYs,
> > including both the AR8031 used by the Minnowboard & the RTL8211E used by
> > the MIPS Boston development board, will stop generating the RX clock
> > when the ethernet link is down (eg. the ethernet cable is unplugged).
> > 
> > Various pieces of functionality in the EG20T MAC, ranging from basics
> > like completing a MAC reset to programming MAC addresses, rely upon the
> > RX clock being provided. When the clock is not provided these pieces of
> > functionality simply never complete, and the busy bits that indicate
> > they're in progress remain set indefinitely.
> > 
> > The pch_gbe driver currently requires that the RX clock is always
> > provided, and attempts to enforce this by disabling the hibernation
> > feature of the AR8031 PHY to keep it generating the RX clock. This patch
> > moves us away from this model by only configuring the MAC when the PHY
> > indicates that the ethernet link is up. When the link is up we should be
> > able to safely expect that the RX clock is being provided, and therefore
> > safely reset & configure the MAC.
> 
> Hi Paul
> 
> I like the concept, but the implementation is not clear. Maybe it just
> needs more details in the commit message. What has the watchdog got to
> do with link up?

pch_gbe_watchdog() polls for the link coming up or going down, so that's
where we find out that the link is up.

> And what happens on link down? Does the MAC need shutting down? I
> don't see such code here.

Well, depending upon the PHY the RX clock might stop which will prevent
parts of the MAC from functioning properly. Exactly which parts I don't
really know - the EG20T documentation is vague & unclear. I do know
that:

  - We won't receive packets any more, of course. This should be fine
    without any extra handling because we just won't see any futher DMA
    complete interrupts (or the associated bit set when polling).

  - A MAC reset won't complete - ie. the pch_gbe_wait_clr_bit() in
    pch_gbe_reset()/pch_gbe_reset_hw() will time out. This I think
    should be OK because after this patch we won't generally reset the
    MAC when the link is down anyway, except perhaps the PCI error_state
    case in pch_gbe_down(). I'm not sure what the reset there is for...

  - Masking or unmasking MAC address registers won't complete - ie. the
    pch_gbe_wait_clr_bit() in pch_gbe_mac_mar_set() or
    pch_gbe_set_multi() will time out. This is again when the link is
    already known to be up, although there is a case in
    __pch_gbe_suspend() which is setting up WoL that I'm not so sure
    about...

Thanks,
    Paul