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

[Andre Przywara <andre.przywara@arm.com>]

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