Re: [PATCH net v1 1/1] ethtool: fix clearing of WoL flags

[Köry Maincent]

On Thu, 19 Oct 2023 11:51:40 +0200
Michal Kubecek <mkubecek@xxxxxxx> 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).