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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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