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

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: <Y++Kw6ayQLnsAQRr@lunn.ch>
Date:   Fri, 17 Feb 2023 15:10:11 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Doug Berger <opendmb@...il.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Wei Fang <wei.fang@....com>,
        Shenwei Wang <shenwei.wang@....com>,
        Clark Wang <xiaoning.wang@....com>,
        NXP Linux Team <linux-imx@....com>,
        UNGLinuxDriver@...rochip.com, Byungho An <bh74.an@...sung.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Oleksij Rempel <linux@...pel-privat.de>
Subject: Re: [PATCH RFC 12/18] net: dsa: mt7530: Call phylib for set_eee and
 get_eee

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@...n.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