Hi Michael, Tom, On ven., mars 28, 2025 at 09:59, Tom Rini <tr...@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 <mwa...@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 <mkorpersh...@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
