diff mbox series

[net] Revert "ethtool: Fix mod state of verbose no_mask bitset"

Message ID 20231019-feature_ptp_bitset_fix-v1-1-70f3c429a221@bootlin.com
State New
Headers
Series [net] Revert "ethtool: Fix mod state of verbose no_mask bitset" |

Commit Message

Köry Maincent Oct. 19, 2023, 1:16 p.m. UTC 
  This reverts commit 108a36d07c01edbc5942d27c92494d1c6e4d45a0.

It was reported that this fix breaks the possibility 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 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.

Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reported-by: Michal Kubecek <mkubecek@suse.cz>
Closes: https://lore.kernel.org/netdev/20231019095140.l6fffnszraeb6iiw@lion.mk-sys.cz/
Cc: stable@vger.kernel.org
Fixes: 108a36d07c01 ("ethtool: Fix mod state of verbose no_mask bitset")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

This patch is reverted for now as we are approaching the end of the
merge-window. The real fix that fix the mod value will be sent later
on the next merge-window.
---
 net/ethtool/bitset.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)


---
base-commit: a602ee3176a81280b829c9f0cf259450f7982168
change-id: 20231019-feature_ptp_bitset_fix-6bf75671b33e

Best regards,

Comments

Michal Kubecek Oct. 19, 2023, 1:56 p.m. UTC | #1 
On Thu, Oct 19, 2023 at 03:16:41PM +0200, Kory Maincent wrote:
> This reverts commit 108a36d07c01edbc5942d27c92494d1c6e4d45a0.
> 
> It was reported that this fix breaks the possibility 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 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.
> 
> Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reported-by: Michal Kubecek <mkubecek@suse.cz>
> Closes: https://lore.kernel.org/netdev/20231019095140.l6fffnszraeb6iiw@lion.mk-sys.cz/
> Cc: stable@vger.kernel.org
> Fixes: 108a36d07c01 ("ethtool: Fix mod state of verbose no_mask bitset")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

> ---
> 
> This patch is reverted for now as we are approaching the end of the
> merge-window. The real fix that fix the mod value will be sent later
> on the next merge-window.
> ---

For the record, the term "merge window" is used for the 2-week interval
between a final and following rc1, not for the whole interval between
two final releases. 

Michal
patchwork-bot+netdevbpf@kernel.org Oct. 19, 2023, 4:30 p.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 19 Oct 2023 15:16:41 +0200 you wrote:
> This reverts commit 108a36d07c01edbc5942d27c92494d1c6e4d45a0.
> 
> It was reported that this fix breaks the possibility 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
> ...
> 
> [...]

Here is the summary with links:
  - [net] Revert "ethtool: Fix mod state of verbose no_mask bitset"
    https://git.kernel.org/netdev/net/c/524515020f25

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/ethtool/bitset.c b/net/ethtool/bitset.c
index 883ed9be81f9..0515d6604b3b 100644
--- a/net/ethtool/bitset.c
+++ b/net/ethtool/bitset.c
@@ -431,10 +431,8 @@  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;
 	struct nlattr *bit_attr;
 	bool no_mask;
-	bool dummy;
 	int rem;
 	int ret;
 
@@ -450,22 +448,8 @@  ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits,
 	}
 
 	no_mask = tb[ETHTOOL_A_BITSET_NOMASK];
-	if (no_mask) {
-		unsigned int nwords = DIV_ROUND_UP(nbits, 32);
-		unsigned int nbytes = nwords * sizeof(u32);
-
-		/* The bitmap size is only the size of the map part without
-		 * its mask part.
-		 */
-		saved_bitmap = kcalloc(nwords, sizeof(u32), GFP_KERNEL);
-		if (!saved_bitmap)
-			return -ENOMEM;
-		memcpy(saved_bitmap, bitmap, nbytes);
-		ethnl_bitmap32_clear(bitmap, 0, nbits, &dummy);
-		orig_bitmap = saved_bitmap;
-	} else {
-		orig_bitmap = bitmap;
-	}
+	if (no_mask)
+		ethnl_bitmap32_clear(bitmap, 0, nbits, mod);
 
 	nla_for_each_nested(bit_attr, tb[ETHTOOL_A_BITSET_BITS], rem) {
 		bool old_val, new_val;
@@ -474,14 +458,13 @@  ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits,
 		if (nla_type(bit_attr) != ETHTOOL_A_BITSET_BITS_BIT) {
 			NL_SET_ERR_MSG_ATTR(extack, bit_attr,
 					    "only ETHTOOL_A_BITSET_BITS_BIT allowed in ETHTOOL_A_BITSET_BITS");
-			ret = -EINVAL;
-			goto out;
+			return -EINVAL;
 		}
 		ret = ethnl_parse_bit(&idx, &new_val, nbits, bit_attr, no_mask,
 				      names, extack);
 		if (ret < 0)
-			goto out;
-		old_val = orig_bitmap[idx / 32] & ((u32)1 << (idx % 32));
+			return ret;
+		old_val = bitmap[idx / 32] & ((u32)1 << (idx % 32));
 		if (new_val != old_val) {
 			if (new_val)
 				bitmap[idx / 32] |= ((u32)1 << (idx % 32));
@@ -491,10 +474,7 @@  ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits,
 		}
 	}
 
-	ret = 0;
-out:
-	kfree(saved_bitmap);
-	return ret;
+	return 0;
 }
 
 static int ethnl_compact_sanity_checks(unsigned int nbits,