Message ID | 20231019070904.521718-1-o.rempel@pengutronix.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2010:b0:403:3b70:6f57 with SMTP id fe16csp207037vqb; Thu, 19 Oct 2023 00:09:32 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHgfnaPLr2lZZe9F8lT6zS+vdhHYmqy2JMHT7LGZA0DZKHSth7HLBaD0123ZOpQeaw1jRc/ X-Received: by 2002:a05:6359:67a5:b0:166:e390:ffd1 with SMTP id sq37-20020a05635967a500b00166e390ffd1mr1209118rwb.19.1697699372078; Thu, 19 Oct 2023 00:09:32 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697699372; cv=none; d=google.com; s=arc-20160816; b=02yeHhxkUWAotmoMHSDbRglhjkSXWTxi1DASL6uxh7/blHoWFOakppuVcFFJGIhUIL IOiG8v8OTzOSOY6ovLa88IxK/YvdUO2hT8zOGQM5mOgPqkxeciQvxmS3MF1Qlvvfz5KN vt/BZA5UZ9Dnb5BK/Ic4N5Fv3wq0jVZMoi5/LLNF683lkmmIRMnhMQbvF5FGgcPjSyra MoWpEpFCy6qd21a5ijyqzFYvXXDhuq2lT5BvMfobRqqTq7Al/loeN26b+UZ3bG2YJLMn TlGcIziexOJVXzZ6Oo+oRz2uMjZHtVBnmCn8OefspMSUjWfl6rDr7Stjrl1+Fe3JE5Eg knjg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=AyaguX9Tpy5Bc1NFKO9stnqZuhyYRzNz1jx6zc0YRVk=; fh=zGUwd9DyZCpxGeUVRDt+VUsI1fxalPlncSMLzdB+iW4=; b=IdNKhHTb8KPE+Vd8vH/EO07DefpP6aOuuZaNm02bYkfiXCQmrL6M2nab1wnqkbaUwL lZ7NnP+tTcTHKhv8HXn8EoMKff6mEwgOHK94irb7hl0Dqj2THH4UOgqw/+3wpAIDBgqp 3Qy/AuIsU1BW11w3Z45ZYYhHb3fOBHs6BBW0VT88ZqU4H++aEJyLktpr1wB9C4r9muJ2 i3L6KuDEWGT+0vYL6J9BW0zS6Zs2J7C7meOHDG3dFrrO9gZVCyTnr+p6hG91AGpohgIO V84V8xhpZJUdf6g721TGqOaC0OHqyXryRLaa5tFvJ8Mu9ow6RpmnQCjo7NqFhQQcBKTb 7gPQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id z16-20020a656650000000b005aba83e3975si3445315pgv.661.2023.10.19.00.09.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Oct 2023 00:09:32 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 191298135606; Thu, 19 Oct 2023 00:09:30 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344826AbjJSHJU (ORCPT <rfc822;zwp10758@gmail.com> + 24 others); Thu, 19 Oct 2023 03:09:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55540 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344804AbjJSHJQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 19 Oct 2023 03:09:16 -0400 Received: from metis.whiteo.stw.pengutronix.de (metis.whiteo.stw.pengutronix.de [IPv6:2a0a:edc0:2:b01:1d::104]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 824AF11B for <linux-kernel@vger.kernel.org>; Thu, 19 Oct 2023 00:09:14 -0700 (PDT) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.whiteo.stw.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <ore@pengutronix.de>) id 1qtN9X-0002RO-TQ; Thu, 19 Oct 2023 09:09:07 +0200 Received: from [2a0a:edc0:0:1101:1d::ac] (helo=dude04.red.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from <ore@pengutronix.de>) id 1qtN9W-002jpd-Oi; Thu, 19 Oct 2023 09:09:06 +0200 Received: from ore by dude04.red.stw.pengutronix.de with local (Exim 4.96) (envelope-from <ore@pengutronix.de>) id 1qtN9W-002Bj7-2H; Thu, 19 Oct 2023 09:09:06 +0200 From: Oleksij Rempel <o.rempel@pengutronix.de> To: "David S. Miller" <davem@davemloft.net>, Andrew Lunn <andrew@lunn.ch>, Eric Dumazet <edumazet@google.com>, Florian Fainelli <f.fainelli@gmail.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Vladimir Oltean <olteanv@gmail.com> Cc: Oleksij Rempel <o.rempel@pengutronix.de>, kernel@pengutronix.de, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Michal Kubecek <mkubecek@suse.cz> Subject: [PATCH net v1 1/1] ethtool: fix clearing of WoL flags Date: Thu, 19 Oct 2023 09:09:04 +0200 Message-Id: <20231019070904.521718-1-o.rempel@pengutronix.de> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: ore@pengutronix.de X-SA-Exim-Scanned: No (on metis.whiteo.stw.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org X-Spam-Status: No, score=-0.8 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 19 Oct 2023 00:09:30 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780166817132972124 X-GMAIL-MSGID: 1780166817132972124 |
Series |
[net,v1,1/1] ethtool: fix clearing of WoL flags
|
|
Commit Message
Oleksij Rempel
Oct. 19, 2023, 7:09 a.m. UTC
With current kernel it is possible to set flags, but not possible to remove
existing WoL flags. For example:
~$ ethtool lan2
...
Supports Wake-on: pg
Wake-on: d
...
~$ ethtool -s lan2 wol gp
~$ ethtool lan2
...
Wake-on: pg
...
~$ ethtool -s lan2 wol d
~$ ethtool lan2
...
Wake-on: pg
...
This patch makes it work as expected
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
net/ethtool/wol.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
Comments
On Thu, Oct 19, 2023 at 09:09:04AM +0200, Oleksij Rempel wrote: > With current kernel it is possible to set flags, but not possible to remove > existing WoL flags. For example: > ~$ ethtool lan2 > ... > Supports Wake-on: pg > Wake-on: d > ... > ~$ ethtool -s lan2 wol gp > ~$ ethtool lan2 > ... > Wake-on: pg > ... > ~$ ethtool -s lan2 wol d > ~$ ethtool lan2 > ... > Wake-on: pg > ... > > This patch makes it work as expected > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > net/ethtool/wol.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c > index 0ed56c9ac1bc..fcefc1bbfa2e 100644 > --- a/net/ethtool/wol.c > +++ b/net/ethtool/wol.c > @@ -108,15 +108,16 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info) > struct net_device *dev = req_info->dev; > struct nlattr **tb = info->attrs; > bool mod = false; > + u32 wolopts = 0; > int ret; > > dev->ethtool_ops->get_wol(dev, &wol); > - ret = ethnl_update_bitset32(&wol.wolopts, WOL_MODE_COUNT, > + ret = ethnl_update_bitset32(&wolopts, WOL_MODE_COUNT, > tb[ETHTOOL_A_WOL_MODES], wol_mode_names, > info->extack, &mod); > if (ret < 0) > return ret; > - if (wol.wolopts & ~wol.supported) { > + if (wolopts & ~wol.supported) { > NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_WOL_MODES], > "cannot enable unsupported WoL mode"); > return -EINVAL; > @@ -132,8 +133,9 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info) > tb[ETHTOOL_A_WOL_SOPASS], &mod); > } > > - if (!mod) > + if (!mod && wolopts == wol.wolopts) > return 0; > + wol.wolopts = wolopts; > ret = dev->ethtool_ops->set_wol(dev, &wol); > if (ret) > return ret; > -- > 2.39.2 This doesn't look right, AFAICS with this patch, the resulting WoL flags would not depend on current values at all, i.e. it would certainly break non-absolute commands like ethtool -s eth0 wol +g ethtool -s eth0 wol -u+g ethtool -s etho wol 32/34 How recent was the kernel where you encountered the issue? I suspect the issue might be related to recent 108a36d07c01 ("ethtool: Fix mod state of verbose no_mask bitset"), I'll look into it closer. Michal
On Thu, Oct 19, 2023 at 11:05:10AM +0200, Michal Kubecek wrote: > On Thu, Oct 19, 2023 at 09:09:04AM +0200, Oleksij Rempel wrote: > > With current kernel it is possible to set flags, but not possible to remove > > existing WoL flags. For example: > > ~$ ethtool lan2 > > ... > > Supports Wake-on: pg > > Wake-on: d > > ... > > ~$ ethtool -s lan2 wol gp > > ~$ ethtool lan2 > > ... > > Wake-on: pg > > ... > > ~$ ethtool -s lan2 wol d > > ~$ ethtool lan2 > > ... > > Wake-on: pg > > ... > > > > This patch makes it work as expected > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > net/ethtool/wol.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c > > index 0ed56c9ac1bc..fcefc1bbfa2e 100644 > > --- a/net/ethtool/wol.c > > +++ b/net/ethtool/wol.c > > @@ -108,15 +108,16 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info) > > struct net_device *dev = req_info->dev; > > struct nlattr **tb = info->attrs; > > bool mod = false; > > + u32 wolopts = 0; > > int ret; > > > > dev->ethtool_ops->get_wol(dev, &wol); > > - ret = ethnl_update_bitset32(&wol.wolopts, WOL_MODE_COUNT, > > + ret = ethnl_update_bitset32(&wolopts, WOL_MODE_COUNT, > > tb[ETHTOOL_A_WOL_MODES], wol_mode_names, > > info->extack, &mod); > > if (ret < 0) > > return ret; > > - if (wol.wolopts & ~wol.supported) { > > + if (wolopts & ~wol.supported) { > > NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_WOL_MODES], > > "cannot enable unsupported WoL mode"); > > return -EINVAL; > > @@ -132,8 +133,9 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info) > > tb[ETHTOOL_A_WOL_SOPASS], &mod); > > } > > > > - if (!mod) > > + if (!mod && wolopts == wol.wolopts) > > return 0; > > + wol.wolopts = wolopts; > > ret = dev->ethtool_ops->set_wol(dev, &wol); > > if (ret) > > return ret; > > -- > > 2.39.2 > > This doesn't look right, AFAICS with this patch, the resulting WoL flags > would not depend on current values at all, i.e. it would certainly break > non-absolute commands like > > ethtool -s eth0 wol +g > ethtool -s eth0 wol -u+g > ethtool -s etho wol 32/34 Wow, I have learned something new :) > How recent was the kernel where you encountered the issue? It is latest net-next. > I suspect the > issue might be related to recent 108a36d07c01 ("ethtool: Fix mod state > of verbose no_mask bitset"), I'll look into it closer. Thx! Regards, Oleksij
On Thu, Oct 19, 2023 at 11:05:10AM +0200, Michal Kubecek wrote: > On Thu, Oct 19, 2023 at 09:09:04AM +0200, Oleksij Rempel wrote: > > With current kernel it is possible to set flags, but not possible to remove > > existing WoL flags. For example: > > ~$ ethtool lan2 > > ... > > Supports Wake-on: pg > > Wake-on: d > > ... > > ~$ ethtool -s lan2 wol gp > > ~$ ethtool lan2 > > ... > > Wake-on: pg > > ... > > ~$ ethtool -s lan2 wol d > > ~$ ethtool lan2 > > ... > > Wake-on: pg > > ... > > > > How recent was the kernel where you encountered the issue? I suspect the > issue might be related to recent 108a36d07c01 ("ethtool: Fix mod state > of verbose no_mask bitset"), I'll look into it closer. The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix mod state of verbose no_mask bitset"). The problem is that a "no mask" verbose bitset only contains bit attributes for bits to be set. This worked correctly before this commit because we were always updating a zero bitmap (since commit 6699170376ab ("ethtool: fix application of verbose no_mask bitset"), that is) so that the rest was left zero naturally. But now the 1->0 change (old_val is true, bit not present in netlink nest) no longer works. To fix the issue while keeping more precise modification tracking introduced by commit 108a36d07c01 ("ethtool: Fix mod state of verbose no_mask bitset"), we would have to either iterate through all possible bits in "no mask" case or update a temporary zero bitmap and then set mod by comparing it to the original (and rewrite the target if they differ). This is exactly what I was trying to avoid from the start but it wouldn't be more complicated than current code. As we are rather late in the 6.6 cycle (rc6 out), the safest solution seems to be reverting commit 108a36d07c01 ("ethtool: Fix mod state of verbose no_mask bitset") in net tree as sending a notification even on a request which turns out to be no-op is not a serious problem (after all, this is what happens for ioctl requests most of the time and IIRC there are more cases where it happens for netlink requests). Then we can fix the change detection properly in net-next in the way proposed in previous paragraph (I would prefer not risking more intrusive changes at this stage). Michal
On Thu, 19 Oct 2023 11:51:40 +0200 Michal Kubecek <mkubecek@suse.cz> wrote: > On Thu, Oct 19, 2023 at 11:05:10AM +0200, Michal Kubecek wrote: > > On Thu, Oct 19, 2023 at 09:09:04AM +0200, Oleksij Rempel wrote: > > > With current kernel it is possible to set flags, but not possible to > > > remove existing WoL flags. For example: > > > ~$ ethtool lan2 > > > ... > > > Supports Wake-on: pg > > > Wake-on: d > > > ... > > > ~$ ethtool -s lan2 wol gp > > > ~$ ethtool lan2 > > > ... > > > Wake-on: pg > > > ... > > > ~$ ethtool -s lan2 wol d > > > ~$ ethtool lan2 > > > ... > > > Wake-on: pg > > > ... > > > > > > > How recent was the kernel where you encountered the issue? I suspect the > > issue might be related to recent 108a36d07c01 ("ethtool: Fix mod state > > of verbose no_mask bitset"), I'll look into it closer. > > The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix > mod state of verbose no_mask bitset"). The problem is that a "no mask" > verbose bitset only contains bit attributes for bits to be set. This > worked correctly before this commit because we were always updating > a zero bitmap (since commit 6699170376ab ("ethtool: fix application of > verbose no_mask bitset"), that is) so that the rest was left zero > naturally. But now the 1->0 change (old_val is true, bit not present in > netlink nest) no longer works. Doh I had not seen this issue! Thanks you for reporting it. I will send the revert then and will update the fix for next merge-window. > To fix the issue while keeping more precise modification tracking > introduced by commit 108a36d07c01 ("ethtool: Fix mod state of verbose > no_mask bitset"), we would have to either iterate through all possible > bits in "no mask" case or update a temporary zero bitmap and then set > mod by comparing it to the original (and rewrite the target if they > differ). This is exactly what I was trying to avoid from the start but > it wouldn't be more complicated than current code. > > As we are rather late in the 6.6 cycle (rc6 out), the safest solution > seems to be reverting commit 108a36d07c01 ("ethtool: Fix mod state of > verbose no_mask bitset") in net tree as sending a notification even on > a request which turns out to be no-op is not a serious problem (after > all, this is what happens for ioctl requests most of the time and IIRC > there are more cases where it happens for netlink requests). Then we can > fix the change detection properly in net-next in the way proposed in > previous paragraph (I would prefer not risking more intrusive changes at > this stage).
On Thu, Oct 19, 2023 at 12:21:14PM +0200, Köry Maincent wrote: > On Thu, 19 Oct 2023 11:51:40 +0200 > Michal Kubecek <mkubecek@suse.cz> wrote: > > > > The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix > > mod state of verbose no_mask bitset"). The problem is that a "no mask" > > verbose bitset only contains bit attributes for bits to be set. This > > worked correctly before this commit because we were always updating > > a zero bitmap (since commit 6699170376ab ("ethtool: fix application of > > verbose no_mask bitset"), that is) so that the rest was left zero > > naturally. But now the 1->0 change (old_val is true, bit not present in > > netlink nest) no longer works. > > Doh I had not seen this issue! Thanks you for reporting it. > I will send the revert then and will update the fix for next merge-window. Something like the diff below (against current mainline) might do the trick but it's just an idea, not even build tested. Michal diff --git a/net/ethtool/bitset.c b/net/ethtool/bitset.c index 883ed9be81f9..4d4398879c95 100644 --- a/net/ethtool/bitset.c +++ b/net/ethtool/bitset.c @@ -74,6 +74,28 @@ static void ethnl_bitmap32_clear(u32 *dst, unsigned int start, unsigned int end, } } +/** + * ethnl_bitmap32_equal() - Compare two bitmaps + * @map1: first bitmap + * @map2: second bitmap + * @nbits: bit size to compare + * + * Return: true if first @nbits are equal, false if not + */ + +static bool ethnl_bitmap32_equal(const u32 *map1, const u32 *map2, + unsigned int nbits) +{ + bool ret; + + if (memcmp(map1, map2, nbits / 32 * sizeof(u32))) + return false; + if (nbits % 32 == 0) + return true; + return !((map1[nbits / 32] ^ map2[nbits / 32]) & + ethnl_lower_bits(nbits % 32)); +} + /** * ethnl_bitmap32_not_zero() - Check if any bit is set in an interval * @map: bitmap to test @@ -431,7 +453,7 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, ethnl_string_array_t names, struct netlink_ext_ack *extack, bool *mod) { - u32 *orig_bitmap, *saved_bitmap = NULL; + u32 *saved_bitmap = NULL; struct nlattr *bit_attr; bool no_mask; bool dummy; @@ -462,9 +484,6 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, return -ENOMEM; memcpy(saved_bitmap, bitmap, nbytes); ethnl_bitmap32_clear(bitmap, 0, nbits, &dummy); - orig_bitmap = saved_bitmap; - } else { - orig_bitmap = bitmap; } nla_for_each_nested(bit_attr, tb[ETHTOOL_A_BITSET_BITS], rem) { @@ -481,7 +500,7 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, names, extack); if (ret < 0) goto out; - old_val = orig_bitmap[idx / 32] & ((u32)1 << (idx % 32)); + old_val = bitmap[idx / 32] & ((u32)1 << (idx % 32)); if (new_val != old_val) { if (new_val) bitmap[idx / 32] |= ((u32)1 << (idx % 32)); @@ -490,6 +509,8 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, *mod = true; } } + if (saved_bitmap) + *mod = ethnl_bitmap32_cmp(saved_bitmap, bitmap, nbits); ret = 0; out:
On Thu, Oct 19, 2023 at 12:50:48PM +0200, Michal Kubecek wrote: [...] > @@ -490,6 +509,8 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, > *mod = true; > } > } > + if (saved_bitmap) > + *mod = ethnl_bitmap32_cmp(saved_bitmap, bitmap, nbits); > > ret = 0; > out: Oops, this should be *mod = !ethnl_bitmap32_equal(saved_bitmap, bitmap, nbits); Michal
On Thu, 19 Oct 2023 12:50:48 +0200 Michal Kubecek <mkubecek@suse.cz> wrote: > On Thu, Oct 19, 2023 at 12:21:14PM +0200, Köry Maincent wrote: > > On Thu, 19 Oct 2023 11:51:40 +0200 > Michal Kubecek <mkubecek@suse.cz> > > wrote: > > > > > > The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix > > > mod state of verbose no_mask bitset"). The problem is that a "no mask" > > > verbose bitset only contains bit attributes for bits to be set. This > > > worked correctly before this commit because we were always updating > > > a zero bitmap (since commit 6699170376ab ("ethtool: fix application of > > > verbose no_mask bitset"), that is) so that the rest was left zero > > > naturally. But now the 1->0 change (old_val is true, bit not present in > > > netlink nest) no longer works. > > > > Doh I had not seen this issue! Thanks you for reporting it. > > I will send the revert then and will update the fix for next merge-window. > > Something like the diff below (against current mainline) might do the > trick but it's just an idea, not even build tested. Seems a good idea without adding too much complexity to the code. Will try that and send it in next merge window. Köry
On 10/19/23 06:27, Köry Maincent wrote: > On Thu, 19 Oct 2023 12:50:48 +0200 > Michal Kubecek <mkubecek@suse.cz> wrote: > >> On Thu, Oct 19, 2023 at 12:21:14PM +0200, Köry Maincent wrote: >>> On Thu, 19 Oct 2023 11:51:40 +0200 > Michal Kubecek <mkubecek@suse.cz> >>> wrote: >>>> >>>> The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix >>>> mod state of verbose no_mask bitset"). The problem is that a "no mask" >>>> verbose bitset only contains bit attributes for bits to be set. This >>>> worked correctly before this commit because we were always updating >>>> a zero bitmap (since commit 6699170376ab ("ethtool: fix application of >>>> verbose no_mask bitset"), that is) so that the rest was left zero >>>> naturally. But now the 1->0 change (old_val is true, bit not present in >>>> netlink nest) no longer works. >>> >>> Doh I had not seen this issue! Thanks you for reporting it. >>> I will send the revert then and will update the fix for next merge-window. >> >> Something like the diff below (against current mainline) might do the >> trick but it's just an idea, not even build tested. > > Seems a good idea without adding too much complexity to the code. > Will try that and send it in next merge window. Not sure what you mean by next merge window, we need a fix for right now, or we need to revert 6699170376ab ("ethtool: fix application of verbose no_mask bitset").
On Thu, Oct 19, 2023 at 09:20:31AM -0700, Florian Fainelli wrote: > On 10/19/23 06:27, Köry Maincent wrote: > > On Thu, 19 Oct 2023 12:50:48 +0200 > > Michal Kubecek <mkubecek@suse.cz> wrote: > > > > > On Thu, Oct 19, 2023 at 12:21:14PM +0200, Köry Maincent wrote: > > > > On Thu, 19 Oct 2023 11:51:40 +0200 > Michal Kubecek <mkubecek@suse.cz> > > > > wrote: > > > > > > > > > > The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix > > > > > mod state of verbose no_mask bitset"). The problem is that a "no mask" > > > > > verbose bitset only contains bit attributes for bits to be set. This > > > > > worked correctly before this commit because we were always updating > > > > > a zero bitmap (since commit 6699170376ab ("ethtool: fix application of > > > > > verbose no_mask bitset"), that is) so that the rest was left zero > > > > > naturally. But now the 1->0 change (old_val is true, bit not present in > > > > > netlink nest) no longer works. > > > > > > > > Doh I had not seen this issue! Thanks you for reporting it. > > > > I will send the revert then and will update the fix for next merge-window. > > > > > > Something like the diff below (against current mainline) might do the > > > trick but it's just an idea, not even build tested. > > > > Seems a good idea without adding too much complexity to the code. > > Will try that and send it in next merge window. > > Not sure what you mean by next merge window, we need a fix for right now, or > we need to revert 6699170376ab ("ethtool: fix application of verbose no_mask > bitset"). Not that one, that's an old commit that was correct from functional point of view (the only problem was that it sometimes triggered a notification even when the value was not changed but that also happens in other places). A revert of commit 108a36d07c01 ("ethtool: Fix mod state of verbose no_mask bitset") is already in net tree as commit 524515020f25 ("Revert "ethtool: Fix mod state of verbose no_mask bitset""). Michal
On 10/19/23 09:45, Michal Kubecek wrote: > On Thu, Oct 19, 2023 at 09:20:31AM -0700, Florian Fainelli wrote: >> On 10/19/23 06:27, Köry Maincent wrote: >>> On Thu, 19 Oct 2023 12:50:48 +0200 >>> Michal Kubecek <mkubecek@suse.cz> wrote: >>> >>>> On Thu, Oct 19, 2023 at 12:21:14PM +0200, Köry Maincent wrote: >>>>> On Thu, 19 Oct 2023 11:51:40 +0200 > Michal Kubecek <mkubecek@suse.cz> >>>>> wrote: >>>>>> >>>>>> The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix >>>>>> mod state of verbose no_mask bitset"). The problem is that a "no mask" >>>>>> verbose bitset only contains bit attributes for bits to be set. This >>>>>> worked correctly before this commit because we were always updating >>>>>> a zero bitmap (since commit 6699170376ab ("ethtool: fix application of >>>>>> verbose no_mask bitset"), that is) so that the rest was left zero >>>>>> naturally. But now the 1->0 change (old_val is true, bit not present in >>>>>> netlink nest) no longer works. >>>>> >>>>> Doh I had not seen this issue! Thanks you for reporting it. >>>>> I will send the revert then and will update the fix for next merge-window. >>>> >>>> Something like the diff below (against current mainline) might do the >>>> trick but it's just an idea, not even build tested. >>> >>> Seems a good idea without adding too much complexity to the code. >>> Will try that and send it in next merge window. >> >> Not sure what you mean by next merge window, we need a fix for right now, or >> we need to revert 6699170376ab ("ethtool: fix application of verbose no_mask >> bitset"). > > Not that one, that's an old commit that was correct from functional > point of view (the only problem was that it sometimes triggered > a notification even when the value was not changed but that also happens > in other places). > > A revert of commit 108a36d07c01 ("ethtool: Fix mod state of verbose > no_mask bitset") is already in net tree as commit 524515020f25 ("Revert > "ethtool: Fix mod state of verbose no_mask bitset""). Got it, thanks!
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c index 0ed56c9ac1bc..fcefc1bbfa2e 100644 --- a/net/ethtool/wol.c +++ b/net/ethtool/wol.c @@ -108,15 +108,16 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info) struct net_device *dev = req_info->dev; struct nlattr **tb = info->attrs; bool mod = false; + u32 wolopts = 0; int ret; dev->ethtool_ops->get_wol(dev, &wol); - ret = ethnl_update_bitset32(&wol.wolopts, WOL_MODE_COUNT, + ret = ethnl_update_bitset32(&wolopts, WOL_MODE_COUNT, tb[ETHTOOL_A_WOL_MODES], wol_mode_names, info->extack, &mod); if (ret < 0) return ret; - if (wol.wolopts & ~wol.supported) { + if (wolopts & ~wol.supported) { NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_WOL_MODES], "cannot enable unsupported WoL mode"); return -EINVAL; @@ -132,8 +133,9 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info) tb[ETHTOOL_A_WOL_SOPASS], &mod); } - if (!mod) + if (!mod && wolopts == wol.wolopts) return 0; + wol.wolopts = wolopts; ret = dev->ethtool_ops->set_wol(dev, &wol); if (ret) return ret;