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

Powered by blists - more mailing lists