RE: [PATCH net-next] net: phy: add wol config options in phy device

[Ronnie.Kunin]

Hi Russell,
I do agree with most of what you posted below, including that the Maxlinear gpy driver has a bug in the handling of interrupts which is the culprit, and that it can be fixed within that driver itself without additions to phylib.

Additional comment / question below

Thanks,
Ronnie

> -----Original Message-----
> From: Russell King <linux@xxxxxxxxxxxxxxx>
> Sent: Tuesday, May 7, 2024 7:54 AM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
> Cc: Andrew Lunn <andrew@xxxxxxx>; netdev@xxxxxxxxxxxxxxx; lxu@xxxxxxxxxxxxx;
> hkallweit1@xxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>
> Subject: Re: [PATCH net-next] net: phy: add wol config options in phy device
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
> 

There is definite value to having the phy be able to effectively communicate which specific wol events it currently has enabled so the mac driver can make better decisions on what to enable or not in the mac hardware (which of course will lead to more efficient power management). While not really needed for the purpose of fixing this driver's bugs, Raju's proposed addition of a wolopts tracking variable to phydev was also providing a direct way to access that information. In the current patch Raju is working on, the first call the lan743x mac driver does in its set_wol() function is to call the phy's set_wol() so that it gives the phy priority in wol handling. I guess when you say that phylib does not need to track this by adding a wolops member to the phydev structure, if we need that information the alternative way is to just immediately call the phy's get_wol() after set_wol() returns, correct ?

> > 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!