| Message ID | E1mx96A-00GCdF-Ei@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phylink: add PCS validation | expand |
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 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); } 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. Maybe the best way is to stick 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. > + 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". --Sean
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. 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.
On Tue, Dec 14, 2021 at 11:27:22PM +0000, Russell King (Oracle) wrote: > I already have a conversion for axienet in my tree, and it doesn't > need a pcs_validate() implementation. I'll provide it below. Forgot to do so... this is obviously untested on real hardware. 8<=== From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> Subject: [PATCH] net: axienet: convert to phylink_pcs Convert axienet to use the phylink_pcs layer, resulting in it no longer being a legacy driver. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/xilinx/xilinx_axienet.h | 1 + .../net/ethernet/xilinx/xilinx_axienet_main.c | 109 +++++++++--------- 2 files changed, 53 insertions(+), 57 deletions(-) diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h index 5b4d153b1492..81f09bd8f11c 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h @@ -434,6 +434,7 @@ struct axienet_local { struct phylink_config phylink_config; struct mdio_device *pcs_phy; + struct phylink_pcs pcs; bool switch_x_sgmii; diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 25082ff3794b..8d90e887c3e5 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -1503,78 +1503,75 @@ static const struct ethtool_ops axienet_ethtool_ops = { .nway_reset = axienet_ethtools_nway_reset, }; -static void axienet_mac_pcs_get_state(struct phylink_config *config, - struct phylink_link_state *state) +static struct axienet_local *pcs_to_axienet_local(struct phylink_pcs *pcs) { - struct net_device *ndev = to_net_dev(config->dev); - struct axienet_local *lp = netdev_priv(ndev); + return container_of(pcs, struct axienet_local, pcs); +} - switch (state->interface) { - case PHY_INTERFACE_MODE_SGMII: - case PHY_INTERFACE_MODE_1000BASEX: - phylink_mii_c22_pcs_get_state(lp->pcs_phy, state); - break; - default: - break; - } +static void axienet_pcs_get_state(struct phylink_pcs *pcs, + struct phylink_link_state *state) +{ + struct mdio_device *pcs_phy = pcs_to_axienet_local(pcs)->pcs_phy; + + phylink_mii_c22_pcs_get_state(pcs_phy, state); } -static void axienet_mac_an_restart(struct phylink_config *config) +static void axienet_pcs_an_restart(struct phylink_pcs *pcs) { - struct net_device *ndev = to_net_dev(config->dev); - struct axienet_local *lp = netdev_priv(ndev); + struct mdio_device *pcs_phy = pcs_to_axienet_local(pcs)->pcs_phy; - phylink_mii_c22_pcs_an_restart(lp->pcs_phy); + phylink_mii_c22_pcs_an_restart(pcs_phy); } -static int axienet_mac_prepare(struct phylink_config *config, unsigned int mode, - phy_interface_t iface) +static int axienet_pcs_config(struct phylink_config *pcs, unsigned int mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit_pause_to_mac) { - struct net_device *ndev = to_net_dev(config->dev); - struct axienet_local *lp = netdev_priv(ndev); + struct net_device *ndev = pcs_to_axienet_local(pcs)->ndev; + struct mdio_device *pcs_phy = pcs_to_axienet_local(pcs)->pcs_phy; int ret; - switch (iface) { - case PHY_INTERFACE_MODE_SGMII: - case PHY_INTERFACE_MODE_1000BASEX: - if (!lp->switch_x_sgmii) - return 0; - - ret = mdiobus_write(lp->pcs_phy->bus, - lp->pcs_phy->addr, - XLNX_MII_STD_SELECT_REG, - iface == PHY_INTERFACE_MODE_SGMII ? - XLNX_MII_STD_SELECT_SGMII : 0); - if (ret < 0) - netdev_warn(ndev, "Failed to switch PHY interface: %d\n", - ret); + ret = mdiobus_write(pcs_phy->bus, pcs_phy->addr, + XLNX_MII_STD_SELECT_REG, + iface == PHY_INTERFACE_MODE_SGMII ? + XLNX_MII_STD_SELECT_SGMII : 0); + if (ret < 0) { + netdev_warn(ndev, "Failed to switch PHY interface: %d\n", + ret); return ret; - default: - return 0; } + + ret = phylink_mii_c22_pcs_config(pcs_phy, mode, interface, advertising); + if (ret < 0) + netdev_warn(ndev, "Failed to configure PCS: %d\n", ret); + + return ret; } -static void axienet_mac_config(struct phylink_config *config, unsigned int mode, - const struct phylink_link_state *state) +static const struct phylink_pcs_ops axienet_pcs_ops = { + .pcs_get_state = axienet_pcs_get_state, + .pcs_config = axienet_pcs_config, + .pcs_an_restart = axienet_pcs_an_restart, +}; + +static struct phylink_pcs *axienet_mac_select_pcs(struct phylink_config *config, + phy_interface_t interface) { struct net_device *ndev = to_net_dev(config->dev); struct axienet_local *lp = netdev_priv(ndev); - int ret; - switch (state->interface) { - case PHY_INTERFACE_MODE_SGMII: - case PHY_INTERFACE_MODE_1000BASEX: - ret = phylink_mii_c22_pcs_config(lp->pcs_phy, mode, - state->interface, - state->advertising); - if (ret < 0) - netdev_warn(ndev, "Failed to configure PCS: %d\n", - ret); - break; + if (interface == PHY_INTERFACE_MODE_1000BASEX || + interface == PHY_INTERFACE_MODE_SGMII) + return &lp->pcs; - default: - break; - } + return NULL; +} + +static void axienet_mac_config(struct phylink_config *config, unsigned int mode, + const struct phylink_link_state *state) +{ + /* nothing meaningful to do */ } static void axienet_mac_link_down(struct phylink_config *config, @@ -1629,9 +1626,7 @@ static void axienet_mac_link_up(struct phylink_config *config, static const struct phylink_mac_ops axienet_phylink_ops = { .validate = phylink_generic_validate, - .mac_pcs_get_state = axienet_mac_pcs_get_state, - .mac_an_restart = axienet_mac_an_restart, - .mac_prepare = axienet_mac_prepare, + .mac_select_pcs = axienet_mac_select_pcs, .mac_config = axienet_mac_config, .mac_link_down = axienet_mac_link_down, .mac_link_up = axienet_mac_link_up, @@ -2040,12 +2035,12 @@ static int axienet_probe(struct platform_device *pdev) ret = -EPROBE_DEFER; goto cleanup_mdio; } - lp->phylink_config.pcs_poll = true; + lp->pcs.ops = &axienet_pcs_ops; + lp->pcs.poll = true; } lp->phylink_config.dev = &ndev->dev; lp->phylink_config.type = PHYLINK_NETDEV; - lp->phylink_config.legacy_pre_march2020 = true; lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10FD | MAC_100FD | MAC_1000FD;
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? > 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. >
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. > > 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. > > 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. As I say above, I don't see there's a problem here - and I think you've mistaken the behaviour of phylink_is_empty_linkmode(). > > > > > + 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. For the explicit case you are highlighting (autoneg), please give an example where both the PCS and MAC would get to "vote" on this bit. Please explain how the MAC would even be involved in autonegotiation. I'm not going to say that this model is perfect, because it isn't. It is adequate for the purposes we need to solve to move the code forwards, and I believe it will allow us to elimate the mac_ops .validate method entirely in a release or two. Once that is done, we will rely on mac_capabilities to tell us what the MAC supports, which is really all that we need the MAC to be telling us. This will become important when we e.g. properly model rate adapting PCS. We can't get close to a model that allows us to do that right now because the .validate method prevents that because that deals with media linkmodes, rather than just the capabilities of the MAC. Our only option right now would be to completely avoid calling the .validate method if we know we have a PCS and completely ignore MAC capabilities. This is an evolutionary step sorting out some of the issues. I'm very sure that there will be things it doesn't do very well identified, and we will need to address those later.
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. 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
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.
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); + 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); return phylink_is_empty_linkmode(supported) ? -EINVAL : 0; diff --git a/include/linux/phylink.h b/include/linux/phylink.h index f44b33cddc4d..381edda618d8 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -416,6 +416,7 @@ struct phylink_pcs { /** * struct phylink_pcs_ops - MAC PCS operations structure. + * @pcs_validate: validate the link configuration. * @pcs_get_state: read the current MAC PCS link state from the hardware. * @pcs_config: configure the MAC PCS for the selected mode and state. * @pcs_an_restart: restart 802.3z BaseX autonegotiation. @@ -423,6 +424,8 @@ struct phylink_pcs { * (where necessary). */ struct phylink_pcs_ops { + int (*pcs_validate)(struct phylink_pcs *pcs, unsigned long *supported, + const struct phylink_link_state *state); void (*pcs_get_state)(struct phylink_pcs *pcs, struct phylink_link_state *state); int (*pcs_config)(struct phylink_pcs *pcs, unsigned int mode, @@ -435,6 +438,23 @@ struct phylink_pcs_ops { }; #if 0 /* For kernel-doc purposes only. */ +/** + * pcs_validate() - validate the link configuration. + * @pcs: a pointer to a &struct phylink_pcs. + * @supported: ethtool bitmask for supported link modes. + * @state: a const pointer to a &struct phylink_link_state. + * + * Validate the interface mode, and advertising's autoneg bit, removing any + * media ethtool link modes that would not be supportable from the supported + * mask. Phylink will propagate the changes to the advertising mask. See the + * &struct phylink_mac_ops validate() method. + * + * Returns -EINVAL if the interface mode/autoneg mode is not supported. + * Returns non-zero positive if the link state can be supported. + */ +int pcs_validate(struct phylink_pcs *pcs, unsigned long *supported, + const struct phylink_link_state *state); + /** * pcs_get_state() - Read the current inband link state from the hardware * @pcs: a pointer to a &struct phylink_pcs.
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(+)