netdev - Re: [PATCH v2 RFC 4/5] ethtool: add linkmode bitmap support to struct ethtool

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1cf62a4b-9837-4957-bd7a-0b648b8b3cae@gmail.com>
Date: Sun, 7 Jan 2024 18:50:15 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Russell King <rmk+kernel@...linux.org.uk>,
 Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, David Miller <davem@...emloft.net>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 RFC 4/5] ethtool: add linkmode bitmap support to struct
 ethtool_keee

On 07.01.2024 18:14, Andrew Lunn wrote:
> On Sat, Jan 06, 2024 at 11:21:31PM +0100, Heiner Kallweit wrote:
>> Add linkmode bitmap members to struct ethtool_keee, but keep the legacy
>> u32 bitmaps for compatibility with existing drivers.
>> Use link_modes.supported not being empty as indicator that a user wants
>> to use the linkmode bitmap members instead of the legacy bitmaps.
> 
> So my fear is, the legacy code will never get cleaned up.
> 
> How many MAC drivers are there which don't use phylib/phylink?
> 
A grep for lp_advertised gives the following, excluding lan78xx and lan743x,
for which I just submitted patches:
https://patchwork.kernel.org/project/netdevbpf/patch/3340ff84-8d7a-404b-8268-732c7f281164@gmail.com/
https://patchwork.kernel.org/project/netdevbpf/patch/b086b296-0a1b-42d4-8e2b-ef6682598185@gmail.com/

drivers/net/usb/r8152.c
drivers/net/usb/ax88179_178a.c
drivers/net/ethernet/qlogic/qede/qede_ethtool.c
drivers/net/ethernet/broadcom/tg3.c
drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
drivers/net/ethernet/broadcom/bnxt/bnxt.c
drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
drivers/net/ethernet/intel/igb/igb_ethtool.c
drivers/net/ethernet/intel/i40e/i40e_ethtool.c
drivers/net/ethernet/intel/igc/igc_ethtool.c
  This one is completely broken, we just talked about it.
drivers/net/ethernet/intel/e1000e/ethtool.c
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

> Maybe i can help out converting them.
> 
>> +++ b/include/linux/ethtool.h
>> @@ -223,6 +223,11 @@ __ethtool_get_link_ksettings(struct net_device *dev,
>>  			     struct ethtool_link_ksettings *link_ksettings);
>>  
>>  struct ethtool_keee {
>> +	struct {
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
>> +	} link_modes;
>>  	u32	supported;
>>  	u32	advertised;
>>  	u32	lp_advertised;
> 
> I don't particularly like having the link_modes struct here. The end
> goal should be that supported, advertised and lp_advertised become
> link modes, and all the drivers are changed to use them.
> 
> Maybe we have one patch which does another global replace,
> supported->supported_u32, advertised->advertised_32, etc, making space
> for link mode symbols. phylib can directly use the new link modes so
> there is no real change in that code. We can then convert the MAC
> drivers not using phylib one by one, and then remove the _u32 members
> at the end.
> 
Agreed. I'll add that.

>       Andrew

Heiner