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

On 10/7/21 12:23 PM, Russell King (Oracle) wrote:

On Thu, Oct 07, 2021 at 12:29:00PM +0100, Russell King (Oracle) wrote:

quoted

Here's a patch which illustrates roughly what I'm thinking at the
moment - only build tested.

mac_select_pcs() should not ever fail in phylink_major_config() - that
would be a bug. I've hooked mac_select_pcs() also into the validate
function so we can catch problems there, but we will need to involve
the PCS in the interface selection for SFPs etc.

Note that mac_select_pcs() must be inconsequential - it's asking the
MAC which PCS it wishes to use for the interface mode.

I am still very much undecided whether we wish phylink to parse the
pcs-handle property and if present, override the MAC - I feel that
logic depends in the MAC driver, since a single PCS can be very
restrictive in terms of what interface modes are supportable. If the
MAC wishes pcs-handle to override its internal ones, then it can
always do:

	if (port->external_pcs)
		return port->external_pcs;

in its mac_select_pcs() implementation. This gives us a bit of future
flexibility.

If we parse pcs-handle in phylink, then if we end up with multiple PCS
to choose from, we then need to work out how to either allow the MAC
driver to tell phylink not to parse pcs-handle, or we need some way for
phylink to ask the MAC "these are the PCS I have, which one should I
use" which is yet another interface.

What I don't like about the patch is the need to query the PCS based on
interface - when we have a SFP plugged in, it may support multiple
interfaces. I think we still need the MAC to restrict what it returns
in its validate() method according to the group of PCS that it has
available for the SFP interface selection to work properly. Things in
this regard should become easier _if_ I can switch phylink over to
selecting interface based on phy_interface_t bitmaps rather than the
current bodge using ethtool link modes, but that needs changes to phylib
and all MAC drivers, otherwise we have to support two entirely separate
ways to select the interface mode.

My argument against that is... I'll end up converting the network
interfaces that I use to the new implementation, and the old version
will start to rot. I've already stopped testing phylink without a PCS
attached for this very reason. The more legacy code we keep, the worse
this problem becomes.

Having finished off the SFP side of the phy_interface_t bitmap
(http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue)
and I think the mac_select_pcs() approach will work.

See commit
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=3e0d51c361f5191111af206e3ed024d4367fce78
where we have a set of phy_interface_t to choose one from, and if
we add PCS selection into that logic, the loop becomes:

static phy_interface_t phylink_select_interface(struct phylink *pl,
						const unsigned long *intf
						const char *intf_name)
{
	phy_interface_t interface, intf;

	...

	interface = PHY_INTERFACE_MODE_NA;
	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
		intf = phylink_sfp_interface_preference[i];

		if (!test_bit(intf, u))
			continue;

		pcs = pl->pcs;
		if (pl->mac_ops->mac_select_pcs) {
			pcs = pl->mac_ops->mac_select_pcs(pl->config, intf);
			if (!pcs)
				continue;
		}

		if (pcs && !test_bit(intf, pcs->supported_interfaces))
			continue;

		interface = intf;
		break;
	}
	...
}

The alternative would be to move some of that logic into
phylink_sfp_config_nophy(), and will mean knocking out bits from
the mask supplied to phylink_select_interface() each time we select
an interface mode that the PCS doesn't support... which sounds rather
more yucky to me.

With appropriate helper functions, I don't think we would need to have a
separate mac_select_pcs callback:

probe()
{
	priv->pl = phylink_create();
	priv->pcs1 = my_internal_pcs;
	priv->pcs2 = phylink_find_pcs();
}

validate()
{
	switch (state->interface) {
	case PHY_INTERFACE_MODE_NA:
	case PHY_INTERFACE_GMII:
		phylink_set(mask, 1000baseT_Full);
		phylink_set(mask, 1000baseX_Full);
		if (one)
			break;
		fallthrough;
	default:
		matched = phylink_set_pcs_modes(mask, priv->pcs1, state);
		matched ||= phylink_set_pcs_modes(mask, priv->pcs2, state);
		if (matched)
			break;
		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
		return;
	}
	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
}

prepare()
{
	switch (state->interface) {
	case PHY_INTERFACE_GMII:
		enable_gmii();
		break;
	default:
		if (phylink_attach_matching_pcs(priv->pl, priv->pcs1, state->interface)) {
			enable_internal_pcs();
			break;
		}
		if (phylink_attach_matching_pcs(priv->pl, priv->pcs2, state->interface)) {
			enable_external_pcs();
			break;	
		}
		BUG();
	}
}

int phylink_set_interface_mode(mask, iface)
{
	/* switch statement from phylink_parse_mode here */
}

bool phylink_set_pcs_modes(mask, pcs, state)
{
	if (state->interface != PHY_INTERFACE_MODE_NA) {
		if (!test_bit(state->interface, pcs->supported_interfaces))
			return false;
		phylink_set_interface_mode(mask, state->interface);
		return true;
	}
	
	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
		if (test_bit(i, pcs->supported_interfaces))
			phylink_set_interface_mode(mask, state->interface);
	}
	return true;
}

bool phylink_attach_matching_pcs(pl, pcs, iface)
{
	if (!test_bit(iface, pcs->supported_interfaces))
		return false;
	phylink_set_pcs(pl, pcs);
	return true;
}

I think this has some advantages over a separate mac_select_pcs():

- In validate() you get all the available interfaces.
- The MAC can set the priority of its PCSs however it likes, which is
  probably good enough for almost every case.
- The MAC doesn't care about what interfaces the PCSs actually support.
- phylink can do whatever logic it wants for selection under the hood.
- From phylink's POV, drivers behave the same way no matter whether they
  use these helpers or whether they just say "If we use SGMII, then use
  our internal PCS)".

--Sean