[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8bfd2b95-2c73-4372-bf63-0c6ab7cd03c8@gmail.com>
Date: Fri, 5 Jan 2024 23:35:29 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Russell King <rmk+kernel@...linux.org.uk>,
David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend
struct ethtool_eee
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@...il.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
>
>
Powered by blists - more mailing lists