[net-next PATCH 0/2] net: add define to describe link speed modes

[net-next PATCH 0/2] net: add define to describe link speed modes

[Christian Marangi]

This is a simple series to add define to describe link speed modes.

Hope the proposed way is acceptable with the enum and define.

This is also needed in the upcoming changes in the netdev trigger for LEDs
where phy_speeds functions is used to declare a more compact array instead
of using a "big enough" approach.

Christian Marangi (2):
  net: ethtool: add define for link speed mode number
  net: phy: leds: use new define for link speed modes number

 drivers/net/phy/phy_led_triggers.c |  2 +-
 include/uapi/linux/ethtool.h       | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.40.1

[net-next PATCH 1/2] net: ethtool: add define for link speed mode number

[Christian Marangi]

Add define to reference the number of link speed mode defined in the
system.

This can be handy for generic parsing of the different link speed mode.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/uapi/linux/ethtool.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f7fba0dc87e5..59f394a663ab 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1884,6 +1884,28 @@ enum ethtool_link_mode_bit_indices {
  * Update drivers/net/phy/phy.c:phy_speed_to_str() and
  * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.
  */
+enum ethtool_link_speeds {
+	SPEED_10 = 0,
+	SPEED_100,
+	SPEED_1000,
+	SPEED_2500,
+	SPEED_5000,
+	SPEED_10000,
+	SPEED_14000,
+	SPEED_20000,
+	SPEED_25000,
+	SPEED_40000,
+	SPEED_50000,
+	SPEED_56000,
+	SPEED_100000,
+	SPEED_200000,
+	SPEED_400000,
+	SPEED_800000,
+
+	/* must be last entry */
+	__LINK_SPEEDS_NUM,
+};
+
 #define SPEED_10		10
 #define SPEED_100		100
 #define SPEED_1000		1000
-- 
2.40.1

Re: [net-next PATCH 1/2] net: ethtool: add define for link speed mode number

[Russell King (Oracle)]

NAK.

You *clearly* didn't look before you leaped.

On Wed, Dec 13, 2023 at 07:15:53PM +0100, Christian Marangi wrote:
> +enum ethtool_link_speeds {
> +	SPEED_10 = 0,
> +	SPEED_100,
> +	SPEED_1000,
...

and from the context immediately below, included in your patch:
>  #define SPEED_10		10
           ^^^^^^^^
>  #define SPEED_100		100
           ^^^^^^^^^
>  #define SPEED_1000		1000
           ^^^^^^^^^^

Your enumerated values will be overridden by the preprocessor
definitions.

Moreover, SPEED_xxx is an already taken namespace and part of the UAPI,
and thus can _not_ be changed. Convention is that SPEED_x will be
defined as the numeric speed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 1/2] net: ethtool: add define for link speed mode number

[Christian Marangi]

On Wed, Dec 13, 2023 at 08:10:42PM +0000, Russell King (Oracle) wrote:
> NAK.
> 
> You *clearly* didn't look before you leaped.
> 
> On Wed, Dec 13, 2023 at 07:15:53PM +0100, Christian Marangi wrote:
> > +enum ethtool_link_speeds {
> > +	SPEED_10 = 0,
> > +	SPEED_100,
> > +	SPEED_1000,
> ...
> 
> and from the context immediately below, included in your patch:
> >  #define SPEED_10		10
>            ^^^^^^^^
> >  #define SPEED_100		100
>            ^^^^^^^^^
> >  #define SPEED_1000		1000
>            ^^^^^^^^^^
> 
> Your enumerated values will be overridden by the preprocessor
> definitions.
> 
> Moreover, SPEED_xxx is an already taken namespace and part of the UAPI,
> and thus can _not_ be changed. Convention is that SPEED_x will be
> defined as the numeric speed.
>

Well yes that is the idea of having the enum to count them and then redefining
them to the correct value. (wasn't trying to introduce new define for
the speed and trying to assign incremental values)

Any idea how to handle this without the enum - redefine thing?

Was trying to find a more automated way than defining the raw number of
the current modes. (but maybe this is not that bad? since on adding more
modes, other values has to be changed so it would be just another value
to document in the comment)

-- 
	Ansuel

Re: [net-next PATCH 1/2] net: ethtool: add define for link speed mode number

[Russell King (Oracle)]

On Wed, Dec 13, 2023 at 09:15:27PM +0100, Christian Marangi wrote:
> On Wed, Dec 13, 2023 at 08:10:42PM +0000, Russell King (Oracle) wrote:
> > NAK.
> > 
> > You *clearly* didn't look before you leaped.
> > 
> > On Wed, Dec 13, 2023 at 07:15:53PM +0100, Christian Marangi wrote:
> > > +enum ethtool_link_speeds {
> > > +	SPEED_10 = 0,
> > > +	SPEED_100,
> > > +	SPEED_1000,
> > ...
> > 
> > and from the context immediately below, included in your patch:
> > >  #define SPEED_10		10
> >            ^^^^^^^^
> > >  #define SPEED_100		100
> >            ^^^^^^^^^
> > >  #define SPEED_1000		1000
> >            ^^^^^^^^^^
> > 
> > Your enumerated values will be overridden by the preprocessor
> > definitions.
> > 
> > Moreover, SPEED_xxx is an already taken namespace and part of the UAPI,
> > and thus can _not_ be changed. Convention is that SPEED_x will be
> > defined as the numeric speed.
> >
> 
> Well yes that is the idea of having the enum to count them and then redefining
> them to the correct value. (wasn't trying to introduce new define for
> the speed and trying to assign incremental values)
> 
> Any idea how to handle this without the enum - redefine thing?
> 
> Was trying to find a more automated way than defining the raw number of
> the current modes. (but maybe this is not that bad? since on adding more
> modes, other values has to be changed so it would be just another value
> to document in the comment)

I think my comment on patch 2 gives some ideas! :D

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 1/2] net: ethtool: add define for link speed mode number

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-ethtool-add-define-for-link-speed-mode-number/20231214-021806
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231213181554.4741-2-ansuelsmth%40gmail.com
patch subject: [net-next PATCH 1/2] net: ethtool: add define for link speed mode number
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20231214/202312141531.bEtUmIwG-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231214/202312141531.bEtUmIwG-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/202312141531.bEtUmIwG-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/net/ethernet/intel/igb/e1000_hw.h:13,
                    from drivers/net/ethernet/intel/igb/e1000_mac.h:7,
                    from drivers/net/ethernet/intel/igb/e1000_82575.c:14:
>> drivers/net/ethernet/intel/igb/e1000_defines.h:256:21: error: expected identifier before numeric constant
     256 | #define SPEED_10    10
         |                     ^~
   include/uapi/linux/ethtool.h:1888:9: note: in expansion of macro 'SPEED_10'
    1888 |         SPEED_10 = 0,
         |         ^~~~~~~~
--
   In file included from drivers/net/ethernet/intel/igc/igc_hw.h:12,
                    from drivers/net/ethernet/intel/igc/igc_base.c:6:
>> drivers/net/ethernet/intel/igc/igc_defines.h:231:33: error: expected identifier before numeric constant
     231 | #define SPEED_10                10
         |                                 ^~
   include/uapi/linux/ethtool.h:1888:9: note: in expansion of macro 'SPEED_10'
    1888 |         SPEED_10 = 0,
         |         ^~~~~~~~


vim +256 drivers/net/ethernet/intel/igb/e1000_defines.h

9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24  255  
9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24 @256  #define SPEED_10    10
9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24  257  #define SPEED_100   100
9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24  258  #define SPEED_1000  1000
ceb5f13b70cd6e drivers/net/ethernet/intel/igb/e1000_defines.h Carolyn Wyborny 2013-04-18  259  #define SPEED_2500  2500
9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24  260  #define HALF_DUPLEX 1
9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24  261  #define FULL_DUPLEX 2
9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24  262  
9d5c824399dea8 drivers/net/igb/e1000_defines.h                Auke Kok        2008-01-24  263  

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

[net-next PATCH 2/2] net: phy: leds: use new define for link speed modes number

[Christian Marangi]

Use new define __LINK_SPEEDS_NUM for the speeds array instead of
declaring a big enough array of 50 elements to handle future link speed
modes.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_led_triggers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_led_triggers.c b/drivers/net/phy/phy_led_triggers.c
index f550576eb9da..40cb0fa9ace0 100644
--- a/drivers/net/phy/phy_led_triggers.c
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -84,7 +84,7 @@ static void phy_led_trigger_unregister(struct phy_led_trigger *plt)
 int phy_led_triggers_register(struct phy_device *phy)
 {
 	int i, err;
-	unsigned int speeds[50];
+	unsigned int speeds[__LINK_SPEEDS_NUM];
 
 	phy->phy_num_led_triggers = phy_supported_speeds(phy, speeds,
 							 ARRAY_SIZE(speeds));
-- 
2.40.1

Re: [net-next PATCH 2/2] net: phy: leds: use new define for link speed modes number

[Russell King (Oracle)]

On Wed, Dec 13, 2023 at 07:15:54PM +0100, Christian Marangi wrote:
> Use new define __LINK_SPEEDS_NUM for the speeds array instead of
> declaring a big enough array of 50 elements to handle future link speed
> modes.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/phy_led_triggers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_led_triggers.c b/drivers/net/phy/phy_led_triggers.c
> index f550576eb9da..40cb0fa9ace0 100644
> --- a/drivers/net/phy/phy_led_triggers.c
> +++ b/drivers/net/phy/phy_led_triggers.c
> @@ -84,7 +84,7 @@ static void phy_led_trigger_unregister(struct phy_led_trigger *plt)
>  int phy_led_triggers_register(struct phy_device *phy)
>  {
>  	int i, err;
> -	unsigned int speeds[50];
> +	unsigned int speeds[__LINK_SPEEDS_NUM];
>  
>  	phy->phy_num_led_triggers = phy_supported_speeds(phy, speeds,
>  							 ARRAY_SIZE(speeds));

Yes, I agree the original code is utterly horrid, and there should be a
definition for its size. However, this is about the only place it would
be used - if you look at the code in phy_supported_speeds() and in
phy_speeds() which it calls, there is nothing in there which would know
the speed.

The only two solution I can think would be either a new function:

size_t phy_num_supported_speeds(struct phy_device *phydev);

or have phy_speeds() return the number of entries if "speeds" was NULL.

Then kmalloc_array() the speed array - but that seems a bit on the
side of things. The code as it stands is safe, because the passed
ARRAY_SIZE() limits the maximum index into speeds[] that will be
written, and it will result in the slower speeds not being added
into the array.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH 2/2] net: phy: leds: use new define for link speed modes number

[Christian Marangi]

On Wed, Dec 13, 2023 at 08:18:38PM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 13, 2023 at 07:15:54PM +0100, Christian Marangi wrote:
> > Use new define __LINK_SPEEDS_NUM for the speeds array instead of
> > declaring a big enough array of 50 elements to handle future link speed
> > modes.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/phy/phy_led_triggers.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/phy_led_triggers.c b/drivers/net/phy/phy_led_triggers.c
> > index f550576eb9da..40cb0fa9ace0 100644
> > --- a/drivers/net/phy/phy_led_triggers.c
> > +++ b/drivers/net/phy/phy_led_triggers.c
> > @@ -84,7 +84,7 @@ static void phy_led_trigger_unregister(struct phy_led_trigger *plt)
> >  int phy_led_triggers_register(struct phy_device *phy)
> >  {
> >  	int i, err;
> > -	unsigned int speeds[50];
> > +	unsigned int speeds[__LINK_SPEEDS_NUM];
> >  
> >  	phy->phy_num_led_triggers = phy_supported_speeds(phy, speeds,
> >  							 ARRAY_SIZE(speeds));
> 
> Yes, I agree the original code is utterly horrid, and there should be a
> definition for its size. However, this is about the only place it would
> be used - if you look at the code in phy_supported_speeds() and in
> phy_speeds() which it calls, there is nothing in there which would know
> the speed.
> 
> The only two solution I can think would be either a new function:
> 
> size_t phy_num_supported_speeds(struct phy_device *phydev);

Maybe this is better to not fill the phy_speeds function with too much
conditions.

> 
> or have phy_speeds() return the number of entries if "speeds" was NULL.
> 
> Then kmalloc_array() the speed array - but that seems a bit on the
> side of things. The code as it stands is safe, because the passed
> ARRAY_SIZE() limits the maximum index into speeds[] that will be
> written, and it will result in the slower speeds not being added
> into the array.
>

The fact that the phy_speed compose an array in descending order seems
wrong to me and not following why it was done like that in the first
place.

Passing for example an array of 3 elements i would expect 10 100 100
speed to be put instead of 800 400 200. (just as an example)

Wonder if it would be sane to do this change. (invert the produced speed
array with ascending order)

A kmalloc_array might be overkill for the task but declaring a random
array of 50 elements is equally bad...

Think I will just implement kmalloc + function to return the count of
speed modes from the settings struct.

-- 
	Ansuel

Re: [net-next PATCH 0/2] net: add define to describe link speed modes

[Andrew Lunn]

On Wed, Dec 13, 2023 at 07:15:52PM +0100, Christian Marangi wrote:
> This is a simple series to add define to describe link speed modes.
> 
> Hope the proposed way is acceptable with the enum and define.
> 
> This is also needed in the upcoming changes in the netdev trigger for LEDs
> where phy_speeds functions is used to declare a more compact array instead
> of using a "big enough" approach.

I'm trying to figure out the 'big picture' here.

The LED trigger will call ksetting_get to get a list of supported link
modes. You can then use the table struct phy_setting settings[] in
phy-core.c to translate the link mode to a speed. You are likely to
get a lot of duplicate speeds, but you can remove them. For each speed
you need to create a sysfs file. Why not just create a linked list,
rather than an array? Or just walk the table and find out how many
different speeds there are and allocate an array of that size. Its
currently 15, which is not that big. And then just use the is_visible
method to hide the ones which are not relevant.

I don't see any need for new enums or tables here, just a function to
look up a link mode in that table and return the speed.

       Andrew

Re: [net-next PATCH 0/2] net: add define to describe link speed modes

[Christian Marangi]

On Thu, Dec 14, 2023 at 12:05:52AM +0100, Andrew Lunn wrote:
> On Wed, Dec 13, 2023 at 07:15:52PM +0100, Christian Marangi wrote:
> > This is a simple series to add define to describe link speed modes.
> > 
> > Hope the proposed way is acceptable with the enum and define.
> > 
> > This is also needed in the upcoming changes in the netdev trigger for LEDs
> > where phy_speeds functions is used to declare a more compact array instead
> > of using a "big enough" approach.
> 
> I'm trying to figure out the 'big picture' here.
> 
> The LED trigger will call ksetting_get to get a list of supported link
> modes. You can then use the table struct phy_setting settings[] in
> phy-core.c to translate the link mode to a speed. You are likely to
> get a lot of duplicate speeds, but you can remove them. For each speed
> you need to create a sysfs file. Why not just create a linked list,
> rather than an array? Or just walk the table and find out how many
> different speeds there are and allocate an array of that size. Its
> currently 15, which is not that big. And then just use the is_visible
> method to hide the ones which are not relevant.
> 
> I don't see any need for new enums or tables here, just a function to
> look up a link mode in that table and return the speed.
>

The big picture was to have an handy define and statically allocate the
array with the max amount of link modes possible without having to
allocate kernel memory at runtime.

With the current way of allocating only the needed space, I have to
parse the settings table 2 times (one to get the number and the second
time to compose the actual array)

Not a problem since these are called only on LED register or when the
device name is called... Just extra code and the fact that kmalloc can
fail with ENOMEM. (but that is almost imposible and would be in an OOM
condition)

-- 
	Ansuel