Re: [PATCH RFC 12/18] net: dsa: mt7530: Call phylib for set_eee and get_eee

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Feb 17, 2023 at 11:48:01AM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 04:42:24AM +0100, Andrew Lunn wrote:
> > phylib should be called in order to manage the EEE settings in the
> > PHY, and to return EEE status such are supported link modes, and what
> > the link peer supports.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/net/dsa/mt7530.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 214450378978..a472353f14f8 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port,
> >  {
> >  	struct mt7530_priv *priv = ds->priv;
> >  	u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port));
> > +	struct dsa_port *dp = dsa_to_port(ds, port);
> >  
> >  	e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN);
> >  	e->tx_lpi_timer = GET_LPI_THRESH(eeecr);
> >  
> > +	if (dp->slave->phydev)
> > +		return phy_ethtool_get_eee(dp->slave->phydev, e);
> 
> Given that DSA makes use of phylink, is there a reason why we can't use
> the phylink wrappers here

Ah, yes, phylink_foo would make a lot of sense. I will fix in the next
version.

> > @@ -3146,6 +3150,8 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
> >  		set |= LPI_MODE_EN;
> >  	mt7530_rmw(priv, MT7530_PMEEECR_P(port), mask, set);
> >  
> > +	if (dp->slave->phydev)
> > +		return phy_ethtool_set_eee(dp->slave->phydev, e);
> >  	return 0;
> 
> 
> Is this the correct place to do the set_eee operation - I mean, the
> register state has been altered (and it looks like it may enable LPI
> irrespective of the negotiated state) but what concerns me is that
> phy_ethtool_set_eee() can fail, and we return failure to userspace
> yet we've modified register state.

There is currently no consistency here between drivers. Some make this
call last, some do it earlier.

Thinking aloud...

Why would it fail? Most error reports are that phy_read() or
phy_write() failed. If that happens, the hardware is in an
inconsistent state anyway. It could be the PHY does not support
EEE. So an -EOPNOTSUPP or similar could be returned here. So long as
phydev->eee_active is never true, it should not matter if the value of
the LPI timer is changed here, it should never be put into use.  Are
there other cases where it could fail?

Maybe we should make the order in MAC set_eee() function
consistent. It is just a bigger change...

	    Andrew