[PATCH 00/18] Annotate switch/case fallthrough cases

[PATCH 00/18] Annotate switch/case fallthrough cases

[Andre Przywara]

C's implicit fallthrough behaviour in switch/case statements can lead to
subtle bugs. Quite some while ago many compilers introduced warnings in
those cases, requiring intentional fallthrough's to be annotated.
    
So far we were not enabling that compiler option, so many ambiguities
and some bugs in the code went unnoticed.

This series adds the required annotations in code paths that the first
stage of the U-Boot CI covers. There is a large number of cases left
in the libbz2 code. The usage of switch/case is borderline insane there,
labels are hidden in macros, and there are no breaks, but just goto's.
Upstream still uses very similar code, without any annotations. I still
am not 100% sure those are meant to fall through or not, and plan to do
further investigations, but didn't want to hold the rest of the patches
back. You can see for yourself by applying patch 18/18 and building for
sandbox64, for instance.

Because of this we cannot quite enable the warning in the Makefile yet,
but those fixes are worth regardless, and be it to increase readability.

Please not that those patches do not fix anything, really, they just add
those fallthrough annotations, so the series is not really critical.

Cheers,
Andre

Andre Przywara (18):
  spl: mmc: properly annotate fallthrough
  zlib: annotate switch/case fallthrough cases
  gadget: f_thor: annotate switch/case fallthrough
  use proper fallthrough annotations
  net/net: fix switch/case fallthrough annotations
  fastboot: annotate switch/case fallthrough case
  net: sun8i-emac: annotate fallthrough
  usb: ohci-hcd: annotate switch/case fallthrough
  usb: xhci: annotate switch/case fallthrough properly (TBF)
  video: annotate switch/case fall-through
  net: e1000: annotate switch/case fallthrough
  mtd: ubi: annotate fallthrough (TBF)
  arm: mach-k3: am62p: annotate switch/case fallthrough
  mtd: spi-nor-tiny: annotate switch/case fallthrough
  mtd: rawnand: nand_base: annotate switch/case fallthrough
  cmd: pmic: annotate switch/case fallthrough
  cmd: spl: annotate switch/case fallthrough
  [DO NOT MERGE] Makefile: enable switch/case fallthrough warnings

 Makefile                              |  1 +
 arch/arm/mach-k3/am62px/am62p5_init.c |  1 +
 cmd/pmic.c                            |  1 +
 cmd/spl.c                             |  2 ++
 common/command.c                      |  2 +-
 common/spl/spl_mmc.c                  |  1 +
 drivers/fastboot/fb_command.c         |  1 +
 drivers/mtd/nand/raw/nand_base.c      |  4 ++++
 drivers/mtd/spi/spi-nor-tiny.c        |  1 +
 drivers/mtd/ubi/attach.c              |  1 +
 drivers/mtd/ubi/build.c               |  3 +++
 drivers/net/e1000.c                   |  2 ++
 drivers/net/sun8i_emac.c              |  1 +
 drivers/usb/gadget/f_thor.c           |  1 +
 drivers/usb/host/ohci-hcd.c           |  3 ++-
 drivers/usb/host/xhci.c               |  2 +-
 drivers/video/video-uclass.c          |  2 ++
 lib/tiny-printf.c                     |  2 +-
 lib/zlib/inflate.c                    | 21 +++++++++++++++++++++
 net/net.c                             |  5 ++---
 20 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 01/18] spl: mmc: properly annotate fallthrough

[Andre Przywara]

Depending on the various MMC boot configurations, we might end up with
trying filesystem mode when a raw image boot failed. This fall-through
in the switch/case statement is explained in a comment, but this is not
visible to the compiler, which still will complain.

Add the proper compiler-visible annotation, to allow enabling the
compiler check in the future.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 common/spl/spl_mmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index fe4230170a0..d06f9f0dee6 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -411,6 +411,7 @@ int spl_mmc_load(struct spl_image_info *spl_image,
 			return 0;
 #endif
 		/* If RAW mode fails, try FS mode. */
+		fallthrough;
 #ifdef CONFIG_SYS_MMCSD_FS_BOOT
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");
-- 
2.25.1

Re: [PATCH 01/18] spl: mmc: properly annotate fallthrough

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Thu, Mar 27, 2025 at 03:32:56PM +0000, Andre Przywara wrote:

> Depending on the various MMC boot configurations, we might end up with
> trying filesystem mode when a raw image boot failed. This fall-through
> in the switch/case statement is explained in a comment, but this is not
> visible to the compiler, which still will complain.
> 
> Add the proper compiler-visible annotation, to allow enabling the
> compiler check in the future.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 02/18] zlib: annotate switch/case fallthrough cases

[Andre Przywara]

The inflate state machine in zlib uses switch/case fall-through's
extensively, as it sometimes advances the state, and lets the
conveniently placed next case statement handle the new state already.
The pattern here is:
		state->mode = LEN;
	case LEN:

Annotate those occasions with the "fallthrough;" macro, to let compilers
know this is fine when using -Wimplicit-fallthrough.

This mimics the upstream commit 76f70abbc73f:
Author:  Mark Adler <madler@alumni.caltech.edu>
Date:    Sun Mar 27 00:12:38 2022 -0700
Subject: Add fallthrough comments for gcc.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 lib/zlib/inflate.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/lib/zlib/inflate.c b/lib/zlib/inflate.c
index b4c72cc2c5c..2b7a464f74f 100644
--- a/lib/zlib/inflate.c
+++ b/lib/zlib/inflate.c
@@ -420,6 +420,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             if (state->flags & 0x0200) CRC2(state->check, hold);
             INITBITS();
             state->mode = TIME;
+            fallthrough;
         case TIME:
             NEEDBITS(32);
             if (state->head != Z_NULL)
@@ -427,6 +428,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             if (state->flags & 0x0200) CRC4(state->check, hold);
             INITBITS();
             state->mode = OS;
+            fallthrough;
         case OS:
             NEEDBITS(16);
             if (state->head != Z_NULL) {
@@ -436,6 +438,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             if (state->flags & 0x0200) CRC2(state->check, hold);
             INITBITS();
             state->mode = EXLEN;
+            fallthrough;
         case EXLEN:
             if (state->flags & 0x0400) {
                 NEEDBITS(16);
@@ -448,6 +451,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             else if (state->head != Z_NULL)
                 state->head->extra = Z_NULL;
             state->mode = EXTRA;
+            fallthrough;
         case EXTRA:
             if (state->flags & 0x0400) {
                 copy = state->length;
@@ -471,6 +475,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
             state->length = 0;
             state->mode = NAME;
+            fallthrough;
         case NAME:
             if (state->flags & 0x0800) {
                 if (have == 0) goto inf_leave;
@@ -492,6 +497,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
                 state->head->name = Z_NULL;
             state->length = 0;
             state->mode = COMMENT;
+            fallthrough;
         case COMMENT:
             if (state->flags & 0x1000) {
                 if (have == 0) goto inf_leave;
@@ -512,6 +518,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             else if (state->head != Z_NULL)
                 state->head->comment = Z_NULL;
             state->mode = HCRC;
+            fallthrough;
         case HCRC:
             if (state->flags & 0x0200) {
                 NEEDBITS(16);
@@ -535,6 +542,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             strm->adler = state->check = REVERSE(hold);
             INITBITS();
             state->mode = DICT;
+            fallthrough;
         case DICT:
             if (state->havedict == 0) {
                 RESTORE();
@@ -542,9 +550,11 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
             strm->adler = state->check = adler32(0L, Z_NULL, 0);
             state->mode = TYPE;
+            fallthrough;
         case TYPE:
 	    schedule();
             if (flush == Z_BLOCK) goto inf_leave;
+            fallthrough;
         case TYPEDO:
             if (state->last) {
                 BYTEBITS();
@@ -590,6 +600,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
                     state->length));
             INITBITS();
             state->mode = COPY;
+            fallthrough;
         case COPY:
             copy = state->length;
             if (copy) {
@@ -625,6 +636,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             Tracev((stderr, "inflate:       table sizes ok\n"));
             state->have = 0;
             state->mode = LENLENS;
+            fallthrough;
         case LENLENS:
             while (state->have < state->ncode) {
                 NEEDBITS(3);
@@ -646,6 +658,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             Tracev((stderr, "inflate:       code lengths ok\n"));
             state->have = 0;
             state->mode = CODELENS;
+            fallthrough;
         case CODELENS:
             while (state->have < state->nlen + state->ndist) {
                 for (;;) {
@@ -720,6 +733,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
             Tracev((stderr, "inflate:       codes ok\n"));
             state->mode = LEN;
+            fallthrough;
         case LEN:
 	    schedule();
             if (have >= 6 && left >= 258) {
@@ -764,6 +778,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
             state->extra = (unsigned)(this.op) & 15;
             state->mode = LENEXT;
+            fallthrough;
         case LENEXT:
             if (state->extra) {
                 NEEDBITS(state->extra);
@@ -772,6 +787,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
             Tracevv((stderr, "inflate:         length %u\n", state->length));
             state->mode = DIST;
+            fallthrough;
         case DIST:
             for (;;) {
                 this = state->distcode[BITS(state->distbits)];
@@ -797,6 +813,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             state->offset = (unsigned)this.val;
             state->extra = (unsigned)(this.op) & 15;
             state->mode = DISTEXT;
+            fallthrough;
         case DISTEXT:
             if (state->extra) {
                 NEEDBITS(state->extra);
@@ -817,6 +834,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
             Tracevv((stderr, "inflate:         distance %u\n", state->offset));
             state->mode = MATCH;
+            fallthrough;
         case MATCH:
             if (left == 0) goto inf_leave;
             copy = out - left;
@@ -872,6 +890,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
 #ifdef GUNZIP
             state->mode = LENGTH;
+            fallthrough;
         case LENGTH:
             if (state->wrap && state->flags) {
                 NEEDBITS(32);
@@ -885,6 +904,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
             }
 #endif
             state->mode = DONE;
+            fallthrough;
         case DONE:
             ret = Z_STREAM_END;
             goto inf_leave;
@@ -894,6 +914,7 @@ __rcode int ZEXPORT inflate(z_streamp strm, int flush)
         case MEM:
             return Z_MEM_ERROR;
         case SYNC:
+            fallthrough;
         default:
             return Z_STREAM_ERROR;
         }
-- 
2.25.1

Re: [PATCH 02/18] zlib: annotate switch/case fallthrough cases

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Thu, Mar 27, 2025 at 03:32:57PM +0000, Andre Przywara wrote:

> The inflate state machine in zlib uses switch/case fall-through's
> extensively, as it sometimes advances the state, and lets the
> conveniently placed next case statement handle the new state already.
> The pattern here is:
> 		state->mode = LEN;
> 	case LEN:
> 
> Annotate those occasions with the "fallthrough;" macro, to let compilers
> know this is fine when using -Wimplicit-fallthrough.
> 
> This mimics the upstream commit 76f70abbc73f:
> Author:  Mark Adler <madler@alumni.caltech.edu>
> Date:    Sun Mar 27 00:12:38 2022 -0700
> Subject: Add fallthrough comments for gcc.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Link: https://github.com/madler/zlib/commit/76f70abbc73f
Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 03/18] gadget: f_thor: annotate switch/case fallthrough

[Andre Przywara]

Even though we seem to catch POWEROFF and EFSCLEAR commands in the THOR
protocol request handling, we ultimately do not seem to handle them
(apart from sending a response), so those commands still print an error
message.

Annotate the switch/case fallthrough in this case, to make this clear to
the compiler.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/usb/gadget/f_thor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
index 54372118348..540b9f88237 100644
--- a/drivers/usb/gadget/f_thor.c
+++ b/drivers/usb/gadget/f_thor.c
@@ -138,6 +138,7 @@ static int process_rqt_cmd(struct udevice *udc, const struct rqt_box *rqt)
 	case RQT_CMD_POWEROFF:
 	case RQT_CMD_EFSCLEAR:
 		send_rsp(udc, rsp);
+		fallthrough;
 	default:
 		printf("Command not supported -> cmd: %d\n", rqt->rqt_data);
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH 03/18] gadget: f_thor: annotate switch/case fallthrough

[Mattijs Korpershoek]

Hi Andre,

Thank you for the patch.

On jeu., mars 27, 2025 at 15:32, Andre Przywara <andre.przywara@arm.com> wrote:

> Even though we seem to catch POWEROFF and EFSCLEAR commands in the THOR
> protocol request handling, we ultimately do not seem to handle them
> (apart from sending a response), so those commands still print an error
> message.
>
> Annotate the switch/case fallthrough in this case, to make this clear to
> the compiler.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/f_thor.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
> index 54372118348..540b9f88237 100644
> --- a/drivers/usb/gadget/f_thor.c
> +++ b/drivers/usb/gadget/f_thor.c
> @@ -138,6 +138,7 @@ static int process_rqt_cmd(struct udevice *udc, const struct rqt_box *rqt)
>  	case RQT_CMD_POWEROFF:
>  	case RQT_CMD_EFSCLEAR:
>  		send_rsp(udc, rsp);
> +		fallthrough;
>  	default:
>  		printf("Command not supported -> cmd: %d\n", rqt->rqt_data);
>  		return -EINVAL;
> -- 
> 2.25.1

[PATCH 04/18] use proper fallthrough annotations

[Andre Przywara]

In some cases in the generic code, we were already using switch/case
fallthrough annotations comments, though in a way which might not be
understood by most compilers.

Replace two non-standard /* no break */ comments with our fallthrough;
statement-like macro, to make this visible to the compiler.
Also use this macro in place of an /* Fall through */ comment, to be
more consistent.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 common/command.c  | 2 +-
 lib/tiny-printf.c | 2 +-
 net/net.c         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/command.c b/common/command.c
index 3f691399cbe..0f9dd06d72b 100644
--- a/common/command.c
+++ b/common/command.c
@@ -484,7 +484,7 @@ int cmd_get_data_size(const char *arg, int default_size)
 		case 'q':
 			if (MEM_SUPPORT_64BIT_DATA)
 				return 8;
-			/* no break */
+			fallthrough;
 		default:
 			return CMD_DATA_SIZE_ERR;
 		}
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 0503c17341f..b8fc8355c4a 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -282,7 +282,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 					break;
 				}
 				islong = true;
-				/* no break */
+				fallthrough;
 			case 'x':
 				if (islong) {
 					num = va_arg(va, unsigned long);
diff --git a/net/net.c b/net/net.c
index 1828f1cca36..5219367e391 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1559,7 +1559,7 @@ common:
 			puts("*** ERROR: `ipaddr' not set\n");
 			return 1;
 		}
-		/* Fall through */
+		fallthrough;
 
 #ifdef CONFIG_CMD_RARP
 	case RARP:
-- 
2.25.1

Re: [PATCH 04/18] use proper fallthrough annotations

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Thu, Mar 27, 2025 at 03:32:59PM +0000, Andre Przywara wrote:

> In some cases in the generic code, we were already using switch/case
> fallthrough annotations comments, though in a way which might not be
> understood by most compilers.
> 
> Replace two non-standard /* no break */ comments with our fallthrough;
> statement-like macro, to make this visible to the compiler.
> Also use this macro in place of an /* Fall through */ comment, to be
> more consistent.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 05/18] net/net: fix switch/case fallthrough annotations

[Andre Przywara]

The net_check_prereq() routine in the generic network handling code
mixes case: labels with #ifdef's, which makes predicting fallthrough
situations tricky. We had two "fall through" comments in the code, but
at the wrong places.

Remove one unneeded comment (no annotations necessary between just empty
labels), and move one other instance to the right place (before any
label sequence).
This makes GCC's implicit fallthrough checker happy.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 net/net.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index 5219367e391..f191f16357c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1525,7 +1525,6 @@ static int net_check_prereq(enum proto_t protocol)
 #if defined(CONFIG_CMD_NFS)
 	case NFS:
 #endif
-		/* Fall through */
 	case TFTPGET:
 	case TFTPPUT:
 		if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
@@ -1539,11 +1538,11 @@ static int net_check_prereq(enum proto_t protocol)
 			puts("*** ERROR: `serverip' not set\n");
 			return 1;
 		}
+		fallthrough;
 #if	defined(CONFIG_CMD_PING) || \
 	defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
 common:
 #endif
-		/* Fall through */
 
 	case NETCONS:
 	case FASTBOOT_UDP:
-- 
2.25.1

Re: [PATCH 05/18] net/net: fix switch/case fallthrough annotations

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Thu, Mar 27, 2025 at 03:33:00PM +0000, Andre Przywara wrote:

> The net_check_prereq() routine in the generic network handling code
> mixes case: labels with #ifdef's, which makes predicting fallthrough
> situations tricky. We had two "fall through" comments in the code, but
> at the wrong places.
> 
> Remove one unneeded comment (no annotations necessary between just empty
> labels), and move one other instance to the right place (before any
> label sequence).
> This makes GCC's implicit fallthrough checker happy.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 05/18] net/net: fix switch/case fallthrough annotations

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

On Thu, Mar 27, 2025 at 03:33:00PM +0000, Andre Przywara wrote:

> The net_check_prereq() routine in the generic network handling code
> mixes case: labels with #ifdef's, which makes predicting fallthrough
> situations tricky. We had two "fall through" comments in the code, but
> at the wrong places.
> 
> Remove one unneeded comment (no annotations necessary between just empty
> labels), and move one other instance to the right place (before any
> label sequence).
> This makes GCC's implicit fallthrough checker happy.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> ---
>  net/net.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/net.c b/net/net.c
> index 5219367e391..f191f16357c 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1525,7 +1525,6 @@ static int net_check_prereq(enum proto_t protocol)
>  #if defined(CONFIG_CMD_NFS)
>  	case NFS:
>  #endif
> -		/* Fall through */
>  	case TFTPGET:
>  	case TFTPPUT:
>  		if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
> @@ -1539,11 +1538,11 @@ static int net_check_prereq(enum proto_t protocol)
>  			puts("*** ERROR: `serverip' not set\n");
>  			return 1;
>  		}
> +		fallthrough;
>  #if	defined(CONFIG_CMD_PING) || \
>  	defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
>  common:
>  #endif
> -		/* Fall through */
>  
>  	case NETCONS:
>  	case FASTBOOT_UDP:

So this one is harder than it looks. With clang, we cannot seemingly
have:
	fallthrough;
#if defined(CONFIG_CMD_PING) || \
    defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
common:
#endif

And gcc was failing on:
	}
#if defined(CONFIG_CMD_PING) || \
    defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
common:
#endif
	fallthrough;

Maybe we can move the label to inside the next set of cases, and then
also add CONFIG_CMD_PING6 to the checks, as that also has 'goto common;'

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 05/18] net/net: fix switch/case fallthrough annotations

[Andre Przywara]

On Tue, 8 Apr 2025 16:29:18 -0600
Tom Rini <trini@konsulko.com> wrote:

Hi Tom,

thanks for staying on this!

> On Thu, Mar 27, 2025 at 03:33:00PM +0000, Andre Przywara wrote:
> 
> > The net_check_prereq() routine in the generic network handling code
> > mixes case: labels with #ifdef's, which makes predicting fallthrough
> > situations tricky. We had two "fall through" comments in the code, but
> > at the wrong places.
> > 
> > Remove one unneeded comment (no annotations necessary between just empty
> > labels), and move one other instance to the right place (before any
> > label sequence).
> > This makes GCC's implicit fallthrough checker happy.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Reviewed-by: Tom Rini <trini@konsulko.com>
> > ---
> >  net/net.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index 5219367e391..f191f16357c 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1525,7 +1525,6 @@ static int net_check_prereq(enum proto_t protocol)
> >  #if defined(CONFIG_CMD_NFS)
> >  	case NFS:
> >  #endif
> > -		/* Fall through */
> >  	case TFTPGET:
> >  	case TFTPPUT:
> >  		if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
> > @@ -1539,11 +1538,11 @@ static int net_check_prereq(enum proto_t protocol)
> >  			puts("*** ERROR: `serverip' not set\n");
> >  			return 1;
> >  		}
> > +		fallthrough;
> >  #if	defined(CONFIG_CMD_PING) || \
> >  	defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> >  common:
> >  #endif
> > -		/* Fall through */
> >  
> >  	case NETCONS:
> >  	case FASTBOOT_UDP:  
> 
> So this one is harder than it looks. With clang, we cannot seemingly
> have:
> 	fallthrough;
> #if defined(CONFIG_CMD_PING) || \
>     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> common:
> #endif
> 
> And gcc was failing on:
> 	}
> #if defined(CONFIG_CMD_PING) || \
>     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> common:
> #endif
> 	fallthrough;

Yes, later stages of the CI told me so already ;-)

> Maybe we can move the label to inside the next set of cases, and then
> also add CONFIG_CMD_PING6 to the checks, as that also has 'goto common;'

I found some other solution: dropping the #if's around the common:
label, then marking this with __maybe_unused instead. Seems to work for
both GCC and clang, and makes the code even more readable.

Will send this among the other gazillion fixes I meanwhile added in a
v2, to a mailbox near you.
If you can't wait: sunxi/fallthrough has the bits, though not yet split
up and without commit messages. Still not passing all checks, but the
CI builds seem to stop early, before revealing all issues, so this is a
piecemeal job :-(

Cheers,
Andre

Re: [PATCH 05/18] net/net: fix switch/case fallthrough annotations

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3339 bytes --]

On Wed, Apr 09, 2025 at 12:53:47AM +0100, Andre Przywara wrote:
> On Tue, 8 Apr 2025 16:29:18 -0600
> Tom Rini <trini@konsulko.com> wrote:
> 
> Hi Tom,
> 
> thanks for staying on this!
> 
> > On Thu, Mar 27, 2025 at 03:33:00PM +0000, Andre Przywara wrote:
> > 
> > > The net_check_prereq() routine in the generic network handling code
> > > mixes case: labels with #ifdef's, which makes predicting fallthrough
> > > situations tricky. We had two "fall through" comments in the code, but
> > > at the wrong places.
> > > 
> > > Remove one unneeded comment (no annotations necessary between just empty
> > > labels), and move one other instance to the right place (before any
> > > label sequence).
> > > This makes GCC's implicit fallthrough checker happy.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > ---
> > >  net/net.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/net/net.c b/net/net.c
> > > index 5219367e391..f191f16357c 100644
> > > --- a/net/net.c
> > > +++ b/net/net.c
> > > @@ -1525,7 +1525,6 @@ static int net_check_prereq(enum proto_t protocol)
> > >  #if defined(CONFIG_CMD_NFS)
> > >  	case NFS:
> > >  #endif
> > > -		/* Fall through */
> > >  	case TFTPGET:
> > >  	case TFTPPUT:
> > >  		if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
> > > @@ -1539,11 +1538,11 @@ static int net_check_prereq(enum proto_t protocol)
> > >  			puts("*** ERROR: `serverip' not set\n");
> > >  			return 1;
> > >  		}
> > > +		fallthrough;
> > >  #if	defined(CONFIG_CMD_PING) || \
> > >  	defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > >  common:
> > >  #endif
> > > -		/* Fall through */
> > >  
> > >  	case NETCONS:
> > >  	case FASTBOOT_UDP:  
> > 
> > So this one is harder than it looks. With clang, we cannot seemingly
> > have:
> > 	fallthrough;
> > #if defined(CONFIG_CMD_PING) || \
> >     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > common:
> > #endif
> > 
> > And gcc was failing on:
> > 	}
> > #if defined(CONFIG_CMD_PING) || \
> >     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > common:
> > #endif
> > 	fallthrough;
> 
> Yes, later stages of the CI told me so already ;-)
> 
> > Maybe we can move the label to inside the next set of cases, and then
> > also add CONFIG_CMD_PING6 to the checks, as that also has 'goto common;'
> 
> I found some other solution: dropping the #if's around the common:
> label, then marking this with __maybe_unused instead. Seems to work for
> both GCC and clang, and makes the code even more readable.
> 
> Will send this among the other gazillion fixes I meanwhile added in a
> v2, to a mailbox near you.
> If you can't wait: sunxi/fallthrough has the bits, though not yet split
> up and without commit messages. Still not passing all checks, but the
> CI builds seem to stop early, before revealing all issues, so this is a
> piecemeal job :-(

I thought I had tried what you suggest but maybe didn't quite get things
aligned right (but I also modified sandbox so it would trigger the
unused warning). That said, I'm applying most of v1 now'ish, and have
only stopped as part of trying to narrow down the seemingly random
evb-ast2600 CI failure.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 05/18] net/net: fix switch/case fallthrough annotations

[Andre Przywara]

On Tue, 8 Apr 2025 19:46:46 -0600
Tom Rini <trini@konsulko.com> wrote:

Hi Tom,

> On Wed, Apr 09, 2025 at 12:53:47AM +0100, Andre Przywara wrote:
> > On Tue, 8 Apr 2025 16:29:18 -0600
> > Tom Rini <trini@konsulko.com> wrote:
> > 
> > Hi Tom,
> > 
> > thanks for staying on this!
> >   
> > > On Thu, Mar 27, 2025 at 03:33:00PM +0000, Andre Przywara wrote:
> > >   
> > > > The net_check_prereq() routine in the generic network handling code
> > > > mixes case: labels with #ifdef's, which makes predicting fallthrough
> > > > situations tricky. We had two "fall through" comments in the code, but
> > > > at the wrong places.
> > > > 
> > > > Remove one unneeded comment (no annotations necessary between just empty
> > > > labels), and move one other instance to the right place (before any
> > > > label sequence).
> > > > This makes GCC's implicit fallthrough checker happy.
> > > > 
> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > > ---
> > > >  net/net.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/net/net.c b/net/net.c
> > > > index 5219367e391..f191f16357c 100644
> > > > --- a/net/net.c
> > > > +++ b/net/net.c
> > > > @@ -1525,7 +1525,6 @@ static int net_check_prereq(enum proto_t protocol)
> > > >  #if defined(CONFIG_CMD_NFS)
> > > >  	case NFS:
> > > >  #endif
> > > > -		/* Fall through */
> > > >  	case TFTPGET:
> > > >  	case TFTPPUT:
> > > >  		if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
> > > > @@ -1539,11 +1538,11 @@ static int net_check_prereq(enum proto_t protocol)
> > > >  			puts("*** ERROR: `serverip' not set\n");
> > > >  			return 1;
> > > >  		}
> > > > +		fallthrough;
> > > >  #if	defined(CONFIG_CMD_PING) || \
> > > >  	defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > > >  common:
> > > >  #endif
> > > > -		/* Fall through */
> > > >  
> > > >  	case NETCONS:
> > > >  	case FASTBOOT_UDP:    
> > > 
> > > So this one is harder than it looks. With clang, we cannot seemingly
> > > have:
> > > 	fallthrough;
> > > #if defined(CONFIG_CMD_PING) || \
> > >     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > > common:
> > > #endif
> > > 
> > > And gcc was failing on:
> > > 	}
> > > #if defined(CONFIG_CMD_PING) || \
> > >     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > > common:
> > > #endif
> > > 	fallthrough;  
> > 
> > Yes, later stages of the CI told me so already ;-)
> >   
> > > Maybe we can move the label to inside the next set of cases, and then
> > > also add CONFIG_CMD_PING6 to the checks, as that also has 'goto common;'  
> > 
> > I found some other solution: dropping the #if's around the common:
> > label, then marking this with __maybe_unused instead. Seems to work for
> > both GCC and clang, and makes the code even more readable.
> > 
> > Will send this among the other gazillion fixes I meanwhile added in a
> > v2, to a mailbox near you.
> > If you can't wait: sunxi/fallthrough has the bits, though not yet split
> > up and without commit messages. Still not passing all checks, but the
> > CI builds seem to stop early, before revealing all issues, so this is a
> > piecemeal job :-(  
> 
> I thought I had tried what you suggest but maybe didn't quite get things
> aligned right (but I also modified sandbox so it would trigger the
> unused warning). That said, I'm applying most of v1 now'ish, and have
> only stopped as part of trying to narrow down the seemingly random
> evb-ast2600 CI failure.

Can you hold back with this patch here for now? I will send my new version
of this one later, which passes more CI. I didn't find more overlaps
between this and my new series, so I wonder if you could apply what you
think is ready from this one (definitely minus this patch, but maybe
others), and then I send another series with new fixes plus what's left
from this one. This would avoid me sending a v2 of this just because of
this one patch.

Cheers,
Andre

Re: [PATCH 05/18] net/net: fix switch/case fallthrough annotations

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4437 bytes --]

On Wed, Apr 09, 2025 at 11:41:37AM +0100, Andre Przywara wrote:
> On Tue, 8 Apr 2025 19:46:46 -0600
> Tom Rini <trini@konsulko.com> wrote:
> 
> Hi Tom,
> 
> > On Wed, Apr 09, 2025 at 12:53:47AM +0100, Andre Przywara wrote:
> > > On Tue, 8 Apr 2025 16:29:18 -0600
> > > Tom Rini <trini@konsulko.com> wrote:
> > > 
> > > Hi Tom,
> > > 
> > > thanks for staying on this!
> > >   
> > > > On Thu, Mar 27, 2025 at 03:33:00PM +0000, Andre Przywara wrote:
> > > >   
> > > > > The net_check_prereq() routine in the generic network handling code
> > > > > mixes case: labels with #ifdef's, which makes predicting fallthrough
> > > > > situations tricky. We had two "fall through" comments in the code, but
> > > > > at the wrong places.
> > > > > 
> > > > > Remove one unneeded comment (no annotations necessary between just empty
> > > > > labels), and move one other instance to the right place (before any
> > > > > label sequence).
> > > > > This makes GCC's implicit fallthrough checker happy.
> > > > > 
> > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > >  net/net.c | 3 +--
> > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/net/net.c b/net/net.c
> > > > > index 5219367e391..f191f16357c 100644
> > > > > --- a/net/net.c
> > > > > +++ b/net/net.c
> > > > > @@ -1525,7 +1525,6 @@ static int net_check_prereq(enum proto_t protocol)
> > > > >  #if defined(CONFIG_CMD_NFS)
> > > > >  	case NFS:
> > > > >  #endif
> > > > > -		/* Fall through */
> > > > >  	case TFTPGET:
> > > > >  	case TFTPPUT:
> > > > >  		if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
> > > > > @@ -1539,11 +1538,11 @@ static int net_check_prereq(enum proto_t protocol)
> > > > >  			puts("*** ERROR: `serverip' not set\n");
> > > > >  			return 1;
> > > > >  		}
> > > > > +		fallthrough;
> > > > >  #if	defined(CONFIG_CMD_PING) || \
> > > > >  	defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > > > >  common:
> > > > >  #endif
> > > > > -		/* Fall through */
> > > > >  
> > > > >  	case NETCONS:
> > > > >  	case FASTBOOT_UDP:    
> > > > 
> > > > So this one is harder than it looks. With clang, we cannot seemingly
> > > > have:
> > > > 	fallthrough;
> > > > #if defined(CONFIG_CMD_PING) || \
> > > >     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > > > common:
> > > > #endif
> > > > 
> > > > And gcc was failing on:
> > > > 	}
> > > > #if defined(CONFIG_CMD_PING) || \
> > > >     defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
> > > > common:
> > > > #endif
> > > > 	fallthrough;  
> > > 
> > > Yes, later stages of the CI told me so already ;-)
> > >   
> > > > Maybe we can move the label to inside the next set of cases, and then
> > > > also add CONFIG_CMD_PING6 to the checks, as that also has 'goto common;'  
> > > 
> > > I found some other solution: dropping the #if's around the common:
> > > label, then marking this with __maybe_unused instead. Seems to work for
> > > both GCC and clang, and makes the code even more readable.
> > > 
> > > Will send this among the other gazillion fixes I meanwhile added in a
> > > v2, to a mailbox near you.
> > > If you can't wait: sunxi/fallthrough has the bits, though not yet split
> > > up and without commit messages. Still not passing all checks, but the
> > > CI builds seem to stop early, before revealing all issues, so this is a
> > > piecemeal job :-(  
> > 
> > I thought I had tried what you suggest but maybe didn't quite get things
> > aligned right (but I also modified sandbox so it would trigger the
> > unused warning). That said, I'm applying most of v1 now'ish, and have
> > only stopped as part of trying to narrow down the seemingly random
> > evb-ast2600 CI failure.
> 
> Can you hold back with this patch here for now? I will send my new version
> of this one later, which passes more CI. I didn't find more overlaps
> between this and my new series, so I wonder if you could apply what you
> think is ready from this one (definitely minus this patch, but maybe
> others), and then I send another series with new fixes plus what's left
> from this one. This would avoid me sending a v2 of this just because of
> this one patch.

I'm planning on applying everything except this one, and 18/18
currently, so that works yes?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 06/18] fastboot: annotate switch/case fallthrough case

[Andre Przywara]

The fastboot command handling uses an implicit switch/case fallthrough
when receiving the OEM_CONSOLE command, but when this command is not
enabled in Kconfig, to report this command as unknown.

Add our "fallthrough;" statement-like macro before the default branch in
the fastboot code, to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/fastboot/fb_command.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index e4484d65aca..2cdbac50ac4 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -186,6 +186,7 @@ void fastboot_multiresponse(int cmd, char *response)
 			}
 			break;
 		}
+		fallthrough;
 	default:
 		fastboot_fail("Unknown multiresponse command", response);
 		break;
-- 
2.25.1

Re: [PATCH 06/18] fastboot: annotate switch/case fallthrough case

[Mattijs Korpershoek]

Hi Andre,

Thank you for the patch.

On jeu., mars 27, 2025 at 15:33, Andre Przywara <andre.przywara@arm.com> wrote:

> The fastboot command handling uses an implicit switch/case fallthrough
> when receiving the OEM_CONSOLE command, but when this command is not
> enabled in Kconfig, to report this command as unknown.
>
> Add our "fallthrough;" statement-like macro before the default branch in
> the fastboot code, to avoid a warning when GCC's -Wimplicit-fallthrough
> warning option is enabled.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/fastboot/fb_command.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index e4484d65aca..2cdbac50ac4 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -186,6 +186,7 @@ void fastboot_multiresponse(int cmd, char *response)
>  			}
>  			break;
>  		}
> +		fallthrough;
>  	default:
>  		fastboot_fail("Unknown multiresponse command", response);
>  		break;
> -- 
> 2.25.1

[PATCH 07/18] net: sun8i-emac: annotate fallthrough

[Andre Przywara]

The Allwinner sun8i EMAC driver uses an implicit switch/case fallthrough
when setting up the MAC/PHY communication protocol, to handle the case
when RMII is requested, but would not be supported by the hardware.

Add our "fallthrough;" statement-like macro before the default branch in
sun8i_emac_set_syscon(), to avoid a warning when GCC's
-Wimplicit-fallthrough warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/sun8i_emac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 0da182d9f4c..8433e7db265 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -335,6 +335,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 			reg |= SC_RMII_EN | SC_ETCS_EXT_GMII;
 			break;
 		}
+		fallthrough;
 	default:
 		debug("%s: Invalid PHY interface\n", __func__);
 		return -EINVAL;
-- 
2.25.1

[PATCH 08/18] usb: ohci-hcd: annotate switch/case fallthrough

[Andre Przywara]

The USB OCHI code uses an implicit switch/case fallthrough after checking
for valid descriptor IDs.

Add our "fallthrough;" statement-like macro before the default branch in
the OHCI code, to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/usb/host/ohci-hcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index c020d13c43d..234a6f3645d 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -1361,7 +1361,8 @@ pkt_print(ohci, NULL, dev, pipe, buffer, transfer_len,
 						wLength));
 				databuf = root_hub_str_index1;
 				OK(len);
-		}
+			}
+			fallthrough;
 		default:
 			stat = USB_ST_STALLED;
 		}
-- 
2.25.1

[PATCH 09/18] usb: xhci: annotate switch/case fallthrough properly

[Andre Przywara]

The USB XHCI code uses an implicit switch/case fallthrough to share code
for handling full speed and low speed transfers.

Add our "fallthrough;" statement-like macro before the second label in
the XHCI code, to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/usb/host/xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d30725d3fca..3ee1f67190f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -352,7 +352,7 @@ static unsigned int xhci_get_endpoint_interval(struct usb_device *udev,
 		 * since it uses the same rules as low speed interrupt
 		 * endpoints.
 		 */
-
+		fallthrough;
 	case USB_SPEED_LOW:
 		if (usb_endpoint_xfer_int(endpt_desc) ||
 		    usb_endpoint_xfer_isoc(endpt_desc)) {
-- 
2.25.1

Re: [PATCH 09/18] usb: xhci: annotate switch/case fallthrough properly

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Thu, Mar 27, 2025 at 03:33:04PM +0000, Andre Przywara wrote:

> The USB XHCI code uses an implicit switch/case fallthrough to share code
> for handling full speed and low speed transfers.
> 
> Add our "fallthrough;" statement-like macro before the second label in
> the XHCI code, to avoid a warning when GCC's -Wimplicit-fallthrough
> warning option is enabled.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 10/18] video: annotate switch/case fall-through

[Andre Przywara]

The generic DM video code uses an implicit switch/case fallthrough to
provide fallback code paths when certain colour depths are not enabled.

Add our "fallthrough;" statement-like macro to the video_fill() function
to avoid a warning when GCC's -Wimplicit-fallthrough warning option is
enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/video/video-uclass.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index ff4f2199585..c684c994b61 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -245,6 +245,7 @@ int video_fill(struct udevice *dev, u32 colour)
 				*ppix++ = colour;
 			break;
 		}
+		fallthrough;
 	case VIDEO_BPP32:
 		if (CONFIG_IS_ENABLED(VIDEO_BPP32)) {
 			u32 *ppix = priv->fb;
@@ -254,6 +255,7 @@ int video_fill(struct udevice *dev, u32 colour)
 				*ppix++ = colour;
 			break;
 		}
+		fallthrough;
 	default:
 		memset(priv->fb, colour, priv->fb_size);
 		break;
-- 
2.25.1

Re: [PATCH 10/18] video: annotate switch/case fall-through

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On Thu, Mar 27, 2025 at 03:33:05PM +0000, Andre Przywara wrote:

> The generic DM video code uses an implicit switch/case fallthrough to
> provide fallback code paths when certain colour depths are not enabled.
> 
> Add our "fallthrough;" statement-like macro to the video_fill() function
> to avoid a warning when GCC's -Wimplicit-fallthrough warning option is
> enabled.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/video/video-uclass.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index ff4f2199585..c684c994b61 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -245,6 +245,7 @@ int video_fill(struct udevice *dev, u32 colour)
>  				*ppix++ = colour;
>  			break;
>  		}
> +		fallthrough;
>  	case VIDEO_BPP32:
>  		if (CONFIG_IS_ENABLED(VIDEO_BPP32)) {
>  			u32 *ppix = priv->fb;
> @@ -254,6 +255,7 @@ int video_fill(struct udevice *dev, u32 colour)
>  				*ppix++ = colour;
>  			break;
>  		}
> +		fallthrough;
>  	default:
>  		memset(priv->fb, colour, priv->fb_size);
>  		break;

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 11/18] net: e1000: annotate switch/case fallthrough

[Andre Przywara]

The E1000 driver uses an implicit switch/case fallthrough for sharing
some code supporting different PHYs.

Add our "fallthrough;" statement-like macro before the two labels in
e1000_set_phy_type(), to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/net/e1000.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 8f432b8637b..b77298070f8 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -4830,6 +4830,7 @@ static int e1000_set_phy_type (struct e1000_hw *hw)
 			hw->phy_type = e1000_phy_igp;
 			break;
 		}
+		fallthrough;
 	case IGP03E1000_E_PHY_ID:
 		hw->phy_type = e1000_phy_igp_3;
 		break;
@@ -4843,6 +4844,7 @@ static int e1000_set_phy_type (struct e1000_hw *hw)
 			hw->phy_type = e1000_phy_gg82563;
 			break;
 		}
+		fallthrough;
 	case BME1000_E_PHY_ID:
 		hw->phy_type = e1000_phy_bm;
 		break;
-- 
2.25.1

[PATCH 12/18] mtd: ubi: annotate fallthrough

[Andre Przywara]

The UBI code uses an implicit switch/case fallthrough when handling two
related cases of bad header errors. Also there is a switch/case for unit
prefix handling (G/M/K), which accumulates multiplications.

Add our "fallthrough;" statement-like macro before the respective labels
in both cases, to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mtd/ubi/attach.c | 1 +
 drivers/mtd/ubi/build.c  | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 2ef8fde3d32..306da509ec4 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -934,6 +934,7 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 			 * be a result of power cut during erasure.
 			 */
 			ai->maybe_bad_peb_count += 1;
+		fallthrough;
 	case UBI_IO_BAD_HDR:
 		if (ec_err)
 			/*
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 104537784ab..50e43928af0 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1400,12 +1400,15 @@ static int __init bytes_str_to_int(const char *str)
 	switch (*endp) {
 	case 'G':
 		result *= 1024;
+		fallthrough;
 	case 'M':
 		result *= 1024;
+		fallthrough;
 	case 'K':
 		result *= 1024;
 		if (endp[1] == 'i' && endp[2] == 'B')
 			endp += 2;
+		fallthrough;
 	case '\0':
 		break;
 	default:
-- 
2.25.1

Re: [PATCH 12/18] mtd: ubi: annotate fallthrough

[Heiko Schocher]

Hello Andre,

On 27.03.25 16:33, Andre Przywara wrote:
> The UBI code uses an implicit switch/case fallthrough when handling two
> related cases of bad header errors. Also there is a switch/case for unit
> prefix handling (G/M/K), which accumulates multiplications.
> 
> Add our "fallthrough;" statement-like macro before the respective labels
> in both cases, to avoid a warning when GCC's -Wimplicit-fallthrough
> warning option is enabled.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   drivers/mtd/ubi/attach.c | 1 +
>   drivers/mtd/ubi/build.c  | 3 +++
>   2 files changed, 4 insertions(+)

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

[PATCH 13/18] arm: mach-k3: am62p: annotate switch/case fallthrough

[Andre Przywara]

The MMC boot mode selection for the TI AM62P series of SoCs uses an
implicit switch/case fallthrough for falling back to some default
boot mode.

Add our "fallthrough;" statement-like macro before the default branch in
the code, to avoid a warning when GCC's -Wimplicit-fallthrough warning
option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/mach-k3/am62px/am62p5_init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-k3/am62px/am62p5_init.c b/arch/arm/mach-k3/am62px/am62p5_init.c
index 14a46fa28d2..01e47deca94 100644
--- a/arch/arm/mach-k3/am62px/am62p5_init.c
+++ b/arch/arm/mach-k3/am62px/am62p5_init.c
@@ -266,6 +266,7 @@ u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device)
 	case BOOT_DEVICE_MMC:
 		if (bootmode_cfg & MAIN_DEVSTAT_PRIMARY_MMC_FS_RAW_MASK)
 			return MMCSD_MODE_RAW;
+		fallthrough;
 	default:
 		return MMCSD_MODE_FS;
 	}
-- 
2.25.1

Re: [PATCH 13/18] arm: mach-k3: am62p: annotate switch/case fallthrough

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On Thu, Mar 27, 2025 at 03:33:08PM +0000, Andre Przywara wrote:

> The MMC boot mode selection for the TI AM62P series of SoCs uses an
> implicit switch/case fallthrough for falling back to some default
> boot mode.
> 
> Add our "fallthrough;" statement-like macro before the default branch in
> the code, to avoid a warning when GCC's -Wimplicit-fallthrough warning
> option is enabled.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 14/18] mtd: spi-nor-tiny: annotate switch/case fallthrough

[Andre Przywara]

The SPI NOR code uses an implicit switch/case fallthrough when checking
different vendors to determine how to deal with extended addressig modes.

Add our "fallthrough;" statement-like macro before some label in the
4-byte addressing mode code, to avoid a warning when GCC's
-Wimplicit-fallthrough warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mtd/spi/spi-nor-tiny.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
index 5755c5eed29..23de64a1520 100644
--- a/drivers/mtd/spi/spi-nor-tiny.c
+++ b/drivers/mtd/spi/spi-nor-tiny.c
@@ -219,6 +219,7 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 	case SNOR_MFR_MICRON:
 		/* Some Micron need WREN command; all will accept it */
 		need_wren = true;
+		fallthrough;
 	case SNOR_MFR_MACRONIX:
 	case SNOR_MFR_WINBOND:
 		if (need_wren)
-- 
2.25.1

Re: [PATCH 14/18] mtd: spi-nor-tiny: annotate switch/case fallthrough

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Thu, Mar 27, 2025 at 03:33:09PM +0000, Andre Przywara wrote:

> The SPI NOR code uses an implicit switch/case fallthrough when checking
> different vendors to determine how to deal with extended addressig modes.
> 
> Add our "fallthrough;" statement-like macro before some label in the
> 4-byte addressing mode code, to avoid a warning when GCC's
> -Wimplicit-fallthrough warning option is enabled.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 15/18] mtd: rawnand: nand_base: annotate switch/case fallthrough

[Andre Przywara]

The raw NAND flash code uses an implicit switch/case fallthrough to
share code when dealing with different ECC modes, and also when handling
some read command.

Add our "fallthrough;" statement-like macro before the respective labels
in the NAND code, to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

This copies the fallthrough annotations that the original kernel code
gained, before this function got refactored there.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mtd/nand/raw/nand_base.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 1b65c6f6443..daf12807c67 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -774,6 +774,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 			       NAND_NCE | NAND_CTRL_CHANGE);
 
 		/* This applies to read commands */
+		fallthrough;
 	default:
 		/*
 		 * If we don't have access to the busy pin, we apply the given
@@ -4974,6 +4975,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		if (!ecc->read_page)
 			ecc->read_page = nand_read_page_hwecc_oob_first;
 
+		fallthrough;
 	case NAND_ECC_HW:
 		/* Use standard hwecc read page function? */
 		if (!ecc->read_page)
@@ -4993,6 +4995,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
 			ecc->write_subpage = nand_write_subpage_hwecc;
 
+		fallthrough;
 	case NAND_ECC_HW_SYNDROME:
 		if ((!ecc->calculate || !ecc->correct || !ecc->hwctl) &&
 		    (!ecc->read_page ||
@@ -5027,6 +5030,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 			ecc->size, mtd->writesize);
 		ecc->mode = NAND_ECC_SOFT;
 
+		fallthrough;
 	case NAND_ECC_SOFT:
 		ecc->calculate = nand_calculate_ecc;
 		ecc->correct = nand_correct_data;
-- 
2.25.1

Re: [PATCH 15/18] mtd: rawnand: nand_base: annotate switch/case fallthrough

[Michael Nazzareno Trimarchi]

On Thu, Mar 27, 2025 at 4:33 PM Andre Przywara <andre.przywara@arm.com>
wrote:

> The raw NAND flash code uses an implicit switch/case fallthrough to
> share code when dealing with different ECC modes, and also when handling
> some read command.
>
> Add our "fallthrough;" statement-like macro before the respective labels
> in the NAND code, to avoid a warning when GCC's -Wimplicit-fallthrough
> warning option is enabled.
>
> This copies the fallthrough annotations that the original kernel code
> gained, before this function got refactored there.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 1b65c6f6443..daf12807c67 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -774,6 +774,7 @@ static void nand_command_lp(struct mtd_info *mtd,
> unsigned int command,
>                                NAND_NCE | NAND_CTRL_CHANGE);
>
>                 /* This applies to read commands */
> +               fallthrough;
>         default:
>                 /*
>                  * If we don't have access to the busy pin, we apply the
> given
> @@ -4974,6 +4975,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>                 if (!ecc->read_page)
>                         ecc->read_page = nand_read_page_hwecc_oob_first;
>
> +               fallthrough;
>         case NAND_ECC_HW:
>                 /* Use standard hwecc read page function? */
>                 if (!ecc->read_page)
> @@ -4993,6 +4995,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>                 if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
>                         ecc->write_subpage = nand_write_subpage_hwecc;
>
> +               fallthrough;
>         case NAND_ECC_HW_SYNDROME:
>                 if ((!ecc->calculate || !ecc->correct || !ecc->hwctl) &&
>                     (!ecc->read_page ||
> @@ -5027,6 +5030,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>                         ecc->size, mtd->writesize);
>                 ecc->mode = NAND_ECC_SOFT;
>
> +               fallthrough;
>         case NAND_ECC_SOFT:
>                 ecc->calculate = nand_calculate_ecc;
>                 ecc->correct = nand_correct_data;
> --
> 2.25.1
>

Reviewed-by: Michael Trimrachi <michael@amarulasolutions.com>

I think I will apply the relative patches on mtd tree

Michael


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

[PATCH 16/18] cmd: pmic: annotate switch/case fallthrough

[Andre Przywara]

The argument parsing code in the pmic command uses an implicit switch/case
fallthrough to handle the common part of having one or two arguments.

Add our "fallthrough;" statement-like macro before the second branch in
the parsing code, to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 cmd/pmic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/cmd/pmic.c b/cmd/pmic.c
index 3ad1b8aa375..e5fc8f97b75 100644
--- a/cmd/pmic.c
+++ b/cmd/pmic.c
@@ -34,6 +34,7 @@ static int do_dev(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 			printf("Can't get PMIC: %s!\n", name);
 			return failure(ret);
 		}
+		fallthrough;
 	case 1:
 		if (!currdev) {
 			printf("PMIC device is not set!\n\n");
-- 
2.25.1

Re: [PATCH 16/18] cmd: pmic: annotate switch/case fallthrough

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

On Thu, Mar 27, 2025 at 03:33:11PM +0000, Andre Przywara wrote:

> The argument parsing code in the pmic command uses an implicit switch/case
> fallthrough to handle the common part of having one or two arguments.
> 
> Add our "fallthrough;" statement-like macro before the second branch in
> the parsing code, to avoid a warning when GCC's -Wimplicit-fallthrough
> warning option is enabled.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 17/18] cmd: spl: annotate switch/case fallthrough

[Andre Przywara]

The argument parsing in the SPL configuration command uses an implicit
switch/case fallthrough when dealing with a different number of
arguments.

Add our "fallthrough;" statement-like macro before the respective labels
in the bootm code, to avoid a warning when GCC's -Wimplicit-fallthrough
warning option is enabled.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 cmd/spl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cmd/spl.c b/cmd/spl.c
index 76fe33762df..379b512f1ff 100644
--- a/cmd/spl.c
+++ b/cmd/spl.c
@@ -60,8 +60,10 @@ static int call_bootm(int argc, char *const argv[], const char *subcommand[])
 	switch (argc) {
 	case 3:
 		bootm_argv[4] = argv[2]; /* fdt addr */
+		fallthrough;
 	case 2:
 		bootm_argv[3] = argv[1]; /* initrd addr */
+		fallthrough;
 	case 1:
 		bootm_argv[2] = argv[0]; /* kernel addr */
 	}
-- 
2.25.1

Re: [PATCH 17/18] cmd: spl: annotate switch/case fallthrough

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

On Thu, Mar 27, 2025 at 03:33:12PM +0000, Andre Przywara wrote:

> The argument parsing in the SPL configuration command uses an implicit
> switch/case fallthrough when dealing with a different number of
> arguments.
> 
> Add our "fallthrough;" statement-like macro before the respective labels
> in the bootm code, to avoid a warning when GCC's -Wimplicit-fallthrough
> warning option is enabled.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 18/18] [DO NOT MERGE] Makefile: enable switch/case fallthrough warnings

[Andre Przywara]

=================================================================
This does not fully work yet, there is at least the libbz2 code
left to be handled. But this patch helps to let CI find more
places where we need annotations.
=================================================================

C's implicit fallthrough behaviour in switch/case statements can lead to
subtle bugs. Quite some while ago many compilers introduced warnings in
those cases, requiring intentional fallthrough's to be annotated.

So far we were not enabling that compiler option, so many ambiguities
and some bugs in the code went unnoticed.

With all places that produced warnings fixed or annotated now, we can
add this compiler option to the build flags, to find new ambiguities
early, before the code gets merged.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 7d83ce16721..46c48556788 100644
--- a/Makefile
+++ b/Makefile
@@ -808,6 +808,7 @@ endif
 # These warnings generated too much noise in a regular build.
 # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
 KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
+KBUILD_CFLAGS += -Wimplicit-fallthrough
 
 # Prohibit date/time macros, which would make the build non-deterministic
 KBUILD_CFLAGS   += $(call cc-option,-Werror=date-time)
-- 
2.25.1

Re: [PATCH 18/18] [DO NOT MERGE] Makefile: enable switch/case fallthrough warnings

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

On Thu, Mar 27, 2025 at 03:33:13PM +0000, Andre Przywara wrote:

> =================================================================
> This does not fully work yet, there is at least the libbz2 code
> left to be handled. But this patch helps to let CI find more
> places where we need annotations.
> =================================================================
> 
> C's implicit fallthrough behaviour in switch/case statements can lead to
> subtle bugs. Quite some while ago many compilers introduced warnings in
> those cases, requiring intentional fallthrough's to be annotated.
> 
> So far we were not enabling that compiler option, so many ambiguities
> and some bugs in the code went unnoticed.
> 
> With all places that produced warnings fixed or annotated now, we can
> add this compiler option to the build flags, to find new ambiguities
> early, before the code gets merged.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)

For the record, once this is ready to be merged I would like to see us
borrow the CC_IMPLICIT_FALLTHROUGH logic from the linux kernel (which
gets the best flags for gcc or clang) instead, but yes, default to
enabling the check.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/18] Annotate switch/case fallthrough cases

[Heinrich Schuchardt]

On 27.03.25 16:32, Andre Przywara wrote:
> C's implicit fallthrough behaviour in switch/case statements can lead to
> subtle bugs. Quite some while ago many compilers introduced warnings in
> those cases, requiring intentional fallthrough's to be annotated.
>
> So far we were not enabling that compiler option, so many ambiguities
> and some bugs in the code went unnoticed.
>
> This series adds the required annotations in code paths that the first
> stage of the U-Boot CI covers. There is a large number of cases left
> in the libbz2 code. The usage of switch/case is borderline insane there,
> labels are hidden in macros, and there are no breaks, but just goto's.
> Upstream still uses very similar code, without any annotations. I still
> am not 100% sure those are meant to fall through or not, and plan to do
> further investigations, but didn't want to hold the rest of the patches
> back. You can see for yourself by applying patch 18/18 and building for
> sandbox64, for instance.

Can we use something like

CFLAGS_REMOVE_bzlib.o = -Wimplicit-fallthrough

in the Makefile if that module is too hard to fix?

Best regards

Heinrich

>
> Because of this we cannot quite enable the warning in the Makefile yet,
> but those fixes are worth regardless, and be it to increase readability.
>
> Please not that those patches do not fix anything, really, they just add
> those fallthrough annotations, so the series is not really critical.
>
> Cheers,
> Andre
>
> Andre Przywara (18):
>    spl: mmc: properly annotate fallthrough
>    zlib: annotate switch/case fallthrough cases
>    gadget: f_thor: annotate switch/case fallthrough
>    use proper fallthrough annotations
>    net/net: fix switch/case fallthrough annotations
>    fastboot: annotate switch/case fallthrough case
>    net: sun8i-emac: annotate fallthrough
>    usb: ohci-hcd: annotate switch/case fallthrough
>    usb: xhci: annotate switch/case fallthrough properly (TBF)
>    video: annotate switch/case fall-through
>    net: e1000: annotate switch/case fallthrough
>    mtd: ubi: annotate fallthrough (TBF)
>    arm: mach-k3: am62p: annotate switch/case fallthrough
>    mtd: spi-nor-tiny: annotate switch/case fallthrough
>    mtd: rawnand: nand_base: annotate switch/case fallthrough
>    cmd: pmic: annotate switch/case fallthrough
>    cmd: spl: annotate switch/case fallthrough
>    [DO NOT MERGE] Makefile: enable switch/case fallthrough warnings
>
>   Makefile                              |  1 +
>   arch/arm/mach-k3/am62px/am62p5_init.c |  1 +
>   cmd/pmic.c                            |  1 +
>   cmd/spl.c                             |  2 ++
>   common/command.c                      |  2 +-
>   common/spl/spl_mmc.c                  |  1 +
>   drivers/fastboot/fb_command.c         |  1 +
>   drivers/mtd/nand/raw/nand_base.c      |  4 ++++
>   drivers/mtd/spi/spi-nor-tiny.c        |  1 +
>   drivers/mtd/ubi/attach.c              |  1 +
>   drivers/mtd/ubi/build.c               |  3 +++
>   drivers/net/e1000.c                   |  2 ++
>   drivers/net/sun8i_emac.c              |  1 +
>   drivers/usb/gadget/f_thor.c           |  1 +
>   drivers/usb/host/ohci-hcd.c           |  3 ++-
>   drivers/usb/host/xhci.c               |  2 +-
>   drivers/video/video-uclass.c          |  2 ++
>   lib/tiny-printf.c                     |  2 +-
>   lib/zlib/inflate.c                    | 21 +++++++++++++++++++++
>   net/net.c                             |  5 ++---
>   20 files changed, 50 insertions(+), 7 deletions(-)
>

Re: [PATCH 00/18] Annotate switch/case fallthrough cases

[Andre Przywara]

On Fri, 28 Mar 2025 11:28:05 +0100
Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

Hi,

> On 27.03.25 16:32, Andre Przywara wrote:
> > C's implicit fallthrough behaviour in switch/case statements can lead to
> > subtle bugs. Quite some while ago many compilers introduced warnings in
> > those cases, requiring intentional fallthrough's to be annotated.
> >
> > So far we were not enabling that compiler option, so many ambiguities
> > and some bugs in the code went unnoticed.
> >
> > This series adds the required annotations in code paths that the first
> > stage of the U-Boot CI covers. There is a large number of cases left
> > in the libbz2 code. The usage of switch/case is borderline insane there,
> > labels are hidden in macros, and there are no breaks, but just goto's.
> > Upstream still uses very similar code, without any annotations. I still
> > am not 100% sure those are meant to fall through or not, and plan to do
> > further investigations, but didn't want to hold the rest of the patches
> > back. You can see for yourself by applying patch 18/18 and building for
> > sandbox64, for instance.  
> 
> Can we use something like
> 
> CFLAGS_REMOVE_bzlib.o = -Wimplicit-fallthrough

Ah, didn't know we have that in U-Boot as well! Sounds promising, and
fixes the sandbox build for me (when using bzlib_decompress.o). I will add
this to the last patch and will check what the CI has to say about this.

Thanks for the heads up!

Cheers,
Andre

> 
> in the Makefile if that module is too hard to fix?
> 
> Best regards
> 
> Heinrich
> 
> >
> > Because of this we cannot quite enable the warning in the Makefile yet,
> > but those fixes are worth regardless, and be it to increase readability.
> >
> > Please not that those patches do not fix anything, really, they just add
> > those fallthrough annotations, so the series is not really critical.
> >
> > Cheers,
> > Andre
> >
> > Andre Przywara (18):
> >    spl: mmc: properly annotate fallthrough
> >    zlib: annotate switch/case fallthrough cases
> >    gadget: f_thor: annotate switch/case fallthrough
> >    use proper fallthrough annotations
> >    net/net: fix switch/case fallthrough annotations
> >    fastboot: annotate switch/case fallthrough case
> >    net: sun8i-emac: annotate fallthrough
> >    usb: ohci-hcd: annotate switch/case fallthrough
> >    usb: xhci: annotate switch/case fallthrough properly (TBF)
> >    video: annotate switch/case fall-through
> >    net: e1000: annotate switch/case fallthrough
> >    mtd: ubi: annotate fallthrough (TBF)
> >    arm: mach-k3: am62p: annotate switch/case fallthrough
> >    mtd: spi-nor-tiny: annotate switch/case fallthrough
> >    mtd: rawnand: nand_base: annotate switch/case fallthrough
> >    cmd: pmic: annotate switch/case fallthrough
> >    cmd: spl: annotate switch/case fallthrough
> >    [DO NOT MERGE] Makefile: enable switch/case fallthrough warnings
> >
> >   Makefile                              |  1 +
> >   arch/arm/mach-k3/am62px/am62p5_init.c |  1 +
> >   cmd/pmic.c                            |  1 +
> >   cmd/spl.c                             |  2 ++
> >   common/command.c                      |  2 +-
> >   common/spl/spl_mmc.c                  |  1 +
> >   drivers/fastboot/fb_command.c         |  1 +
> >   drivers/mtd/nand/raw/nand_base.c      |  4 ++++
> >   drivers/mtd/spi/spi-nor-tiny.c        |  1 +
> >   drivers/mtd/ubi/attach.c              |  1 +
> >   drivers/mtd/ubi/build.c               |  3 +++
> >   drivers/net/e1000.c                   |  2 ++
> >   drivers/net/sun8i_emac.c              |  1 +
> >   drivers/usb/gadget/f_thor.c           |  1 +
> >   drivers/usb/host/ohci-hcd.c           |  3 ++-
> >   drivers/usb/host/xhci.c               |  2 +-
> >   drivers/video/video-uclass.c          |  2 ++
> >   lib/tiny-printf.c                     |  2 +-
> >   lib/zlib/inflate.c                    | 21 +++++++++++++++++++++
> >   net/net.c                             |  5 ++---
> >   20 files changed, 50 insertions(+), 7 deletions(-)
> >  
> 

Re: [PATCH 00/18] Annotate switch/case fallthrough cases

[Andre Przywara]

On Fri, 28 Mar 2025 10:39:59 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

Hi,

> On Fri, 28 Mar 2025 11:28:05 +0100
> Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> Hi,
> 
> > On 27.03.25 16:32, Andre Przywara wrote:  
> > > C's implicit fallthrough behaviour in switch/case statements can lead to
> > > subtle bugs. Quite some while ago many compilers introduced warnings in
> > > those cases, requiring intentional fallthrough's to be annotated.
> > >
> > > So far we were not enabling that compiler option, so many ambiguities
> > > and some bugs in the code went unnoticed.
> > >
> > > This series adds the required annotations in code paths that the first
> > > stage of the U-Boot CI covers. There is a large number of cases left
> > > in the libbz2 code. The usage of switch/case is borderline insane there,
> > > labels are hidden in macros, and there are no breaks, but just goto's.
> > > Upstream still uses very similar code, without any annotations. I still
> > > am not 100% sure those are meant to fall through or not, and plan to do
> > > further investigations, but didn't want to hold the rest of the patches
> > > back. You can see for yourself by applying patch 18/18 and building for
> > > sandbox64, for instance.    
> > 
> > Can we use something like
> > 
> > CFLAGS_REMOVE_bzlib.o = -Wimplicit-fallthrough  
> 
> Ah, didn't know we have that in U-Boot as well! Sounds promising, and
> fixes the sandbox build for me (when using bzlib_decompress.o). I will add
> this to the last patch and will check what the CI has to say about this.

So for the records: silencing the libbzip2 warnings uncovered a whole new
bunch of warnings, in stage 1 still. Some compilers used in the CI (clang?)
seem to be more picky about how to annotate, so a pure comment (/* fall
through */) would not cut it, it has to be the attribute - provided by our
"statement macro".
So I fixed those quickly, stuffed them into some patch, and now the first
CI stage (test.py) passes - but only to uncover a large number of new
warnings in the world build.
So I will keep on patching, as some kind of procrastination project ;-)

Cheers,
Andre

> 
> Cheers,
> Andre
> 
> > 
> > in the Makefile if that module is too hard to fix?
> > 
> > Best regards
> > 
> > Heinrich
> >   
> > >
> > > Because of this we cannot quite enable the warning in the Makefile yet,
> > > but those fixes are worth regardless, and be it to increase readability.
> > >
> > > Please not that those patches do not fix anything, really, they just add
> > > those fallthrough annotations, so the series is not really critical.
> > >
> > > Cheers,
> > > Andre
> > >
> > > Andre Przywara (18):
> > >    spl: mmc: properly annotate fallthrough
> > >    zlib: annotate switch/case fallthrough cases
> > >    gadget: f_thor: annotate switch/case fallthrough
> > >    use proper fallthrough annotations
> > >    net/net: fix switch/case fallthrough annotations
> > >    fastboot: annotate switch/case fallthrough case
> > >    net: sun8i-emac: annotate fallthrough
> > >    usb: ohci-hcd: annotate switch/case fallthrough
> > >    usb: xhci: annotate switch/case fallthrough properly (TBF)
> > >    video: annotate switch/case fall-through
> > >    net: e1000: annotate switch/case fallthrough
> > >    mtd: ubi: annotate fallthrough (TBF)
> > >    arm: mach-k3: am62p: annotate switch/case fallthrough
> > >    mtd: spi-nor-tiny: annotate switch/case fallthrough
> > >    mtd: rawnand: nand_base: annotate switch/case fallthrough
> > >    cmd: pmic: annotate switch/case fallthrough
> > >    cmd: spl: annotate switch/case fallthrough
> > >    [DO NOT MERGE] Makefile: enable switch/case fallthrough warnings
> > >
> > >   Makefile                              |  1 +
> > >   arch/arm/mach-k3/am62px/am62p5_init.c |  1 +
> > >   cmd/pmic.c                            |  1 +
> > >   cmd/spl.c                             |  2 ++
> > >   common/command.c                      |  2 +-
> > >   common/spl/spl_mmc.c                  |  1 +
> > >   drivers/fastboot/fb_command.c         |  1 +
> > >   drivers/mtd/nand/raw/nand_base.c      |  4 ++++
> > >   drivers/mtd/spi/spi-nor-tiny.c        |  1 +
> > >   drivers/mtd/ubi/attach.c              |  1 +
> > >   drivers/mtd/ubi/build.c               |  3 +++
> > >   drivers/net/e1000.c                   |  2 ++
> > >   drivers/net/sun8i_emac.c              |  1 +
> > >   drivers/usb/gadget/f_thor.c           |  1 +
> > >   drivers/usb/host/ohci-hcd.c           |  3 ++-
> > >   drivers/usb/host/xhci.c               |  2 +-
> > >   drivers/video/video-uclass.c          |  2 ++
> > >   lib/tiny-printf.c                     |  2 +-
> > >   lib/zlib/inflate.c                    | 21 +++++++++++++++++++++
> > >   net/net.c                             |  5 ++---
> > >   20 files changed, 50 insertions(+), 7 deletions(-)
> > >    
> >   
> 

Re: [PATCH 00/18] Annotate switch/case fallthrough cases

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On Fri, Mar 28, 2025 at 01:45:54PM +0000, Andre Przywara wrote:
> On Fri, 28 Mar 2025 10:39:59 +0000
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
> Hi,
> 
> > On Fri, 28 Mar 2025 11:28:05 +0100
> > Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > 
> > Hi,
> > 
> > > On 27.03.25 16:32, Andre Przywara wrote:  
> > > > C's implicit fallthrough behaviour in switch/case statements can lead to
> > > > subtle bugs. Quite some while ago many compilers introduced warnings in
> > > > those cases, requiring intentional fallthrough's to be annotated.
> > > >
> > > > So far we were not enabling that compiler option, so many ambiguities
> > > > and some bugs in the code went unnoticed.
> > > >
> > > > This series adds the required annotations in code paths that the first
> > > > stage of the U-Boot CI covers. There is a large number of cases left
> > > > in the libbz2 code. The usage of switch/case is borderline insane there,
> > > > labels are hidden in macros, and there are no breaks, but just goto's.
> > > > Upstream still uses very similar code, without any annotations. I still
> > > > am not 100% sure those are meant to fall through or not, and plan to do
> > > > further investigations, but didn't want to hold the rest of the patches
> > > > back. You can see for yourself by applying patch 18/18 and building for
> > > > sandbox64, for instance.    
> > > 
> > > Can we use something like
> > > 
> > > CFLAGS_REMOVE_bzlib.o = -Wimplicit-fallthrough  
> > 
> > Ah, didn't know we have that in U-Boot as well! Sounds promising, and
> > fixes the sandbox build for me (when using bzlib_decompress.o). I will add
> > this to the last patch and will check what the CI has to say about this.
> 
> So for the records: silencing the libbzip2 warnings uncovered a whole new
> bunch of warnings, in stage 1 still. Some compilers used in the CI (clang?)
> seem to be more picky about how to annotate, so a pure comment (/* fall
> through */) would not cut it, it has to be the attribute - provided by our
> "statement macro".
> So I fixed those quickly, stuffed them into some patch, and now the first
> CI stage (test.py) passes - but only to uncover a large number of new
> warnings in the world build.
> So I will keep on patching, as some kind of procrastination project ;-)

So, for the flag, what I would like is to borrow the
CONFIG_CC_IMPLICIT_FALLTHROUGH logic from the kernel. Then we should be
able to do:
CFLAGS_REMOVE_bzlib.o += $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
I think.

Then as follow-up convert any of our just /* fallthrough */ to
'fallthrough;'.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: (subset) [PATCH 00/18] Annotate switch/case fallthrough cases

[Tom Rini]

On Thu, 27 Mar 2025 15:32:55 +0000, Andre Przywara wrote:

> C's implicit fallthrough behaviour in switch/case statements can lead to
> subtle bugs. Quite some while ago many compilers introduced warnings in
> those cases, requiring intentional fallthrough's to be annotated.
> 
> So far we were not enabling that compiler option, so many ambiguities
> and some bugs in the code went unnoticed.
> 
> [...]

Applied to u-boot/master, thanks!

[01/18] spl: mmc: properly annotate fallthrough
        commit: a6a9d3273346138fadb1a173fc2f5e9d0e61215a
[02/18] zlib: annotate switch/case fallthrough cases
        commit: 3d907a5a490b79b876a3f9c325b483a116f29b7e
[03/18] gadget: f_thor: annotate switch/case fallthrough
        commit: 2938eb1e022d7e1de23d89f941bc07b0776a2549
[04/18] use proper fallthrough annotations
        commit: 26b2482f124ba831e40a44ea0cb093203fd8d747
[06/18] fastboot: annotate switch/case fallthrough case
        commit: 06b1ebfe521d2729cd97db7dfed62469081f0ec3
[07/18] net: sun8i-emac: annotate fallthrough
        commit: 5ddb7d1265633d855889831bcad0ee4b9ea4a0d3
[08/18] usb: ohci-hcd: annotate switch/case fallthrough
        commit: 921e4d480dff3206020eacd4873795d0e57d7c20
[09/18] usb: xhci: annotate switch/case fallthrough properly
        commit: 4d108c884b5bd5cef13fa1eb11a65eefd4c48d3f
[10/18] video: annotate switch/case fall-through
        commit: 2c22efbb37b00fc353ffee56bf0b54c38639e08e
[11/18] net: e1000: annotate switch/case fallthrough
        commit: 960d3d933dbda88aed6d4fcf0711dbc7448ffd83
[12/18] mtd: ubi: annotate fallthrough
        commit: 64bc0124581b49093f5c348f5debe8889a56fedd
[13/18] arm: mach-k3: am62p: annotate switch/case fallthrough
        commit: bc3e28e11b7a8b0fb1765556642409e4006a33f7
[14/18] mtd: spi-nor-tiny: annotate switch/case fallthrough
        commit: 452dfcc3b461c1bfbc2126623f42f551000a9ca3
[15/18] mtd: rawnand: nand_base: annotate switch/case fallthrough
        commit: 3f61113c276ffe52380cec159b47e6024249ee2d
[16/18] cmd: pmic: annotate switch/case fallthrough
        commit: d29a90c8ce4a2e27f502f5ac5a051f7bcfd66e11
[17/18] cmd: spl: annotate switch/case fallthrough
        commit: 9ce2986e7e57277bac3c528412c0bb4443b7003a
-- 
Tom