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

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

On Thu, Dec 16, 2021 at 10:42:08AM -0500, Sean Anderson wrote:
> 
> 
> 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@armlinux.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.

Yes - we can do these sorts of things later if it turns out there is
a requirement to do so. At the moment, having been through all the
drivers recently, I don't see the need for it yet.

The only driver that may come close is xpcs, and the patches I've
proposed there don't need it - I just populate the PCS support 
by calling into xpcs.

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