[PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

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.

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;
 
+	memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
+	cur_wol.cmd = ETHTOOL_GWOL;
+	dev->ethtool_ops->get_wol(dev, &cur_wol);
+
 	if (copy_from_user(&wol, useraddr, sizeof(wol)))
 		return -EFAULT;
 
+	if (wol.wolopts & ~cur_wol.supported)
+		return -EOPNOTSUPP;
+
+	if (wol.wolopts == cur_wol.wolopts)
+		return 0;
+
 	ret = dev->ethtool_ops->set_wol(dev, &wol);
 	if (ret)
 		return ret;
-- 
2.7.4


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Simon Horman]

+ 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?

> 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?

>  
> +	memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
> +	cur_wol.cmd = ETHTOOL_GWOL;
> +	dev->ethtool_ops->get_wol(dev, &cur_wol);
> +
>  	if (copy_from_user(&wol, useraddr, sizeof(wol)))
>  		return -EFAULT;
>  
> +	if (wol.wolopts & ~cur_wol.supported)
> +		return -EOPNOTSUPP;
> +
> +	if (wol.wolopts == cur_wol.wolopts)
> +		return 0;
> +
>  	ret = dev->ethtool_ops->set_wol(dev, &wol);
>  	if (ret)
>  		return ret;

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Jakub Kicinski]

On Thu, 1 Jun 2023 17:55:21 +0200 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

Sorry to chime in but always prefer running get_maintainer on the patch
rather than a file path. File path misses stuff like Fixes tags.
If it was up to me that option would have been removed :(

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 2278 bytes --]



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

>>   
>> +	memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
>> +	cur_wol.cmd = ETHTOOL_GWOL;
>> +	dev->ethtool_ops->get_wol(dev, &cur_wol);
>> +
>>   	if (copy_from_user(&wol, useraddr, sizeof(wol)))
>>   		return -EFAULT;
>>   
>> +	if (wol.wolopts & ~cur_wol.supported)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (wol.wolopts == cur_wol.wolopts)
>> +		return 0;
>> +
>>   	ret = dev->ethtool_ops->set_wol(dev, &wol);
>>   	if (ret)
>>   		return ret;

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Simon Horman]

On Thu, Jun 01, 2023 at 09:03:10AM -0700, Jakub Kicinski wrote:
> On Thu, 1 Jun 2023 17:55:21 +0200 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
> Sorry to chime in but always prefer running get_maintainer on the patch
> rather than a file path. File path misses stuff like Fixes tags.
> If it was up to me that option would have been removed :(

Yes, sorry about that.

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]



On 6/1/2023 9:03 AM, Jakub Kicinski wrote:
> On Thu, 1 Jun 2023 17:55:21 +0200 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
> 
> Sorry to chime in but always prefer running get_maintainer on the patch
> rather than a file path. File path misses stuff like Fixes tags.
> If it was up to me that option would have been removed :(
Apologies, Will do next time.

Justin

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 2657 bytes --]



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.

Justin

>>> +    memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
>>> +    cur_wol.cmd = ETHTOOL_GWOL;
>>> +    dev->ethtool_ops->get_wol(dev, &cur_wol);
>>> +
>>>       if (copy_from_user(&wol, useraddr, sizeof(wol)))
>>>           return -EFAULT;
>>> +    if (wol.wolopts & ~cur_wol.supported)
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if (wol.wolopts == cur_wol.wolopts)
>>> +        return 0;
>>> +
>>>       ret = dev->ethtool_ops->set_wol(dev, &wol);
>>>       if (ret)
>>>           return ret;

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2915 bytes --]

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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Andrew Lunn]

> > > 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.

	Andrew

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

+ Woojung, UNGLinuxDriver

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.
> 
> 	Andrew

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

RE: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Woojung.Huh]

> 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

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Simon Horman]

On Thu, Jun 01, 2023 at 09:22:50AM -0700, 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.

I think that would be my suggestion.
But I'm unsure of the magnitude of change involved.
Or how risky it is in terms of introducing regressions.
In any case, perhaps it's best to start small.

> > > 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.

It seems, from other discussion in a different sub-thread, that we are
likely clear there :)

As that was my only real reservation wrt this patch:

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Jakub Kicinski]

On Wed, 31 May 2023 13:53:49 -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.
> 
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

If I understand the discussion correctly the patch is ready to be
applied. Could you repost it? It has been marked as "changes requested"
in patchwork, I'm not 100% sure about the reason.

[PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

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.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
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;
 
+	memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
+	cur_wol.cmd = ETHTOOL_GWOL;
+	dev->ethtool_ops->get_wol(dev, &cur_wol);
+
 	if (copy_from_user(&wol, useraddr, sizeof(wol)))
 		return -EFAULT;
 
+	if (wol.wolopts & ~cur_wol.supported)
+		return -EOPNOTSUPP;
+
+	if (wol.wolopts == cur_wol.wolopts)
+		return 0;
+
 	ret = dev->ethtool_ops->set_wol(dev, &wol);
 	if (ret)
 		return ret;
-- 
2.7.4


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

[Jakub Kicinski]

On Mon,  5 Jun 2023 11:46:16 -0700 Justin Chen wrote:
> +	if (wol.wolopts & ~cur_wol.supported)
> +		return -EOPNOTSUPP;

One small comment - I think we should return -EINVAL here.
That's what netlink return and we seem to mostly return -EOPNOTSUPP
if the operation is completely not supported.
-- 
pw-bot: cr