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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Michael, Tom,

On ven., mars 28, 2025 at 09:59, Tom Rini <trini@konsulko.com> wrote:

> 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.

I think that using if (IS_ENABLED(CONFIG...) is more readable in this
case.
However ...

>
>> 
>> >
>> > > ---
>> > > 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.

... my suggestion does not work without adding the defines and stub
functions to the lwip header.
Apologies about that :( And thank you for being patient with my remarks.

This patch is fine as-is, and I don't want ifdeffery vs if IS_ENABLED to
block this contribution.

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

>
> 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?

I'd like to do this future cleanup. I could not file an issue on
https://source.denx.de/u-boot/custodians/u-boot-net so I have opened one
in the -dfu tree here:

https://source.denx.de/u-boot/custodians/u-boot-dfu/-/issues/5

>
> -- 
> Tom