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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Mar 17, 2020 at 03:48:50PM +0000, Jose Abreu wrote:
> From: Russell King <rmk+kernel@armlinux.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