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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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!
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
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 <[email protected]>
> ---
> 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!
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!
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 <[email protected]>
> > ---
> > 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
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
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
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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