| Message ID | 20230819093941.15163-1-paul.greenwalt@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: Add basic E830 support | expand |
On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote: > The need to map Ethtool forced speeds to Ethtool supported link modes is > common among drivers. To support this move the supported link modes maps > implementation from the qede driver. This is an efficient solution > introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes > maps on module init") for qede driver. > > ethtool_forced_speed_maps_init() should be called during driver init > with an array of struct ethtool_forced_speed_map to populate the > mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized > the struct ethtool_forced_speed_map. > > The qede driver was compile tested only. > > Suggested-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> > --- > v2: move qede Ethtool speed to link modes mapping to be shared by other > drivers (Andrew) Hi Paul, thanks for your efforts in adding a mechanism to share code. It's a great step in the right direction. Perhaps I am on the wrong track here, but it seems to me that the approach you have taken, which is to move the definitions of the symbols into a header file, is, perhaps, not the best. For one thing it will end up with duplicate definitions of the symbols - one for each object in which they are included. For another, and this more a symtom than an actual problem, a (W=1) build now complains about symbols that are defined but not used. ./include/linux/ethtool.h:1190:18: warning: 'ethtool_forced_speed_800000' defined but not used [-Wunused-const-variable=] 1190 | static const u32 ethtool_forced_speed_800000[] __initconst = { ... I suspect a better approach is to leave the symbol definitions in a .c file, one that is linked in such a way that it is available to code that uses it - be it modules or built-in. And to make declarations of those symbols available via a header file.
On 8/20/2023 7:45 AM, Simon Horman wrote: > On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote: >> The need to map Ethtool forced speeds to Ethtool supported link modes is >> common among drivers. To support this move the supported link modes maps >> implementation from the qede driver. This is an efficient solution >> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes >> maps on module init") for qede driver. >> >> ethtool_forced_speed_maps_init() should be called during driver init >> with an array of struct ethtool_forced_speed_map to populate the >> mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized >> the struct ethtool_forced_speed_map. >> >> The qede driver was compile tested only. >> >> Suggested-by: Andrew Lunn <andrew@lunn.ch> >> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> >> --- >> v2: move qede Ethtool speed to link modes mapping to be shared by other >> drivers (Andrew) > > Hi Paul, > > thanks for your efforts in adding a mechanism to share code. > It's a great step in the right direction. > > Perhaps I am on the wrong track here, but it seems to me that the approach > you have taken, which is to move the definitions of the symbols into a > header file, is, perhaps, not the best. For one thing it will end up with > duplicate definitions of the symbols - one for each object in which they > are included. > > For another, and this more a symtom than an actual problem, > a (W=1) build now complains about symbols that are defined but not used. > > ./include/linux/ethtool.h:1190:18: warning: 'ethtool_forced_speed_800000' defined but not used [-Wunused-const-variable=] 1190 | static const u32 ethtool_forced_speed_800000[] __initconst = { > ... > > I suspect a better approach is to leave the symbol definitions in > a .c file, one that is linked in such a way that it is available > to code that uses it - be it modules or built-in. And to make > declarations of those symbols available via a header file. Simon, thanks for for your suggestion and I apologize the warning wasn't caught.
On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote: > The need to map Ethtool forced speeds to Ethtool supported link modes is > common among drivers. To support this move the supported link modes maps > implementation from the qede driver. This is an efficient solution > introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes > maps on module init") for qede driver. > > ethtool_forced_speed_maps_init() should be called during driver init > with an array of struct ethtool_forced_speed_map to populate the > mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized > the struct ethtool_forced_speed_map. Is there any way to reuse this table: https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L161 Seems silly to have multiple tables if this one can be made to work. It is also used a lot more than anything you will add, which has just two users so far, so problems with it a likely to be noticed faster. Andrew
On 8/20/2023 11:54 AM, Andrew Lunn wrote: > On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote: >> The need to map Ethtool forced speeds to Ethtool supported link modes is >> common among drivers. To support this move the supported link modes maps >> implementation from the qede driver. This is an efficient solution >> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes >> maps on module init") for qede driver. >> >> ethtool_forced_speed_maps_init() should be called during driver init >> with an array of struct ethtool_forced_speed_map to populate the >> mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized >> the struct ethtool_forced_speed_map. > > Is there any way to reuse this table: > > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L161 > > Seems silly to have multiple tables if this one can be made to work. > It is also used a lot more than anything you will add, which has just > two users so far, so problems with it a likely to be noticed faster. > > Andrew Yes, we'll can look into that.
From: Paul Greenwalt <paul.greenwalt@intel.com> Date: Sat, 19 Aug 2023 02:39:41 -0700 > The need to map Ethtool forced speeds to Ethtool supported link modes is > common among drivers. To support this move the supported link modes maps > implementation from the qede driver. This is an efficient solution > introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes > maps on module init") for qede driver. [...] > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 62b61527bcc4..245fd4a8d85b 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -1052,4 +1052,78 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add, > * next string. > */ > extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...); > + > +/* Link mode to forced speed capabilities maps */ > +struct ethtool_forced_speed_map { > + u32 speed; > + __ETHTOOL_DECLARE_LINK_MODE_MASK(caps); > + > + const u32 *cap_arr; > + u32 arr_size; Please re-layout this as follows: 1. caps; 2. cap_arr; 3. arr_size; 4. speed. Or in any other way that wouldn't provoke holes on 64-bit systems. I wasn't really familiar with that when initially adding these definitions :D > +}; > + > +#define ETHTOOL_FORCED_SPEED_MAP(value) \ > +{ \ > + .speed = SPEED_##value, \ > + .cap_arr = ethtool_forced_speed_##value, \ > + .arr_size = ARRAY_SIZE(ethtool_forced_speed_##value), \ > +} > + > +static const u32 ethtool_forced_speed_1000[] __initconst = { 2 reasons I don't like this: 1. static const in a header file. This implies code duplication -- every object file will have its own copy. If we want to reuse them, they should be declared global in one place (somewhere in net/ethtool), without __initconst unfortunately. 2. __initconst in a header file. I can refer to that declaration somewhere not in the initialization code and catch modpost or section reference issues. If we want to make those global and accessible from the drivers, it should not have any non-standard section placement. > + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, > + ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, > + ETHTOOL_LINK_MODE_1000baseX_Full_BIT, BTW, can't we share the target bitmaps instead? I mean, why not initialize those somewhere in net/ethtool and export them, instead of exporting the source of initialization? > +}; > + > +static const u32 ethtool_forced_speed_10000[] __initconst = { > + ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseR_FEC_BIT, > + ETHTOOL_LINK_MODE_10000baseCR_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT, > +}; > + > +static const u32 ethtool_forced_speed_20000[] __initconst = { > + ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT, > +}; > + > +static const u32 ethtool_forced_speed_25000[] __initconst = { > + ETHTOOL_LINK_MODE_25000baseKR_Full_BIT, > + ETHTOOL_LINK_MODE_25000baseCR_Full_BIT, > + ETHTOOL_LINK_MODE_25000baseSR_Full_BIT, > +}; > + > +static const u32 ethtool_forced_speed_40000[] __initconst = { > + ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT, > + ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT, > + ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT, > + ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT, > +}; > + > +static const u32 ethtool_forced_speed_50000[] __initconst = { > + ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT, > + ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT, > + ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT, > +}; > + > +static const u32 ethtool_forced_speed_100000[] __initconst = { > + ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT, > + ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT, > + ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT, > + ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT, > +}; > + > +/** > + * ethtool_forced_speed_maps_init > + * @maps: Pointer to an array of Ethtool forced speed map > + * @size: Array size > + * > + * Initialize an array of Ethtool forced speed map to Ethtool link modes. This > + * should be called during driver module init. > + */ > +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, > + u32 size); > #endif /* _LINUX_ETHTOOL_H */ > diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c > index 0b0ce4f81c01..ac1fdd636bc1 100644 > --- a/net/ethtool/ioctl.c > +++ b/net/ethtool/ioctl.c > @@ -3388,3 +3388,18 @@ void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *flow) > kfree(flow); > } > EXPORT_SYMBOL(ethtool_rx_flow_rule_destroy); > + > +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, > + u32 size) Alternatively, to avoid passing size here, you can just terminate @maps array with zeroed element and treat it as the array end. > +{ > + u32 i; > + > + for (i = 0; i < size; i++) { for (u32 i = 0 ... > + struct ethtool_forced_speed_map *map = &maps[i]; > + > + linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps); > + map->cap_arr = NULL; > + map->arr_size = 0; These two are needed only if your @map really refer to __initdata/__initconst variables. > + } > +} > +EXPORT_SYMBOL(ethtool_forced_speed_maps_init); Not sure ioctl.c in the best place for that. Thanks, Olek
On Sun, Aug 20, 2023 at 12:20:43PM -0700, Greenwalt, Paul wrote: > > > On 8/20/2023 11:54 AM, Andrew Lunn wrote: > > On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote: > >> The need to map Ethtool forced speeds to Ethtool supported link modes is > >> common among drivers. To support this move the supported link modes maps > >> implementation from the qede driver. This is an efficient solution > >> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes > >> maps on module init") for qede driver. > >> > >> ethtool_forced_speed_maps_init() should be called during driver init > >> with an array of struct ethtool_forced_speed_map to populate the > >> mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized > >> the struct ethtool_forced_speed_map. > > > > Is there any way to reuse this table: > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L161 > > > > Seems silly to have multiple tables if this one can be made to work. > > It is also used a lot more than anything you will add, which has just > > two users so far, so problems with it a likely to be noticed faster. > > > > Andrew > > Yes, we'll can look into that. I think it would be better to leave the maps in the code of respective drivers, as they are too much hardware related. Even for a single speed, the sets of supported link modes may vary between the devices.
On 8/23/2023 10:56 AM, Pawel Chmielewski wrote: > On Sun, Aug 20, 2023 at 12:20:43PM -0700, Greenwalt, Paul wrote: >> >> >> On 8/20/2023 11:54 AM, Andrew Lunn wrote: >>> On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote: >>>> The need to map Ethtool forced speeds to Ethtool supported link modes is >>>> common among drivers. To support this move the supported link modes maps >>>> implementation from the qede driver. This is an efficient solution >>>> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes >>>> maps on module init") for qede driver. >>>> >>>> ethtool_forced_speed_maps_init() should be called during driver init >>>> with an array of struct ethtool_forced_speed_map to populate the >>>> mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized >>>> the struct ethtool_forced_speed_map. >>> >>> Is there any way to reuse this table: >>> >>> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L161 >>> >>> Seems silly to have multiple tables if this one can be made to work. >>> It is also used a lot more than anything you will add, which has just >>> two users so far, so problems with it a likely to be noticed faster. >>> >>> Andrew >> >> Yes, we'll can look into that. > > I think it would be better to leave the maps in the code of respective drivers, as they are too much hardware related. > Even for a single speed, the sets of supported link modes may vary between the devices. > Isn't that what the per-driver bitwise AND is doing after the fact? That's how qede worked: it converted the mapping from speed and then combined it with some device support maps to avoid allowing speeds which weren't supported.. it would be nice to reuse the same mapping that is common everywhere. I suspect the PHY code already has some mechanism to support device specific since not all PHYs support all link modes either....
> it would be nice to reuse the same mapping that is common everywhere. I > suspect the PHY code already has some mechanism to support device > specific since not all PHYs support all link modes either.... phydev->supported is initialised with all the link modes a PHY supports. The MAC driver when gets a chance to remove any it does not support, phylink has similar concepts, the MAC, PHY, and SFP declare what they each support and then it all gets combined to resolve to a link mode, to use. So it should be possible to make this generic with the help of the MAC driver indicating a mask of what is actually supports. Andrew
Hi Paul,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
[also build test WARNING on next-20230824]
[cannot apply to tnguy-next-queue/dev-queue tnguy-net-queue/dev-queue net/main linus/master horms-ipvs/master v6.5-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Paul-Greenwalt/ice-Add-E830-device-IDs-MAC-type-and-registers/20230821-095200
base: net-next/main
patch link: https://lore.kernel.org/r/20230819093941.15163-1-paul.greenwalt%40intel.com
patch subject: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps
config: i386-randconfig-063-20230824 (https://download.01.org/0day-ci/archive/20230824/202308241737.GzJ3EfdT-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230824/202308241737.GzJ3EfdT-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308241737.GzJ3EfdT-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/qlogic/qede/qede_ethtool.c:204:33: sparse: sparse: symbol 'qede_forced_speed_maps' was not declared. Should it be static?
From: Pawel Chmielewski <pawel.chmielewski@intel.com> Date: Wed, 23 Aug 2023 19:56:24 +0200 > On Sun, Aug 20, 2023 at 12:20:43PM -0700, Greenwalt, Paul wrote: >> >> >> On 8/20/2023 11:54 AM, Andrew Lunn wrote: >>> On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote: >>>> The need to map Ethtool forced speeds to Ethtool supported link modes is >>>> common among drivers. To support this move the supported link modes maps >>>> implementation from the qede driver. This is an efficient solution >>>> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes >>>> maps on module init") for qede driver. >>>> >>>> ethtool_forced_speed_maps_init() should be called during driver init >>>> with an array of struct ethtool_forced_speed_map to populate the >>>> mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized >>>> the struct ethtool_forced_speed_map. >>> >>> Is there any way to reuse this table: >>> >>> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L161 >>> >>> Seems silly to have multiple tables if this one can be made to work. >>> It is also used a lot more than anything you will add, which has just >>> two users so far, so problems with it a likely to be noticed faster. >>> >>> Andrew >> >> Yes, we'll can look into that. BTW, drivers/net/ethernet/qlogic/qed/qed_main.c drivers/net/pcs/pcs-xpcs.c also have similar stuff and could probably make use of the generic stuff you're doing as well (qed_main was also done by me). Not speaking of drivers/net/phy/phylink.c We probably should unify all that... Let me think how we could do that. Andrew's idea is good. But most high-speed NICs, which have a standalone management firmware for PHY, don't use phylib/phylink. So in order to be able to unify all that, they should have ->supported bitmap somewhere else. Not sure struct net_device is the best place... If I recall Phylink logics correctly (it's been a while since I last time was working with my embedded project), 1) in the NIC (MAC) driver, you initialize ->supported with *speeds* and stuff like duplex, no link modes; 2) Phylink core sets the corresponding link mode bits; 3) phylib core then clears the bits unsupported by the PHY IIRC The third step in case with those NICs with FW-managed PHYs should be done manually in the MAC driver somewhere. Like "I am qede and I don't support mode XX at 50Gbps, but support the rest, so I clear that one bit". Then the networking core should be able to play this association game itself. That would remove a good amount of boilerplating. > > I think it would be better to leave the maps in the code of respective drivers, as they are too much hardware related. > Even for a single speed, the sets of supported link modes may vary between the devices. > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan Thanks, Olek
> Let me think how we could do that. > Andrew's idea is good. But most high-speed NICs, which have a standalone > management firmware for PHY, don't use phylib/phylink. > So in order to be able to unify all that, they should have ->supported > bitmap somewhere else. Not sure struct net_device is the best place... I would probably keep it in the driver priv structure, and just pass it as needed. So long as you only need one or two values, i don't see the need for a shared structure. > If I recall Phylink logics correctly (it's been a while since I last > time was working with my embedded project), > > 1) in the NIC (MAC) driver, you initialize ->supported with *speeds* and > stuff like duplex, no link modes; > 2) Phylink core sets the corresponding link mode bits; > 3) phylib core then clears the bits unsupported by the PHY IIRC No, not really. All i think you need is a low level helper. So don't worry too much about how phylink works, just implement that low level helper passing in values as needed, not phylib or phylink structure. What i don't want is a second infrastructure to be built for those MAC drivers which don't use Linux to control the PHY. Either share a few helpers, or swap to phylink. > The third step in case with those NICs with FW-managed PHYs should be > done manually in the MAC driver somewhere. Like "I am qede and I don't > support mode XX at 50Gbps, but support the rest, so I clear that one bit". I don't think that will work. New bits keep getting added, more speeds added. So 'support the rest' is not well defined. You need an explicit list of link modes the driver needs. We already have code to convert an array of link mode bits into an actual mask, e.g: linkmode_set_bit_array(phy_basic_t1_features_array, ARRAY_SIZE(phy_basic_t1_features_array), phy_basic_t1_features); Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: Fri, 25 Aug 2023 15:47:20 +0200 >> Let me think how we could do that. >> Andrew's idea is good. But most high-speed NICs, which have a standalone >> management firmware for PHY, don't use phylib/phylink. >> So in order to be able to unify all that, they should have ->supported >> bitmap somewhere else. Not sure struct net_device is the best place... > > I would probably keep it in the driver priv structure, and just pass > it as needed. So long as you only need one or two values, i don't see > the need for a shared structure. > >> If I recall Phylink logics correctly (it's been a while since I last >> time was working with my embedded project), >> >> 1) in the NIC (MAC) driver, you initialize ->supported with *speeds* and >> stuff like duplex, no link modes; >> 2) Phylink core sets the corresponding link mode bits; >> 3) phylib core then clears the bits unsupported by the PHY IIRC > > No, not really. > > All i think you need is a low level helper. So don't worry too much > about how phylink works, just implement that low level helper passing > in values as needed, not phylib or phylink structure. > > What i don't want is a second infrastructure to be built for those MAC > drivers which don't use Linux to control the PHY. Either share a few > helpers, or swap to phylink. I'd love those drivers to be swapped to phylink, but I doubt that will happen :D > >> The third step in case with those NICs with FW-managed PHYs should be >> done manually in the MAC driver somewhere. Like "I am qede and I don't >> support mode XX at 50Gbps, but support the rest, so I clear that one bit". > > I don't think that will work. New bits keep getting added, more speeds > added. So 'support the rest' is not well defined. You need an explicit Ah, correct. > list of link modes the driver needs. We already have code to convert > an array of link mode bits into an actual mask, e.g: > > linkmode_set_bit_array(phy_basic_t1_features_array, > ARRAY_SIZE(phy_basic_t1_features_array), > phy_basic_t1_features); Now I got lost a bit in what we do really want to share now, as less sharing was indirectly rejected by "you can share more, let PHY/whatever take care of this" and now wider sharing was indirectly rejected by "that won't work" :D From what I understood, all we want now is the stuff introduced by the original patch from that thread, but without "generic" speed arrays definitions? > > Andrew Thanks, Olek
On Fri, Aug 25, 2023 at 03:47:20PM +0200, Andrew Lunn wrote: > > Let me think how we could do that. > > Andrew's idea is good. But most high-speed NICs, which have a standalone > > management firmware for PHY, don't use phylib/phylink. > > So in order to be able to unify all that, they should have ->supported > > bitmap somewhere else. Not sure struct net_device is the best place... > > I would probably keep it in the driver priv structure, and just pass > it as needed. So long as you only need one or two values, i don't see > the need for a shared structure. > > > If I recall Phylink logics correctly (it's been a while since I last > > time was working with my embedded project), > > > > 1) in the NIC (MAC) driver, you initialize ->supported with *speeds* and > > stuff like duplex, no link modes; > > 2) Phylink core sets the corresponding link mode bits; > > 3) phylib core then clears the bits unsupported by the PHY IIRC > > No, not really. > > All i think you need is a low level helper. So don't worry too much > about how phylink works, just implement that low level helper passing > in values as needed, not phylib or phylink structure. > > What i don't want is a second infrastructure to be built for those MAC > drivers which don't use Linux to control the PHY. Either share a few > helpers, or swap to phylink. > Let me check if I understand correctly- is that what was sent with the v3 [1] , with the initialization helper (ethtool_forced_speed_maps_init) and the structure map in the ethtool code? Or do you have another helper in mind? [1] https://lore.kernel.org/netdev/20230823180633.2450617-5-pawel.chmielewski@intel.com/T/#m208153896dfd623da278427285d3bda25a74ef95
> Let me check if I understand correctly- is that what was sent with the > v3 [1] , with the initialization helper (ethtool_forced_speed_maps_init) > and the structure map in the ethtool code? Or do you have another helper > in mind? Sorry for the late reply, been on vacation. The main thing is you try to reuse the table: static const struct phy_setting settings[] = {} If you can build your helper on top of phy_lookup_setting() even better. You don't need a phy_device to use those. Andrew
On Sun, Sep 03, 2023 at 04:00:57PM +0200, Andrew Lunn wrote: > > Let me check if I understand correctly- is that what was sent with the > > v3 [1] , with the initialization helper (ethtool_forced_speed_maps_init) > > and the structure map in the ethtool code? Or do you have another helper > > in mind? > > Sorry for the late reply, been on vacation. > > The main thing is you try to reuse the table: > > static const struct phy_setting settings[] = {} > > If you can build your helper on top of phy_lookup_setting() even > better. You don't need a phy_device to use those. > > Andrew Thank you for the clarification, I'll write it and propose in the next version. Most probably split this refactoring from the original series. Pawel
On Sun, Sep 03, 2023 at 04:00:57PM +0200, Andrew Lunn wrote: > > Let me check if I understand correctly- is that what was sent with the > > v3 [1] , with the initialization helper (ethtool_forced_speed_maps_init) > > and the structure map in the ethtool code? Or do you have another helper > > in mind? > > Sorry for the late reply, been on vacation. > > The main thing is you try to reuse the table: > > static const struct phy_setting settings[] = {} > > If you can build your helper on top of phy_lookup_setting() even > better. You don't need a phy_device to use those. > > Andrew Thank for the hint Andrew! I took a look into the phy-core code, and a little into phylink. However, I still have the same concern regarding modes that are supported/unsupported by hardware (managed by the firmware in our case). Let's say I'm only looking for duplex modes and iterate over speeds with advertised modes map as an argument for phy_lookup_setting. In this case, I still need another table/map of hardware compatible link modes to check against. Theese are actually the maps we'd like to keep in the driver (and proposed in [1]), so maybe the simple intersect check between them and the advertised modes is sufficient? [1] https://lore.kernel.org/netdev/20230823180633.2450617-4-pawel.chmielewski@intel.com/
From: Pawel Chmielewski <pawel.chmielewski@intel.com> Date: Thu, 14 Sep 2023 16:27:28 +0200 > On Sun, Sep 03, 2023 at 04:00:57PM +0200, Andrew Lunn wrote: >>> Let me check if I understand correctly- is that what was sent with the >>> v3 [1] , with the initialization helper (ethtool_forced_speed_maps_init) >>> and the structure map in the ethtool code? Or do you have another helper >>> in mind? >> >> Sorry for the late reply, been on vacation. >> >> The main thing is you try to reuse the table: >> >> static const struct phy_setting settings[] = {} >> >> If you can build your helper on top of phy_lookup_setting() even >> better. You don't need a phy_device to use those. >> >> Andrew > > Thank for the hint Andrew! I took a look into the phy-core code, > and a little into phylink. However, I still have the same concern Here I'd like to add that we're planning to try to use Phylink in ice soon. It may take a while and will most likely require core code expansion, since Phylink was originally developed for embedded HW and DeviceTree and doesn't fully support PCI devices. Let's see how it goes. > regarding modes that are supported/unsupported by hardware (managed > by the firmware in our case). Let's say I'm only looking for duplex > modes and iterate over speeds with advertised modes map as an argument > for phy_lookup_setting. In this case, I still need another table/map of > hardware compatible link modes to check against. Theese are actually > the maps we'd like to keep in the driver (and proposed in [1]), so > maybe the simple intersect check between them and the advertised modes > is sufficient? > > [1] https://lore.kernel.org/netdev/20230823180633.2450617-4-pawel.chmielewski@intel.com/ Thanks, Olek
On Thu, Sep 14, 2023 at 04:27:28PM +0200, Pawel Chmielewski wrote: > On Sun, Sep 03, 2023 at 04:00:57PM +0200, Andrew Lunn wrote: > > > Let me check if I understand correctly- is that what was sent with the > > > v3 [1] , with the initialization helper (ethtool_forced_speed_maps_init) > > > and the structure map in the ethtool code? Or do you have another helper > > > in mind? > > > > Sorry for the late reply, been on vacation. > > > > The main thing is you try to reuse the table: > > > > static const struct phy_setting settings[] = {} > > > > If you can build your helper on top of phy_lookup_setting() even > > better. You don't need a phy_device to use those. > > > > Andrew > > Thank for the hint Andrew! I took a look into the phy-core code, > and a little into phylink. However, I still have the same concern > regarding modes that are supported/unsupported by hardware (managed > by the firmware in our case). Let's say I'm only looking for duplex > modes and iterate over speeds with advertised modes map as an argument > for phy_lookup_setting. In this case, I still need another table/map of > hardware compatible link modes to check against. Theese are actually > the maps we'd like to keep in the driver (and proposed in [1]), so > maybe the simple intersect check between them and the advertised modes > is sufficient? The idea was you have a mask of link modes which your hardware actually supports. You then ask the core code, give me a link mode which fulfils this speed and duplex, taking into account the mask. Andrew
> Here I'd like to add that we're planning to try to use Phylink in ice > soon. It may take a while and will most likely require core code > expansion, since Phylink was originally developed for embedded HW and > DeviceTree and doesn't fully support PCI devices. Cool. And yes, not having device tree will require some changes, but i don't think it will require changes to the core of phylink, just the edges where you instantiate phylink, using a different configuration mechanism. Andrew
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c index 95820cf1cd6c..a01acd48f7eb 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c @@ -201,90 +201,20 @@ static const char qede_tests_str_arr[QEDE_ETHTOOL_TEST_MAX][ETH_GSTRING_LEN] = { /* Forced speed capabilities maps */ -struct qede_forced_speed_map { - u32 speed; - __ETHTOOL_DECLARE_LINK_MODE_MASK(caps); - - const u32 *cap_arr; - u32 arr_size; -}; - -#define QEDE_FORCED_SPEED_MAP(value) \ -{ \ - .speed = SPEED_##value, \ - .cap_arr = qede_forced_speed_##value, \ - .arr_size = ARRAY_SIZE(qede_forced_speed_##value), \ -} - -static const u32 qede_forced_speed_1000[] __initconst = { - ETHTOOL_LINK_MODE_1000baseT_Full_BIT, - ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, - ETHTOOL_LINK_MODE_1000baseX_Full_BIT, -}; - -static const u32 qede_forced_speed_10000[] __initconst = { - ETHTOOL_LINK_MODE_10000baseT_Full_BIT, - ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, - ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, - ETHTOOL_LINK_MODE_10000baseR_FEC_BIT, - ETHTOOL_LINK_MODE_10000baseCR_Full_BIT, - ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, - ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, - ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT, -}; - -static const u32 qede_forced_speed_20000[] __initconst = { - ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT, -}; - -static const u32 qede_forced_speed_25000[] __initconst = { - ETHTOOL_LINK_MODE_25000baseKR_Full_BIT, - ETHTOOL_LINK_MODE_25000baseCR_Full_BIT, - ETHTOOL_LINK_MODE_25000baseSR_Full_BIT, -}; - -static const u32 qede_forced_speed_40000[] __initconst = { - ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT, - ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT, - ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT, - ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT, -}; - -static const u32 qede_forced_speed_50000[] __initconst = { - ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT, - ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT, - ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT, -}; - -static const u32 qede_forced_speed_100000[] __initconst = { - ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT, - ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT, - ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT, - ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT, -}; - -static struct qede_forced_speed_map qede_forced_speed_maps[] __ro_after_init = { - QEDE_FORCED_SPEED_MAP(1000), - QEDE_FORCED_SPEED_MAP(10000), - QEDE_FORCED_SPEED_MAP(20000), - QEDE_FORCED_SPEED_MAP(25000), - QEDE_FORCED_SPEED_MAP(40000), - QEDE_FORCED_SPEED_MAP(50000), - QEDE_FORCED_SPEED_MAP(100000), +struct ethtool_forced_speed_map qede_forced_speed_maps[] __ro_after_init = { + ETHTOOL_FORCED_SPEED_MAP(1000), + ETHTOOL_FORCED_SPEED_MAP(10000), + ETHTOOL_FORCED_SPEED_MAP(20000), + ETHTOOL_FORCED_SPEED_MAP(25000), + ETHTOOL_FORCED_SPEED_MAP(40000), + ETHTOOL_FORCED_SPEED_MAP(50000), + ETHTOOL_FORCED_SPEED_MAP(100000), }; void __init qede_forced_speed_maps_init(void) { - struct qede_forced_speed_map *map; - u32 i; - - for (i = 0; i < ARRAY_SIZE(qede_forced_speed_maps); i++) { - map = qede_forced_speed_maps + i; - - linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps); - map->cap_arr = NULL; - map->arr_size = 0; - } + ethtool_forced_speed_maps_init(qede_forced_speed_maps, + ARRAY_SIZE(qede_forced_speed_maps)); } /* Ethtool callbacks */ @@ -564,8 +494,8 @@ static int qede_set_link_ksettings(struct net_device *dev, const struct ethtool_link_ksettings *cmd) { const struct ethtool_link_settings *base = &cmd->base; + const struct ethtool_forced_speed_map *map; struct qede_dev *edev = netdev_priv(dev); - const struct qede_forced_speed_map *map; struct qed_link_output current_link; struct qed_link_params params; u32 i; diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 62b61527bcc4..245fd4a8d85b 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -1052,4 +1052,78 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add, * next string. */ extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...); + +/* Link mode to forced speed capabilities maps */ +struct ethtool_forced_speed_map { + u32 speed; + __ETHTOOL_DECLARE_LINK_MODE_MASK(caps); + + const u32 *cap_arr; + u32 arr_size; +}; + +#define ETHTOOL_FORCED_SPEED_MAP(value) \ +{ \ + .speed = SPEED_##value, \ + .cap_arr = ethtool_forced_speed_##value, \ + .arr_size = ARRAY_SIZE(ethtool_forced_speed_##value), \ +} + +static const u32 ethtool_forced_speed_1000[] __initconst = { + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, + ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, + ETHTOOL_LINK_MODE_1000baseX_Full_BIT, +}; + +static const u32 ethtool_forced_speed_10000[] __initconst = { + ETHTOOL_LINK_MODE_10000baseT_Full_BIT, + ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, + ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, + ETHTOOL_LINK_MODE_10000baseR_FEC_BIT, + ETHTOOL_LINK_MODE_10000baseCR_Full_BIT, + ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, + ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, + ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT, +}; + +static const u32 ethtool_forced_speed_20000[] __initconst = { + ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT, +}; + +static const u32 ethtool_forced_speed_25000[] __initconst = { + ETHTOOL_LINK_MODE_25000baseKR_Full_BIT, + ETHTOOL_LINK_MODE_25000baseCR_Full_BIT, + ETHTOOL_LINK_MODE_25000baseSR_Full_BIT, +}; + +static const u32 ethtool_forced_speed_40000[] __initconst = { + ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT, + ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT, + ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT, + ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT, +}; + +static const u32 ethtool_forced_speed_50000[] __initconst = { + ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT, + ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT, + ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT, +}; + +static const u32 ethtool_forced_speed_100000[] __initconst = { + ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT, + ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT, + ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT, + ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT, +}; + +/** + * ethtool_forced_speed_maps_init + * @maps: Pointer to an array of Ethtool forced speed map + * @size: Array size + * + * Initialize an array of Ethtool forced speed map to Ethtool link modes. This + * should be called during driver module init. + */ +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, + u32 size); #endif /* _LINUX_ETHTOOL_H */ diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 0b0ce4f81c01..ac1fdd636bc1 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -3388,3 +3388,18 @@ void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *flow) kfree(flow); } EXPORT_SYMBOL(ethtool_rx_flow_rule_destroy); + +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, + u32 size) +{ + u32 i; + + for (i = 0; i < size; i++) { + struct ethtool_forced_speed_map *map = &maps[i]; + + linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps); + map->cap_arr = NULL; + map->arr_size = 0; + } +} +EXPORT_SYMBOL(ethtool_forced_speed_maps_init);
The need to map Ethtool forced speeds to Ethtool supported link modes is common among drivers. To support this move the supported link modes maps implementation from the qede driver. This is an efficient solution introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes maps on module init") for qede driver. ethtool_forced_speed_maps_init() should be called during driver init with an array of struct ethtool_forced_speed_map to populate the mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized the struct ethtool_forced_speed_map. The qede driver was compile tested only. Suggested-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> --- v2: move qede Ethtool speed to link modes mapping to be shared by other drivers (Andrew) --- .../net/ethernet/qlogic/qede/qede_ethtool.c | 92 +++---------------- include/linux/ethtool.h | 74 +++++++++++++++ net/ethtool/ioctl.c | 15 +++ 3 files changed, 100 insertions(+), 81 deletions(-)