netdev - Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration

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]

Message-ID: <Z4tuhzHwiKFIGZ5e@shell.armlinux.org.uk>
Date: Sat, 18 Jan 2025 09:04:07 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	Andrew Lunn <andrew+netdev@...n.ch>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	UNGLinuxDriver@...rochip.com, Phil Elwell <phil@...pberrypi.org>
Subject: Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support
 with phylink integration

On Sat, Jan 18, 2025 at 08:22:15AM +0100, Oleksij Rempel wrote:
> On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> > I'm unsure about many DSA drivers. mt753x:
> > 
> >         u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
> > 
> >         if (e->tx_lpi_timer > 0xFFF)
> >                 return -EINVAL;
> > 
> >         set = LPI_THRESH_SET(e->tx_lpi_timer);
> >         if (!e->tx_lpi_enabled)
> >                 /* Force LPI Mode without a delay */
> >                 set |= LPI_MODE_EN;
> >         mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
> > 
> > Why force LPI *without* a delay if tx_lpi_enabled is false? This
> > seems to go against the documented API:
> > 
> >  * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> >  *      that eee was negotiated.
> 
> According to MT7531 manual, I would say, the code is not correct:
> https://repo.librerouter.org/misc/lr2/MT7531_switch_Reference_Manual_for_Development_Board.pdf
> 
> The LPI_MODE_EN_Px bit has following meaning:
> 
> When there is no packet to be transmitted, and the idle time is greater
> than P2_LPI_THRESHOLD, the TXMAC will automatically enter LPI (Low
> Power Idle) mode and send EEE LPI frame to the link partner.
> 0: LPI mode depends on the P2_LPI_THRESHOLD.
> 1: Let the system enter the LPI mode immediately and send EEE LPI frame
>    to the link partner.

Okay, so LPI_MODE_EN_Px causes it to disregard the LPI timer, and enter
LPI mode immediately. Thus, the code should never set LPI_MODE_EN_Px.

> This chip seems to not have support for tx_lpi_enabled != eee_enabled
> configuration.

Sorry, I don't see your reasoning there - and I think your
interpretation is different from the documentation (which is
the whole point of having a generic implementation in phylib
to avoid these kinds of different interpretation.)

 * @eee_enabled: EEE configured mode (enabled/disabled).
 * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
 *      that eee was negotiated.

           eee on|off
                  Enables/disables the device support of EEE.

           tx-lpi on|off
                  Determines whether the device should assert its Tx LPI.

The way phylib interprets eee_enabled is whether EEE is advertised
to the remote device or not. If EEE is not advertised, then EEE is
not negotiated, and thus EEE will not become active. If EEE is not
active, then LPI must not be asserted. tx_lpi_enabled defines whether,
given that EEE has been negotiated, whether LPI should be signalled
after the LPI timer has expired.

phylib deals with all this logic, and its all encoded into the
phydev->enable_tx_lpi flag to give consistency of implementation.

Thus, phydev->enable_tx_lpi is only true when eee_enabled && eee
negotiated at the specified speed && tx_lpi_enabled. In that state,
LPI is expected to be signalled after the LPI timer has expired.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!