Messages in this thread | | | From | <> | Subject | RE: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol | Date | Thu, 1 Jun 2023 20:41:41 +0000 |
| |
> On 6/1/23 11:48 AM, Andrew Lunn wrote: > >>>> 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. > > > > The Microchip developers are reasonably responsive. So we should Cc: > > them.
set_wol in drivers/net/phy/microchip.c is used to set the flag to avoid PHY power down at suspend time. Looks it is old-fashioned now because frame work is not calling suspend after calling get_wol. We will make a patch for it.
BTW, this patch is checking MAC driver set_wol and get_wol. So I don't think it breaks drivers/net/phy/microchip.c suspend operation anyway.
Thanks. Woojung
| |