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: <20230810125117.GD13300@pengutronix.de>
Date: Thu, 10 Aug 2023 14:51:17 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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.

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

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |