netdev - Re: [RFC net-next 2/5] net: phylink: add separate pcs operations structure

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Mar 2020 15:56:10 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC net-next 2/5] net: phylink: add separate pcs operations
 structure

On Tue, Mar 17, 2020 at 03:48:50PM +0000, Jose Abreu wrote:
> From: Russell King <rmk+kernel@...linux.org.uk>
> Date: Mar/17/2020, 14:52:51 (UTC+00:00)
> 
> > -static void phylink_mac_an_restart(struct phylink *pl)
> > +static void phylink_mac_pcs_an_restart(struct phylink *pl)
> >  {
> >  	if (pl->link_config.an_enabled &&
> > -	    phy_interface_mode_is_8023z(pl->link_config.interface))
> > -		pl->mac_ops->mac_an_restart(pl->config);
> > +	    phy_interface_mode_is_8023z(pl->link_config.interface)) {
> 
> Please consider removing this condition and just rely on an_enabled field. 
> I have USXGMII support for Clause 73 Autoneg so this won't work with 
> that.

That is actually incorrect.  SGMII can have an_enabled being true, but
SGMII is not an autonegotiation between the MAC and PHY - it is merely
a mechanism for the PHY to inform the MAC what the results of _its_
negotiation are.

I suspect USXGMII is the same since it is just an "upgraded" version of
SGMII.  Please can you check whether there really is any value in trying
(and likely failing) to restart the "handshake" with the PHY from the
MAC end, rather than requesting the PHY to restart negotiation on its
media side.

> Overall, looks nice but I don't see a mechanism to restart AutoNeg in case 
> of failure. i.e. following PHYLIB implementation you should add a call to 
> restart_aneg in the state machine. This was just a quick look so I may 
> have missed this.

phylink doesn't have a state machine.  At what point do you think that
restarting negotiation (and which negotiation in the case of SGMII or
presumably USXGMII) would be appropriate?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up