Re: [PATCH net-next 2/7] net: phylink: add pcs_validate() method

On 12/14/21 6:27 PM, Russell King (Oracle) wrote:

On Tue, Dec 14, 2021 at 02:49:13PM -0500, Sean Anderson wrote:

quoted

Hi Russell,

On 12/14/21 9:48 AM, Russell King (Oracle) wrote:

quoted

Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.

Signed-off-by: Russell King (Oracle) <redacted>
---
  drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
  include/linux/phylink.h   | 20 ++++++++++++++++++++
  2 files changed, 51 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index c7035d65e159..420201858564 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
  					struct phylink_link_state *state)
  {
  	struct phylink_pcs *pcs;
+	int ret;

+	/* Get the PCS for this interface mode */
  	if (pl->mac_ops->mac_select_pcs) {
  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
  		if (IS_ERR(pcs))
  			return PTR_ERR(pcs);
+	} else {
+		pcs = pl->pcs;
+	}
+
+	if (pcs) {
+		/* The PCS, if present, must be setup before phylink_create()
+		 * has been called. If the ops is not initialised, print an
+		 * error and backtrace rather than oopsing the kernel.
+		 */
+		if (!pcs->ops) {
+			phylink_err(pl, "interface %s: uninitialised PCS\n",
+				    phy_modes(state->interface));
+			dump_stack();
+			return -EINVAL;
+		}
+
+		/* Validate the link parameters with the PCS */
+		if (pcs->ops->pcs_validate) {
+			ret = pcs->ops->pcs_validate(pcs, supported, state);

I wonder if we can add a pcs->supported_interfaces. That would let me
write something like

I have two arguments against that:

1) Given that .mac_select_pcs should not return a PCS that is not
    appropriate for the provided state->interface, I don't see what
    use having a supported_interfaces member in the PCS would give.
    All that phylink would end up doing is validating that the MAC
    was giving us a sane PCS.

The MAC may not know what the PCS can support. For example, the xilinx
PCS/PMA can be configured to support 1000BASE-X, SGMII, both, or
neither. How else should the mac find out what is supported?

2) In the case of a static PCS (in other words, one attached just
    after phylink_create_pcs()) the PCS is known at creation time,
    so limiting phylink_config.supported_interfaces according to the
    single attached interface seems sane, rather than phylink having
    to repeatedly recalculate the bitwise-and between both
    supported_interface masks.

quoted

static int xilinx_pcs_validate(struct phylink_pcs *pcs,
			       unsigned long *supported,
			       struct phylink_link_state *state)
{
	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };

	phylink_set_port_modes(mask);
	phylink_set(mask, Autoneg);
	phylink_get_linkmodes(mask, state->interface,
			      MAC_10FD | MAC_100FD | MAC_1000FD);

	linkmode_and(supported, supported, mask);
}

This would be buggy - doesn't the PCS allow pause frames through?

Yes. I noticed this after writing my above email :)

I already have a conversion for axienet in my tree, and it doesn't
need a pcs_validate() implementation. I'll provide it below.

quoted

And of course, the above could become phylink_pcs_validate_generic with
the addition of a pcs->pcs_capabilities member.

The only wrinkle is that we need to handle PHY_INTERFACE_MODE_NA,
because of the pcs = pl->pcs assignment above. This would require doing
the phylink_validate_any dance again.

Why do you think PHY_INTERFACE_MODE_NA needs handling? If this is not
set in phylink_config.supported_interfaces (which it should never be)
then none of the validation will be called with this.

If the MAC has no supported_interfaces and calls phylink_set_pcs, but
does not implement mac_select_pcs, then you can have something like

    phylink_validate(NA)
        phylink_validate_mac_and_pcs(NA)
            pcs = pl->pcs;
            pcs->ops->pcs_validate(NA)
                phylink_get_linkmodes(NA)
                /* returns just Pause and Asym_Pause linkmodes */
            /* nonzero, so pcs_validate thinks it's fine */
    /* phylink_validate returns 0, but there are no valid interfaces */

The special PHY_INTERFACE_MODE_NA meaning "give us everything you have"
is something I want to get rid of, and is something that I am already
explicitly not supporting for pcs_validate(). It doesn't work with the
mac_select_pcs() model, since that can't return all PCS that may be
used.

quoted

	if (state->interface == PHY_INTERFACE_MODE_NA)
		return -EINVAL;

at the top of phylink_pcs_validate_generic (perhaps with a warning).
That would catch any MACs who use a PCS which wants the MAC to have
supported_interfaces.

... which could be too late.

You can't detect this in advance, since a MAC can choose to attach
whatever PCS it wants at any time. So all you can do is warn about it so
people report it as a bug instead of wondering why their ethernet won't
configure.

quoted

quoted

+			if (ret < 0 || phylink_is_empty_linkmode(supported))
+				return -EINVAL;
+
+			/* Ensure the advertising mask is a subset of the
+			 * supported mask.
+			 */
+			linkmode_and(state->advertising, state->advertising,
+				     supported);
+		}
  	}

+	/* Then validate the link parameters with the MAC */
  	pl->mac_ops->validate(pl->config, supported, state);

Shouldn't the PCS stuff happen here? Later in the series, you do things
like

	if (phy_interface_mode_is_8023z(state->interface) &&
	    !phylink_test(state->advertising, Autoneg))
		return -EINVAL;

but there's nothing to stop a mac validate from coming along and saying
"we don't support autonegotiation".

How is autonegotiation a property of the MAC when there is a PCS?
In what situation is autonegotiation terminated at the MAC when
there is a PCS present?

*shrug* it doesn't make a difference really as long as the MAC and PCS
play nice. But validate works by masking out bits, so you can only
really test for a bit after everyone has gotten their chance to veto
things. Which is why I think it is strange that the PCS check comes
first.

--Sean

The only case I can think of is where the PCS is tightly tied to the
MAC, and in that case you end up with a choice whether or not to model
a PCS in software. This is the case with mvneta and mvpp2 - there is
no separation of the MAC and PCS in the hardware register design. There
is one register that controls pause/duplex advertisement and speeds
irrespective of the PHY interface, whether the interface mode to the
external world is 1000BASE-X, SGMII, QSGMII, RGMII etc. mvpp2 is
slightly different in that it re-uses the GMAC design from mvneta for
speeds <= 2.5G, and an entirely separate XLG implementation for 5G
and 10G. Here, we model these as two separate PCS that we choose
between depending on the interface.