netdev - Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend 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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 4 Jan 2024 18:16:56 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: Russell King <rmk+kernel@...linux.org.uk>,
	David Miller <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend
 struct ethtool_eee

On Mon, Jan 01, 2024 at 10:23:15PM +0100, Heiner Kallweit wrote:
> In order to pass EEE link modes beyond bit 32 to userspace we have to
> complement the 32 bit bitmaps in struct ethtool_eee with linkmode
> bitmaps. Therefore, similar to ethtool_link_settings and
> ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of
> the reserved fields in struct ethtool_eee as flag that an instance
> of struct ethtool_eee is embedded in a struct ethtool_keee, thus the
> linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---
>  include/linux/ethtool.h      | 18 ++++++++++++++++++
>  include/uapi/linux/ethtool.h |  4 +++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index cfcd952a1..3b46405dd 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
>  
> +struct ethtool_keee {
> +	struct ethtool_eee eee;
> +	struct {
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
> +	} link_modes;
> +	bool use_link_modes;
> +};

I know its a lot more work, but its not how i would do it.

1) Add struct ethtool_keee which is a straight copy of ethtool_eee.

2) Then modify every in kernel MAC driver using ethtool_eee to
actually take ethtool_keee. Since its identical, its just a function
prototype change.

3) Then i would add some helpers to get and set eee bits. The initial
version would be limited to 32 bits, and expect to be passed a pointer
to a u32. Them modify all the MAC drivers which manipulate the
supported, advertising and lp_advertising to use these helpers.

4) Lastly, flip supported, advertising and lp_advertising to
ETHTOOL_DECLARE_LINK_MODE_MASK, modify the helpers, and fixup the
IOCTL API to convert to legacy u32 etc.

The first 2 steps are a patch each. Step 3 is a lot of patches, one
per MAC driver, but the changes should be simple and easy to
review. And then 4 is probably a single patch.

Doing it like this, we have a clean internal API.

      Andrew