Messages in this thread | | | Date | Thu, 1 Jun 2023 11:37:43 -0700 | Subject | Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol | From | Florian Fainelli <> |
| |
On 6/1/23 11:27, Justin Chen wrote: > > > On 6/1/23 9:22 AM, Justin Chen wrote: >> >> >> On 6/1/2023 8:55 AM, Simon Horman wrote: >>> + Daniil Tatianin <d-tatianin@yandex-team.ru>, Andrew Lunn >>> <andrew@lunn.ch> >>> as per ./scripts/get_maintainer.pl --git-min-percent 25 >>> net/ethtool/ioctl.c >>> >>> On Wed, May 31, 2023 at 01:53:49PM -0700, Justin Chen wrote: >>>> The netlink version of set_wol checks for not supported wolopts and >>>> avoids >>>> setting wol when the correct wolopt is already set. If we do the >>>> same with >>>> the ioctl version then we can remove these checks from the driver >>>> layer. >>> >>> Hi Justin, >>> >>> Are you planning follow-up patches for the driver layer? >>> >> >> I was planning to for the Broadcom drivers since those I can test. But >> I could do it across the board if that is preferred. >> >>>> Signed-off-by: Justin Chen <justin.chen@broadcom.com> >>>> --- >>>> net/ethtool/ioctl.c | 14 ++++++++++++-- >>>> 1 file changed, 12 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c >>>> index 6bb778e10461..80f456f83db0 100644 >>>> --- a/net/ethtool/ioctl.c >>>> +++ b/net/ethtool/ioctl.c >>>> @@ -1436,15 +1436,25 @@ static int ethtool_get_wol(struct net_device >>>> *dev, char __user *useraddr) >>>> static int ethtool_set_wol(struct net_device *dev, char __user >>>> *useraddr) >>>> { >>>> - struct ethtool_wolinfo wol; >>>> + struct ethtool_wolinfo wol, cur_wol; >>>> int ret; >>>> - if (!dev->ethtool_ops->set_wol) >>>> + if (!dev->ethtool_ops->get_wol || !dev->ethtool_ops->set_wol) >>>> return -EOPNOTSUPP; >>> >>> Are there cases where (in-tree) drivers provide set_wol byt not get_wol? >>> If so, does this break their set_wol support? >>> >> >> My original thought was to match netlink set wol behavior. So drivers >> that do that won't work with netlink set_wol right now. I'll skim >> around to see if any drivers do this. But I would reckon this should >> be a driver fix. >> >> Thanks, >> Justin >> > > I see a driver at drivers/net/phy/microchip.c. But this is a phy driver > set_wol hook.
That part of the driver appears to be dead code. It attempts to pretend to support Wake-on-LAN, but it does not do any specific programming of wake-up filters, nor does it implement get_wol. It also does not make use of the recently introduced PHY_ALWAYS_CALL_SUSPEND flag.
When it is time to determine whether to suspend the PHY or not, eventually phy_suspend() will call phy_ethtool_get_wol(). Since no get_wol is implemented, the wol.wolopts will remain zero, therefore we will just suspend the PHY.
I suspect this was added to work around MAC drivers that may forcefully try to suspend the PHY, but that should not even be possible these days.
I would just remove that logic from microchip.c entirely. -- Florian
[unhandled content-type:application/pkcs7-signature] | |