Fix fallthrough warning (#11)

[aesophor]

fixes #11

[antoineco]

Thanks for the fix @aesophor 🙌 And thanks for rebasing on my 5.4 branch :) I'll look into the problem in detail and merge this later today.

[antoineco]

@aesophor just one note:

The GCC manual mentions

Since there are occasions where a switch case fall through is desirable, GCC provides an attribute, __attribute__ ((fallthrough)), that is to be used along with a null statement to suppress this warning that would normally occur:

switch (cond)
  {
  case 1:
    bar (0);
    __attribute__ ((fallthrough));
  default:
    …
  }

While it also mentions

Instead of the these attributes, it is also possible to add a fallthrough comment to silence the warning.

the behaviour varies depending on the n in -Wimplicit-fallthrough=n, which feels slightly less portable to me.

May I suggest we also use __attribute__ ((fallthrough)); instead of a comment in this code?

[aesophor]

@antoineco Thanks so much for your detailed reply!

I replaced the comment with __attribute__ ((fallthrough)); but the driver won't compile.

/home/aesophor/Code/broadcom-wl [git::fix-fallthru-warning *] [aesophor@adagio] [17:22]
> make
KBUILD_NOPEDANTIC=1 make -C /lib/modules/`uname -r`/build M=`pwd`
make[1]: Entering directory '/usr/src/linux-5.4.28-gentoo'
CFG80211 API is prefered for this kernel version
Using CFG80211 API
  AR      /home/aesophor/Code/broadcom-wl/built-in.a
  CC [M]  /home/aesophor/Code/broadcom-wl/src/shared/linux_osl.o
  CC [M]  /home/aesophor/Code/broadcom-wl/src/wl/sys/wl_linux.o
  CC [M]  /home/aesophor/Code/broadcom-wl/src/wl/sys/wl_iw.o
  CC [M]  /home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.o
/home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.c: In function ‘wl_set_auth_type’:
/home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.c:818:19: error: expected ‘)’ before ‘__attribute__’
  818 |   __attribute__ ((fallthrough));
      |                   ^
      |                   )
/home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.c:818:31: error: expected identifier or ‘(’ before ‘)’ token
  818 |   __attribute__ ((fallthrough));
      |                               ^
/home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.c:818:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
  818 |   __attribute__ ((fallthrough));
      |   ^~~~~~~~~~~~~
In file included from /home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.c:43:
/home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.h:57:5: warning: this statement may fall through [-Wimplicit-fallthrough=]
   57 |  if (wl_dbg_level & WL_DBG_DBG) {   \
      |     ^
/home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.c:817:3: note: in expansion of macro ‘WL_DBG’
  817 |   WL_DBG(("network eap\n"));
      |   ^~~~~~
/home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.c:819:2: note: here
  819 |  default:
      |  ^~~~~~~
make[2]: *** [scripts/Makefile.build:266: /home/aesophor/Code/broadcom-wl/src/wl/sys/wl_cfg80211_hybrid.o] Error 1
make[1]: *** [Makefile:1691: /home/aesophor/Code/broadcom-wl] Error 2
make[1]: Leaving directory '/usr/src/linux-5.4.28-gentoo'
make: *** [Makefile:159: all] Error 2

Looking at the build log, I think maybe C90 doesn't support statement attribute syntax? It says a ')' is expected before __attribute__, which looks like function attribute syntax.

Edit: I've looked into the gcc manual but haven't found whether C90 supports __attribute__ ((fallthrough)); or not.

[antoineco]

Interesting. The attribute is still documented in the GCC 9.3.0 documentation (I should have linked that above actually, sorry): https://gcc.gnu.org/onlinedocs/gcc-9.3.0/gcc/Warning-Options.html#index-Wimplicit-fallthrough

[antoineco]

Here's another reference to that particular attribute, in the same docs: https://gcc.gnu.org/onlinedocs/gcc-9.3.0/gcc/Statement-Attributes.html#Statement-Attributes

Now I'm puzzled... 🤔

[aesophor]

I wrote a tiny program and compiled it with -std=gnu89 and -std=gnu90 respectively, and they both work... It's really weird, I'll see if I can get this kernel module to compile later.

/home/aesophor/Code [aesophor@adagio] [18:16]
> cat statement-attribute-test.c
int main() {
        int dummy = 0;
        int i = 0;

        switch (i) {
        case 1:
                dummy = 1;
                break;
        case 2:
                dummy = 2;
                __attribute__ ((fallthrough));
        default:
                dummy = 3;
                break;
        }
}

/home/aesophor/Code [aesophor@adagio] [18:16]
> gcc -o out statement-attribute-test.c -Wimplicit-fallthrough=5 -std=gnu89

/home/aesophor/Code [aesophor@adagio] [18:16]
> gcc -o out statement-attribute-test.c -Wimplicit-fallthrough=5 -std=gnu90

[antoineco]

Does this demo program fail with -std=gnu11? (the default)

If the attribute is poorly supported I guess we can also use the magic comment, even if it's a bit more hacky. The goal is to make this work for as many people as possible after all.

[aesophor]

Does this demo program fail with -std=gnu11? (the default)

It compiles successfully.

/home/aesophor/Code [aesophor@adagio] [18:17]
> gcc -o out statement-attribute-test.c -Wimplicit-fallthrough=5 -std=gnu11

/home/aesophor/Code [aesophor@adagio] [18:27]
> gcc -o out statement-attribute-test.c -Wimplicit-fallthrough=5

[aesophor]

I finally got the module to compile with __attribute__ ((__fallthrough__));.

After reading the conversations on mailing list [1][2], it turns out currently Linux kernel uses the magical comment to silence this warning in most places. However, someone has added a fallthrough macro which expands to __attribute__ ((__fallthrough__)) (Not sure why there's an extra pair of double underscore...)

So I found the definition of this fallthrough macro in the kernel source tree: source: https://github.com/torvalds/linux/blob/master/include/linux/compiler_attributes.h#L200

#if __has_attribute(__fallthrough__)
# define fallthrough                    __attribute__((__fallthrough__))
#else
# define fallthrough                    do {} while (0)  /* fallthrough */
#endif

Although in most places of Linux kernel they still use the magical comment, I think using this macro is fine?

[antoineco]

Unfortunately this macro was only added in the 5.4 kernel, so that will break for all people that use a kernel below that version.

Bottom line, the magic comment is probably the best approach in terms of compatibility.

Anyway, nice catch, and thanks a lot for investigating! 👍 As soon as you revert the macro change I'll be ready to merge.

[aesophor]

@antoineco

Oops, sorry I should have noticed that!

Thank you very much for maintaining this driver :+1: I'm really grateful, without it I would have lost my wifi interface for good lol...

[antoineco]

You're welcome!