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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 13 Dec 2023 17:56:57 +0100
From:   Christian Marangi <ansuelsmth@...il.com>
To:     Marek Behún <kabel@...nel.org>
Cc:     Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Daniel Golle <daniel@...rotopia.org>,
        "David S. Miller" <davem@...emloft.net>,
        Li Zetao <lizetao1@...wei.com>, linux-kernel@...r.kernel.org,
        linux-leds@...r.kernel.org
Subject: Re: [PATCH v2 1/2] leds: trigger: netdev: display only supported
 link speed attribute

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