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

[Tom Rini <trini@konsulko.com>]

[-- 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 --]