Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Heiner Kallweit <hkallweit1@gmail.com>]

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