netdev - Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

Open Source and information security mailing list archives

Message-ID: <a9fee3a7-8c31-e048-32eb-ed82b8233aee@intel.com>
Date: Fri, 25 Aug 2023 15:19:51 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Pawel Chmielewski <pawel.chmielewski@...el.com>
CC: "Greenwalt, Paul" <paul.greenwalt@...el.com>, <aelior@...vell.com>, Andrew
 Lunn <andrew@...n.ch>, <intel-wired-lan@...ts.osuosl.org>,
	<manishc@...vell.com>, <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced
 speed to supported link modes maps

From: Pawel Chmielewski <pawel.chmielewski@...el.com>
Date: Wed, 23 Aug 2023 19:56:24 +0200

> On Sun, Aug 20, 2023 at 12:20:43PM -0700, Greenwalt, Paul wrote:
>>
>>
>> On 8/20/2023 11:54 AM, Andrew Lunn wrote:
>>> On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote:
>>>> The need to map Ethtool forced speeds to  Ethtool supported link modes is
>>>> common among drivers. To support this move the supported link modes maps
>>>> implementation from the qede driver. This is an efficient solution
>>>> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes
>>>> maps on module init") for qede driver.
>>>>
>>>> ethtool_forced_speed_maps_init() should be called during driver init
>>>> with an array of struct ethtool_forced_speed_map to populate the
>>>> mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized
>>>> the struct ethtool_forced_speed_map.
>>>
>>> Is there any way to reuse this table:
>>>
>>> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L161
>>>
>>> Seems silly to have multiple tables if this one can be made to work.
>>> It is also used a lot more than anything you will add, which has just
>>> two users so far, so problems with it a likely to be noticed faster.
>>>
>>> 	Andrew
>>
>> Yes, we'll can look into that.

BTW,

drivers/net/ethernet/qlogic/qed/qed_main.c
drivers/net/pcs/pcs-xpcs.c

also have similar stuff and could probably make use of the generic stuff
you're doing as well (qed_main was also done by me).
Not speaking of

drivers/net/phy/phylink.c

We probably should unify all that...

Let me think how we could do that.
Andrew's idea is good. But most high-speed NICs, which have a standalone
management firmware for PHY, don't use phylib/phylink.
So in order to be able to unify all that, they should have ->supported
bitmap somewhere else. Not sure struct net_device is the best place...

If I recall Phylink logics correctly (it's been a while since I last
time was working with my embedded project),

1) in the NIC (MAC) driver, you initialize ->supported with *speeds* and
   stuff like duplex, no link modes;
2) Phylink core sets the corresponding link mode bits;
3) phylib core then clears the bits unsupported by the PHY IIRC

The third step in case with those NICs with FW-managed PHYs should be
done manually in the MAC driver somewhere. Like "I am qede and I don't
support mode XX at 50Gbps, but support the rest, so I clear that one bit".
Then the networking core should be able to play this association game
itself. That would remove a good amount of boilerplating.

> 
> I think it would be better to leave the maps in the code of respective drivers, as they are too much hardware related. 
> Even for a single speed, the sets of supported link modes may vary between the devices.
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@...osl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Thanks,
Olek

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