| Message ID | a044621e-07f3-4387-9573-015f255db895@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: add support for EEE linkmodes beyond bit 32 | expand |
On Mon, 1 Jan 2024 22:23:15 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:
> + * @is_member_of_keee: struct is member of a struct ethtool_keee
I don't like how the name of a field in a UAPI structure refers to
kernel internals.
Can we rename it somehow?
Marek
On 04.01.2024 17:27, Marek Behún wrote: > On Mon, 1 Jan 2024 22:23:15 +0100 > Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> + * @is_member_of_keee: struct is member of a struct ethtool_keee > > I don't like how the name of a field in a UAPI structure refers to > kernel internals. > Actually this new member of struct ethtool_eee is irrelevant to userspace. It just has to be member of struct ethtool_eee because that's what we pass to the kernel EEE ethtool callbacks. OK, in theory we could also pass the new flag as a new member of struct net_device, but this would be hacky IMO. > Can we rename it somehow? > I'm open for suggestions. > Marek Heiner
On Mon, Jan 01, 2024 at 10:23:15PM +0100, Heiner Kallweit wrote: > In order to pass EEE link modes beyond bit 32 to userspace we have to > complement the 32 bit bitmaps in struct ethtool_eee with linkmode > bitmaps. Therefore, similar to ethtool_link_settings and > ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of > the reserved fields in struct ethtool_eee as flag that an instance > of struct ethtool_eee is embedded in a struct ethtool_keee, thus the > linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > include/linux/ethtool.h | 18 ++++++++++++++++++ > include/uapi/linux/ethtool.h | 4 +++- > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index cfcd952a1..3b46405dd 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) > #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \ > DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS) > > +struct ethtool_keee { > + struct ethtool_eee eee; > + struct { > + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); > + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); > + __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); > + } link_modes; > + bool use_link_modes; > +}; I know its a lot more work, but its not how i would do it. 1) Add struct ethtool_keee which is a straight copy of ethtool_eee. 2) Then modify every in kernel MAC driver using ethtool_eee to actually take ethtool_keee. Since its identical, its just a function prototype change. 3) Then i would add some helpers to get and set eee bits. The initial version would be limited to 32 bits, and expect to be passed a pointer to a u32. Them modify all the MAC drivers which manipulate the supported, advertising and lp_advertising to use these helpers. 4) Lastly, flip supported, advertising and lp_advertising to ETHTOOL_DECLARE_LINK_MODE_MASK, modify the helpers, and fixup the IOCTL API to convert to legacy u32 etc. The first 2 steps are a patch each. Step 3 is a lot of patches, one per MAC driver, but the changes should be simple and easy to review. And then 4 is probably a single patch. Doing it like this, we have a clean internal API. Andrew
On 04.01.2024 18:16, Andrew Lunn wrote: > On Mon, Jan 01, 2024 at 10:23:15PM +0100, Heiner Kallweit wrote: >> In order to pass EEE link modes beyond bit 32 to userspace we have to >> complement the 32 bit bitmaps in struct ethtool_eee with linkmode >> bitmaps. Therefore, similar to ethtool_link_settings and >> ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of >> the reserved fields in struct ethtool_eee as flag that an instance >> of struct ethtool_eee is embedded in a struct ethtool_keee, thus the >> linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> include/linux/ethtool.h | 18 ++++++++++++++++++ >> include/uapi/linux/ethtool.h | 4 +++- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index cfcd952a1..3b46405dd 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) >> #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \ >> DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS) >> >> +struct ethtool_keee { >> + struct ethtool_eee eee; >> + struct { >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); >> + } link_modes; >> + bool use_link_modes; >> +}; > > I know its a lot more work, but its not how i would do it. > > 1) Add struct ethtool_keee which is a straight copy of ethtool_eee. > I think I would make some (compatible) changes. Member cmd isn't needed, and the type of the boolean values that is __u32 currently I'd change to bool. > 2) Then modify every in kernel MAC driver using ethtool_eee to > actually take ethtool_keee. Since its identical, its just a function > prototype change. > > 3) Then i would add some helpers to get and set eee bits. The initial > version would be limited to 32 bits, and expect to be passed a pointer > to a u32. Them modify all the MAC drivers which manipulate the > supported, advertising and lp_advertising to use these helpers. > > 4) Lastly, flip supported, advertising and lp_advertising to > ETHTOOL_DECLARE_LINK_MODE_MASK, modify the helpers, and fixup the > IOCTL API to convert to legacy u32 etc. > > The first 2 steps are a patch each. Step 3 is a lot of patches, one > per MAC driver, but the changes should be simple and easy to > review. And then 4 is probably a single patch. > I hope we get away with step 2 being a single patch, as it is a pure mechanical change. Typically a series is needed with one patch per module, then having to wait for the ACK from the respective maintainers. Can we get an OK upfront for the single patch approach? > Doing it like this, we have a clean internal API. > This indeed looks like the cleanest solution approach to me. One benefit would be that we can almost completely get rid of the strict data structure based coupling between userspace and kernel. However this applies only if we keep the ioctl interface out of scope. Is it consensus that the ioctl interface is considered legacy and extended functionality may be implemented for the netlink interface only? An upfront maintainer OK would be helpful. I don't like the link settings ioctl handshake too much, which is needed to communicate the number of bits in a linkmode bitmap to userspace. Presumably we had to reuse this approach for EEE linkmode bitmaps if we want to support linkmode bitmaps over ioctl. > Andrew > > Heiner
On 04.01.2024 18:16, Andrew Lunn wrote: > On Mon, Jan 01, 2024 at 10:23:15PM +0100, Heiner Kallweit wrote: >> In order to pass EEE link modes beyond bit 32 to userspace we have to >> complement the 32 bit bitmaps in struct ethtool_eee with linkmode >> bitmaps. Therefore, similar to ethtool_link_settings and >> ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of >> the reserved fields in struct ethtool_eee as flag that an instance >> of struct ethtool_eee is embedded in a struct ethtool_keee, thus the >> linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> include/linux/ethtool.h | 18 ++++++++++++++++++ >> include/uapi/linux/ethtool.h | 4 +++- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index cfcd952a1..3b46405dd 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) >> #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \ >> DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS) >> >> +struct ethtool_keee { >> + struct ethtool_eee eee; >> + struct { >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); >> + } link_modes; >> + bool use_link_modes; >> +}; > > I know its a lot more work, but its not how i would do it. > > 1) Add struct ethtool_keee which is a straight copy of ethtool_eee. > > 2) Then modify every in kernel MAC driver using ethtool_eee to > actually take ethtool_keee. Since its identical, its just a function > prototype change. > > 3) Then i would add some helpers to get and set eee bits. The initial > version would be limited to 32 bits, and expect to be passed a pointer > to a u32. Them modify all the MAC drivers which manipulate the > supported, advertising and lp_advertising to use these helpers. > Looking at the ethtool EEE ops implementation of the relevant 16 drivers i see quite some fixing/refactoring need. Just look at igc_ethtool_get_eee(): edata->supported = SUPPORTED_Autoneg; edata->advertised = SUPPORTED_Autoneg; edata->lp_advertised = SUPPORTED_Autoneg; This doesn't make sense at all, this function never worked and apparently nobody ever noticed this. Maybe the author meant edata->supported |= SUPPORTED_Autoneg, but even this wouldn't make sense for an EEE mode bitmap. I'd prefer to separate the needed refactoring/fixing from the EEE linkmode bitmap extension, therefore omit step 3. Steps 1 and 2 are good, they allow to decouple struct ethtool_keee from ethtool_eee, so we can simplify struct ethtool_keee and reduce it to what's needed on kernel side. > 4) Lastly, flip supported, advertising and lp_advertising to > ETHTOOL_DECLARE_LINK_MODE_MASK, modify the helpers, and fixup the > IOCTL API to convert to legacy u32 etc. > > The first 2 steps are a patch each. Step 3 is a lot of patches, one > per MAC driver, but the changes should be simple and easy to > review. And then 4 is probably a single patch. > > Doing it like this, we have a clean internal API. > > Andrew > >
> Looking at the ethtool EEE ops implementation of the relevant 16 drivers i see > quite some fixing/refactoring need. Just look at igc_ethtool_get_eee(): > > edata->supported = SUPPORTED_Autoneg; > edata->advertised = SUPPORTED_Autoneg; > edata->lp_advertised = SUPPORTED_Autoneg; > > This doesn't make sense at all, this function never worked and apparently > nobody ever noticed this. Maybe the author meant > edata->supported |= SUPPORTED_Autoneg, but even this wouldn't make sense > for an EEE mode bitmap. Yes, i noticed this as well. EEE is not too well defined, but this is wrong. Since it never worked, just deleting this is fine, and leave it to Intel engineers to set actual bitmaps. > I'd prefer to separate the needed refactoring/fixing from the EEE linkmode > bitmap extension, therefore omit step 3. > > Steps 1 and 2 are good, they allow to decouple struct ethtool_keee from > ethtool_eee, so we can simplify struct ethtool_keee and reduce it to what's > needed on kernel side. I would not do too much refactoring. I have a big patchset which refactors most of the phylib driven drivers code for EEE, removing a lot of it and pushing it into phylib. Its been sat in my repo a while and i need to find the time/energy to post it and get it merged. Andrew
On Sat, Jan 06, 2024 at 12:15:18AM +0100, Andrew Lunn wrote: > I would not do too much refactoring. I have a big patchset which > refactors most of the phylib driven drivers code for EEE, removing a > lot of it and pushing it into phylib. Its been sat in my repo a while > and i need to find the time/energy to post it and get it merged. Strangely, I have similar for phylink... I was thinking about sending it out during the last cycle, but I guess next cycle will do.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index cfcd952a1..3b46405dd 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \ DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS) +struct ethtool_keee { + struct ethtool_eee eee; + struct { + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); + __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); + } link_modes; + bool use_link_modes; +}; + +static inline struct ethtool_keee *ethtool_eee2keee(struct ethtool_eee *eee) +{ + if (!eee->is_member_of_keee) + return NULL; + + return container_of(eee, struct ethtool_keee, eee); +} + /* drivers must ignore base.cmd and base.link_mode_masks_nwords * fields, but they are allowed to overwrite them (will be ignored). */ diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 0787d561a..ffc5ab130 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -365,6 +365,7 @@ struct ethtool_eeprom { * @tx_lpi_timer: Time in microseconds the interface delays prior to asserting * its tx lpi (after reaching 'idle' state). Effective only when eee * was negotiated and tx_lpi_enabled was set. + * @is_member_of_keee: struct is member of a struct ethtool_keee * @reserved: Reserved for future use; see the note on reserved space. */ struct ethtool_eee { @@ -376,7 +377,8 @@ struct ethtool_eee { __u32 eee_enabled; __u32 tx_lpi_enabled; __u32 tx_lpi_timer; - __u32 reserved[2]; + __u8 is_member_of_keee; + __u8 reserved[7]; }; /**
In order to pass EEE link modes beyond bit 32 to userspace we have to complement the 32 bit bitmaps in struct ethtool_eee with linkmode bitmaps. Therefore, similar to ethtool_link_settings and ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of the reserved fields in struct ethtool_eee as flag that an instance of struct ethtool_eee is embedded in a struct ethtool_keee, thus the linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- include/linux/ethtool.h | 18 ++++++++++++++++++ include/uapi/linux/ethtool.h | 4 +++- 2 files changed, 21 insertions(+), 1 deletion(-)