netdev - Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

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: <ZNTjQnufpCPMEEwd@shell.armlinux.org.uk>
Date: Thu, 10 Aug 2023 14:16:50 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Marek Vasut <marex@...x.de>, Andrew Lunn <andrew@...n.ch>,
	Wei Fang <wei.fang@....com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Oleksij Rempel <linux@...pel-privat.de>,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

On Thu, Aug 10, 2023 at 02:51:17PM +0200, Oleksij Rempel wrote:
> On Thu, Aug 10, 2023 at 11:34:02AM +0100, Russell King (Oracle) wrote:
> > On Thu, Aug 10, 2023 at 11:01:53AM +0100, Russell King (Oracle) wrote:
> > > On Thu, Aug 10, 2023 at 02:49:55AM +0200, Marek Vasut wrote:
> > > > On 8/10/23 00:06, Andrew Lunn wrote:
> > > > > On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > > > > > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > > > > > 
> > > > > > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > > > > > will be more changes if we use clock provider/consume mechanism.
> > > > > > > 
> > > > > > > Less changes is not always best. What happens when a different PHY is
> > > > > > > used?
> > > > > > 
> > > > > > Then the system wouldn't be affected by this AR803x specific behavior.
> > > > > 
> > > > > Do you know it really is specific to the AR803x? Turning the clock off
> > > > > seams a reasonable thing to do when saving power, or when there is no
> > > > > link partner.
> > > > 
> > > > This hibernation behavior seem specific to this PHY, I haven't seen it on
> > > > another PHY connected to the EQoS so far.
> > > 
> > > Marvell PHYs can be programmed so that RXCLK stops when the PHY
> > > enters power down or energy-detect state, although it defaults to
> > > always keeping the RGMII interface powered (and thus providing a
> > > clock.)
> > > 
> > > One Micrel PHY - "To save more power, the KSZ9031RNX stops the RX_CLK
> > > clock output to the MAC after 10 or more RX_CLK clock
> > > cycles have occurred in the receive LPI state." which seems to imply
> > > if EEE is enabled, then the receive clock will be stopped when
> > > entering low-power state.
> > > 
> > > I've said this several times in this thread - I think we need a bit
> > > in the PHY device's dev_flags to allow the MAC to say "do not power
> > > down the receive clock" which is used by the PHY drivers to (a) program
> > > the hardware to prevent the receive clock being stopped in situations
> > > such as the AR803x hibernate mode, and (b) to program the hardware not
> > > to stop the receive clock when entering EEE low power. This does seem
> > > to be a generic thing and not specific to just one PHY - especially as
> > > the stopping of clocks when entering EEE low power is a IEEE 802.3
> > > defined thing.
> > 
> > Like this:
> ... 
> >  
> > +        phy_disable_interrupts(phydev);
> > +
> >  	/* Start out supporting everything. Eventually,
> >  	 * a controller will attach, and may modify one
> >  	 * or both of these values
> > @@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
> >  	return 0;
> >  }
> >  
> > -static void phy_shutdown(struct device *dev)
> > -{
> > -	struct phy_device *phydev = to_phy_device(dev);
> > -
> > -	if (phydev->state == PHY_READY || !phydev->attached_dev)
> > -		return;
> > -
> > -	phy_disable_interrupts(phydev);
> > -}
> > -
> 
> Except of shutdown part from other discussion, looks fine for me.

Sigh... clearly I can't cope with email, especially when most of the
subject line is hidden! All I see in the index is "Re: [PATCH] net: phy:
at803x: Improve hi" for this thread, and I can't remember what the
other thread was... and it's buried in email.

> What will be the best way to solve this issue for DSA switches attached to
> MAC with RGMII RXC requirements?

I have no idea - the problem there is the model that has been adopted
in Linux is that there is no direct relationship between the DSA switch
and the MAC like there is with a PHY.

The MAC sees only a fixed-link, as does the DSA switch. So from the
software perspective, it's:

MAC ---- fixed-link

Switch ---- fixed-link

And it just so happens that packets pass between the two by "magic".
With a PHY, there is a direct relationship:

MAC ---- PHY

The MAC has a direct relationship with the PHY to the extent that the
MAC driver is involved in managing the PHY through phylink/phylib.

The effect of that de-coupling is that the MAC driver becomes quite
independent of the switch driver - I don't believe the switch driver
can query the MAC driver at probe time, nor the other way. If I'm
mistaken about that, someone needs to say!

What that leaves us with is firmware telling the switch driver...

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