Message ID | 20250228145540.2209551-3-maxime.chevallier@bootlin.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | net: phy: Rework linkmodes handling in a dedicated file | expand |
On Fri, Feb 28, 2025 at 03:55:27PM +0100, Maxime Chevallier wrote: > The canonical definition for all the link modes is in linux/ethtool.h, > which is complemented by the link_mode_params array stored in > net/ethtool/common.h . That array contains all the metadata about each > of these modes, including the Speed and Duplex information. > > Phylib and phylink needs that information as well for internal > management of the link, which was done by duplicating that information > in locally-stored arrays and lookup functions. This makes it easy for > developpers adding new modes to forget modifying phylib and phylink > accordingly. > > However, the link_mode_params array in net/ethtool/common.c is fairly > inefficient to search through, as it isn't sorted in any manner. Phylib > and phylink perform a lot of lookup operations, mostly to filter modes > by speed and/or duplex. > > We therefore introduce the link_caps private array in phy_caps.c, that > indexes linkmodes in a more efficient manner. Each element associated a > tuple <speed, duplex> to a bitfield of all the linkmodes runs at these > speed/duplex. > > We end-up with an array that's fairly short, easily addressable and that > it optimised for the typical use-cases of phylib/phylink. > > That array is initialized at the same time as phylib. As the > link_mode_params array is part of the net stack, which phylink depends > on, it should always be accessible from phylib. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > --- > drivers/net/phy/Makefile | 2 +- > drivers/net/phy/phy-caps.h | 44 ++++++++++++++++++++ > drivers/net/phy/phy_caps.c | 78 ++++++++++++++++++++++++++++++++++++ > drivers/net/phy/phy_device.c | 2 + > 4 files changed, 125 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/phy/phy-caps.h > create mode 100644 drivers/net/phy/phy_caps.c > > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index c8dac6e92278..7e800619162b 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -2,7 +2,7 @@ > # Makefile for Linux PHY drivers > > libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \ > - linkmode.o phy_link_topology.o > + linkmode.o phy_link_topology.o phy_caps.o > mdio-bus-y += mdio_bus.o mdio_device.o > > ifdef CONFIG_MDIO_DEVICE > diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h > new file mode 100644 > index 000000000000..846d483269f6 > --- /dev/null > +++ b/drivers/net/phy/phy-caps.h > @@ -0,0 +1,44 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * link caps internal header, for link modes <-> capabilities <-> interfaces > + * conversions. > + */ > + > +#ifndef __PHY_CAPS_H > +#define __PHY_CAPS_H > + > +#include <linux/ethtool.h> > + > +enum { > + LINK_CAPA_10HD = 0, > + LINK_CAPA_10FD, > + LINK_CAPA_100HD, > + LINK_CAPA_100FD, > + LINK_CAPA_1000HD, > + LINK_CAPA_1000FD, > + LINK_CAPA_2500FD, > + LINK_CAPA_5000FD, > + LINK_CAPA_10000FD, > + LINK_CAPA_20000FD, > + LINK_CAPA_25000FD, > + LINK_CAPA_40000FD, > + LINK_CAPA_50000FD, > + LINK_CAPA_56000FD, > + LINK_CAPA_100000FD, > + LINK_CAPA_200000FD, > + LINK_CAPA_400000FD, > + LINK_CAPA_800000FD, > + > + __LINK_CAPA_LAST = LINK_CAPA_800000FD, > + __LINK_CAPA_MAX, > +}; > + > +struct link_capabilities { > + int speed; > + unsigned int duplex; > + __ETHTOOL_DECLARE_LINK_MODE_MASK(linkmodes); > +}; > + > +void phy_caps_init(void); > + > +#endif /* __PHY_CAPS_H */ > diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c > new file mode 100644 > index 000000000000..367ca7110ddc > --- /dev/null > +++ b/drivers/net/phy/phy_caps.c > @@ -0,0 +1,78 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#include <linux/ethtool.h> > +#include <linux/linkmode.h> > +#include <linux/phy.h> > + > +#include "phy-caps.h" > + > +static struct link_capabilities link_caps[__LINK_CAPA_MAX] __ro_after_init = { > + { SPEED_10, DUPLEX_HALF, {0} }, /* LINK_CAPA_10HD */ > + { SPEED_10, DUPLEX_FULL, {0} }, /* LINK_CAPA_10FD */ > + { SPEED_100, DUPLEX_HALF, {0} }, /* LINK_CAPA_100HD */ > + { SPEED_100, DUPLEX_FULL, {0} }, /* LINK_CAPA_100FD */ > + { SPEED_1000, DUPLEX_HALF, {0} }, /* LINK_CAPA_1000HD */ > + { SPEED_1000, DUPLEX_FULL, {0} }, /* LINK_CAPA_1000FD */ > + { SPEED_2500, DUPLEX_FULL, {0} }, /* LINK_CAPA_2500FD */ > + { SPEED_5000, DUPLEX_FULL, {0} }, /* LINK_CAPA_5000FD */ > + { SPEED_10000, DUPLEX_FULL, {0} }, /* LINK_CAPA_10000FD */ > + { SPEED_20000, DUPLEX_FULL, {0} }, /* LINK_CAPA_20000FD */ > + { SPEED_25000, DUPLEX_FULL, {0} }, /* LINK_CAPA_25000FD */ > + { SPEED_40000, DUPLEX_FULL, {0} }, /* LINK_CAPA_40000FD */ > + { SPEED_50000, DUPLEX_FULL, {0} }, /* LINK_CAPA_50000FD */ > + { SPEED_56000, DUPLEX_FULL, {0} }, /* LINK_CAPA_56000FD */ > + { SPEED_100000, DUPLEX_FULL, {0} }, /* LINK_CAPA_100000FD */ > + { SPEED_200000, DUPLEX_FULL, {0} }, /* LINK_CAPA_200000FD */ > + { SPEED_400000, DUPLEX_FULL, {0} }, /* LINK_CAPA_400000FD */ > + { SPEED_800000, DUPLEX_FULL, {0} }, /* LINK_CAPA_800000FD */ > +}; > + > +static int speed_duplex_to_capa(int speed, unsigned int duplex) > +{ > + if (duplex == DUPLEX_UNKNOWN || > + (speed > SPEED_1000 && duplex != DUPLEX_FULL)) > + return -EINVAL; > + > + switch (speed) { > + case SPEED_10: return duplex == DUPLEX_FULL ? > + LINK_CAPA_10FD : LINK_CAPA_10HD; > + case SPEED_100: return duplex == DUPLEX_FULL ? > + LINK_CAPA_100FD : LINK_CAPA_100HD; > + case SPEED_1000: return duplex == DUPLEX_FULL ? > + LINK_CAPA_1000FD : LINK_CAPA_1000HD; > + case SPEED_2500: return LINK_CAPA_2500FD; > + case SPEED_5000: return LINK_CAPA_5000FD; > + case SPEED_10000: return LINK_CAPA_10000FD; > + case SPEED_20000: return LINK_CAPA_20000FD; > + case SPEED_25000: return LINK_CAPA_25000FD; > + case SPEED_40000: return LINK_CAPA_40000FD; > + case SPEED_50000: return LINK_CAPA_50000FD; > + case SPEED_56000: return LINK_CAPA_56000FD; > + case SPEED_100000: return LINK_CAPA_100000FD; > + case SPEED_200000: return LINK_CAPA_200000FD; > + case SPEED_400000: return LINK_CAPA_400000FD; > + case SPEED_800000: return LINK_CAPA_800000FD; I think one of the issues you mentioned is about the need to update several places as new linkmodes are added. One of the side effects of adding new linkmodes is that they generally come with faster speeds, so this is a place that needs to be updated along with the table above. I'm not sure whether this makes that problem better or worse - if a new linkmode is added with a SPEED_*, the author of such a change has to be on the ball to update these, and I'm not sure that'll happen.
Hello Russell, On Fri, 28 Feb 2025 15:41:26 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Fri, Feb 28, 2025 at 03:55:27PM +0100, Maxime Chevallier wrote: > > The canonical definition for all the link modes is in linux/ethtool.h, > > which is complemented by the link_mode_params array stored in > > net/ethtool/common.h . That array contains all the metadata about each > > of these modes, including the Speed and Duplex information. > > > > Phylib and phylink needs that information as well for internal > > management of the link, which was done by duplicating that information > > in locally-stored arrays and lookup functions. This makes it easy for > > developpers adding new modes to forget modifying phylib and phylink > > accordingly. > > > > However, the link_mode_params array in net/ethtool/common.c is fairly > > inefficient to search through, as it isn't sorted in any manner. Phylib > > and phylink perform a lot of lookup operations, mostly to filter modes > > by speed and/or duplex. > > > > We therefore introduce the link_caps private array in phy_caps.c, that > > indexes linkmodes in a more efficient manner. Each element associated a > > tuple <speed, duplex> to a bitfield of all the linkmodes runs at these > > speed/duplex. > > > > We end-up with an array that's fairly short, easily addressable and that > > it optimised for the typical use-cases of phylib/phylink. > > > > That array is initialized at the same time as phylib. As the > > link_mode_params array is part of the net stack, which phylink depends > > on, it should always be accessible from phylib. > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > --- [...] > > +static int speed_duplex_to_capa(int speed, unsigned int duplex) > > +{ > > + if (duplex == DUPLEX_UNKNOWN || > > + (speed > SPEED_1000 && duplex != DUPLEX_FULL)) > > + return -EINVAL; > > + > > + switch (speed) { > > + case SPEED_10: return duplex == DUPLEX_FULL ? > > + LINK_CAPA_10FD : LINK_CAPA_10HD; > > + case SPEED_100: return duplex == DUPLEX_FULL ? > > + LINK_CAPA_100FD : LINK_CAPA_100HD; > > + case SPEED_1000: return duplex == DUPLEX_FULL ? > > + LINK_CAPA_1000FD : LINK_CAPA_1000HD; > > + case SPEED_2500: return LINK_CAPA_2500FD; > > + case SPEED_5000: return LINK_CAPA_5000FD; > > + case SPEED_10000: return LINK_CAPA_10000FD; > > + case SPEED_20000: return LINK_CAPA_20000FD; > > + case SPEED_25000: return LINK_CAPA_25000FD; > > + case SPEED_40000: return LINK_CAPA_40000FD; > > + case SPEED_50000: return LINK_CAPA_50000FD; > > + case SPEED_56000: return LINK_CAPA_56000FD; > > + case SPEED_100000: return LINK_CAPA_100000FD; > > + case SPEED_200000: return LINK_CAPA_200000FD; > > + case SPEED_400000: return LINK_CAPA_400000FD; > > + case SPEED_800000: return LINK_CAPA_800000FD; > > I think one of the issues you mentioned is about the need to update > several places as new linkmodes are added. > > One of the side effects of adding new linkmodes is that they generally > come with faster speeds, so this is a place that needs to be updated > along with the table above. > > I'm not sure whether this makes that problem better or worse - if a > new linkmode is added with a SPEED_*, the author of such a change has > to be on the ball to update these, and I'm not sure that'll happen. That's true indeed, and it seems that's already the case today (there's no MAC_800000FD in phylink for example). It seems to me that NICs with very fast speeds don't care at all about phylib nor phylink... So yeah this work does not address the complexity of adding a new speed :( To solve that, we could consider some macro magic to define new speeds with an associated LINK_CAPA, however that's quite the overhaul. Do you see that new enum for link_caps as a blocking point for that series ? Maxime
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index c8dac6e92278..7e800619162b 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -2,7 +2,7 @@ # Makefile for Linux PHY drivers libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \ - linkmode.o phy_link_topology.o + linkmode.o phy_link_topology.o phy_caps.o mdio-bus-y += mdio_bus.o mdio_device.o ifdef CONFIG_MDIO_DEVICE diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h new file mode 100644 index 000000000000..846d483269f6 --- /dev/null +++ b/drivers/net/phy/phy-caps.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * link caps internal header, for link modes <-> capabilities <-> interfaces + * conversions. + */ + +#ifndef __PHY_CAPS_H +#define __PHY_CAPS_H + +#include <linux/ethtool.h> + +enum { + LINK_CAPA_10HD = 0, + LINK_CAPA_10FD, + LINK_CAPA_100HD, + LINK_CAPA_100FD, + LINK_CAPA_1000HD, + LINK_CAPA_1000FD, + LINK_CAPA_2500FD, + LINK_CAPA_5000FD, + LINK_CAPA_10000FD, + LINK_CAPA_20000FD, + LINK_CAPA_25000FD, + LINK_CAPA_40000FD, + LINK_CAPA_50000FD, + LINK_CAPA_56000FD, + LINK_CAPA_100000FD, + LINK_CAPA_200000FD, + LINK_CAPA_400000FD, + LINK_CAPA_800000FD, + + __LINK_CAPA_LAST = LINK_CAPA_800000FD, + __LINK_CAPA_MAX, +}; + +struct link_capabilities { + int speed; + unsigned int duplex; + __ETHTOOL_DECLARE_LINK_MODE_MASK(linkmodes); +}; + +void phy_caps_init(void); + +#endif /* __PHY_CAPS_H */ diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c new file mode 100644 index 000000000000..367ca7110ddc --- /dev/null +++ b/drivers/net/phy/phy_caps.c @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +#include <linux/ethtool.h> +#include <linux/linkmode.h> +#include <linux/phy.h> + +#include "phy-caps.h" + +static struct link_capabilities link_caps[__LINK_CAPA_MAX] __ro_after_init = { + { SPEED_10, DUPLEX_HALF, {0} }, /* LINK_CAPA_10HD */ + { SPEED_10, DUPLEX_FULL, {0} }, /* LINK_CAPA_10FD */ + { SPEED_100, DUPLEX_HALF, {0} }, /* LINK_CAPA_100HD */ + { SPEED_100, DUPLEX_FULL, {0} }, /* LINK_CAPA_100FD */ + { SPEED_1000, DUPLEX_HALF, {0} }, /* LINK_CAPA_1000HD */ + { SPEED_1000, DUPLEX_FULL, {0} }, /* LINK_CAPA_1000FD */ + { SPEED_2500, DUPLEX_FULL, {0} }, /* LINK_CAPA_2500FD */ + { SPEED_5000, DUPLEX_FULL, {0} }, /* LINK_CAPA_5000FD */ + { SPEED_10000, DUPLEX_FULL, {0} }, /* LINK_CAPA_10000FD */ + { SPEED_20000, DUPLEX_FULL, {0} }, /* LINK_CAPA_20000FD */ + { SPEED_25000, DUPLEX_FULL, {0} }, /* LINK_CAPA_25000FD */ + { SPEED_40000, DUPLEX_FULL, {0} }, /* LINK_CAPA_40000FD */ + { SPEED_50000, DUPLEX_FULL, {0} }, /* LINK_CAPA_50000FD */ + { SPEED_56000, DUPLEX_FULL, {0} }, /* LINK_CAPA_56000FD */ + { SPEED_100000, DUPLEX_FULL, {0} }, /* LINK_CAPA_100000FD */ + { SPEED_200000, DUPLEX_FULL, {0} }, /* LINK_CAPA_200000FD */ + { SPEED_400000, DUPLEX_FULL, {0} }, /* LINK_CAPA_400000FD */ + { SPEED_800000, DUPLEX_FULL, {0} }, /* LINK_CAPA_800000FD */ +}; + +static int speed_duplex_to_capa(int speed, unsigned int duplex) +{ + if (duplex == DUPLEX_UNKNOWN || + (speed > SPEED_1000 && duplex != DUPLEX_FULL)) + return -EINVAL; + + switch (speed) { + case SPEED_10: return duplex == DUPLEX_FULL ? + LINK_CAPA_10FD : LINK_CAPA_10HD; + case SPEED_100: return duplex == DUPLEX_FULL ? + LINK_CAPA_100FD : LINK_CAPA_100HD; + case SPEED_1000: return duplex == DUPLEX_FULL ? + LINK_CAPA_1000FD : LINK_CAPA_1000HD; + case SPEED_2500: return LINK_CAPA_2500FD; + case SPEED_5000: return LINK_CAPA_5000FD; + case SPEED_10000: return LINK_CAPA_10000FD; + case SPEED_20000: return LINK_CAPA_20000FD; + case SPEED_25000: return LINK_CAPA_25000FD; + case SPEED_40000: return LINK_CAPA_40000FD; + case SPEED_50000: return LINK_CAPA_50000FD; + case SPEED_56000: return LINK_CAPA_56000FD; + case SPEED_100000: return LINK_CAPA_100000FD; + case SPEED_200000: return LINK_CAPA_200000FD; + case SPEED_400000: return LINK_CAPA_400000FD; + case SPEED_800000: return LINK_CAPA_800000FD; + } + + return -EINVAL; +} + +/** + * phy_caps_init() - Initializes the link_caps array from the link_mode_params. + */ +void phy_caps_init(void) +{ + const struct link_mode_info *linkmode; + int i, capa; + + /* Fill the caps array from net/ethtool/common.c */ + for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) { + linkmode = &link_mode_params[i]; + capa = speed_duplex_to_capa(linkmode->speed, linkmode->duplex); + + if (capa < 0) + continue; + + __set_bit(i, link_caps[capa].linkmodes); + } +} diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index a38d399f244b..9c573555ac49 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -42,6 +42,7 @@ #include <linux/unistd.h> #include "phylib-internal.h" +#include "phy-caps.h" MODULE_DESCRIPTION("PHY library"); MODULE_AUTHOR("Andy Fleming"); @@ -3795,6 +3796,7 @@ static int __init phy_init(void) if (rc) goto err_ethtool_phy_ops; + phy_caps_init(); features_init(); rc = phy_driver_register(&genphy_c45_driver, THIS_MODULE);
The canonical definition for all the link modes is in linux/ethtool.h, which is complemented by the link_mode_params array stored in net/ethtool/common.h . That array contains all the metadata about each of these modes, including the Speed and Duplex information. Phylib and phylink needs that information as well for internal management of the link, which was done by duplicating that information in locally-stored arrays and lookup functions. This makes it easy for developpers adding new modes to forget modifying phylib and phylink accordingly. However, the link_mode_params array in net/ethtool/common.c is fairly inefficient to search through, as it isn't sorted in any manner. Phylib and phylink perform a lot of lookup operations, mostly to filter modes by speed and/or duplex. We therefore introduce the link_caps private array in phy_caps.c, that indexes linkmodes in a more efficient manner. Each element associated a tuple <speed, duplex> to a bitfield of all the linkmodes runs at these speed/duplex. We end-up with an array that's fairly short, easily addressable and that it optimised for the typical use-cases of phylib/phylink. That array is initialized at the same time as phylib. As the link_mode_params array is part of the net stack, which phylink depends on, it should always be accessible from phylib. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/net/phy/Makefile | 2 +- drivers/net/phy/phy-caps.h | 44 ++++++++++++++++++++ drivers/net/phy/phy_caps.c | 78 ++++++++++++++++++++++++++++++++++++ drivers/net/phy/phy_device.c | 2 + 4 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 drivers/net/phy/phy-caps.h create mode 100644 drivers/net/phy/phy_caps.c