Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Russell King (Oracle)]

On Tue, Oct 05, 2021 at 12:03:50PM -0400, Sean Anderson wrote:
> Hi Russell,
> 
> On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> > On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
> > > +static void macb_pcs_get_state(struct phylink_pcs *pcs,
> > > +			       struct phylink_link_state *state)
> > > +{
> > > +	struct macb *bp = pcs_to_macb(pcs);
> > > +
> > > +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> > > +		state->interface = PHY_INTERFACE_MODE_SGMII;
> > > +	else
> > > +		state->interface = PHY_INTERFACE_MODE_1000BASEX;
> > 
> > There is no requirement to set state->interface here. Phylink doesn't
> > cater for interface changes when reading the state. As documented,
> > phylink will set state->interface already before calling this function
> > to indicate what interface mode it is currently expecting from the
> > hardware.
> 
> Ok, so instead I should be doing something like
> 
> if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> 	interface = PHY_INTERFACE_MODE_SGMII;
> else
> 	interface = PHY_INTERFACE_MODE_1000BASEX;
> 
> if (interface != state->interface) {
> 	state->link = 0;
> 	return;
> }

Why would it be different? If we've called the pcs_config method to
set the interface to one mode, why would it change?

> > There has been the suggestion that we should allow in-band AN to be
> > disabled in 1000base-X if we're in in-band mode according to the
> > ethtool state.
> 
> This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
> another _encode variant? I hadn't done this here because the logic was
> only one if statement.
> 
> > I have a patch that adds that.
> 
> Have you posted it?

I haven't - it is a patch from Robert Hancock, "net: phylink: Support
disabling autonegotiation for PCS". I've had it in my tree for a while,
but I do want to make some changes to it before re-posting.

> > You can't actually abort at this point - phylink will print the error
> > and carry on regardless. The checking is all done via the validate()
> > callback and if that indicates the interface mode is acceptable, then
> > it should be accepted.
> 
> Ok, so where can the PCS NAK an interface? This is the only callback
> which has a return code, so I assumed this was the correct place to say
> "no, we don't support this." This is what lynx_pcs_config does as well.

At the moment, the PCS doesn't get to NAK an inappropriate interface.
That's currently the job of the MAC's validate callback with the
assumtion that the MAC knows what interfaces are supportable.

Trying to do it later, once the configuration has been worked out can
_only_ lead to a failure of some kind - in many paths, there is no way
to report the problem except by printing a message into the kernel log.

For example, by the time we reach pcs_config(), we've already prepared
the MAC for a change to the interface, we've told the MAC to configure
for that interface. Now the PCS rejects it - we have no record of the
old configuration to restore. Even if we had a way to restore it, then
we could return an error to the user - but the user doesn't get to
control the interface themselves. If it was the result of a PHY changing
its interface, then what - we can only log an error to the kernel log.
If it's the result of a SFP being plugged in, we have no way to
renegotiate.

pcs_config() is too late to be making decisions about whether the
requested configuration is acceptable or not. It needs to be done as
part of the validation step.

However, the validation step is not purely just validation, but it's
negotiation too for SFPs to be able to work out what interface mode
they should use in combination with the support that the MAC/PCS
offers.

I do feel that the implementation around the validation/selection of
interface for SFP etc is starting to creak, and I've some patches that
introduce a bitmap of interface types that are supported by the various
components. I haven't had the motivation to finish that off as my last
attempt at making a phylink API change was not pleasant in terms of
either help updating network drivers or getting patches tested. So I
now try to avoid phylink API changes at all cost.

People have whinged that phylink's API changes too quickly... I'm
guessing we're now going to get other people arguing that it needs
change to fix issues like this...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!