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

["Marek Behún" <kabel@kernel.org>]

On Thu, 4 Jan 2024 17:06:54 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> 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/

I did indeed miss it :) Thanks