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
