* ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes?
@ 2024-01-04 15:14 Marek Behún
2024-01-04 15:36 ` Andrew Lunn
2024-01-04 16:17 ` Maxime Chevallier
0 siblings, 2 replies; 6+ messages in thread
From: Marek Behún @ 2024-01-04 15:14 UTC (permalink / raw)
To: Michal Kubecek, netdev; +Cc: Andrew Lunn, Russell King, Heiner Kallweit
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? 2024-01-04 15:14 ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? Marek Behún @ 2024-01-04 15:36 ` Andrew Lunn 2024-01-04 16:06 ` Heiner Kallweit 2024-01-04 16:39 ` Heiner Kallweit 2024-01-04 16:17 ` Maxime Chevallier 1 sibling, 2 replies; 6+ messages in thread From: Andrew Lunn @ 2024-01-04 15:36 UTC (permalink / raw) To: Marek Behún; +Cc: Michal Kubecek, netdev, Russell King, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? 2024-01-04 15:36 ` Andrew Lunn @ 2024-01-04 16:06 ` Heiner Kallweit 2024-01-04 16:21 ` Marek Behún 2024-01-04 16:39 ` Heiner Kallweit 1 sibling, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2024-01-04 16:06 UTC (permalink / raw) To: Andrew Lunn, Marek Behún; +Cc: Michal Kubecek, netdev, Russell King 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? 2024-01-04 16:06 ` Heiner Kallweit @ 2024-01-04 16:21 ` Marek Behún 0 siblings, 0 replies; 6+ messages in thread From: Marek Behún @ 2024-01-04 16:21 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Andrew Lunn, Michal Kubecek, netdev, Russell King 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? 2024-01-04 15:36 ` Andrew Lunn 2024-01-04 16:06 ` Heiner Kallweit @ 2024-01-04 16:39 ` Heiner Kallweit 1 sibling, 0 replies; 6+ messages in thread From: Heiner Kallweit @ 2024-01-04 16:39 UTC (permalink / raw) To: Andrew Lunn, Marek Behún; +Cc: Michal Kubecek, netdev, Russell King 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? 2024-01-04 15:14 ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? Marek Behún 2024-01-04 15:36 ` Andrew Lunn @ 2024-01-04 16:17 ` Maxime Chevallier 1 sibling, 0 replies; 6+ messages in thread From: Maxime Chevallier @ 2024-01-04 16:17 UTC (permalink / raw) To: Marek Behún Cc: Michal Kubecek, netdev, Andrew Lunn, Russell King, Heiner Kallweit 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-04 16:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-04 15:14 ethtool ioctl ABI: preferred way to expand uapi structure ethtool_eee for additional link modes? Marek Behún 2024-01-04 15:36 ` Andrew Lunn 2024-01-04 16:06 ` Heiner Kallweit 2024-01-04 16:21 ` Marek Behún 2024-01-04 16:39 ` Heiner Kallweit 2024-01-04 16:17 ` Maxime Chevallier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.