Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Thu, 19 Oct 2023 11:51:40 +0200
From:   Michal Kubecek <mkubecek@suse.cz>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Kory Maincent <kory.maincent@bootlin.com>,
        "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>, kernel@pengutronix.de,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net v1 1/1] ethtool: fix clearing of WoL flags
Message-ID: <20231019095140.l6fffnszraeb6iiw@lion.mk-sys.cz>
References: <20231019070904.521718-1-o.rempel@pengutronix.de>
 <20231019090510.bbcmh7stzqqgchdd@lion.mk-sys.cz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="tpdxxlqwiqidmzdl"
Content-Disposition: inline
In-Reply-To: <20231019090510.bbcmh7stzqqgchdd@lion.mk-sys.cz>
Precedence: bulk


--tpdxxlqwiqidmzdl
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 re=
move
> > 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
> > ...
> >=20
>=20
> 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

--tpdxxlqwiqidmzdl
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEEWN3j3bieVmp26mKO538sG/LRdpUFAmUw/CgACgkQ538sG/LR
dpU2pQgAiW+HDx0vS9hgoro2w7gEAC0jlJKSLt6D3D1kHT2A737InqxKzryMB8NM
iyp+2XGVWRQPhMoZVSBZ/vgFQwCxsu1HOPNP5Xh8nIMWA4e2P3U5poqK1nVkg53N
P03gigkrx/bBxijYCwNAWlAiCLHRI3jUihTlXXjku3BogLuWnbMz75ueNwct64Zy
0r6lt28aqYyFAJhkZJG+GBUIxfte0MQt3TqQiarUd5lJjrvhkhyNJMByEtgjnElz
g2sZ+EtxCKTOS7rNCoOkl+2uMKDXCqebfPb+ZMwCXkVJNKn5G6zNPIo88wCFfUgk
CLcFyw33UUda+wg8flEoSO47edwWFQ==
=cRHe
-----END PGP SIGNATURE-----

--tpdxxlqwiqidmzdl--