Re: [PATCH v2 1/2] leds: trigger: netdev: display only supported link speed attribute

[Christian Marangi]

On Wed, Dec 13, 2023 at 04:37:57PM +0100, Marek Behún wrote:
> > +	/* Display only supported entry */
> > +	if (attr == &dev_attr_link_10.attr &&
> > +	    (test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported_link_speed) ||
> > +	     test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported_link_speed)))
> > +		return attr->mode;
> > +
> > +	if (attr == &dev_attr_link_100.attr &&
> > +	    (test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported_link_speed) ||
> > +	     test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported_link_speed)))
> > +		return attr->mode;
> > +
> > +	if (attr == &dev_attr_link_1000.attr &&
> > +	    (test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported_link_speed) ||
> > +	     test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported_link_speed)))
> > +		return attr->mode;
> > +
> > +	if (attr == &dev_attr_link_2500.attr &&
> > +	    test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported_link_speed))
> > +		return attr->mode;
> > +
> > +	if (attr == &dev_attr_link_5000.attr &&
> > +	    test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported_link_speed))
> > +		return attr->mode;
> > +
> > +	if (attr == &dev_attr_link_10000.attr &&
> > +	    test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported_link_speed))
> > +		return attr->mode;
> 
> Why only the T modes? There are much more ethtool modes for these
> speeds, for example at least 5 modes for 1000 mbps speed:
>   1000baseT_Half
>   1000baseT_Full
>   1000baseKX_Full
>   1000baseX_Full
>   1000baseT1_Full
> 
> There are also 2 possible modes for 2500 mbps
>   2500baseT
>   2500baseX
> 
> Ditto for 10 mbps and 100 mbps.
> 
> So if you're doing this, why not do it properly?

My concern was filling the thing with all kind of modes and have an if
hell.

> 
> There is an aarray
>   static const struct phy_setting settings[]
> in
>   drivers/net/phy/phy-core.c
> and a function phy_speeds() which will tell you which speeds are
> supported for a given ethtool mode bitmap.
> 
> Or you can add another function there,
>   bool phy_speed_supported(unsigned long mask, unsigned int speed)
> and use that one.
> 

Ok this is very handy and just perfect for the task. Problem is that
adding a function makes this again a cross subsystem patch and
problematic to merge...

And having phy_speed_supported would make the function very heavy as the
settings struct needs to be parsed multiple times...

Still I see the settings struct doesn't provide a way to comunicate the
sum of all the modes. I already have a patch in mind for that (usual
enum define hell) but that is again cross subsystem thing...

I see phy declare a array of 50 element to have enough space for all the
link speed modes. Think I will have to do the same here and update
later to a better implementation.

-- 
	Ansuel