From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes Date: Tue, 25 Sep 2018 11:35:48 -0700 Message-ID: <8de18c23-d846-cbb4-c852-9b7d97a95c11@gmail.com> References: <20180924205420.31309-1-f.fainelli@gmail.com> <20180924205420.31309-4-f.fainelli@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, UNGLinuxDriver@microchip.com, steve.glendinning@shawell.net, keescook@chromium.org, akurz@blala.de, hayeswang@realtek.com, kai.heng.feng@canonical.com, grundler@chromium.org, zhongjiang@huawei.com, bigeasy@linutronix.de, ran.wang_1@nxp.com, edumazet@google.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org To: Woojung.Huh@microchip.com, netdev@vger.kernel.org Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org 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 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