Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <6579e25b.5d0a0220.2445e.1753@mx.google.com>
Date:   Wed, 13 Dec 2023 17:56:57 +0100
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Marek =?iso-8859-1?Q?Beh=FAn?= <kabel@kernel.org>
Cc:     Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
        Daniel Golle <daniel@makrotopia.org>,
        "David S. Miller" <davem@davemloft.net>,
        Li Zetao <lizetao1@huawei.com>, linux-kernel@vger.kernel.org,
        linux-leds@vger.kernel.org
Subject: Re: [PATCH v2 1/2] leds: trigger: netdev: display only supported
 link speed attribute
References: <20231213150033.17057-1-ansuelsmth@gmail.com>
 <20231213163757.1d576bb7@dellmb>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231213163757.1d576bb7@dellmb>
Precedence: bulk

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