[PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Paul Greenwalt]

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(-)

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);
-- 
2.39.2

Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Simon Horman]

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.

Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Greenwalt, Paul]

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.

Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Andrew Lunn]

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

Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Greenwalt, Paul]

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.

Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Pawel Chmielewski]

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.

Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Jacob Keller]

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....

Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Andrew Lunn]

> 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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Alexander Lobakin]

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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Andrew Lunn]

> 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			       

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Alexander Lobakin]

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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Pawel Chmielewski]

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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Andrew Lunn]

> 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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Pawel Chmielewski]

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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Pawel Chmielewski]

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/

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Alexander Lobakin]

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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Andrew Lunn]

> 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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Andrew Lunn]

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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Alexander Lobakin]

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

Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[kernel test robot]

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?

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki