* [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol
@ 2023-06-07 23:14 Justin Chen
2023-06-08 16:01 ` Florian Fainelli
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Justin Chen @ 2023-06-07 23:14 UTC (permalink / raw)
To: netdev
Cc: bcm-kernel-feedback-list, florian.fainelli, Justin Chen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, Daniil Tatianin, Ido Schimmel, Marco Bonelli,
Wolfram Sang, Jiri Pirko, Gal Pressman, Vincent Mailhol,
Kuniyuki Iwashima, open list
[-- Attachment #1: Type: text/plain, Size: 1403 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>
---
v2
- Return -EINVAL instead of -EOPNOTSUPP
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..37b582225854 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 -EINVAL;
+
+ 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 --]
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol
2023-06-07 23:14 [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol Justin Chen
@ 2023-06-08 16:01 ` Florian Fainelli
2023-06-09 2:40 ` patchwork-bot+netdevbpf
2023-06-09 20:47 ` Justin Chen
2 siblings, 0 replies; 5+ messages in thread
From: Florian Fainelli @ 2023-06-08 16:01 UTC (permalink / raw)
To: Justin Chen, netdev
Cc: bcm-kernel-feedback-list, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Andrew Lunn, Daniil Tatianin,
Ido Schimmel, Marco Bonelli, Wolfram Sang, Jiri Pirko,
Gal Pressman, Vincent Mailhol, Kuniyuki Iwashima, open list
[-- Attachment #1: Type: text/plain, Size: 461 bytes --]
On 6/7/2023 4:14 PM, 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.
>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
--
Florian
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol
2023-06-07 23:14 [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol Justin Chen
2023-06-08 16:01 ` Florian Fainelli
@ 2023-06-09 2:40 ` patchwork-bot+netdevbpf
2023-06-09 20:47 ` Justin Chen
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-06-09 2:40 UTC (permalink / raw)
To: Justin Chen
Cc: netdev, bcm-kernel-feedback-list, florian.fainelli, davem,
edumazet, kuba, pabeni, andrew, d-tatianin, idosch, marco,
wsa+renesas, jiri, gal, mailhol.vincent, kuniyu, linux-kernel
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 7 Jun 2023 16:14:11 -0700 you 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.
>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
>
> [...]
Here is the summary with links:
- [v2,net-next] ethtool: ioctl: improve error checking for set_wol
https://git.kernel.org/netdev/net-next/c/55b24334c0f2
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol
2023-06-07 23:14 [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol Justin Chen
2023-06-08 16:01 ` Florian Fainelli
2023-06-09 2:40 ` patchwork-bot+netdevbpf
@ 2023-06-09 20:47 ` Justin Chen
2023-06-10 6:18 ` Jakub Kicinski
2 siblings, 1 reply; 5+ messages in thread
From: Justin Chen @ 2023-06-09 20:47 UTC (permalink / raw)
To: netdev
Cc: bcm-kernel-feedback-list, florian.fainelli, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Andrew Lunn,
Daniil Tatianin, Ido Schimmel, Marco Bonelli, Wolfram Sang,
Jiri Pirko, Gal Pressman, Vincent Mailhol, Kuniyuki Iwashima,
open list
[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]
On 6/7/23 4:14 PM, 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.
>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> ---
> v2
> - Return -EINVAL instead of -EOPNOTSUPP
>
> 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..37b582225854 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 -EINVAL;
> +
> + if (wol.wolopts == cur_wol.wolopts)
> + return 0;
> +
> ret = dev->ethtool_ops->set_wol(dev, &wol);
> if (ret)
> return ret;
Was thinking more about this patch. I realized we don't account for the
different sopass case.
# ethtool -s eth0 wol s sopass 11:22:33:44:55:66
# ethtool -s eth0 wol s sopass 22:44:55:66:77:88
For this case, the second sopass values won't be stored.
Can you drop this patch? I will submit another version.
Thanks,
Justin
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol
2023-06-09 20:47 ` Justin Chen
@ 2023-06-10 6:18 ` Jakub Kicinski
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2023-06-10 6:18 UTC (permalink / raw)
To: Justin Chen
Cc: netdev, bcm-kernel-feedback-list, florian.fainelli,
David S. Miller, Eric Dumazet, Paolo Abeni, Andrew Lunn,
Daniil Tatianin, Ido Schimmel, Marco Bonelli, Wolfram Sang,
Jiri Pirko, Gal Pressman, Vincent Mailhol, Kuniyuki Iwashima,
open list
On Fri, 9 Jun 2023 13:47:22 -0700 Justin Chen wrote:
> Was thinking more about this patch. I realized we don't account for the
> different sopass case.
> # ethtool -s eth0 wol s sopass 11:22:33:44:55:66
> # ethtool -s eth0 wol s sopass 22:44:55:66:77:88
>
> For this case, the second sopass values won't be stored.
>
> Can you drop this patch? I will submit another version.
We can't drop patches, it'd mess up commit IDs and basing
trees on top of net-next would be a major PITA for people.
Please send a fix on top (with a Fixes tag making it clear
that the problem has not reached any -rc kernel).
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-10 6:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-07 23:14 [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol Justin Chen
2023-06-08 16:01 ` Florian Fainelli
2023-06-09 2:40 ` patchwork-bot+netdevbpf
2023-06-09 20:47 ` Justin Chen
2023-06-10 6:18 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).