Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible

[Tom Rini <trini@konsulko.com>]

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

On Thu, Jan 05, 2023 at 12:37:58PM +0100, Patrick DELAUNAY wrote:
> Hi Tom,
> 
> On 1/3/23 21:35, Tom Rini wrote:
> > On Thu, Dec 15, 2022 at 10:15:50AM +0100, Patrick Delaunay wrote:
> > > Much of the fastboot code predates the introduction of Kconfig and
> > > has quite a few #ifdefs in it which is unnecessary now that we can use
> > > IS_ENABLED() et al.
> > > 
> > > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > > ---
> > > 
> > >   cmd/fastboot.c                  |  35 +++++------
> > >   drivers/fastboot/fb_command.c   | 104 ++++++++++++--------------------
> > >   drivers/fastboot/fb_common.c    |  11 ++--
> > >   drivers/fastboot/fb_getvar.c    |  49 ++++++---------
> > >   drivers/usb/gadget/f_fastboot.c |   7 +--
> > >   include/fastboot.h              |  13 ----
> > >   net/fastboot.c                  |   8 +--
> > >   7 files changed, 82 insertions(+), 145 deletions(-)
> > > 
> > > diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> > > index b498e4b22bb3..b94dbd548843 100644
> > > --- a/cmd/fastboot.c
> > > +++ b/cmd/fastboot.c
> > > @@ -19,8 +19,14 @@
> > >   static int do_fastboot_udp(int argc, char *const argv[],
> > >   			   uintptr_t buf_addr, size_t buf_size)
> > >   {
> > > -#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
> > > -	int err = net_loop(FASTBOOT);
> > > +	int err;
> > > +
> > > +	if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
> > > +		pr_err("Fastboot UDP not enabled\n");
> > > +		return CMD_RET_FAILURE;
> > > +	}
> > > +
> > > +	err = net_loop(FASTBOOT);
> > >   	if (err < 0) {
> > >   		printf("fastboot udp error: %d\n", err);
> > > @@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const argv[],
> > >   	}
> > >   	return CMD_RET_SUCCESS;
> > > -#else
> > > -	pr_err("Fastboot UDP not enabled\n");
> > > -	return CMD_RET_FAILURE;
> > > -#endif
> > >   }
> > This probably needs to become an if (CONFIG_IS_ENABLED(...)) { ... }
> > else { ... } in order to remain size-neutral.
> 
> 
> Are you sure ?
> 
> 
> {
>     if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
>         ....
>         return CMD_RET_FAILURE;
>     }
> 
>     ....
> 
>     return CMD_RET_SUCCESS;
> }
> 
> 
> For me, it is exactly the same size after compiler/linker than :
> 
> 
> {
>     if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
>         ....
>         return CMD_RET_FAILURE;
>     } else {
>     ....
>           return CMD_RET_SUCCESS;
> 
>     }
> 
> }
> 
> 
> if UDP_FUNCTION_FASTBOOTis activated or not....
> 
> or I forget something during the Christmas break.

If you've size-tested and it's the same, OK. I'm just worried about
strings not being discarded since that's sometimes an unexpected side
effect / bug.

-- 
Tom

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