Message-ID: <ZjoWSJNS0BbeySuQ@shell.armlinux.org.uk>
Date: Tue, 7 May 2024 12:53:44 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org, lxu@...linear.com,
	hkallweit1@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.org,
	UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next] net: phy: add wol config options in phy device

On Tue, May 07, 2024 at 03:50:39PM +0530, Raju Lakkaraju wrote:
> Hi Russell King,
> 
> Sorry for late response
> 
> If we have phy's wolopts which holds the user configuration, Ethernet MAC
> device can configure Power Manager's WOL registers whether handle only 
> PHY interrupts or MAC's WOL functionality.

That is the responsibility of the MAC driver to detect whether the MAC
needs to be programmed for WoL, or whether the PHY is doing the wakeup.
This doesn't need phylib to do any tracking.

> In existing code, we don't have any information about PHY's user configure
> to configure the PM mode

So you want the MAC driver to access your new phydev->wolopts. What if
there isn't a PHY, or the PHY is on a pluggable module (e.g. SFP.)
No, you don't want to have phylib tracking this for the MAC. The MAC
needs to track this itself if required.

> The 05/02/2024 16:59, Russell King (Oracle) wrote:
> > and why the PHY isn't retaining it.
> 
> mxl-gpy driver does not have soft_reset( ) function.
> In resume sequence, mxl-gpy driver is clearing the WOL configuration and
> interrupt i.e. gpy_config_init( ) and gpy_config_intr( )

That sounds like the bug in this instance.

If a PHY driver has different behaviour from what's expected then it's
buggy, and implementing workarounds in phylib rather than addressing
the buggy driver is a no-no. Sorry.

Why is mxl-gpy always masking and acknowledging interrupts in
gpy_config_init()? This goes completely against what phylib expects.
Interrupts are supposed to be managed by the config_intr() method,
not the config_init() method.

Moreover, if phydev->interrupts == PHY_INTERRUPT_ENABLED, then we
expect interrupts to remain enabled, yet mxl-gpy *always* disables
all interrupts in gpy_config_init() and then re-enables them in
gpy_config_intr() leaving out the WoL interrupt.

Given that gpy_config_intr() is called immediately after
gpy_config_init() in phy_init_hw(), this is nonsense, and it is this
nonsense that is at the root of the problem here. This is *not*
expected PHY driver behaviour.

See for example the at803x driver, specifically at803x_config_intr().
When PHY_INTERRUPT_ENABLED, it doesn't clear the WoL interrupt (via
the AT803X_INTR_ENABLE_WOL bit.)

The dp83822 driver enables the WoL interrupt in dp83822_config_intr()
if not in fibre mode and interupts are requested to be enabled.

The dp83867 driver leaves the WoL interrupt untouched in
dp83867_config_intr() if interrupts are requested to be enabled - if
it was already enabled (via set_wol()) then that state is preserved
if interrupts are being used. dp83869 is the same.

motorcomm doesn't support interrupts, but does appear to use the
interrupt pin for WoL, and doesn't touch the interrupt mask state in
config_intr/config_init.

mscc looks broken in a similar way to mxl-gpy - even worse, if
userspace hammers on the set_wol() method, it'll read the interrupt
status which appears to clear the status - possibly preventing real
interrupts to be delivered. It also does the
clear-MII_VSC85XX_INT_MASK-on-config_init() which will break WoL.


So, in summary, please fix mxl-gpy.c not to clear the interrupt mask
in the config_init() method. The contents of gpy_config_init() needs
to move to gpy_config_intr(), and it needs not to mask the WoL
interrupt if phydev->interrupts == PHY_INTERRUPT_ENABLED.

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

Powered by blists - more mailing lists