Introduce a new member named 'wolopts' to the 'phy_device' structure to
store the user-specified Wake-on-LAN (WOL) settings. Update this member
within the phy driver's 'set_wol()' function whenever the WOL configuration
is modified by the user.
Currently, when the system resumes from sleep, the 'phy_init_hw()' function
resets the PHY's configuration and interrupts, which leads to problems upon
subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
we can ensure that the PHY's WOL configuration is correctly reapplied
through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
the issue
Signed-off-by: Raju Lakkaraju <[email protected]>
---
drivers/net/phy/mxl-gpy.c | 5 +++++
drivers/net/phy/phy_device.c | 5 +++++
include/linux/phy.h | 2 ++
3 files changed, 12 insertions(+)
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index b2d36a3a96f1..6edb29a1d77e 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
struct net_device *attach_dev = phydev->attached_dev;
int ret;
+ phydev->wolopts = 0;
if (wol->wolopts & WAKE_MAGIC) {
/* MAC address - Byte0:Byte1:Byte2:Byte3:Byte4:Byte5
* VPSPEC2_WOL_AD45 = Byte0:Byte1
@@ -725,6 +726,8 @@ static int gpy_set_wol(struct phy_device *phydev,
ret = phy_read(phydev, PHY_ISTAT);
if (ret < 0)
return ret;
+
+ phydev->wolopts |= WAKE_MAGIC;
} else {
/* Disable magic packet matching */
ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
@@ -748,6 +751,8 @@ static int gpy_set_wol(struct phy_device *phydev,
if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC))
phy_trigger_machine(phydev);
+ phydev->wolopts |= WAKE_PHY;
+
return 0;
}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 616bd7ba46cb..9740f08ad98e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2038,6 +2038,11 @@ int phy_suspend(struct phy_device *phydev)
if (phydev->suspended)
return 0;
+ if (phydev->wolopts) {
+ wol.wolopts = phydev->wolopts;
+ phy_ethtool_set_wol(phydev, &wol);
+ }
+
phy_ethtool_get_wol(phydev, &wol);
phydev->wol_enabled = wol.wolopts || (netdev && netdev->wol_enabled);
/* If the device has WOL enabled, we cannot suspend the PHY */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3ddfe7fe781a..4a628202699c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -615,6 +615,7 @@ struct macsec_ops;
* handling shall be postponed until PHY has resumed
* @irq_rerun: Flag indicating interrupts occurred while PHY was suspended,
* requiring a rerun of the interrupt handler after resume
+ * @wolopts: User requested wake-on-lan configuration
* @interface: enum phy_interface_t value
* @possible_interfaces: bitmap if interface modes that the attached PHY
* will switch between depending on media speed.
@@ -687,6 +688,7 @@ struct phy_device {
u32 dev_flags;
+ u32 wolopts;
phy_interface_t interface;
DECLARE_PHY_INTERFACE_MASK(possible_interfaces);
--
2.34.1
On Tue, 2024-04-30 at 10:36 +0530, Raju Lakkaraju wrote:
> Introduce a new member named 'wolopts' to the 'phy_device' structure to
> store the user-specified Wake-on-LAN (WOL) settings. Update this member
> within the phy driver's 'set_wol()' function whenever the WOL configuration
> is modified by the user.
>
> Currently, when the system resumes from sleep, the 'phy_init_hw()' function
> resets the PHY's configuration and interrupts, which leads to problems upon
> subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
> we can ensure that the PHY's WOL configuration is correctly reapplied
> through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
> the issue
>
> Signed-off-by: Raju Lakkaraju <[email protected]>
> ---
> drivers/net/phy/mxl-gpy.c | 5 +++++
> drivers/net/phy/phy_device.c | 5 +++++
> include/linux/phy.h | 2 ++
> 3 files changed, 12 insertions(+)
>
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index b2d36a3a96f1..6edb29a1d77e 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
> struct net_device *attach_dev = phydev->attached_dev;
> int ret;
>
> + phydev->wolopts = 0;
> if (wol->wolopts & WAKE_MAGIC) {
> /* MAC address - Byte0:Byte1:Byte2:Byte3:Byte4:Byte5
> * VPSPEC2_WOL_AD45 = Byte0:Byte1
> @@ -725,6 +726,8 @@ static int gpy_set_wol(struct phy_device *phydev,
> ret = phy_read(phydev, PHY_ISTAT);
> if (ret < 0)
> return ret;
> +
> + phydev->wolopts |= WAKE_MAGIC;
I'm wondering if 'phydev->wolopts' could/should be handled in the
common code.
> } else {
> /* Disable magic packet matching */
> ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> @@ -748,6 +751,8 @@ static int gpy_set_wol(struct phy_device *phydev,
> if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC))
> phy_trigger_machine(phydev);
>
> + phydev->wolopts |= WAKE_PHY;
> +
> return 0;
> }
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 616bd7ba46cb..9740f08ad98e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2038,6 +2038,11 @@ int phy_suspend(struct phy_device *phydev)
> if (phydev->suspended)
> return 0;
>
> + if (phydev->wolopts) {
> + wol.wolopts = phydev->wolopts;
> + phy_ethtool_set_wol(phydev, &wol);
The above will fail when the phy does not provide wol operations -
should never happen when 'wolopts != 0', but possibly worth catching
the error?
Thanks,
Paolo
On Tue, Apr 30, 2024 at 10:36:35AM +0530, Raju Lakkaraju wrote:
> Introduce a new member named 'wolopts' to the 'phy_device' structure to
> store the user-specified Wake-on-LAN (WOL) settings. Update this member
> within the phy driver's 'set_wol()' function whenever the WOL configuration
> is modified by the user.
>
> Currently, when the system resumes from sleep, the 'phy_init_hw()' function
> resets the PHY's configuration and interrupts, which leads to problems upon
> subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
> we can ensure that the PHY's WOL configuration is correctly reapplied
> through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
> the issue
Sorry it took a white to review this.
>
> Signed-off-by: Raju Lakkaraju <[email protected]>
> ---
> drivers/net/phy/mxl-gpy.c | 5 +++++
> drivers/net/phy/phy_device.c | 5 +++++
> include/linux/phy.h | 2 ++
> 3 files changed, 12 insertions(+)
>
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index b2d36a3a96f1..6edb29a1d77e 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
> struct net_device *attach_dev = phydev->attached_dev;
> int ret;
>
> + phydev->wolopts = 0;
Is this specific to mlx-gpy?
You should be trying to solve the problem for all PHYs which support
WoL. So i expect the core to be doing most of the work. In fact, i
don't think there is any need for driver specific code.
phy_ethtool_set_wol() can set phydev->wolopts after calling
phydev->drv->set_wol(). If it returns an error, including -ENOTSUPP,
set phydev->wolopts to 0, otherwise set it to wolopts.
> @@ -2038,6 +2038,11 @@ int phy_suspend(struct phy_device *phydev)
> if (phydev->suspended)
> return 0;
>
> + if (phydev->wolopts) {
> + wol.wolopts = phydev->wolopts;
> + phy_ethtool_set_wol(phydev, &wol);
> + }
Why on suspend? I would expect it to be on resume, after the PHY has
been reset.
I also think you need to save sopass[] in phydev, since some PHYs
support WAKE_MAGICSECURE. Just because mlx-gpy does not need the
password does not mean we should ignore it in general. I also think it
is safe to store in memory. Its is not a highly confidential
password. I would not be too surprised if some PHYs have the registers
read/write rather than write only.
Andrew
On Thu, May 02, 2024 at 04:51:42PM +0200, Andrew Lunn wrote:
> On Tue, Apr 30, 2024 at 10:36:35AM +0530, Raju Lakkaraju wrote:
> > Introduce a new member named 'wolopts' to the 'phy_device' structure to
> > store the user-specified Wake-on-LAN (WOL) settings. Update this member
> > within the phy driver's 'set_wol()' function whenever the WOL configuration
> > is modified by the user.
> >
> > Currently, when the system resumes from sleep, the 'phy_init_hw()' function
> > resets the PHY's configuration and interrupts, which leads to problems upon
> > subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
> > we can ensure that the PHY's WOL configuration is correctly reapplied
> > through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
> > the issue
>
> Sorry it took a white to review this.
>
> >
> > Signed-off-by: Raju Lakkaraju <[email protected]>
> > ---
> > drivers/net/phy/mxl-gpy.c | 5 +++++
> > drivers/net/phy/phy_device.c | 5 +++++
> > include/linux/phy.h | 2 ++
> > 3 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> > index b2d36a3a96f1..6edb29a1d77e 100644
> > --- a/drivers/net/phy/mxl-gpy.c
> > +++ b/drivers/net/phy/mxl-gpy.c
> > @@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
> > struct net_device *attach_dev = phydev->attached_dev;
> > int ret;
> >
> > + phydev->wolopts = 0;
>
> Is this specific to mlx-gpy?
>
> You should be trying to solve the problem for all PHYs which support
> WoL. So i expect the core to be doing most of the work. In fact, i
> don't think there is any need for driver specific code.
It would be good to hear exactly why its necessary for phylib to track
this state, and why the PHY isn't retaining it.
I suspect this may have something to do with resets - the PHY being
hardware reset when coming out of resume (resulting in all state
being lost.) What's resetting it would also be good to track down
(as in hardware, firmware, or the kernel.)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 5/2/24 08:59, Russell King (Oracle) wrote:
> On Thu, May 02, 2024 at 04:51:42PM +0200, Andrew Lunn wrote:
>> On Tue, Apr 30, 2024 at 10:36:35AM +0530, Raju Lakkaraju wrote:
>>> Introduce a new member named 'wolopts' to the 'phy_device' structure to
>>> store the user-specified Wake-on-LAN (WOL) settings. Update this member
>>> within the phy driver's 'set_wol()' function whenever the WOL configuration
>>> is modified by the user.
>>>
>>> Currently, when the system resumes from sleep, the 'phy_init_hw()' function
>>> resets the PHY's configuration and interrupts, which leads to problems upon
>>> subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
>>> we can ensure that the PHY's WOL configuration is correctly reapplied
>>> through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
>>> the issue
>>
>> Sorry it took a white to review this.
>>
>>>
>>> Signed-off-by: Raju Lakkaraju <[email protected]>
>>> ---
>>> drivers/net/phy/mxl-gpy.c | 5 +++++
>>> drivers/net/phy/phy_device.c | 5 +++++
>>> include/linux/phy.h | 2 ++
>>> 3 files changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
>>> index b2d36a3a96f1..6edb29a1d77e 100644
>>> --- a/drivers/net/phy/mxl-gpy.c
>>> +++ b/drivers/net/phy/mxl-gpy.c
>>> @@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
>>> struct net_device *attach_dev = phydev->attached_dev;
>>> int ret;
>>>
>>> + phydev->wolopts = 0;
>>
>> Is this specific to mlx-gpy?
>>
>> You should be trying to solve the problem for all PHYs which support
>> WoL. So i expect the core to be doing most of the work. In fact, i
>> don't think there is any need for driver specific code.
>
> It would be good to hear exactly why its necessary for phylib to track
> this state, and why the PHY isn't retaining it.
Agreed. I contemplated doing something similar while adding support for
Wake-on-LAN to the Broadcom PHY driver, but eventually convinced myself
this was not necessary as the hardware was capable of retaining the
wake-up event, and that the PHY driver *must* be able to charge the PHY
device for wake-up purposes, even on a cold boot.
>
> I suspect this may have something to do with resets - the PHY being
> hardware reset when coming out of resume (resulting in all state
> being lost.) What's resetting it would also be good to track down
> (as in hardware, firmware, or the kernel.)
>
Since it is possible to override the soft_reset callback called by
phy_init_hw(), I would be inclined to make this a driver specific
solution by doing something like:
mxl_gphy_soft_reset(...)
/* Save WoL status */
priv->wol_enabled = ...
return genphy_soft_reset()
--
Florian
> It would be good to hear exactly why its necessary for phylib to track
> this state, and why the PHY isn't retaining it.
I _think_ it is a hardware reset. The call path is probably:
mdio_bus_phy_resume():
phy_init_hw():
phy_device_reset()
mdio_device_reset()
But it would be good to get this confirmed.
Andrew
Hi Paolo,
Thank you for review comments.
The 05/02/2024 15:49, Paolo Abeni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, 2024-04-30 at 10:36 +0530, Raju Lakkaraju wrote:
> > @@ -725,6 +726,8 @@ static int gpy_set_wol(struct phy_device *phydev,
> > ret = phy_read(phydev, PHY_ISTAT);
> > if (ret < 0)
> > return ret;
> > +
> > + phydev->wolopts |= WAKE_MAGIC;
>
> I'm wondering if 'phydev->wolopts' could/should be handled in the
> common code.
Ok. I will change.
>
> > } else {
> > /* Disable magic packet matching */
> > ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> > @@ -748,6 +751,8 @@ static int gpy_set_wol(struct phy_device *phydev,
> > if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC))
> > phy_trigger_machine(phydev);
> >
> > + phydev->wolopts |= WAKE_PHY;
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 616bd7ba46cb..9740f08ad98e 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -2038,6 +2038,11 @@ int phy_suspend(struct phy_device *phydev)
> > if (phydev->suspended)
> > return 0;
> >
> > + if (phydev->wolopts) {
> > + wol.wolopts = phydev->wolopts;
> > + phy_ethtool_set_wol(phydev, &wol);
>
> The above will fail when the phy does not provide wol operations -
> should never happen when 'wolopts != 0', but possibly worth catching
> the error?
In another mail thread, Andrew gave different comment.
I will address that comment.
>
> Thanks,
>
> Paolo
>
--
Thanks,
Raju
Hi Andrew,
Thank you for review comments.
The 05/02/2024 16:51, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, Apr 30, 2024 at 10:36:35AM +0530, Raju Lakkaraju wrote:
> > Introduce a new member named 'wolopts' to the 'phy_device' structure to
> > store the user-specified Wake-on-LAN (WOL) settings. Update this member
> > within the phy driver's 'set_wol()' function whenever the WOL configuration
> > is modified by the user.
> >
> > Currently, when the system resumes from sleep, the 'phy_init_hw()' function
> > resets the PHY's configuration and interrupts, which leads to problems upon
> > subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
> > we can ensure that the PHY's WOL configuration is correctly reapplied
> > through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
> > the issue
>
> Sorry it took a white to review this.
>
Based on your review comments, I will update this.
> >
> > Signed-off-by: Raju Lakkaraju <[email protected]>
> > ---
> > drivers/net/phy/mxl-gpy.c | 5 +++++
> > drivers/net/phy/phy_device.c | 5 +++++
> > include/linux/phy.h | 2 ++
> > 3 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> > index b2d36a3a96f1..6edb29a1d77e 100644
> > --- a/drivers/net/phy/mxl-gpy.c
> > +++ b/drivers/net/phy/mxl-gpy.c
> > @@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
> > struct net_device *attach_dev = phydev->attached_dev;
> > int ret;
> >
> > + phydev->wolopts = 0;
>
> Is this specific to mlx-gpy?
>
Currently I have GPY211C PHY along with PCI11414 chip hardware. That's reason
I add these changes for GPY211C PHY.
I test the changes on my board.
> You should be trying to solve the problem for all PHYs which support
> WoL. So i expect the core to be doing most of the work. In fact, i
> don't think there is any need for driver specific code.
Ok. I will change.
>
> phy_ethtool_set_wol() can set phydev->wolopts after calling
> phydev->drv->set_wol(). If it returns an error, including -ENOTSUPP,
> set phydev->wolopts to 0, otherwise set it to wolopts.
>
Ok.
One quick question.
some of the options (ex. WAKE_PHY, WAKE_MAGIC etc) support on PHY and other
options (ex. WAKE_UCAST, WAKE_MAGICSECURE etc) on MAC of Ethernet device.
Suppose, user configure the combination (i.e. wol gu) option,
Is PHY flag should hold combination option or only PHY supported option ?
Ex:
$ sudo ethtool -s enp5s0 wol gu
Output of phydev's wolopts flag values should be 0x00000022 or 0x00000020 ?
In this case, PHY support WAKE_MAGIC and MAC support WAKE_UCAST
Anyhow, even phy's wolopts holds the user configuration value, get_wol( )
function read from PHY register and display only "g"
> > @@ -2038,6 +2038,11 @@ int phy_suspend(struct phy_device *phydev)
> > if (phydev->suspended)
> > return 0;
> >
> > + if (phydev->wolopts) {
> > + wol.wolopts = phydev->wolopts;
> > + phy_ethtool_set_wol(phydev, &wol);
> > + }
>
> Why on suspend? I would expect it to be on resume, after the PHY has
> been reset.
Ok. I will change.
May be in phy_init_hw( ) function is better place to re-config the WOL
>
> I also think you need to save sopass[] in phydev, since some PHYs
> support WAKE_MAGICSECURE. Just because mlx-gpy does not need the
> password does not mean we should ignore it in general. I also think it
> is safe to store in memory. Its is not a highly confidential
> password. I would not be too surprised if some PHYs have the registers
> read/write rather than write only.
Ok. I will add sopass[] also.
>
> Andrew
--
Thanks,
Raju
> One quick question.
> some of the options (ex. WAKE_PHY, WAKE_MAGIC etc) support on PHY and other
> options (ex. WAKE_UCAST, WAKE_MAGICSECURE etc) on MAC of Ethernet device.
>
> Suppose, user configure the combination (i.e. wol gu) option,
> Is PHY flag should hold combination option or only PHY supported option ?
I don't think it actually matters. The user is going to end up
invoking the PHY set wol. The PHY will setup what it can. On resume
you are going to call the PHYs set wol function again. It should do
the same as it did the first time. So if the PHY ignored WAKE_UCAST
and WAKE_MAGICSECURE the first time, it should also ignore them the
second time.
> Ok. I will change.
> May be in phy_init_hw( ) function is better place to re-config the WOL
You should first answer Russell question. It could be a totally
different scheme might come of of the discussion with Russell.
Andrew
On Mon, May 06, 2024 at 03:28:33AM +0200, Andrew Lunn wrote:
> You should first answer Russell question. It could be a totally
> different scheme might come of of the discussion with Russell.
Yes please. We definitely need to get to the bottom of what is going
on here _before_ we come up with a solution.
If we don't understand the problem, whatever fix gets proposed is
probably going to be nothing more than a bodge job - and then someone
else is going to have a problem and propose another bodge job and so
we'll end up with bodge job on top of more bodge jobs.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Russell King,
Sorry for late response
The 05/02/2024 16:59, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Thu, May 02, 2024 at 04:51:42PM +0200, Andrew Lunn wrote:
> > On Tue, Apr 30, 2024 at 10:36:35AM +0530, Raju Lakkaraju wrote:
> > > Introduce a new member named 'wolopts' to the 'phy_device' structure to
> > > store the user-specified Wake-on-LAN (WOL) settings. Update this member
> > > within the phy driver's 'set_wol()' function whenever the WOL configuration
> > > is modified by the user.
> > >
> > > Currently, when the system resumes from sleep, the 'phy_init_hw()' function
> > > resets the PHY's configuration and interrupts, which leads to problems upon
> > > subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
> > > we can ensure that the PHY's WOL configuration is correctly reapplied
> > > through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
> > > the issue
> >
> > Sorry it took a white to review this.
> >
> > >
> > > Signed-off-by: Raju Lakkaraju <[email protected]>
> > > ---
> > > drivers/net/phy/mxl-gpy.c | 5 +++++
> > > drivers/net/phy/phy_device.c | 5 +++++
> > > include/linux/phy.h | 2 ++
> > > 3 files changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> > > index b2d36a3a96f1..6edb29a1d77e 100644
> > > --- a/drivers/net/phy/mxl-gpy.c
> > > +++ b/drivers/net/phy/mxl-gpy.c
> > > @@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
> > > struct net_device *attach_dev = phydev->attached_dev;
> > > int ret;
> > >
> > > + phydev->wolopts = 0;
> >
> > Is this specific to mlx-gpy?
> >
> > You should be trying to solve the problem for all PHYs which support
> > WoL. So i expect the core to be doing most of the work. In fact, i
> > don't think there is any need for driver specific code.
>
> It would be good to hear exactly why its necessary for phylib to track
> this state,
Andrew suggested that if PHY device support WOL, Ethernet MAC device should be in
sleep expect interrrupt handling functionality.
To achive this, if we have phy device's wolopts, based on user configuration,
we can configure phy WOL or Ethernet MAC WOL.
PCI11x1x chip MAC can support WOL (i.e. WAKE_MAGIC, WAKE_SECURE, WAKE_UCAST,
WAKE_MCAST, WAKE_BCAST, WAKE_PHY)
GPY211C PHY connect as External PHY to PCI11x1x ethernet device to achive
2.5G/1G/100M/10Mpbs with either SGMII or 2500Base-X mode. GPY211C PHY can
support WOL (i.e. WAKE_PHY or WAKE_MAGIC).
Similarly, KSZ91931 PHY connect as External PHY to PCI11x1x ethernet device to
achive 1G/100M/10Mbps with RGMII or GMII mode. But, KSZ9131 PHY driver does
not support WOL.
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.
In existing code, we don't have any information about PHY's user configure to
configure the PM mode
> 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( )
> I suspect this may have something to do with resets - the PHY being
> hardware reset when coming out of resume (resulting in all state
> being lost.) What's resetting it would also be good to track down
> (as in hardware, firmware, or the kernel.)
>
In case of resume, the following sequence executes:
pci_pm_resume( )
--> lan743x_pm_resume()
--> lan743x_netdev_open( )
--> phy_connect_direct( )
--> phy_attach_direct( )
--> phy_init_hw( )
--> gpy_config_init( )
--> gpy_config_intr( )
In existing gpy_config_init( ) and gpy_config_intr( ) function don't have any
indication about whether execute in initial operation or WOL's resume
operation.
If we avoid clearing Interrupts in WOL's resume case, we need not re-configure
the WOL on PHY.
I add the following code and test. It's working as expected.
In File: drivers/net/phy/mxl-gpy.c
In gpy_config_init( ) function:
+ if (!phydev->wolopts) {
/* Mask all interrupts */
ret = phy_write(phydev, PHY_IMASK, 0);
if (ret)
return ret;
+ }
In gpy_config_intr( ) function:
+ if (phydev->wolopts)
+ mask |= (phy_read(phydev, PHY_IMASK) &
+ (PHY_IMASK_WOL | PHY_IMASK_LSTC));
+
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
Thanks,
Raju
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!
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 <[email protected]>
> Sent: Tuesday, May 7, 2024 7:54 AM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: Andrew Lunn <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; UNGLinuxDriver <[email protected]>
> 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!
On Tue, May 07, 2024 at 04:18:27PM +0000, [email protected] wrote:
> > 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 ?
Depends what the driver wants to do.
From the subset of drivers that implement WoL and use phylib:
drivers/net/ethernet/socionext/sni_ave.c:
ave_init()
ave_suspend() - to save the wolopts from the PHY
ave_resume() - to restore them to the PHY - so presumably
working around a buggy PHY driver, but no idea which
PHY driver because although it uses DT, there's no way
to know from DT which PHYs get used on this platform.
drivers/net/ethernet/ti/cpsw_ethtool.c:
does nothing more than forwarding the set_wol()/get_wol()
ethtool calls to phylib.
drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
enetc_set_wol() merely sets the device for wakeup as appropriate
based on the wolopts after passing it to phylib.
drivers/net/ethernet/microchip/lan743x_ethtool.c:
lan743x_ethtool_set_wol() tracks the wolopts *before* passing
to phylib, and enables the device for wakeup as appropriate.
The whole:
"return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
: -ENETDOWN;"
thing at the end is buggy. You're updating the adapters state
and the device wakeup enable, _then_ checking whether we have a
phydev. If we don't have a phydev, you're then telling userspace
that the request to change the WoL settings failed but you've
already changed your configuration!
Moreover, looking at lan743x_ethtool_get_wol(), you set
WAKE_MAGIC | WAKE_PHY irrespective of what the PHY actually
supports. This makes a total nonsense of the purpose of the
supported flags here.
I guess the _reason_ you do this is because the PHY may not be
present (because you look it up in the .ndo_open() method) and
thus you're trying to work around that... but then set_wol()
fails in that instance. This all seems crazy.
Broadcom bcmgenet also connects to the PHY in .ndo_open() and
has sane semantics for .get_wol/.set_wol. I'd suggest having
a look at that driver...
drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c:
bcmgenet_set_wol() saves the value of wolopts in its
private data structure and bcmgenet_wol_power_down_cfg()/
bcmgenet_wol_power_up_cfg() uses this to decide what to
power down/up.
Now, let's look at what ethtool already does for us when attempting
a set_wol() peration.
1. It retrieves the current WoL configuration from the netdev into
'wol'.
2. It modifies wol.wolopts according to the users request.
3. It validates that the options set in wol.wolopts do _not_ include
anything that is not reported as supported (in other words, no
bits are set that aren't in wol.supported.)
4. It deals with the WoL SecureOn™ password.
5. It calls the netdev to set the new configuration.
6. If successful, it updates the netdev's flag which indicates whether
WoL is currently enabled _in some form_ for this netdev.
Ergo, network device drivers are *not* supposed to report modes in
wol.supported that they do not support, and thus a network driver
can be assured that wol.wolopts will not contain any modes that are
not supported.
Therefore, if a network device wishes to track which WoL modes are
currently enabled, it can do this simply by:
1. calling phy_ethtool_set_wol(), and if that call is successful, then
2. save the value of wol.wolopts to its own private data structure to
determine what it needs to do at suspend/resume.
This should be independent of which modes the PHY supports, because the
etwork driver should be using phy_ethtool_get_wol() to retrieve the
modes which the PHY supports, and then augmenting the wol.supported
mask with whatever additional modes the network driver supports
beyond that.
So, there is no real need for a network driver to query phylib for the
current configuration except possibly during probe or when connecting
to its PHY.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Thanks for the detailed write-up Russell, much appreciated.
Yes, the lan743x wol handling is currently severely broken. Raju was in the process of fixing that (the patch I mentioned in my previous email that will call the phy driver first to give it priority) when he ran into issues with the mxl-gpy driver that led to this thread.
Understood that the mac driver should filter out any wol options it received through .set_wol that the mac supports but that the underlying phy does not support (as per what the phy driver reported earlier over its own .get_wol in the wol.supported field), prior to calling the Phy's set_wol (that is yet another bug that needs to be fixed in the current lan743x driver). That way the mac driver can then properly track (and save if it needs to) the currently enabled phy options and it own enabled wol options.
Hopefully, with the clarity you have provided on how this whole thing should be done, Raju will be submitting patches to fix both drivers soon.
Thanks again and regards,
Ronnie
> -----Original Message-----
> From: Russell King <[email protected]>
> Sent: Tuesday, May 7, 2024 1:57 PM
> To: Ronnie Kunin - C21729 <[email protected]>
> Cc: Raju Lakkaraju - I30499 <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> UNGLinuxDriver <[email protected]>
> 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 04:18:27PM +0000, [email protected] wrote:
> > > 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 ?
>
> Depends what the driver wants to do.
>
> From the subset of drivers that implement WoL and use phylib:
>
> drivers/net/ethernet/socionext/sni_ave.c:
> ave_init()
> ave_suspend() - to save the wolopts from the PHY
> ave_resume() - to restore them to the PHY - so presumably
> working around a buggy PHY driver, but no idea which
> PHY driver because although it uses DT, there's no way
> to know from DT which PHYs get used on this platform.
>
> drivers/net/ethernet/ti/cpsw_ethtool.c:
> does nothing more than forwarding the set_wol()/get_wol()
> ethtool calls to phylib.
>
> drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
> enetc_set_wol() merely sets the device for wakeup as appropriate
> based on the wolopts after passing it to phylib.
>
> drivers/net/ethernet/microchip/lan743x_ethtool.c:
> lan743x_ethtool_set_wol() tracks the wolopts *before* passing
> to phylib, and enables the device for wakeup as appropriate.
> The whole:
> "return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
> : -ENETDOWN;"
> thing at the end is buggy. You're updating the adapters state
> and the device wakeup enable, _then_ checking whether we have a
> phydev. If we don't have a phydev, you're then telling userspace
> that the request to change the WoL settings failed but you've
> already changed your configuration!
>
> Moreover, looking at lan743x_ethtool_get_wol(), you set
> WAKE_MAGIC | WAKE_PHY irrespective of what the PHY actually
> supports. This makes a total nonsense of the purpose of the
> supported flags here.
>
> I guess the _reason_ you do this is because the PHY may not be
> present (because you look it up in the .ndo_open() method) and
> thus you're trying to work around that... but then set_wol()
> fails in that instance. This all seems crazy.
>
> Broadcom bcmgenet also connects to the PHY in .ndo_open() and
> has sane semantics for .get_wol/.set_wol. I'd suggest having
> a look at that driver...
>
> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c:
> bcmgenet_set_wol() saves the value of wolopts in its
> private data structure and bcmgenet_wol_power_down_cfg()/
> bcmgenet_wol_power_up_cfg() uses this to decide what to
> power down/up.
>
> Now, let's look at what ethtool already does for us when attempting a set_wol() peration.
>
> 1. It retrieves the current WoL configuration from the netdev into
> 'wol'.
> 2. It modifies wol.wolopts according to the users request.
> 3. It validates that the options set in wol.wolopts do _not_ include
> anything that is not reported as supported (in other words, no
> bits are set that aren't in wol.supported.) 4. It deals with the WoL SecureOn™ password.
> 5. It calls the netdev to set the new configuration.
> 6. If successful, it updates the netdev's flag which indicates whether
> WoL is currently enabled _in some form_ for this netdev.
>
> Ergo, network device drivers are *not* supposed to report modes in wol.supported that they do not
> support, and thus a network driver can be assured that wol.wolopts will not contain any modes that are
> not supported.
>
> Therefore, if a network device wishes to track which WoL modes are currently enabled, it can do this
> simply by:
>
> 1. calling phy_ethtool_set_wol(), and if that call is successful, then 2. save the value of wol.wolopts to
> its own private data structure to
> determine what it needs to do at suspend/resume.
>
> This should be independent of which modes the PHY supports, because the etwork driver should be
> using phy_ethtool_get_wol() to retrieve the modes which the PHY supports, and then augmenting the
> wol.supported mask with whatever additional modes the network driver supports beyond that.
>
> So, there is no real need for a network driver to query phylib for the current configuration except
> possibly during probe or when connecting to its PHY.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!