diff mbox series

[net-next,04/11] wimax: fix duplicate initializer warning

Message ID 20201026213040.3889546-4-arnd@kernel.org
State New
Headers show
Series [net-next,01/11] atm: horizon: shut up clang null pointer arithmetic warning | expand

Commit Message

Arnd Bergmann Oct. 26, 2020, 9:29 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

gcc -Wextra points out multiple fields that use the same index '1'
in the wimax_gnl_policy definition:

net/wimax/stack.c:393:29: warning: initialized field overwritten [-Woverride-init]
net/wimax/stack.c:397:28: warning: initialized field overwritten [-Woverride-init]
net/wimax/stack.c:398:26: warning: initialized field overwritten [-Woverride-init]

This seems to work since all four use the same NLA_U32 value, but it
still appears to be wrong. In addition, there is no intializer for
WIMAX_GNL_MSG_PIPE_NAME, which uses the same index '2' as
WIMAX_GNL_RFKILL_STATE.

Johannes already changed this twice to improve it, but I don't think
there is a good solution, so try to work around it by using a
numeric index and adding comments.

Cc: Johannes Berg <johannes.berg@intel.com>
Fixes: 3b0f31f2b8c9 ("genetlink: make policy common to family")
Fixes: b61a5eea5904 ("wimax: use genl_register_family_with_ops()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 net/wimax/stack.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Johannes Berg Oct. 27, 2020, 7:22 a.m. UTC | #1
On Mon, 2020-10-26 at 22:29 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> gcc -Wextra points out multiple fields that use the same index '1'
> in the wimax_gnl_policy definition:
> 
> net/wimax/stack.c:393:29: warning: initialized field overwritten [-Woverride-init]
> net/wimax/stack.c:397:28: warning: initialized field overwritten [-Woverride-init]
> net/wimax/stack.c:398:26: warning: initialized field overwritten [-Woverride-init]
> 
> This seems to work since all four use the same NLA_U32 value, but it
> still appears to be wrong. In addition, there is no intializer for
> WIMAX_GNL_MSG_PIPE_NAME, which uses the same index '2' as
> WIMAX_GNL_RFKILL_STATE.

That's funny. This means that WIMAX_GNL_MSG_PIPE_NAME was never used,
since it is meant to be a string, and that won't (usually) fit into 4
bytes...

I suppose that's all an artifact of wimax being completely and utterly
dead anyway. We should probably just remove it.

> Johannes already changed this twice to improve it, but I don't think
> there is a good solution, so try to work around it by using a
> numeric index and adding comments.

Yeah, though maybe there's a better solution now.

Given that we (again and properly) have per-ops policy support, which
really was the thing that broke it here (the commit 3b0f31f2b8c9 you
mentioned), we could split this up into per-ops policies and do the
right thing in the separate policies.

OTOH, that really just makes it use more space, for no discernible
effect to userspace.


So as far as the warning fix is concerned:

Acked-by: Johannes Berg <johannes@sipsolutions.net>


Looks like I introduced a bug there with WIMAX_GNL_MSG_PIPE_NAME, but
obviously nobody cared.

johannes
Arnd Bergmann Oct. 27, 2020, 11:51 a.m. UTC | #2
On Tue, Oct 27, 2020 at 8:22 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Mon, 2020-10-26 at 22:29 +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > gcc -Wextra points out multiple fields that use the same index '1'
> > in the wimax_gnl_policy definition:
> >
> > net/wimax/stack.c:393:29: warning: initialized field overwritten [-Woverride-init]
> > net/wimax/stack.c:397:28: warning: initialized field overwritten [-Woverride-init]
> > net/wimax/stack.c:398:26: warning: initialized field overwritten [-Woverride-init]
> >
> > This seems to work since all four use the same NLA_U32 value, but it
> > still appears to be wrong. In addition, there is no intializer for
> > WIMAX_GNL_MSG_PIPE_NAME, which uses the same index '2' as
> > WIMAX_GNL_RFKILL_STATE.
>
> That's funny. This means that WIMAX_GNL_MSG_PIPE_NAME was never used,
> since it is meant to be a string, and that won't (usually) fit into 4
> bytes...
>
> I suppose that's all an artifact of wimax being completely and utterly
> dead anyway. We should probably just remove it.

Makes sense. I checked
https://en.wikipedia.org/wiki/List_of_WiMAX_networks, and it appears
that these entries are all stale, after everyone has migrated to LTE
or discontinued their service altogether.

NetworkManager appears to have dropped userspace support in 2015
https://bugzilla.gnome.org/show_bug.cgi?id=747846, the
www.linuxwimax.org site had already shut down earlier.

WiMax is apparently still being deployed on airport campus
networks ("AeroMACS"), but in a frequency band that was not
supported by the old Intel 2400m (used in Sandy Bridge laptops
and earlier), which is the only driver using the kernel's wimax
stack.

Inaky, do you have any additional information about possible
users? If we are sure there are none, then I'd suggest removing
all the wimax code directly, otherwise it could go through
drivers/staging/ for a release or two (and move it back in case
there are users after all). I can send a patch if you like.

> So as far as the warning fix is concerned:
>
> Acked-by: Johannes Berg <johannes@sipsolutions.net>
>

Thanks!

        Arnd
Perez-Gonzalez, Inaky Oct. 27, 2020, 2:51 p.m. UTC | #3
> From: Arnd Bergmann <arnd@kernel.org>

> 

> Makes sense. I checked

> https://en.wikipedia.org/wiki/List_of_WiMAX_networks, and it appears

> that these entries are all stale, after everyone has migrated to LTE

> or discontinued their service altogether.

> 

> NetworkManager appears to have dropped userspace support in 2015

> https://bugzilla.gnome.org/show_bug.cgi?id=747846, the

> www.linuxwimax.org site had already shut down earlier.

> 

> WiMax is apparently still being deployed on airport campus

> networks ("AeroMACS"), but in a frequency band that was not

> supported by the old Intel 2400m (used in Sandy Bridge laptops

> and earlier), which is the only driver using the kernel's wimax

> stack.

> 

> Inaky, do you have any additional information about possible

> users? If we are sure there are none, then I'd suggest removing

> all the wimax code directly, otherwise it could go through

> drivers/staging/ for a release or two (and move it back in case

> there are users after all). I can send a patch if you like.


I have not

Every now and then I get the occasional message from a student or
researcher asking for support about a production network, but they
have dwindled in the last years.

My vote would be to scrap the whole thing; if there are die hard
users, they can always rise up and move it back from staging.
diff mbox series

Patch

diff --git a/net/wimax/stack.c b/net/wimax/stack.c
index b6dd9d956ed8..3a62af3f80bf 100644
--- a/net/wimax/stack.c
+++ b/net/wimax/stack.c
@@ -388,17 +388,24 @@  void wimax_dev_init(struct wimax_dev *wimax_dev)
 }
 EXPORT_SYMBOL_GPL(wimax_dev_init);
 
+/*
+ * There are multiple enums reusing the same values, adding
+ * others is only possible if they use a compatible policy.
+ */
 static const struct nla_policy wimax_gnl_policy[WIMAX_GNL_ATTR_MAX + 1] = {
-	[WIMAX_GNL_RESET_IFIDX] = { .type = NLA_U32, },
-	[WIMAX_GNL_RFKILL_IFIDX] = { .type = NLA_U32, },
-	[WIMAX_GNL_RFKILL_STATE] = {
-		.type = NLA_U32		/* enum wimax_rf_state */
-	},
-	[WIMAX_GNL_STGET_IFIDX] = { .type = NLA_U32, },
-	[WIMAX_GNL_MSG_IFIDX] = { .type = NLA_U32, },
-	[WIMAX_GNL_MSG_DATA] = {
-		.type = NLA_UNSPEC,	/* libnl doesn't grok BINARY yet */
-	},
+	/*
+	 * WIMAX_GNL_RESET_IFIDX, WIMAX_GNL_RFKILL_IFIDX,
+	 * WIMAX_GNL_STGET_IFIDX, WIMAX_GNL_MSG_IFIDX
+	 */
+	[1] = { .type = NLA_U32, },
+	/*
+	 * WIMAX_GNL_RFKILL_STATE, WIMAX_GNL_MSG_PIPE_NAME
+	 */
+	[2] = { .type = NLA_U32, }, /* enum wimax_rf_state */
+	/*
+	 * WIMAX_GNL_MSG_DATA
+	 */
+	[3] = { .type = NLA_UNSPEC, }, /* libnl doesn't grok BINARY yet */
 };
 
 static const struct genl_small_ops wimax_gnl_ops[] = {