netdev - Re: [PATCH RFC net-next 10/10] net: dsa: allow use of phylink managed EEE support

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: <Z4bC77mwoeypDAdH@shell.armlinux.org.uk>
Date: Tue, 14 Jan 2025 20:02:55 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Bryan Whitehead <bryan.whitehead@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Marcin Wojtas <marcin.s.wojtas@...il.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
	UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH RFC net-next 10/10] net: dsa: allow use of phylink
 managed EEE support

On Tue, Jan 14, 2025 at 09:26:56PM +0200, Vladimir Oltean wrote:
> On Tue, Jan 14, 2025 at 02:02:50PM +0000, Russell King (Oracle) wrote:
> > In order to allow DSA drivers to use phylink managed EEE, changes are
> > necessary to the DSA .set_eee() and .get_eee() methods. Where drivers
> > make use of phylink managed EEE, these should just pass the method on
> > to their phylink implementation without calling the DSA specific
> > operations.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > ---
> 
> What is the reason for including this patch with this set, where
> it is of no use until at least one DSA driver provides the new API
> implementations?

No criticism against you, but I guess you didn't read the cover
message? I tend not to read cover messages. I've been wondering for a
while now whether anyone actually does and thus whether they are worth
the effort of writing anything beyond providing a message ID to tie a
series together and a diffstat for the series.

Here's the relevant bit:

"The remainder of the patches convert mvneta, lan743x and stmmac, add
support for mvneta, and add the basics that will be necessary into the
DSA code for DSA drivers to make use of this.

"I would like to get patches 1 through 9 into net-next before the
merge window, but we're running out of time for that."

So, it's included in this RFC series not because I'm intending it to
be merged, but so that people can see what DSA requires to make it
functional there, and provide review comments if they see fit - which
you have done, thanks.

I'm sure if I'd said "I have patches for DSA" you'd have responded
asking to see them!

Of course, I do have changes that will require this - mt753x - but
that patch is not quite ready because this series I've posted has seen
a few changes recently to remove stuff that was never settled (the
LPI timer questions, whether it should be validated, clamped, should
phylink provide a software timer if the LPI timer is out of range,
etc.) That's more proof that text in cover messages is utterly
useless!

> >  net/dsa/user.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > index c74f2b2b92de..6912d2d57486 100644
> > --- a/net/dsa/user.c
> > +++ b/net/dsa/user.c
> > @@ -1233,16 +1233,23 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)
> >  	if (!ds->ops->support_eee || !ds->ops->support_eee(ds, dp->index))
> >  		return -EOPNOTSUPP;
> >  
> > -	/* Port's PHY and MAC both need to be EEE capable */
> > -	if (!dev->phydev)
> > -		return -ENODEV;
> > -
> > -	if (!ds->ops->set_mac_eee)
> > -		return -EOPNOTSUPP;
> > +	/* If the port is using phylink managed EEE, then get_mac_eee is
> 
> set_mac_eee() is what is unnecessary.

Thanks for spotting that.

> > +	 * unnecessary.
> > +	 */
> > +	if (!ds->phylink_mac_ops ||
> > +	    !ds->phylink_mac_ops->mac_disable_tx_lpi ||
> > +	    !ds->phylink_mac_ops->mac_enable_tx_lpi) {
> 
> Does it make sense to export pl->mac_supports_eee_ops wrapped into a
> helper function and call that here? To avoid making DSA too tightly
> coupled with the phylink MAC operation names.

I could wrap the test up in an inline function which would avoid the
duplication. Thanks for the suggestion.

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