Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>,
	Doug Berger <opendmb@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Byungho An <bh74.an@samsung.com>,
	Clark Wang <xiaoning.wang@nxp.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Jose Abreu <joabreu@synopsys.com>,
	Justin Chen <justin.chen@broadcom.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-stm32@st-md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	netdev@vger.kernel.org, NXP Linux Team <linux-imx@nxp.com>,
	Paolo Abeni <pabeni@redhat.com>,
	Shenwei Wang <shenwei.wang@nxp.com>, Wei Fang <wei.fang@nxp.com>
Subject: Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()
Date: Tue, 6 Feb 2024 13:12:33 +0000	[thread overview]
Message-ID: <ZcIwQcn3qlk0UjS4@shell.armlinux.org.uk> (raw)
In-Reply-To: <20240206112024.3jxtcru3dupeirnj@skbuf>

On Tue, Feb 06, 2024 at 01:20:24PM +0200, Vladimir Oltean wrote:
> On Sun, Feb 04, 2024 at 12:13:28PM +0000, Russell King (Oracle) wrote:
> > b53_get_mac_eee() sets both eee_enabled and eee_active, and then
> > returns zero.
> > 
> > dsa_slave_get_eee(), which calls this function, will then continue to
> > call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there
> > is no PHY present, otherwise calling phy_ethtool_get_eee() which in
> > turn will call genphy_c45_ethtool_get_eee().
> 
> Nitpick: If you need to resend, the function name changed to
> dsa_user_get_eee().

Thanks.

> > @@ -2227,16 +2227,10 @@ EXPORT_SYMBOL(b53_eee_init);
> >  int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e)
> >  {
> >  	struct b53_device *dev = ds->priv;
> > -	struct ethtool_keee *p = &dev->ports[port].eee;
> > -	u16 reg;
> >  
> >  	if (is5325(dev) || is5365(dev))
> >  		return -EOPNOTSUPP;
> >  
> > -	b53_read16(dev, B53_EEE_PAGE, B53_EEE_LPI_INDICATE, &reg);
> > -	e->eee_enabled = p->eee_enabled;
> > -	e->eee_active = !!(reg & BIT(port));
> > -
> 
> I know next to nothing about EEE and especially the implementation on
> Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE
> completely redundant? Is it actually in the system's best interest to
> ignore it?

That's a review comment that should have been made when the original
change to phylib was done, because it's already ignored in kernels
today since the commit changing phylib that I've referenced in this
series - since e->eee_enabled and e->eee_active will be overwritten by
phylib.

If we need B53_EEE_LPI_INDICATE to do something, then we need to have
a discussion about it, and decide how that fits in with the EEE
interface, and how to work around phylib's implementation.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel