diff mbox series

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

Message ID 20231213181554.4741-3-ansuelsmth@gmail.com
State New
Headers
Series net: add define to describe link speed modes |

Commit Message

Christian Marangi Dec. 13, 2023, 6:15 p.m. UTC 
  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(-)

Comments

Russell King (Oracle) Dec. 13, 2023, 8:18 p.m. UTC | #1 
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.
Christian Marangi Dec. 13, 2023, 8:34 p.m. UTC | #2 
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.
diff mbox series

Patch

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