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

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

[Michael Walle]

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>
---
Alternatively, we could add the defines and stub functions to the lwip
header.
---
 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)
 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
 	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
 		break;
 
 	case FASTBOOT_COMMAND_CONTINUE:
+#if CONFIG_IS_ENABLED(NET)
 		net_set_state(NETLOOP_SUCCESS);
+#endif
 		break;
 
 	case FASTBOOT_COMMAND_REBOOT:
-- 
2.39.5

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

[Michael Walle]

Sorry this should have been individual patches. Please read the subject 
as
"[PATCH 1/1]" or just "[PATCH]".

-michael

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

[Jerome Forissier]

Hi Michael,

On 3/12/25 08:36, Michael Walle wrote:
> 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>
> ---
> Alternatively, we could add the defines and stub functions to the lwip
> header.
> ---
>  cmd/fastboot.c               | 4 ++++
>  drivers/fastboot/Kconfig     | 1 -
>  drivers/fastboot/fb_common.c | 4 ++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
[...]

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Thanks,
-- 
Jerome

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

[Mattijs Korpershoek]

Hi Michael,

Thank you for the patch.

On mer., mars 12, 2025 at 08:36, Michael Walle <mwalle@kernel.org> wrote:

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

> ---
> Alternatively, we could add the defines and stub functions to the lwip
> header.
> ---
>  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.

>  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

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

[Michael Walle]

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

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

[Tom Rini]

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

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

[Mattijs Korpershoek]

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

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

[Mattijs Korpershoek]

Hi,

On Wed, 12 Mar 2025 08:36:55 +0100, Michael Walle wrote:
> 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.
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/3] fastboot: lift restrictions on !NET_LWIP for USB
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/d3c9f810f2b317ba376b4d9556e63a043b210355

--
Mattijs