[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <53909c11-825a-489f-822a-dd4829dc8041@lunn.ch>
Date: Sat, 6 Jan 2024 00:15:18 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Heiner Kallweit <hkallweit1@...il.com>
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
> 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
Powered by blists - more mailing lists