| Message ID | 20180924205420.31309-4-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: usb: Check for Wake-on-LAN modes | expand |
Hi Florian, > @@ -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. 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, > >> @@ -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? > >> device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts); >> >> phy_ethtool_set_wol(netdev->phydev, wol); > > > Thanks. > Woojung >
Hi Florian, > >> + 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? > 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, > >>>> + 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? >> > 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);
Hi Florian, > @@ -1401,19 +1401,10 @@ static int lan78xx_set_wol(struct net_device ... > + 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
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);
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(+)