Re: [PATCH 1/3] fastboot: lift restrictions on !NET_LWIP for USB

[Tom Rini <trini@konsulko.com>]

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

On Fri, Mar 28, 2025 at 10:06:12AM +0100, Michael Walle wrote:
> Hi Mattijs,
> 
> > > Fastboot works either over TCP, UDP or USB. The latter doesn't have
> > > anything to do with networking, thus should work just fine with
> > > regardless which network stack is selected. In practice, header symbols
> > > are used inside common code paths. Add some ifdeffery to guard against
> > > that.
> > >
> > > This will make fastboot over USB work with the new LWIP stack.
> > >
> > > Signed-off-by: Michael Walle <mwalle@kernel.org>
> >
> > checkpatch.pl reports some issues with this:
> >
> > $ ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD
> >
> > <snip>
> > WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
> > <snip>
> >
> > Some occurences below could be fixed. Could you please have a look?
> 
> I've seen these. More below.

That is also my least favorite checkpatch warning, and it's a warning
not an error. Best judgement is needed about making things readable
rather than just silencing checkpatch.

> 
> >
> > > ---
> > > Alternatively, we could add the defines and stub functions to the lwip
> > > header.
> 
> This is relevant :)
> 
> > > ---
> > >  cmd/fastboot.c               | 4 ++++
> > >  drivers/fastboot/Kconfig     | 1 -
> > >  drivers/fastboot/fb_common.c | 4 ++++
> > >  3 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> > > index d4cfc0c7a28..be84a482b81 100644
> > > --- a/cmd/fastboot.c
> > > +++ b/cmd/fastboot.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/printk.h>
> > >  #include <linux/stringify.h>
> > >  
> > > +#if CONFIG_IS_ENABLED(NET)
> >
> > I think this can be dropped. I hope that since it's a static function,
> > -if there are no users in the file- the compiler will optimize it out.
> > Note: I have not verified this, so I might be wrong.
> >
> > If you measure and see size changes between keeping the #if and not
> > keeping it, please ignore this comment.
> 
> No, because net_loop(), net_set_state() and NETLOOP_SUCCESS is only
> defined in net-legacy.h. Thus we need this ifdeffery.. Unless of
> course, we add the enums and function stubs to the new lwip net
> inlcude. I don't know if that's a good idea though.

Yeah, I'm not sure that's worth it either, or at least as maybe only a
future clean-up or investigate more. Perhaps filing an issue on
source.denx.de on the -net tree so it's not forgotten?

-- 
Tom

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