[PATCH net 0/7] net: usb: Check for Wake-on-LAN modes

Hi all,

Most of our USB Ethernet drivers don't seem to be checking properly
whether the user is supplying a correct Wake-on-LAN mode to enter, so
the experience as an user could be confusing, since it would generally
lead to either no wake-up, or the device not being marked for wake-up.

Please review!

Florian Fainelli (7):
  asix: Check for supported Wake-on-LAN modes
  ax88179_178a: Check for supported Wake-on-LAN modes
  lan78xx: Check for supported Wake-on-LAN modes
  sr9800: Check for supported Wake-on-LAN modes
  r8152: Check for supported Wake-on-LAN Modes
  smsc75xx: Check for Wake-on-LAN modes
  smsc95xx: Check for Wake-on-LAN modes

 drivers/net/usb/asix_common.c  | 3 +++
 drivers/net/usb/ax88179_178a.c | 3 +++
 drivers/net/usb/lan78xx.c      | 3 +++
 drivers/net/usb/r8152.c        | 3 +++
 drivers/net/usb/smsc75xx.c     | 3 +++
 drivers/net/usb/smsc95xx.c     | 3 +++
 drivers/net/usb/sr9800.c       | 3 +++
 7 files changed, 21 insertions(+)

-- 
2.17.1

The driver currently silently accepts unsupported Wake-on-LAN modes
(other than WAKE_PHY or WAKE_MAGIC) without reporting that to the user,
which is confusing.

Fixes: 2e55cc7210fe ("[PATCH] USB: usbnet (3/9) module for ASIX Ethernet adapters")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/usb/asix_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index e95dd12edec4..023b8d0bf175 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c

@@ -607,6 +607,9 @@ int asix_set_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
 	struct usbnet *dev = netdev_priv(net);
 	u8 opt = 0;
 
+	if (wolinfo->wolopts & ~(WAKE_PHY | WAKE_MAGIC))
+		return -EINVAL;
+
 	if (wolinfo->wolopts & WAKE_PHY)
 		opt |= AX_MONITOR_LINK;
 	if (wolinfo->wolopts & WAKE_MAGIC)

-- 
2.17.1

The driver currently silently accepts unsupported Wake-on-LAN modes
(other than WAKE_PHY or WAKE_MAGIC) without reporting that to the user,
which is confusing.

Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/usb/ax88179_178a.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 9e8ad372f419..2207f7a7d1ff 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c

@@ -566,6 +566,9 @@ ax88179_set_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
 	struct usbnet *dev = netdev_priv(net);
 	u8 opt = 0;
 
+	if (wolinfo->wolopts & ~(WAKE_PHY | WAKE_MAGIC))
+		return -EINVAL;
+
 	if (wolinfo->wolopts & WAKE_PHY)
 		opt |= AX_MONITOR_MODE_RWLC;
 	if (wolinfo->wolopts & WAKE_MAGIC)

-- 
2.17.1

The driver currently silently accepts unsupported Wake-on-LAN modes
(other than WAKE_PHY or WAKE_MAGIC) without reporting that to the user,
which is confusing.

Fixes: 19a38d8e0aa3 ("USB2NET : SR9800 : One chip USB2.0 USB2NET SR9800 Device Driver Support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/usb/sr9800.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/sr9800.c b/drivers/net/usb/sr9800.c
index 9277a0f228df..35f39f23d881 100644
--- a/drivers/net/usb/sr9800.c
+++ b/drivers/net/usb/sr9800.c

@@ -421,6 +421,9 @@ sr_set_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
 	struct usbnet *dev = netdev_priv(net);
 	u8 opt = 0;
 
+	if (wolinfo->wolopts & ~(WAKE_PHY | WAKE_MAGIC))
+		return -EINVAL;
+
 	if (wolinfo->wolopts & WAKE_PHY)
 		opt |= SR_MONITOR_LINK;
 	if (wolinfo->wolopts & WAKE_MAGIC)

-- 
2.17.1

The driver does not check for Wake-on-LAN modes specified by an user,
but will conditionally set the device as wake-up enabled or not based on
that, which could be a very confusing user experience.

Fixes: 21ff2e8976b1 ("r8152: support WOL")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/usb/r8152.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2cd71bdb6484..f1b5201cc320 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -4506,6 +4506,9 @@ static int rtl8152_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	if (!rtl_can_wakeup(tp))
 		return -EOPNOTSUPP;
 
+	if (wol->wolopts & ~WAKE_ANY)
+		return -EINVAL;
+
 	ret = usb_autopm_get_interface(tp->intf);
 	if (ret < 0)
 		goto out_set_wol;

-- 
2.17.1

The driver does not check for Wake-on-LAN modes specified by an user,
but will conditionally set the device as wake-up enabled or not based on
that, which could be a very confusing user experience.

Fixes: 6c636503260d ("smsc75xx: add wol magic packet support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/usb/smsc75xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 05553d252446..e5a4cbb366dc 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c

@@ -731,6 +731,9 @@ static int smsc75xx_ethtool_set_wol(struct net_device *net,
 	struct smsc75xx_priv *pdata = (struct smsc75xx_priv *)(dev->data[0]);
 	int ret;
 
+	if (wolinfo->wolopts & ~SUPPORTED_WAKE)
+		return -EINVAL;
+
 	pdata->wolopts = wolinfo->wolopts & SUPPORTED_WAKE;
 
 	ret = device_set_wakeup_enable(&dev->udev->dev, pdata->wolopts);

-- 
2.17.1

The driver does not check for Wake-on-LAN modes specified by an user,
but will conditionally set the device as wake-up enabled or not based on
that, which could be a very confusing user experience.

Fixes: e0e474a83c18 ("smsc95xx: add wol magic packet support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/usb/smsc95xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 06b4d290784d..262e7a3c23cb 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c

@@ -774,6 +774,9 @@ static int smsc95xx_ethtool_set_wol(struct net_device *net,
 	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	int ret;
 
+	if (wolinfo->wolopts & ~SUPPORTED_WAKE)
+		return -EINVAL;
+
 	pdata->wolopts = wolinfo->wolopts & SUPPORTED_WAKE;
 
 	ret = device_set_wakeup_enable(&dev->udev->dev, pdata->wolopts);

-- 
2.17.1

The driver supports a fair amount of Wake-on-LAN modes, but is not
checking that the user specified one that is supported.

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/usb/lan78xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index a9991c5f4736..e0a588d1b8e6 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c

@@ -1415,6 +1415,9 @@ static int lan78xx_set_wol(struct net_device *netdev,
 	if (wol->wolopts & WAKE_ARP)
 		pdata->wol |= WAKE_ARP;
 
+	if (pdata->wol == 0)
+		return -EINVAL;
+
 	device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);
 
 	phy_ethtool_set_wol(netdev->phydev, wol);

-- 
2.17.1

Hi Florian,

It will make function return when disabling WOL.
Is there other place handling this scenario?

 	device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);

 	phy_ethtool_set_wol(netdev->phydev, wol);

Thanks.
Woojung

On 09/25/2018 10:19 AM, Woojung.Huh@microchip.com wrote:

Hi Florian,

quoted

@@ -1415,6 +1415,9 @@ static int lan78xx_set_wol(struct net_device *netdev,
 	if (wol->wolopts & WAKE_ARP)
 		pdata->wol |= WAKE_ARP;

+	if (pdata->wol == 0)
+		return -EINVAL;
+

It will make function return when disabling WOL.

Huh, yes, good point.

Is there other place handling this scenario?

How do you mean?

quoted

 	device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);

 	phy_ethtool_set_wol(netdev->phydev, wol);

Thanks.
Woojung

-- 
Florian

Hi Florian,

quoted

quoted

+	if (pdata->wol == 0)
+		return -EINVAL;
+

It will make function return when disabling WOL.

Huh, yes, good point.

quoted

Is there other place handling this scenario?

How do you mean?

I meant there is another path I might miss when disabling WOL 
than this xxx_set_wol().

Thanks
Woojung

On 09/25/2018 10:32 AM, Woojung.Huh@microchip.com wrote:

Hi Florian,

quoted

quoted

quoted

+	if (pdata->wol == 0)
+		return -EINVAL;
+

It will make function return when disabling WOL.

Huh, yes, good point.

quoted

Is there other place handling this scenario?

How do you mean?

I meant there is another path I might miss when disabling WOL 
than this xxx_set_wol().

I don't think so, at least not from the ethtool perspective, this should
fix the issue before, and simplifying the code, since all we are doing
it taking a bitmask, checking each bit we support, and again, make it
the same bitmask in pdata->wol, can you test that? If you have a new
enough version of ethtool, try using: ethtool -s <iface> wol f, which
was added recently and which this driver does not support:

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index a9991c5f4736..2e37028ef6ca 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c

@@ -1401,19 +1401,10 @@ static int lan78xx_set_wol(struct net_device

*netdev,
        if (ret < 0)
                return ret;

-       pdata->wol = 0;
-       if (wol->wolopts & WAKE_UCAST)
-               pdata->wol |= WAKE_UCAST;
-       if (wol->wolopts & WAKE_MCAST)
-               pdata->wol |= WAKE_MCAST;
-       if (wol->wolopts & WAKE_BCAST)
-               pdata->wol |= WAKE_BCAST;
-       if (wol->wolopts & WAKE_MAGIC)
-               pdata->wol |= WAKE_MAGIC;
-       if (wol->wolopts & WAKE_PHY)
-               pdata->wol |= WAKE_PHY;
-       if (wol->wolopts & WAKE_ARP)
-               pdata->wol |= WAKE_ARP;
+       if (pdata->wol & ~WAKE_ALL)
+               return -EINVAL;
+
+       pdata->wol = wol->wolopts;

        device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);


-- 
Florian

Hi Florian,

...

+       if (pdata->wol & ~WAKE_ALL)
+               return -EINVAL;

If I understand correctly, it needs to check againt "wol->wolopts" than pdata->wol.

+
+       pdata->wol = wol->wolopts;

Thanks.
Woojung

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 24 Sep 2018 13:54:13 -0700

Most of our USB Ethernet drivers don't seem to be checking properly
whether the user is supplying a correct Wake-on-LAN mode to enter, so
the experience as an user could be confusing, since it would generally
lead to either no wake-up, or the device not being marked for wake-up.

Please review!

I saw some discussion on patch #3, is there going to be a new version
of this series or should I apply this one?

Thanks.

On 09/26/2018 05:12 PM, David Miller wrote:

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 24 Sep 2018 13:54:13 -0700

quoted

Most of our USB Ethernet drivers don't seem to be checking properly
whether the user is supplying a correct Wake-on-LAN mode to enter, so
the experience as an user could be confusing, since it would generally
lead to either no wake-up, or the device not being marked for wake-up.

Please review!

I saw some discussion on patch #3, is there going to be a new version
of this series or should I apply this one?

I will be sending a v2, please discard this one, thanks!
-- 
Florian