| 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
| ||
|
Date: Fri, 12 Apr 2024 06:52:43 +0200 From: Oleksij Rempel <o.rempel@...gutronix.de> To: Florian Fainelli <florian.fainelli@...adcom.com> Cc: Andrew Lunn <andrew@...n.ch>, Doug Berger <opendmb@...il.com>, Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org Subject: Re: [PATCH net-next] net: genet: Fixup EEE On Thu, Apr 11, 2024 at 10:04:13AM -0700, Florian Fainelli wrote: > On 4/11/24 07:35, Andrew Lunn wrote: > > On Thu, Apr 11, 2024 at 07:17:36AM +0200, Oleksij Rempel wrote: > > > Hi Florian, > > > > > > On Wed, Apr 10, 2024 at 10:48:26AM -0700, Florian Fainelli wrote: > > > > > > > I am seeing a functional difference with and without your patch however, and > > > > also, there appears to be something wrong within the bcmgenet driver after > > > > PHYLIB having absorbed the EEE configuration. Both cases we start on boot > > > > with: > > > > > > > > # ethtool --show-eee eth0 > > > > EEE settings for eth0: > > > > EEE status: disabled > > > > Tx LPI: disabled > > > > Supported EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Link partner advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > > > > > I would expect the EEE status to be enabled, that's how I remember it > > > > before. > > > > > > Yes, current default kernel implementation is to use EEE if available. > > > > We do however seem to be inconsistent in this example. EEE seems to be > > disabled, yet it is advertising? Or is it showing what we would > > advertise, when EEE is enabled? > > What I consider to be the "canonical" behavior is the following on boot: > > # ethtool --show-eee eth0 > EEE settings for eth0: > EEE status: enabled - active > Tx LPI: disabled > Supported EEE link modes: 100baseT/Full > 1000baseT/Full > Advertised EEE link modes: 100baseT/Full > 1000baseT/Full > Link partner advertised EEE link modes: 100baseT/Full > 1000baseT/Full > > whereby we advertise EEE, the link partner does too, and the adjust_link > callback determined that we could EEE as a result via phy_init_eee(). This > is seen on 6.8. > > Starting with 6.9-rc and Andrew's series to rework EEE, I have the behavior > provided before: > > # ethtool --show-eee eth0 > EEE settings for eth0: > EEE status: disabled > Tx LPI: disabled > Supported EEE link modes: 100baseT/Full > 1000baseT/Full > Advertised EEE link modes: 100baseT/Full > 1000baseT/Full > Link partner advertised EEE link modes: 100baseT/Full > 1000baseT/Full > > whereby we need user intervention to opt-in and have EEE enabled with: > > ethtool --set-eee eth0 eee on > > This presents users with a difference in behavior which we might get > regression reports for. The phy_support_eee() is missing. This should be added on phy attach if EEE is supported by MAC. > > > > > > Now, with your patch, once I turn on EEE with: > > > > > > > > # ethtool --set-eee eth0 eee on > > > > # ethtool --show-eee eth0 > > > > EEE settings for eth0: > > > > EEE status: enabled - active > > > > Tx LPI: disabled > > > > Supported EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Link partner advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > # > > > > > > > > there is no change to the EEE_CTRL register to set the EEE_EN, this only > > > > happens when doing: > > > > > > > > # ethtool --set-eee eth0 eee on tx-lpi on > > > > > > > > which is consistent with the patch, but I don't think this is quite correct > > > > as I remembered that "eee on" meant enable EEE for the RX path, and "tx-lpi > > > > on" meant enable EEE for the TX path? > > > > > > Yes. More precisely, with "eee on" we allow the PHY to advertise EEE > > > link modes. On link_up, if both sides are agreed to use EEE, MAC is > > > configured to process LPI opcodes from the PHY and send LPI opcodes to > > > the PHY if "tx-lpi on" was configured too. tx-lpi will not be enabled in > > > case of "eee off". > > > > Florian seems to be suggesting the RX and TX path could have different > > configurations? RX EEE could be enabled but TX EEE disabled? That i > > don't understand, in terms of auto-neg. auto-neg is for the link as a > > whole, it does not appear to allow different results for each > > direction. Does tx-lpi only make sense when EEE is forced, not > > auto-neg'ed? > > To me the 'tx-lpi' parameter allows for an additional level of local control > of whether TX path should sent LPI or not, irrespective of forced versus > auto-negotiated EEE capability. Hm.. many of combined devices like MACs with integrated PHYs or PHYs with integrated EEE-LPI support (SmartEEE...) do hardware EEE with autoneg. configuring tx-lpi without "eee on" would not bring much. > I am not sure why the API was defined like it was in the first place and > what was the rationale for offering a separate 'tx-lpi', this might have > been based upon a real or hypothetical use case. > > If we are to honor the separate controls, we would have to agree on their > meaning and we had discussed this before with Oleksij. I can imagine, this can be used to optimize for some traffic use cases. "tx-lpi off" is in general equal to "tx-timer 0" or tx-timer some big number. May be, it was a workaround, since recommended initial tx-timer value depend on the linkspeed. At least some of microchip devices have special recommendations about it. > EEE_EN means that the Ethernet MAC (called UNIMAC in this block) enables > clock gating signaling from the PHY up to DMA interface, this is how the > power savings are actually realized on the digital logic side. Hm.. so, on this MAC, at leas partial LPI mode will still work without EEE_EN? Beside, I see there is UMAC_EEE_WAKE_TIMER, which is not configured. This one is very important, the wake time depend on the link speed, the PHY and potentially link partner. If not properly configure, EEE may fail. Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists