Message-ID: <99059dd8-808d-ddaf-3e65-85b1f7652b7d@seco.com>
Date:   Thu, 16 Dec 2021 10:42:08 -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 7:32 PM, Russell King (Oracle) wrote:
> On Tue, Dec 14, 2021 at 06:54:16PM -0500, Sean Anderson wrote:
>> 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?
>
> I'll reply by asking a more relevant question at this point.
>
> If we've asked for a PCS for 1000BASE-X via .mac_select_pcs() and a
> PCS is returned that does not support 1000BASE-X, what happens then?
> The system level says 1000BASE-X was supported when it isn't...
> That to me sounds like bug.

Well, there are two ways to approach this, IMO, and both involve some
kind of supported_interfaces bitmap. The underlying constraint here is
that the MAC doesn't really know/care at compile-time what the PCS
supports.

- The MAC always returns the external PCS, since that is what the user
   configured. In this case, the PCS is responsible for ensuring that the
   interface is supported. If phylink does not do this check, then it
   must be done in pcs_validate().
- The MAC inspects the PCS's supported_interfaces bitmap, and only
   returns it from mac_select_pcs if it matches.

Sure, if the user says

	pcs-handle = <&my_1000basex_only_pcs>;
	phy-mode = "sgmii";

then this is a misconfiguration, but it is something which we have to
catch, and which the MAC shouldn't detect without additional
information.

>> > 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 */
>
> No, you don't end up in that situation, because phylink_validate() will
> not return 0. It will return -EINVAL. We are not checking for an empty
> supported mask, we are checking for a supported mask that contains no
> linkmodes - this is an important difference between linkmode_empty()
> and phylink_is_empty_linkmode(). The former checks for the linkmode
> bitmap containing all zeros, the latter doesn't care about the media
> bits, autoneg, pause or asympause linkmode bits. If all other bits are
> zero, it returns true, causing phylink_validate_mac_and_pcs() to return
> -EINVAL.

OK, then there should be no issue here.

--Sean

Powered by blists - more mailing lists