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

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] [thread-next>] [day] [month] [year] [list]

Date: Fri, 25 Aug 2023 15:57:08 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Andrew Lunn <andrew@...n.ch>
CC: Pawel Chmielewski <pawel.chmielewski@...el.com>, "Greenwalt, Paul"
	<paul.greenwalt@...el.com>, <aelior@...vell.com>,
	<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: Andrew Lunn <andrew@...n.ch>
Date: Fri, 25 Aug 2023 15:47:20 +0200

>> 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...
> 
> I would probably keep it in the driver priv structure, and just pass
> it as needed. So long as you only need one or two values, i don't see
> the need for a shared structure.
> 
>> 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
> 
> No, not really.
> 
> All i think you need is a low level helper. So don't worry too much
> about how phylink works, just implement that low level helper passing
> in values as needed, not phylib or phylink structure.
> 
> What i don't want is a second infrastructure to be built for those MAC
> drivers which don't use Linux to control the PHY. Either share a few
> helpers, or swap to phylink.

I'd love those drivers to be swapped to phylink, but I doubt that will
happen :D

> 
>> 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".
> 
> I don't think that will work. New bits keep getting added, more speeds
> added. So 'support the rest' is not well defined. You need an explicit

Ah, correct.

> list of link modes the driver needs. We already have code to convert
> an array of link mode bits into an actual mask, e.g:
> 
>         linkmode_set_bit_array(phy_basic_t1_features_array,
>                                ARRAY_SIZE(phy_basic_t1_features_array),
>                                phy_basic_t1_features);

Now I got lost a bit in what we do really want to share now, as less
sharing was indirectly rejected by "you can share more, let PHY/whatever
take care of this" and now wider sharing was indirectly rejected by
"that won't work" :D
>From what I understood, all we want now is the stuff introduced by the
original patch from that thread, but without "generic" speed arrays
definitions?

> 
> 	Andrew			       

Thanks,
Olek