Message-ID: <3a3716a1-54be-e218-5c1e-a794b208aa53@seco.com>
Date:   Tue, 14 Dec 2021 18:54:16 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Marcin Wojtas <mw@...ihalf.com>, netdev@...r.kernel.org,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: 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:
>> Hi Russell,
>>
>> On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
>> > 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) <rmk+kernel@...linux.org.uk>
>> > ---
>> >   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.
>
>> 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.
>
>> 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.
>
>> 	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.

>> > +			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.
>

Powered by blists - more mailing lists