netdev - Re: ethtool ioctl ABI: preferred way to expand uapi structure 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]

Message-ID: <ceaee76d-d785-4931-ad4a-ddba06365308@gmail.com>
Date: Thu, 4 Jan 2024 17:06:54 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Andrew Lunn <andrew@...n.ch>, Marek Behún
 <kabel@...nel.org>
Cc: Michal Kubecek <mkubecek@...e.cz>, netdev@...r.kernel.org,
 Russell King <linux@...linux.org.uk>
Subject: Re: ethtool ioctl ABI: preferred way to expand uapi structure
 ethtool_eee for additional link modes?

On 04.01.2024 16:36, Andrew Lunn wrote:
> On Thu, Jan 04, 2024 at 04:14:16PM +0100, Marek Behún wrote:
>> Hello,
>>
>> the legacy ioctls ETHTOOL_GSET and ETHTOOL_SSET, which pass structure
>> ethtool_cmd, were superseded by ETHTOOL_GLINKSETTINGS and
>> ETHTOOL_SLINKSETTINGS.
>>
>> This was done because the original structure only contains 32-bit words
>> for supported, advertising and lp_advertising link modes. The new
>> structure ethtool_link_settings contains member
>>   s8 link_mode_masks_nwords;
>> and a flexible array
>>   __u32 link_mode_masks[];
>> in order to overcome this issue.
>>
>> But currently we still have only legacy structure ethtool_eee for EEE
>> settings:
>>   struct ethtool_eee {
>>     __u32 cmd;
>>     __u32 supported;
>>     __u32 advertised;
>>     __u32 lp_advertised;
>>     __u32 eee_active;
>>     __u32 eee_enabled;
>>     __u32 tx_lpi_enabled;
>>     __u32 tx_lpi_timer;
>>     __u32 reserved[2];
>>   };
>>
>> Thus ethtool is unable to get/set EEE configuration for example for
>> 2500base-T and 5000base-T link modes, which are now available in
>> several PHY drivers.
>>
>> We can remedy this by either:
>>
>> - adding another ioctl for EEE settings, as was done with the GSET /
>>   SSET
>>
>> - using the original ioctl, but making the structure flexible (we can
>>   replace the reserved fields with information that the array is
>>   flexible), i.e.:
>>
>>   struct ethtool_eee {
>>     __u32 cmd;
>>     __u32 supported;
>>     __u32 advertised;
>>     __u32 lp_advertised;
>>     __u32 eee_active;
>>     __u32 eee_enabled;
>>     __u32 tx_lpi_enabled;
>>     __u32 tx_lpi_timer;
>>     s8 link_mode_masks_nwords; /* zero if legacy 32-bit link modes */
>>     __u8 reserved[7];
>>     __u32 link_mode_masks[];
>>     /* filled in if link_mode_masks_nwords > 0, with layout:
>>      * __u32 map_supported[link_mode_masks_nwords];
>>      * __u32 map_advertised[link_mode_masks_nwords];
>>      * __u32 map_lp_advertised[link_mode_masks_nwords];
>>      */
>>   };
>>
>>   this way we will be left with another 7 reserved bytes for future (is
>>   this enough?)
>>
>> What would you prefer?
> 
> There are two different parts here. The kAPI, and the internal API.
> 
> For the kAPI, i would not touch the IOCTL interface, since its
> deprecated. The netlink API for EEE uses bitset32. However, i think
> the message format for a bitset32 and a generic bitset is the same, so
> i think you can just convert that without breaking userspace. But you
> should check with Michal Kubecek to be sure.
> 
> For the internal API, i personally would assess the work needed to
> change supported, advertised and lp_advertised into generic linkmode
> bitmaps. Any MAC drivers using phylib/phylink probably don't touch
> them, so you just need to change the phylib helpers. Its the MAC
> drivers not using phylib which will need more work. But i've no idea
> how much work that is. Ideally they all get changed, so we have a
> uniform clean API.
> 
In case you missed it: Few days ago I posted a series that adds full
EEE linkmode bitmap support to the ethtool netlink interface.
The good news is that no changes to the userspace tool are needed.

https://lore.kernel.org/netdev/783d4a61-2f08-41fc-b91d-bd5f512586a2@gmail.com/T/

>     Andrew

Heiner