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

["Michael Walle" <mwalle@kernel.org>]

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

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.

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

-michael

>
> >  static int do_fastboot_udp(int argc, char *const argv[],
> >  			   uintptr_t buf_addr, size_t buf_size)
> >  {
> > @@ -55,6 +56,7 @@ static int do_fastboot_tcp(int argc, char *const argv[],
> >  
> >  	return CMD_RET_SUCCESS;
> >  }
> > +#endif
> >  
> >  static int do_fastboot_usb(int argc, char *const argv[],
> >  			   uintptr_t buf_addr, size_t buf_size)
> > @@ -160,10 +162,12 @@ NXTARG:
> >  
> >  	fastboot_init((void *)buf_addr, buf_size);
> >  
> > +#if CONFIG_IS_ENABLED(NET)
> >  	if (!strcmp(argv[1], "udp"))
> >  		return do_fastboot_udp(argc, argv, buf_addr, buf_size);
> >  	if (!strcmp(argv[1], "tcp"))
> >  		return do_fastboot_tcp(argc, argv, buf_addr, buf_size);
> > +#endif
>
> This can be replaced with:
>
> 	if (IS_ENABLED(CONFIG_NET)) {
> 		if (!strcmp(argv[1], "udp"))
> 			return do_fastboot_udp(argc, argv, buf_addr, buf_size);
> 		if (!strcmp(argv[1], "tcp"))
> 			return do_fastboot_tcp(argc, argv, buf_addr, buf_size);
> 	}
>
> >  	if (!strcmp(argv[1], "usb")) {
> >  		argv++;
> >  		argc--;
> > diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> > index 1eb460f5a02..70207573de2 100644
> > --- a/drivers/fastboot/Kconfig
> > +++ b/drivers/fastboot/Kconfig
> > @@ -1,6 +1,5 @@
> >  menu "Fastboot support"
> >  	depends on CMDLINE
> > -	depends on !NET_LWIP
> >  
> >  config FASTBOOT
> >  	bool
> > diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> > index 12ffb463deb..68f92c4b887 100644
> > --- a/drivers/fastboot/fb_common.c
> > +++ b/drivers/fastboot/fb_common.c
> > @@ -183,11 +183,15 @@ void fastboot_handle_boot(int command, bool success)
> >  	switch (command) {
> >  	case FASTBOOT_COMMAND_BOOT:
> >  		fastboot_boot();
> > +#if CONFIG_IS_ENABLED(NET)
> >  		net_set_state(NETLOOP_SUCCESS);
> > +#endif
>
> This can be replaced with:
>
> 		if (IS_ENABLED((CONFIG_NET))
> 		    net_set_state(NETLOOP_SUCCESS);
>
> >  		break;
> >  
> >  	case FASTBOOT_COMMAND_CONTINUE:
> > +#if CONFIG_IS_ENABLED(NET)
> >  		net_set_state(NETLOOP_SUCCESS);
> > +#endif
>
> Same here.
>
> >  		break;
> >  
> >  	case FASTBOOT_COMMAND_REBOOT:
> > -- 
> > 2.39.5


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