[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YbkoWsMPgw5RsQCo@shell.armlinux.org.uk>
Date: Tue, 14 Dec 2021 23:27:22 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Sean Anderson <sean.anderson@...o.com>
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 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.
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?
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.
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.
> > + 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?
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists