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

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

[Marek Behún]

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?

Marek

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

[Andrew Lunn]

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

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

[Heiner Kallweit]

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

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

[Maxime Chevallier]

Hello Marek,

On Thu, 4 Jan 2024 16:14:16 +0100
Marek Behún <kabel@kernel.org> 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.

If I am not mistake, I think this series from Heiner tries to address
exactly that :
https://lore.kernel.org/netdev/783d4a61-2f08-41fc-b91d-bd5f512586a2@gmail.com/

Thanks,

Maxime

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

[Marek Behún]

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

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

[Heiner Kallweit]

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.
> 
An architectural comment regarding the series I sent:
I chose to be compatible with the current get_eee/set_eee callbacks.
There might be concerns that the "check whether our struct ethtool_eee
instance is embedded in a struct ethtool_keee instance" isn't too nice
from an architectural point of view.
Alternative would be too add new callbacks with a struct ethtool_keee
argument. This may be cleaner, but would require additional code in
the ethtool netlink layer for supporting old and new version of the
callbacks.

>     Andrew

Heiner