Re: [net-next PATCH v6] net: macb: Fix several edge cases in validate

["Russell King (Oracle)" <linux@armlinux.org.uk>]

Hi Sean,

Today's update...

On Fri, Nov 19, 2021 at 06:56:09PM +0000, Russell King (Oracle) wrote:
> > Say that you are a MAC with an integrated PCS (so you handle everything
> > in the MAC driver). You support GMII full and half duplex, but your PCS
> > only supports 1000BASE-X with full duplex. The naïve bitmap is
> > 
> > supported_interfaces = PHY_INTERFACE_GMII | PHY_INTERFACE_1000BASEX;
> > mac_capabilities = MAC_10 | MAC_100 | MAC_1000;
> > 
> > but this will report 1000BASE-X as supporting both full and half duplex.
> > So you still need a custom validate() in order to report the correct link
> > modes.
> 
> First, let me say that I've now been through all phylink users in the
> kernel, converting them, and there are seven cases where the generic
> helper can't be used. The validate() method isn't going away (at least
> not just yet) which allows unexpected situations to still be handled.
> The current state of this conversion is as follows.
> 
> In terms of network drivers:
> 
> mvneta and mvpp2: the generic helper is used but needs to be wrapped
>   since these specify that negotiation must not be disabled in
>   "1000BASE-X" mode, which includes the up-clocked 2500BASE-X.

Over the weekend, I've been able to eliminate these two with the
mac_select_pcs() method, and adding a pcs_validate() callback to the
PCS operations. The updated patches are in the net-queue branch.

> > > > Right now, "no pcs" is really not an option I'm afraid. The presence
> > > > of a PCS changes the phylink behaviour slightly . This is one of my
> > > > bug-bears. The evolution of phylink has meant that we need to keep
> > > > compatibility with how phylink used to work before we split the PCS
> > > > support - and we detect that by whether there is a PCS to determine
> > > > whether we need to operate with that compatibility. It probably was
> > > > a mistake to do that in hind sight.
> > 
> > Of course it's an option :)
> 
> I'm saying that with phylink as it is, phylink does _not_ support there
> being no PCS in a non-legacy driver, since the presence of a PCS is a
> flag that the driver is non-legacy. It changes the circumstances in
> which the mac_config() method is called.
> 
> If we want the PCS to become optional, then we need phylink to deal with
> detecting non-legacy drivers some other way (e.g. a legacy-mode flag in
> struct phylink_config), or we need to eliminate the legacy drivers by
> either converting them (which is always my preference) or deleting them.

... and over the weekend, I've implemented a flag "legacy_pre_march2020"
which is used to flag which drivers predate the changes to add PCS
support, allowing phylink to know whether they are legacy drivers or
not irrespective of whether they use a PCS.

So, I've updated the "mac_select_pcs" patch to allow NULL as a valid
return, and use error-pointers to indicate failure.

Then there's a following patch that adds a "pcs_validate" method which
gives the PCS an opportunity to have its say in the MAC validation
without the MAC having code to call the PCS - this change allowed mvneta
and mvpp2 to use phylink_generic_validate().

So, this is another step towards the end goal.

I'm planning to get lkp chew on the "legacy_pre_march2020" before I send
those out to netdev (which will be the next batch of patches), and then
will be the patches that add the ground work to allow DSA drivers to
become properly non-legacy.

Please have a look at my net-queue - are you happier with the
mac_select_pcs method now?

Once we've got this settled, and remaining drivers converted, my plan is
to kill off the old .validate method entirely. At that point,
"mac_capabilities" then becomes exactly that - the capabilities of the
MAC block irrespective of anything else.

Given bcm_sf2, I may need to add a method which allows the MAC to modify
those capabilities depending on the interface - bcm_sf2's pause frame
capabilities appear to be dependent on the interface mode. I'm tempted
to reverse the logic I have in bcm_sf2 in these patches - set
MAC_ASYM_PAUSE | MAC_SYM_PAUSE in mac_capabilities, and exclude them for
interface types that don't support pause frames.

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