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

Message

Christian Marangi Dec. 13, 2023, 6:15 p.m. UTC

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

Comments

Andrew Lunn Dec. 13, 2023, 11:05 p.m. UTC | #1

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

Christian Marangi Dec. 13, 2023, 11:10 p.m. UTC | #2

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)