Re: ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes?

[Andrew Lunn <andrew@lunn.ch>]

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.

    Andrew