This patch series implement the following fixes:
1. Disable WOL upon resume in order to restore full data path operation
2. Support WOL in MAC even when PHY does not
3. Address problems with wake option flags configuration sequences
Raju Lakkaraju (3):
net: lan743x: disable WOL upon resume to restore full data path
operation
net: lan743x: support WOL in MAC even when PHY does not
net: lan743x: Address problems with wake option flags configuration
sequences
.../net/ethernet/microchip/lan743x_ethtool.c | 24 +++++++++++++++++--
drivers/net/ethernet/microchip/lan743x_main.c | 24 ++++++++++++++++++-
drivers/net/ethernet/microchip/lan743x_main.h | 24 +++++++++++++++++++
3 files changed, 69 insertions(+), 3 deletions(-)
--
2.34.1
Wake options handling has been reworked as follows:
a. We only enable secure on magic packet when both secure and magic wol
options are requested together.
b. If secure-on magic packet had been previously enabled, and a subsequent
command does not include it, we add it. This was done to workaround a
problem with the 'pm-suspend' application which is unaware of secure-on
magic packet being enabled and can unintentionally disable it prior to
putting the system into suspend.
Fixes: 6b3768ac8e2b3 ("net: lan743x: Add support to Secure-ON WOL")
Signed-off-by: Raju Lakkaraju <[email protected]>
---
drivers/net/ethernet/microchip/lan743x_ethtool.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 4899582b3d1d..cda4df2ac1cd 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1165,6 +1165,16 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
struct lan743x_adapter *adapter = netdev_priv(netdev);
int ret;
+ if (wol->wolopts && wol->wolopts != adapter->wolopts &&
+ adapter->wolopts & WAKE_MAGICSECURE) {
+ wol->wolopts |= WAKE_MAGICSECURE;
+ netif_warn(adapter, drv, adapter->netdev,
+ "Ensure secure-on magic packet remains enabled if not explicitly disabled\n");
+ }
+
+ if ((wol->wolopts & WAKE_MAGICSECURE) && !(wol->wolopts & WAKE_MAGIC))
+ return -EINVAL;
+
if (netdev->phydev) {
ret = phy_ethtool_set_wol(netdev->phydev, wol);
if (ret != -EOPNOTSUPP && ret != 0)
--
2.34.1
Allow WOL support if MAC supports it, even if the PHY does not support it
Fixes: e9e13b6adc338 ("lan743x: fix for potential NULL pointer dereference
with bare card")
Signed-off-by: Raju Lakkaraju <[email protected]>
---
drivers/net/ethernet/microchip/lan743x_ethtool.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index a2b3f4433ca8..4899582b3d1d 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1163,6 +1163,17 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
struct ethtool_wolinfo *wol)
{
struct lan743x_adapter *adapter = netdev_priv(netdev);
+ int ret;
+
+ if (netdev->phydev) {
+ ret = phy_ethtool_set_wol(netdev->phydev, wol);
+ if (ret != -EOPNOTSUPP && ret != 0)
+ return ret;
+
+ if (ret == -EOPNOTSUPP)
+ netif_info(adapter, drv, adapter->netdev,
+ "phy does not support WOL\n");
+ }
adapter->wolopts = 0;
if (wol->wolopts & WAKE_UCAST)
@@ -1187,8 +1198,7 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
- return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
- : -ENETDOWN;
+ return 0;
}
#endif /* CONFIG_PM */
--
2.34.1
The 02/26/2024 13:39, Raju Lakkaraju wrote:
> Allow WOL support if MAC supports it, even if the PHY does not support it
>
> Fixes: e9e13b6adc338 ("lan743x: fix for potential NULL pointer dereference
> with bare card")
>
Please no spaces between the tags. And you should not split on multiple
line Fixes tag.
> Signed-off-by: Raju Lakkaraju <[email protected]>
> ---
> drivers/net/ethernet/microchip/lan743x_ethtool.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> index a2b3f4433ca8..4899582b3d1d 100644
> --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -1163,6 +1163,17 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
> struct ethtool_wolinfo *wol)
> {
> struct lan743x_adapter *adapter = netdev_priv(netdev);
> + int ret;
> +
> + if (netdev->phydev) {
> + ret = phy_ethtool_set_wol(netdev->phydev, wol);
> + if (ret != -EOPNOTSUPP && ret != 0)
> + return ret;
> +
> + if (ret == -EOPNOTSUPP)
> + netif_info(adapter, drv, adapter->netdev,
> + "phy does not support WOL\n");
> + }
>
> adapter->wolopts = 0;
> if (wol->wolopts & WAKE_UCAST)
> @@ -1187,8 +1198,7 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
>
> device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
>
> - return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
> - : -ENETDOWN;
> + return 0;
> }
> #endif /* CONFIG_PM */
>
> --
> 2.34.1
>
--
/Horatiu
> > + if (ret == -EOPNOTSUPP)
> > + netif_info(adapter, drv, adapter->netdev,
> > + "phy does not support WOL\n");
netdev_dbg(). We don't really care who is doing WoL, if its the MAC,
the PHY, or a bit of both. So there is no need to spam the log with
this.
On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> Wake options handling has been reworked as follows:
> a. We only enable secure on magic packet when both secure and magic wol
> options are requested together.
> b. If secure-on magic packet had been previously enabled, and a subsequent
> command does not include it, we add it. This was done to workaround a
> problem with the 'pm-suspend' application which is unaware of secure-on
> magic packet being enabled and can unintentionally disable it prior to
> putting the system into suspend.
This seems to be in a bit of a grey area. But ethtool says:
wol p|u|m|b|a|g|s|f|d...
Sets Wake-on-LAN options. Not all devices support this.
The argument to this option is a string of characters speci‐
fying which options to enable.
p Wake on PHY activity
u Wake on unicast messages
m Wake on multicast messages
b Wake on broadcast messages
a Wake on ARP
g Wake on MagicPacket™
s Enable SecureOn™ password for MagicPacket™
f Wake on filter(s)
d Disable (wake on nothing). This option
clears all previous options.
d clears everything. All other things enable one option. There does
not appear to be a way to disable a single option, and i would say,
adding options is incremental.
So:
> a. We only enable secure on magic packet when both secure and magic wol
> options are requested together.
I don't think they need to be requested together. I think you can
first enable Wake on MagicPacket and then in a second call to ethtool
Enable SecureOn password for MagicPacket. I also don't think it would
unreasonable to accept Enable SecureOn password for MagicPacket and
have that imply Wake on MagicPacket.
And:
> b. If secure-on magic packet had been previously enabled, and a subsequent
> command does not include it, we add it.
If there has not been a d, secure-on magic packet should remain
enabled until there is a d.
Andrew
Hi Horatiu,
> -----Original Message-----
> From: Horatiu Vultur - M31836 <[email protected]>
> Sent: Monday, February 26, 2024 1:59 PM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Bryan Whitehead - C21958
> <[email protected]>; [email protected];
> UNGLinuxDriver <[email protected]>
> Subject: Re: [PATCH net 2/3] net: lan743x: support WOL in MAC even when
> PHY does not
>
> The 02/26/2024 13:39, Raju Lakkaraju wrote:
> > Allow WOL support if MAC supports it, even if the PHY does not support
> > it
> >
> > Fixes: e9e13b6adc338 ("lan743x: fix for potential NULL pointer
> > dereference with bare card")
> >
> Please no spaces between the tags. And you should not split on multiple line
> Fixes tag.
>
I will fix this issue.
> > Signed-off-by: Raju Lakkaraju <[email protected]>
> > ---
> > drivers/net/ethernet/microchip/lan743x_ethtool.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > index a2b3f4433ca8..4899582b3d1d 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > @@ -1163,6 +1163,17 @@ static int lan743x_ethtool_set_wol(struct
> net_device *netdev,
> > struct ethtool_wolinfo *wol)
> > {
> > struct lan743x_adapter *adapter = netdev_priv(netdev);
> > + int ret;
> > +
> > + if (netdev->phydev) {
> > + ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > + if (ret != -EOPNOTSUPP && ret != 0)
> > + return ret;
> > +
> > + if (ret == -EOPNOTSUPP)
> > + netif_info(adapter, drv, adapter->netdev,
> > + "phy does not support WOL\n");
> > + }
> >
> > adapter->wolopts = 0;
> > if (wol->wolopts & WAKE_UCAST)
> > @@ -1187,8 +1198,7 @@ static int lan743x_ethtool_set_wol(struct
> > net_device *netdev,
> >
> > device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol-
> >wolopts);
> >
> > - return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
> > - : -ENETDOWN;
> > + return 0;
> > }
> > #endif /* CONFIG_PM */
> >
> > --
> > 2.34.1
> >
>
> --
> /Horatiu
Thanks,
Raju
Hi Andrew,
Thank you for review comments.
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Tuesday, February 27, 2024 7:28 AM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Bryan Whitehead - C21958
> <[email protected]>; [email protected];
> UNGLinuxDriver <[email protected]>
> Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option
> flags configuration sequences
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> > Wake options handling has been reworked as follows:
> > a. We only enable secure on magic packet when both secure and magic wol
> > options are requested together.
> > b. If secure-on magic packet had been previously enabled, and a
> subsequent
> > command does not include it, we add it. This was done to workaround a
> > problem with the 'pm-suspend' application which is unaware of secure-on
> > magic packet being enabled and can unintentionally disable it prior to
> > putting the system into suspend.
>
> This seems to be in a bit of a grey area. But ethtool says:
>
> wol p|u|m|b|a|g|s|f|d...
> Sets Wake-on-LAN options. Not all devices support this.
> The argument to this option is a string of characters speci‐
> fying which options to enable.
> p Wake on PHY activity
> u Wake on unicast messages
> m Wake on multicast messages
> b Wake on broadcast messages
> a Wake on ARP
> g Wake on MagicPacket™
> s Enable SecureOn™ password for MagicPacket™
> f Wake on filter(s)
> d Disable (wake on nothing). This option
> clears all previous options.
>
> d clears everything. All other things enable one option. There does not
> appear to be a way to disable a single option, and i would say, adding options
> is incremental.
>
Yes. "d" clears everything.
But, if we enable "g" then enable "a", "g" option overwritten by "a"
Please find the attached log information
> So:
>
> > a. We only enable secure on magic packet when both secure and magic wol
> > options are requested together.
>
> I don't think they need to be requested together. I think you can first enable
> Wake on MagicPacket and then in a second call to ethtool Enable SecureOn
> password for MagicPacket. I also don't think it would unreasonable to accept
> Enable SecureOn password for MagicPacket and have that imply Wake on
> MagicPacket.
>
If we need to enable any 2 options, we have to provide both options together.
i.e.
sudo ethtool -s enp9s0 wol ga
> And:
>
> > b. If secure-on magic packet had been previously enabled, and a
> subsequent
> > command does not include it, we add it.
>
> If there has not been a d, secure-on magic packet should remain enabled until
> there is a d.
>
This is not happened with existing "Ethtool".
Please find the log information in an attachment file.
> Andrew
Thanks,
Raju
On Thu, Feb 29, 2024 at 08:59:20AM +0000, [email protected] wrote:
> Hi Andrew,
>
> Thank you for review comments.
>
> > -----Original Message-----
> > From: Andrew Lunn <[email protected]>
> > Sent: Tuesday, February 27, 2024 7:28 AM
> > To: Raju Lakkaraju - I30499 <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected]; linux-
> > [email protected]; Bryan Whitehead - C21958
> > <[email protected]>; [email protected];
> > UNGLinuxDriver <[email protected]>
> > Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option
> > flags configuration sequences
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> > > Wake options handling has been reworked as follows:
> > > a. We only enable secure on magic packet when both secure and magic wol
> > > options are requested together.
> > > b. If secure-on magic packet had been previously enabled, and a
> > subsequent
> > > command does not include it, we add it. This was done to workaround a
> > > problem with the 'pm-suspend' application which is unaware of secure-on
> > > magic packet being enabled and can unintentionally disable it prior to
> > > putting the system into suspend.
> >
> > This seems to be in a bit of a grey area. But ethtool says:
> >
> > wol p|u|m|b|a|g|s|f|d...
> > Sets Wake-on-LAN options. Not all devices support this.
> > The argument to this option is a string of characters speci‐
> > fying which options to enable.
> > p Wake on PHY activity
> > u Wake on unicast messages
> > m Wake on multicast messages
> > b Wake on broadcast messages
> > a Wake on ARP
> > g Wake on MagicPacket™
> > s Enable SecureOn™ password for MagicPacket™
> > f Wake on filter(s)
> > d Disable (wake on nothing). This option
> > clears all previous options.
> >
> > d clears everything. All other things enable one option. There does not
> > appear to be a way to disable a single option, and i would say, adding options
> > is incremental.
> >
>
> Yes. "d" clears everything.
> But, if we enable "g" then enable "a", "g" option overwritten by "a"
This is where i say it is a bit of a grey area. For me, they are
incremental. You can enable a and then later enable g, and you should
have both enabled.
> Please find the attached log information
> > So:
> >
> > > a. We only enable secure on magic packet when both secure and magic wol
> > > options are requested together.
> >
> > I don't think they need to be requested together. I think you can first enable
> > Wake on MagicPacket and then in a second call to ethtool Enable SecureOn
> > password for MagicPacket. I also don't think it would unreasonable to accept
> > Enable SecureOn password for MagicPacket and have that imply Wake on
> > MagicPacket.
> >
>
> If we need to enable any 2 options, we have to provide both options together.
> i.e.
> sudo ethtool -s enp9s0 wol ga
which i think is wrong. A driver should allow incremental adding WoL
options.
>
> > And:
> >
> > > b. If secure-on magic packet had been previously enabled, and a
> > subsequent
> > > command does not include it, we add it.
> >
> > If there has not been a d, secure-on magic packet should remain enabled until
> > there is a d.
> >
>
> This is not happened with existing "Ethtool".
> Please find the log information in an attachment file.
That could just be a driver bug.
Take a step back. Is there any clear documentation about how ethtool
-s wol should really work? Any comments in the code? Any man page
documentation.
Lets first understand how it is expected to work. Then we can decided
if the driver is implementing it correctly.
Andrew
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Thursday, February 29, 2024 8:36 PM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Bryan Whitehead - C21958
> <[email protected]>; [email protected];
> UNGLinuxDriver <[email protected]>
> Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option
> flags configuration sequences
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Thu, Feb 29, 2024 at 08:59:20AM +0000, [email protected]
> wrote:
> > Hi Andrew,
> >
> > Thank you for review comments.
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <[email protected]>
> > > Sent: Tuesday, February 27, 2024 7:28 AM
> > > To: Raju Lakkaraju - I30499 <[email protected]>
> > > Cc: [email protected]; [email protected]; [email protected];
> > > linux- [email protected]; Bryan Whitehead - C21958
> > > <[email protected]>; [email protected];
> > > UNGLinuxDriver <[email protected]>
> > > Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with
> > > wake option flags configuration sequences
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> > > > Wake options handling has been reworked as follows:
> > > > a. We only enable secure on magic packet when both secure and magic
> wol
> > > > options are requested together.
> > > > b. If secure-on magic packet had been previously enabled, and a
> > > subsequent
> > > > command does not include it, we add it. This was done to workaround
> a
> > > > problem with the 'pm-suspend' application which is unaware of
> secure-on
> > > > magic packet being enabled and can unintentionally disable it prior to
> > > > putting the system into suspend.
> > >
> > > This seems to be in a bit of a grey area. But ethtool says:
> > >
> > > wol p|u|m|b|a|g|s|f|d...
> > > Sets Wake-on-LAN options. Not all devices support this.
> > > The argument to this option is a string of characters speci‐
> > > fying which options to enable.
> > > p Wake on PHY activity
> > > u Wake on unicast messages
> > > m Wake on multicast messages
> > > b Wake on broadcast messages
> > > a Wake on ARP
> > > g Wake on MagicPacket™
> > > s Enable SecureOn™ password for MagicPacket™
> > > f Wake on filter(s)
> > > d Disable (wake on nothing). This option
> > > clears all previous options.
> > >
> > > d clears everything. All other things enable one option. There does
> > > not appear to be a way to disable a single option, and i would say,
> > > adding options is incremental.
> > >
> >
> > Yes. "d" clears everything.
> > But, if we enable "g" then enable "a", "g" option overwritten by "a"
>
> This is where i say it is a bit of a grey area. For me, they are incremental. You
> can enable a and then later enable g, and you should have both enabled.
>
Yes. But, it's "Ethtool" issue.
Even though ethtool get the WOL configuration before to set, over written with wanted wol request configuration.
> > Please find the attached log information
> > > So:
> > >
> > > > a. We only enable secure on magic packet when both secure and magic
> wol
> > > > options are requested together.
> > >
> > > I don't think they need to be requested together. I think you can
> > > first enable Wake on MagicPacket and then in a second call to
> > > ethtool Enable SecureOn password for MagicPacket. I also don't think
> > > it would unreasonable to accept Enable SecureOn password for
> > > MagicPacket and have that imply Wake on MagicPacket.
> > >
> >
> > If we need to enable any 2 options, we have to provide both options
> together.
> > i.e.
> > sudo ethtool -s enp9s0 wol ga
>
> which i think is wrong. A driver should allow incremental adding WoL options.
>
Ok.
> >
> > > And:
> > >
> > > > b. If secure-on magic packet had been previously enabled, and a
> > > subsequent
> > > > command does not include it, we add it.
> > >
> > > If there has not been a d, secure-on magic packet should remain
> > > enabled until there is a d.
> > >
> >
> > This is not happened with existing "Ethtool".
> > Please find the log information in an attachment file.
>
> That could just be a driver bug.
>
> Take a step back. Is there any clear documentation about how ethtool -s wol
> should really work? Any comments in the code? Any man page
> documentation.
>
I'm not able to find any more "ethtool" documentations other than ethtool man page.
I have gone through the ethtool application code.
(git://git.kernel.org/pub/scm/network/ethtool/ethtool.git) and check the WOL options.
In ioctl method (Not netlink), do_sset( ) function, before set the parameters, first get the WOL configuration.
(i.e. in ethtool.c file, Line no. 3294)
Then, assign/overwrite WOL wanted config to wolopts parameter.
Existing Code: From Line 3290 - 3312
#######################################################################
3290 if (gwol_changed) {
3291 struct ethtool_wolinfo wol;
3292
3293 wol.cmd = ETHTOOL_GWOL;
3294 err = send_ioctl(ctx, &wol);
3295 if (err < 0) {
3296 perror("Cannot get current wake-on-lan settings");
3297 } else {
3298 /* Change everything the user specified. */
3299 if (wol_change)
3300 wol.wolopts = wol_wanted;
3301 if (sopass_change) {
3302 int i;
3303 for (i = 0; i < SOPASS_MAX; i++)
3304 wol.sopass[i] = sopass_wanted[i];
3305 }
3306
3307 /* Try to perform the update. */
3308 wol.cmd = ETHTOOL_SWOL;
3309 err = send_ioctl(ctx, &wol);
3310 if (err < 0)
3311 perror("Cannot set new wake-on-lan settings");
3312 }
#######################################################################
Current issue is overwriting wolopts with wol_wanted
i.e.
3300 wol.wolopts = wol_wanted;
Instead of over write, Need to "OR" the wolopts and wol_wanted
i.e.
3300 wol.wolopts |= wol_wanted
Final code changes should be:
#######################################################################
3290 if (gwol_changed) {
3291 struct ethtool_wolinfo wol;
3292
3293 wol.cmd = ETHTOOL_GWOL;
3294 err = send_ioctl(ctx, &wol);
3295 if (err < 0) {
3296 perror("Cannot get current wake-on-lan settings");
3297 } else {
3298 /* Change everything the user specified. */
3299 if (wol_change && wol_wanted)
3300 wol.wolopts |= wol_wanted;
else if (wol_change && !wol_wanted)
wol.wolopts = 0
3301 if (sopass_change) {
3302 int i;
3303 for (i = 0; i < SOPASS_MAX; i++)
3304 wol.sopass[i] = sopass_wanted[i];
3305 }
3306
3307 /* Try to perform the update. */
3308 wol.cmd = ETHTOOL_SWOL;
3309 err = send_ioctl(ctx, &wol);
3310 if (err < 0)
3311 perror("Cannot set new wake-on-lan settings");
3312 }
#######################################################################
Similar changes require in netlink functions also.
> Lets first understand how it is expected to work. Then we can decided if the
> driver is implementing it correctly.
>
> Andrew
Please find the "ethtool" application code main file as attachment file.
Thanks,
Raju
Hi Andrew,
For Ethtool netlink, we need to change the "ethnl_set_wol( )" to add option is incremental.
Can you please check those changes OK or not.
Thanks,
Raju.
> -----Original Message-----
> From: Raju Lakkaraju - I30499
> Sent: Friday, March 1, 2024 3:52 PM
> To: Andrew Lunn <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Bryan Whitehead - C21958
> <[email protected]>; [email protected];
> UNGLinuxDriver <[email protected]>
> Subject: RE: [PATCH net 3/3] net: lan743x: Address problems with wake option
> flags configuration sequences
>
> Hi Andrew,
>
> > -----Original Message-----
> > From: Andrew Lunn <[email protected]>
> > Sent: Thursday, February 29, 2024 8:36 PM
> > To: Raju Lakkaraju - I30499 <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > linux- [email protected]; Bryan Whitehead - C21958
> > <[email protected]>; [email protected];
> > UNGLinuxDriver <[email protected]>
> > Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake
> > option flags configuration sequences
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > On Thu, Feb 29, 2024 at 08:59:20AM +0000, [email protected]
> > wrote:
> > > Hi Andrew,
> > >
> > > Thank you for review comments.
> > >
> > > > -----Original Message-----
> > > > From: Andrew Lunn <[email protected]>
> > > > Sent: Tuesday, February 27, 2024 7:28 AM
> > > > To: Raju Lakkaraju - I30499 <[email protected]>
> > > > Cc: [email protected]; [email protected]; [email protected];
> > > > linux- [email protected]; Bryan Whitehead - C21958
> > > > <[email protected]>; [email protected];
> > > > UNGLinuxDriver <[email protected]>
> > > > Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with
> > > > wake option flags configuration sequences
> > > >
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > know the content is safe
> > > >
> > > > On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> > > > > Wake options handling has been reworked as follows:
> > > > > a. We only enable secure on magic packet when both secure and
> > > > > magic
> > wol
> > > > > options are requested together.
> > > > > b. If secure-on magic packet had been previously enabled, and a
> > > > subsequent
> > > > > command does not include it, we add it. This was done to
> > > > > workaround
> > a
> > > > > problem with the 'pm-suspend' application which is unaware of
> > secure-on
> > > > > magic packet being enabled and can unintentionally disable it prior
> to
> > > > > putting the system into suspend.
> > > >
> > > > This seems to be in a bit of a grey area. But ethtool says:
> > > >
> > > > wol p|u|m|b|a|g|s|f|d...
> > > > Sets Wake-on-LAN options. Not all devices support this.
> > > > The argument to this option is a string of characters speci‐
> > > > fying which options to enable.
> > > > p Wake on PHY activity
> > > > u Wake on unicast messages
> > > > m Wake on multicast messages
> > > > b Wake on broadcast messages
> > > > a Wake on ARP
> > > > g Wake on MagicPacket™
> > > > s Enable SecureOn™ password for MagicPacket™
> > > > f Wake on filter(s)
> > > > d Disable (wake on nothing). This option
> > > > clears all previous options.
> > > >
> > > > d clears everything. All other things enable one option. There
> > > > does not appear to be a way to disable a single option, and i
> > > > would say, adding options is incremental.
> > > >
> > >
> > > Yes. "d" clears everything.
> > > But, if we enable "g" then enable "a", "g" option overwritten by "a"
> >
> > This is where i say it is a bit of a grey area. For me, they are
> > incremental. You can enable a and then later enable g, and you should have
> both enabled.
> >
>
> Yes. But, it's "Ethtool" issue.
> Even though ethtool get the WOL configuration before to set, over written
> with wanted wol request configuration.
>
> > > Please find the attached log information
> > > > So:
> > > >
> > > > > a. We only enable secure on magic packet when both secure and
> > > > > magic
> > wol
> > > > > options are requested together.
> > > >
> > > > I don't think they need to be requested together. I think you can
> > > > first enable Wake on MagicPacket and then in a second call to
> > > > ethtool Enable SecureOn password for MagicPacket. I also don't
> > > > think it would unreasonable to accept Enable SecureOn password for
> > > > MagicPacket and have that imply Wake on MagicPacket.
> > > >
> > >
> > > If we need to enable any 2 options, we have to provide both options
> > together.
> > > i.e.
> > > sudo ethtool -s enp9s0 wol ga
> >
> > which i think is wrong. A driver should allow incremental adding WoL
> options.
> >
>
> Ok.
>
> > >
> > > > And:
> > > >
> > > > > b. If secure-on magic packet had been previously enabled, and a
> > > > subsequent
> > > > > command does not include it, we add it.
> > > >
> > > > If there has not been a d, secure-on magic packet should remain
> > > > enabled until there is a d.
> > > >
> > >
> > > This is not happened with existing "Ethtool".
> > > Please find the log information in an attachment file.
> >
> > That could just be a driver bug.
> >
> > Take a step back. Is there any clear documentation about how ethtool
> > -s wol should really work? Any comments in the code? Any man page
> > documentation.
> >
>
> I'm not able to find any more "ethtool" documentations other than ethtool
> man page.
>
> I have gone through the ethtool application code.
> (git://git.kernel.org/pub/scm/network/ethtool/ethtool.git) and check the WOL
> options.
>
> In ioctl method (Not netlink), do_sset( ) function, before set the parameters,
> first get the WOL configuration.
> (i.e. in ethtool.c file, Line no. 3294)
>
> Then, assign/overwrite WOL wanted config to wolopts parameter.
>
> Existing Code: From Line 3290 - 3312
> ################################################################
> #######
> 3290 if (gwol_changed) {
> 3291 struct ethtool_wolinfo wol;
> 3292
> 3293 wol.cmd = ETHTOOL_GWOL;
> 3294 err = send_ioctl(ctx, &wol);
> 3295 if (err < 0) {
> 3296 perror("Cannot get current wake-on-lan settings");
> 3297 } else {
> 3298 /* Change everything the user specified. */
> 3299 if (wol_change)
> 3300 wol.wolopts = wol_wanted;
> 3301 if (sopass_change) {
> 3302 int i;
> 3303 for (i = 0; i < SOPASS_MAX; i++)
> 3304 wol.sopass[i] = sopass_wanted[i];
> 3305 }
> 3306
> 3307 /* Try to perform the update. */
> 3308 wol.cmd = ETHTOOL_SWOL;
> 3309 err = send_ioctl(ctx, &wol);
> 3310 if (err < 0)
> 3311 perror("Cannot set new wake-on-lan settings");
> 3312 }
> ################################################################
> #######
> Current issue is overwriting wolopts with wol_wanted i.e.
> 3300 wol.wolopts = wol_wanted;
>
> Instead of over write, Need to "OR" the wolopts and wol_wanted i.e.
> 3300 wol.wolopts |= wol_wanted
>
> Final code changes should be:
> ################################################################
> #######
> 3290 if (gwol_changed) {
> 3291 struct ethtool_wolinfo wol;
> 3292
> 3293 wol.cmd = ETHTOOL_GWOL;
> 3294 err = send_ioctl(ctx, &wol);
> 3295 if (err < 0) {
> 3296 perror("Cannot get current wake-on-lan settings");
> 3297 } else {
> 3298 /* Change everything the user specified. */
> 3299 if (wol_change && wol_wanted)
> 3300 wol.wolopts |= wol_wanted;
> else if (wol_change && !wol_wanted)
> wol.wolopts = 0
> 3301 if (sopass_change) {
> 3302 int i;
> 3303 for (i = 0; i < SOPASS_MAX; i++)
> 3304 wol.sopass[i] = sopass_wanted[i];
> 3305 }
> 3306
> 3307 /* Try to perform the update. */
> 3308 wol.cmd = ETHTOOL_SWOL;
> 3309 err = send_ioctl(ctx, &wol);
> 3310 if (err < 0)
> 3311 perror("Cannot set new wake-on-lan settings");
> 3312 }
> ################################################################
> #######
>
> Similar changes require in netlink functions also.
>
For Netlink:
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -107,16 +107,26 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
struct net_device *dev = req_info->dev;
struct nlattr **tb = info->attrs;
+ __u32 wolopts = 0;
bool mod = false;
int ret;
dev->ethtool_ops->get_wol(dev, &wol);
- ret = ethnl_update_bitset32(&wol.wolopts, WOL_MODE_COUNT,
+ ret = ethnl_update_bitset32(&wolopts, WOL_MODE_COUNT,
tb[ETHTOOL_A_WOL_MODES], wol_mode_names,
info->extack, &mod);
if (ret < 0)
return ret;
- if (wol.wolopts & ~wol.supported) {
+
+ if (wolopts) {
+ wol.wolopts |= wolopts;
+ } else {
+ wol.wolopts = 0;
+ memset(&wol.sopass, 0, sizeof(wol.sopass));
+ mod = true;
+ }
+
+ if (wolopts & ~wol.supported) {
NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_WOL_MODES],
"cannot enable unsupported WoL mode");
return -EINVAL;
###############################################################
Please find the an attachment file.
> > Lets first understand how it is expected to work. Then we can decided
> > if the driver is implementing it correctly.
> >
> > Andrew
>
> Please find the "ethtool" application code main file as attachment file.
>
> Thanks,
> Raju
> I'm not able to find any more "ethtool" documentations other than ethtool man page.
>
> I have gone through the ethtool application code.
> (git://git.kernel.org/pub/scm/network/ethtool/ethtool.git) and check the WOL options.
>
> In ioctl method (Not netlink), do_sset( ) function, before set the parameters, first get the WOL configuration.
> (i.e. in ethtool.c file, Line no. 3294)
>
> Then, assign/overwrite WOL wanted config to wolopts parameter.
Thanks for documenting all this. So it does seem like wol options
overwrite, rather than add incrementally. So i would say the correct
'fix' here is to document that in the man page, because at the moment
it is not clear what should happen.
Now i need to go back and look at what your original issue was...
Andrew
On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> Wake options handling has been reworked as follows:
> a. We only enable secure on magic packet when both secure and magic wol
> options are requested together.
So it appears unclear what should happen here.
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/bcm-phy-lib.c#L909
WAKE_MAGICSECURE is a standalone option. You do not need
WAKE_MAGIC. And even i you did request both WAKE_MAGIC and
WAKE_MAGICSECURE, the WAKE_MAGIC would be ignored.
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/dp83822.c#L153
WAKE_MAGICSECURE is a standalone option. You do not need
WAKE_MAGIC. However, unlike the broadcom device, you can have both
WAKE_MAGIC and WAKE_MAGICSECURE at the same time. They are not
mutually exclusive.
This also looks to be true for other dp8**** devices.
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/mscc/mscc_main.c#L318
WAKE_MAGICSECURE is a standalone option. You do not need
WAKE_MAGIC. Also, you can have both WAKE_MAGIC and WAKE_MAGICSECURE at
the same time. They are not mutually exclusive.
So i think your point a. above is questionable. Can the hardware
support both magic and secure magic at the same time? If so, follow
the TI way of doing it. If you cannot do both at the same time, and
that is requested, you should probably return -EOPNOTSUPP. That is
probably better than what the broadcom driver does, silently ignore
WAKE_MAGIC.
> b. If secure-on magic packet had been previously enabled, and a subsequent
> command does not include it, we add it. This was done to workaround a
> problem with the 'pm-suspend' application which is unaware of secure-on
> magic packet being enabled and can unintentionally disable it prior to
> putting the system into suspend.
The kernel should not be working around broken userspace. But i also
suspect this is to do with it being unclear if WOL options are
incremental or not. Since it seems that they are not incremental, it
does not matter if "If secure-on magic packet had been previously
enable". pm-suspend is setting Wol how it wants it, which you say is
plain magic. So magic is what the PHY driver should do. Feel free to
submit patches to pm-suspend to make it understand secure magic, or
not touch WoL at all with the assumption it has already been setup by
something else.
Andrew
Hi Andrew,
Thank you for valuable information.
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Thursday, March 7, 2024 5:23 AM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Bryan Whitehead - C21958
> <[email protected]>; [email protected];
> UNGLinuxDriver <[email protected]>
> Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option
> flags configuration sequences
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> > Wake options handling has been reworked as follows:
> > a. We only enable secure on magic packet when both secure and magic wol
> > options are requested together.
>
> So it appears unclear what should happen here.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/bcm-phy-
> lib.c#L909
>
> WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> And even i you did request both WAKE_MAGIC and WAKE_MAGICSECURE,
> the WAKE_MAGIC would be ignored.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/dp83822.c#L153
>
> WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> However, unlike the broadcom device, you can have both WAKE_MAGIC and
> WAKE_MAGICSECURE at the same time. They are not mutually exclusive.
>
> This also looks to be true for other dp8**** devices.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/mscc/mscc_mai
> n.c#L318
>
> WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> Also, you can have both WAKE_MAGIC and WAKE_MAGICSECURE at the same
> time. They are not mutually exclusive.
>
> So i think your point a. above is questionable. Can the hardware support both
> magic and secure magic at the same time? If so, follow the TI way of doing it.
Yes. I will do.
> If you cannot do both at the same time, and that is requested, you should
> probably return -EOPNOTSUPP. That is probably better than what the
> broadcom driver does, silently ignore WAKE_MAGIC.
>
> > b. If secure-on magic packet had been previously enabled, and a
> subsequent
> > command does not include it, we add it. This was done to workaround a
> > problem with the 'pm-suspend' application which is unaware of secure-on
> > magic packet being enabled and can unintentionally disable it prior to
> > putting the system into suspend.
>
Ok. I will try to fix in 'pm-suspend' application
> The kernel should not be working around broken userspace. But i also
> suspect this is to do with it being unclear if WOL options are incremental or
> not. Since it seems that they are not incremental, it does not matter if "If
> secure-on magic packet had been previously enable". pm-suspend is setting
> Wol how it wants it, which you say is plain magic. So magic is what the PHY
> driver should do. Feel free to submit patches to pm-suspend to make it
> understand secure magic, or not touch WoL at all with the assumption it has
> already been setup by something else.
>
> Andrew
Thanks,
Raju
Hi Andrew
> -----Original Message-----
> From: Raju Lakkaraju - I30499 <[email protected]>
> Sent: Friday, March 8, 2024 3:21 AM
> To: Andrew Lunn <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; [email protected];
> Bryan Whitehead - C21958 <[email protected]>; [email protected];
> UNGLinuxDriver <[email protected]>
> Subject: RE: [PATCH net 3/3] net: lan743x: Address problems with wake option flags configuration
> sequences
>
> Hi Andrew,
>
> Thank you for valuable information.
>
> > -----Original Message-----
> > From: Andrew Lunn <[email protected]>
> > Sent: Thursday, March 7, 2024 5:23 AM
> > To: Raju Lakkaraju - I30499 <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > linux- [email protected]; Bryan Whitehead - C21958
> > <[email protected]>; [email protected];
> > UNGLinuxDriver <[email protected]>
> > Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake
> > option flags configuration sequences
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> > > Wake options handling has been reworked as follows:
> > > a. We only enable secure on magic packet when both secure and magic wol
> > > options are requested together.
> >
> > So it appears unclear what should happen here.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/bcm-phy
> > -
> > lib.c#L909
> >
> > WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> > And even i you did request both WAKE_MAGIC and WAKE_MAGICSECURE, the
> > WAKE_MAGIC would be ignored.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/dp83822
> > .c#L153
> >
> > WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> > However, unlike the broadcom device, you can have both WAKE_MAGIC and
> > WAKE_MAGICSECURE at the same time. They are not mutually exclusive.
> >
> > This also looks to be true for other dp8**** devices.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/mscc/ms
> > cc_mai
> > n.c#L318
> >
> > WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> > Also, you can have both WAKE_MAGIC and WAKE_MAGICSECURE at the same
> > time. They are not mutually exclusive.
> >
> > So i think your point a. above is questionable. Can the hardware
> > support both magic and secure magic at the same time? If so, follow the TI way of doing it.
I guess the question here is what "support both magic and secure magic at the same time" means ...
The original (i.e. not secure) Magic packet specification says it is a frame like this:
DESTINATION, SOURCE, MISC_PRE, 6 x FF, 16 x MAC_ADDR, MISC_POST, FCS
So a "magic packet pattern" consists of six 'FF' bytes (synchronization pattern) immediately followed by 16 consecutive repetitions of the mac address of the device and this can be anywhere within the frame.
(a) Note that if magic packet wake is enabled in a device and an incoming frame includes a "magic packet pattern", whatever is before (MISC_PRE) and after (MISC_POST) the "magic packet pattern" does not matter, the device will generate a wake up event.
The "Secure-on" magic packet, adds a 6 byte password immediately after the 16 repetitions of the mac address of the device to the pattern that has to be matched, so the frames looks like this:
DESTINATION, SOURCE, MISC_PRE, 6 x FF, 16 x MAC_ADDR, 6-BYTE_PWD, MISC_POST, FCS
(b) Note that If secure-on magic packet wake is enabled in a device an incoming frame will only cause a wake event if the whole pattern *including the password* matches.
(a) and (b) are mutually exclusive behaviors for frames that do not carry passwords (i.e. they have the FCS right after the 16th repletion of the Mac address) or have a non-matching password, so you cannot really enable both simultaneously (at least not in the sense that you would be able to comply with both standards for all possible frames simultaneously).
>
> Yes. I will do.
>
I understand that the TI devices give the *impression* of supporting both, but based on what I explained above, even if you accept WAKE_MAGIC and WAKE_MAGICSEGURE on a set and report them both back as enabled on a get; whatever behavior your hardware does will not be fully compliant to both specs simultaneously anyway. I discussed this with Raju and what we decided to do for our driver/device is that if you pass both WAKE_MAGIC and WAKE_MAGICSEGURE flags to us we will report them back as both being enabled in a subsequent get as you suggested, but the behavior of our driver/hardware will be as if you had only enabled WAKE_MAGIC.
> > If you cannot do both at the same time, and that is requested, you
> > should probably return -EOPNOTSUPP. That is probably better than what
> > the broadcom driver does, silently ignore WAKE_MAGIC.
> >
> > > b. If secure-on magic packet had been previously enabled, and a
> > subsequent
> > > command does not include it, we add it. This was done to workaround a
> > > problem with the 'pm-suspend' application which is unaware of secure-on
> > > magic packet being enabled and can unintentionally disable it prior to
> > > putting the system into suspend.
> >
>
> Ok. I will try to fix in 'pm-suspend' application
>
> > The kernel should not be working around broken userspace. But i also
> > suspect this is to do with it being unclear if WOL options are
> > incremental or not. Since it seems that they are not incremental, it
> > does not matter if "If secure-on magic packet had been previously
> > enable". pm-suspend is setting Wol how it wants it, which you say is
> > plain magic. So magic is what the PHY driver should do. Feel free to
> > submit patches to pm-suspend to make it understand secure magic, or
> > not touch WoL at all with the assumption it has already been setup by something else.
> >
> > Andrew
>
> Thanks,
> Raju
> I understand that the TI devices give the *impression* of supporting
> both, but based on what I explained above, even if you accept
> WAKE_MAGIC and WAKE_MAGICSEGURE on a set and report them both back
> as enabled on a get; whatever behavior your hardware does will not
> be fully compliant to both specs simultaneously anyway. I discussed
> this with Raju and what we decided to do for our driver/device is
> that if you pass both WAKE_MAGIC and WAKE_MAGICSEGURE flags to us we
> will report them back as both being enabled in a subsequent get as
> you suggested, but the behavior of our driver/hardware will be as if
> you had only enabled WAKE_MAGIC.
So i agree having WAKE_MAGIC and WAKE_MAGICSECURE at the same time
seems very odd. So i see no real problem limiting the driver to only
one or the other. However, if the user does ask for both, i would say
silently ignoring one is incorrect. You should return -EOPNOTUPP to
make it clear you don't support both at the same time.
I would also say that silently ignore the Secure version is probably
the worst choice. Things should be secure by default...
Andrew
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Tuesday, March 12, 2024 5:41 PM
> To: Ronnie Kunin - C21729 <[email protected]>
> Cc: Raju Lakkaraju - I30499 <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Bryan Whitehead - C21958
> <[email protected]>; [email protected]; UNGLinuxDriver
> <[email protected]>
> Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option flags configuration
> sequences
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> > I understand that the TI devices give the *impression* of supporting
> > both, but based on what I explained above, even if you accept
> > WAKE_MAGIC and WAKE_MAGICSEGURE on a set and report them both back as
> > enabled on a get; whatever behavior your hardware does will not be
> > fully compliant to both specs simultaneously anyway. I discussed this
> > with Raju and what we decided to do for our driver/device is that if
> > you pass both WAKE_MAGIC and WAKE_MAGICSEGURE flags to us we will
> > report them back as both being enabled in a subsequent get as you
> > suggested, but the behavior of our driver/hardware will be as if you
> > had only enabled WAKE_MAGIC.
>
> So i agree having WAKE_MAGIC and WAKE_MAGICSECURE at the same time seems very odd. So i see no
To me it is not just a little odd, *strictly speaking* as mentioned before it is an impossibility, since no
hardware can do both at the same time because they have mutually exclusive requirements
for some frames.
> real problem limiting the driver to only one or the other. However, if the user does ask for both, i would
> say silently ignoring one is incorrect. You should return -EOPNOTUPP to make it clear you don't support
> both at the same time.
>
> I would also say that silently ignore the Secure version is probably the worst choice. Things should be
> secure by default...
>
> Andrew
We were just trying to accommodate your previous request to accept both "if the hardware supports it".
And even though I didn't like it, this was an attempt to answer my previous question: "what does it
mean to support both magic and secure magic at the same time ?" in some way that might make sense.
It is not that the purpose was to "silently ignore" the secure flag (that's why we would still return it as being
set on a subsequent get), we just took the interpretation that both flags together meant the user wanted
to do an "OR" of both matching conditions (secure and non secure). I see your preference would be to do
an "AND" of the two matching conditions citing security concerns. Honestly, I don't think there is a best or
worse way, in my opinion the user does not really understand what he is doing if he Is asking to enable both
secure and non-secure behaviors simultaneously, so security is probably down the drain already anyway.
In that sense I would have agreed with your recommendation that the best course of action would have
been to only accept one flag individually and fail with -EOPNOTUPP if both come simultaneously. And
being mutually exclusive at the definition level that really should have applied to all drivers and hardware
(not just Microchip's).
But then I looked at the actual definition of the flags themselves in the header file and I see this:
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1993
#define WAKE_MAGIC (1 << 5)
#define WAKE_MAGICSECURE (1 << 6) /* only meaningful if WAKE_MAGIC */
And even the ethtool manual says this
g Wake on MagicPacket(tm)
s Enable SecureOn(tm) password for MagicPacket(tm)
So the "only meaningful" comment seems to imply the original intention of these flags was that
WAKE_MAGICSECURE is an optional modifier for WAKE_MAGIC. Since as Raju showed the ethertool
application always overwrites previous settings (does not preserve anything) then you can only use
WAKE_MAGICSECURE *simultaneously* with WAKE_MAGIC and not in a standalone manner.
The ethtool manual seems to me to reinforce this since if says "Enable SecureOn password FOR magic
packet", rather that "Enable SecureON MagicPacket", so the 's' option is something that enables the
addition of a password to the 'g' Option.
So back to the beginning it is unclear what should happen...
I'd say we seem to have 3 different approaches. Which way should we go now?
1. Follow the definition of the flags in ethtool.h and ethtool manual:
- accept WAKE_MAGIC standalone and wake on regular magic packet matching
- accept WAKE_MAGIC and WAKE_MAGICSECURE simultaneously and only wake on secure magic
packet with valid password matching.
- reject WAKE_MAGICSECURE standalone
Note that this is not how any of the current drivers work and does not follow the conclusions from your
last email either
2. Treat WAKE_MAGIC as a request for magic packet behavior; and WAKE_MAGICSECURE as a request for
secure-on magic packet behavior. Since they are mutually exclusive only accept them individually and
reject it if they come simultaneously. This does not match the flags definitions or documentation, and
it is not how any of the existing drivers work, but it has consistency to it and it is the way you were
leaning in the last email based on what we knew by them.
3. Follow some of the other existing drivers' code behavior (Broadcom, TI or MSCC), which do not seem to
match the flag definitions (because they all accept WAKE_MAGICSECURE standalone) and we do not really
know what the hardware exactly does in some of the flag combinations / received frame stimuli. I'd rather
not do this since we (Microchip) will probably end up behaving in yet some different behavior from
everybody else for at least some frame stimuli and not match any documentation either.
My opinion with this latest info from headers / man is that we should follow #1. What do you think Andrew?
Ronnie
On 3/12/2024 5:22 PM, [email protected] wrote:
>
>> -----Original Message-----
>> From: Andrew Lunn <[email protected]>
>> Sent: Tuesday, March 12, 2024 5:41 PM
>> To: Ronnie Kunin - C21729 <[email protected]>
>> Cc: Raju Lakkaraju - I30499 <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected]; Bryan Whitehead - C21958
>> <[email protected]>; [email protected]; UNGLinuxDriver
>> <[email protected]>
>> Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option flags configuration
>> sequences
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>>> I understand that the TI devices give the *impression* of supporting
>>> both, but based on what I explained above, even if you accept
>>> WAKE_MAGIC and WAKE_MAGICSEGURE on a set and report them both back as
>>> enabled on a get; whatever behavior your hardware does will not be
>>> fully compliant to both specs simultaneously anyway. I discussed this
>>> with Raju and what we decided to do for our driver/device is that if
>>> you pass both WAKE_MAGIC and WAKE_MAGICSEGURE flags to us we will
>>> report them back as both being enabled in a subsequent get as you
>>> suggested, but the behavior of our driver/hardware will be as if you
>>> had only enabled WAKE_MAGIC.
>>
>> So i agree having WAKE_MAGIC and WAKE_MAGICSECURE at the same time seems very odd. So i see no
>
> To me it is not just a little odd, *strictly speaking* as mentioned before it is an impossibility, since no
> hardware can do both at the same time because they have mutually exclusive requirements
> for some frames.
Agreed, this is definitively the case for the hardware that I maintain.
>
>> real problem limiting the driver to only one or the other. However, if the user does ask for both, i would
>> say silently ignoring one is incorrect. You should return -EOPNOTUPP to make it clear you don't support
>> both at the same time.
>>
>> I would also say that silently ignore the Secure version is probably the worst choice. Things should be
>> secure by default...
>>
>> Andrew
>
> We were just trying to accommodate your previous request to accept both "if the hardware supports it".
> And even though I didn't like it, this was an attempt to answer my previous question: "what does it
> mean to support both magic and secure magic at the same time ?" in some way that might make sense.
> It is not that the purpose was to "silently ignore" the secure flag (that's why we would still return it as being
> set on a subsequent get), we just took the interpretation that both flags together meant the user wanted
> to do an "OR" of both matching conditions (secure and non secure). I see your preference would be to do
> an "AND" of the two matching conditions citing security concerns. Honestly, I don't think there is a best or
> worse way, in my opinion the user does not really understand what he is doing if he Is asking to enable both
> secure and non-secure behaviors simultaneously, so security is probably down the drain already anyway.
>
> In that sense I would have agreed with your recommendation that the best course of action would have
> been to only accept one flag individually and fail with -EOPNOTUPP if both come simultaneously. And
> being mutually exclusive at the definition level that really should have applied to all drivers and hardware
> (not just Microchip's).
>
> But then I looked at the actual definition of the flags themselves in the header file and I see this:
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1993
>
> #define WAKE_MAGIC (1 << 5)
> #define WAKE_MAGICSECURE (1 << 6) /* only meaningful if WAKE_MAGIC */
>
> And even the ethtool manual says this
>
> g Wake on MagicPacket(tm)
> s Enable SecureOn(tm) password for MagicPacket(tm)
>
> So the "only meaningful" comment seems to imply the original intention of these flags was that
> WAKE_MAGICSECURE is an optional modifier for WAKE_MAGIC. Since as Raju showed the ethertool
> application always overwrites previous settings (does not preserve anything) then you can only use
> WAKE_MAGICSECURE *simultaneously* with WAKE_MAGIC and not in a standalone manner.
> The ethtool manual seems to me to reinforce this since if says "Enable SecureOn password FOR magic
> packet", rather that "Enable SecureON MagicPacket", so the 's' option is something that enables the
> addition of a password to the 'g' Option.
>
> So back to the beginning it is unclear what should happen...
> I'd say we seem to have 3 different approaches. Which way should we go now?
> 1. Follow the definition of the flags in ethtool.h and ethtool manual:
> - accept WAKE_MAGIC standalone and wake on regular magic packet matching
> - accept WAKE_MAGIC and WAKE_MAGICSECURE simultaneously and only wake on secure magic
> packet with valid password matching.
> - reject WAKE_MAGICSECURE standalone
> Note that this is not how any of the current drivers work and does not follow the conclusions from your
> last email either
This seems reasonable to me, and as you say it matches the header
comment. Question is whether the enforcement of WAKE_MAGICSECURE implies
WAKE_MAGIC at the core ethtool level, or if this is up to individual
drivers. Also, how many drivers need to be fixed?
> 2. Treat WAKE_MAGIC as a request for magic packet behavior; and WAKE_MAGICSECURE as a request for
> secure-on magic packet behavior. Since they are mutually exclusive only accept them individually and
> reject it if they come simultaneously. This does not match the flags definitions or documentation, and
> it is not how any of the existing drivers work, but it has consistency to it and it is the way you were
> leaning in the last email based on what we knew by them.
I suspect this might be breaking user-space in surprising ways and we
would eventually get a report about that requesting the behavior to be
changed.
> 3. Follow some of the other existing drivers' code behavior (Broadcom, TI or MSCC), which do not seem to
> match the flag definitions (because they all accept WAKE_MAGICSECURE standalone) and we do not really
> know what the hardware exactly does in some of the flag combinations / received frame stimuli. I'd rather
> not do this since we (Microchip) will probably end up behaving in yet some different behavior from
> everybody else for at least some frame stimuli and not match any documentation either.
I agree about the not clear behavior though for bcm-phy-lib.c,
specifying both will eventually have WAKE_MAGICSECURE "win" given how
the code is structured (assuming I can remember my own code properly).
>
> My opinion with this latest info from headers / man is that we should follow #1. What do you think Andrew?
That would be my inclination for new drivers, or drivers that we are
fixing, like lan743x. For existing drivers unfortunately we might have
to preserve the incorrect behavior.
--
Florian
So i think we need to agree on best practices for new drivers and
document what that is, probably in both the ethtool man page and
include/uapi/linux/ethtool.h
* WAKE_MAGIC on its own is O.K.
* WAKE_MAGICSECURE without WAKE_MAGIC is invalid. -EINVAL
* WAKE_MAGIC + WAKE_MAGICSECURE means only secure magic WoL.
* Each driver needs to enforce this, due to backwards compatibility.
Some may decide not to.
Can we agree on this?
Andrew
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Wednesday, March 13, 2024 3:53 PM
> To: Florian Fainelli <[email protected]>
> Cc: Ronnie Kunin - C21729 <[email protected]>; Raju Lakkaraju - I30499
> <[email protected]>; [email protected]; [email protected]; [email protected];
> [email protected]; Bryan Whitehead - C21958 <[email protected]>;
> [email protected]; UNGLinuxDriver <[email protected]>
> Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option flags configuration
> sequences
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> So i think we need to agree on best practices for new drivers and document what that is, probably in
> both the ethtool man page and include/uapi/linux/ethtool.h
>
> * WAKE_MAGIC on its own is O.K.
> * WAKE_MAGICSECURE without WAKE_MAGIC is invalid. -EINVAL
> * WAKE_MAGIC + WAKE_MAGICSECURE means only secure magic WoL.
> * Each driver needs to enforce this, due to backwards compatibility.
> Some may decide not to.
>
> Can we agree on this?
Sounds good to me.
Raju (and other MCHP folks) are out this week due to a company shutdown, but he should be able to submit a new patch for lan743x as described above when back next week.
>
> Andrew
On 3/13/24 12:52, Andrew Lunn wrote:
> So i think we need to agree on best practices for new drivers and
> document what that is, probably in both the ethtool man page and
> include/uapi/linux/ethtool.h
>
> * WAKE_MAGIC on its own is O.K.
> * WAKE_MAGICSECURE without WAKE_MAGIC is invalid. -EINVAL
> * WAKE_MAGIC + WAKE_MAGICSECURE means only secure magic WoL.
> * Each driver needs to enforce this, due to backwards compatibility.
> Some may decide not to.
>
> Can we agree on this?
Yes, that works for me as well.
--
Florian