Re: [PATCH 2/2] staging: vchiq_arm: add prototype of function vchiq_platform_init()

[Dan Carpenter]

On Fri, Mar 04, 2022 at 04:30:15PM -0300, Gaston Gonzalez wrote:
> On Thu, Mar 03, 2022 at 03:25:47PM +0300, Dan Carpenter wrote:
> > On Wed, Mar 02, 2022 at 06:36:38PM -0300, Gaston Gonzalez wrote:
> > > Fix "no previous prototype" W=1 warning by adding the prototype of the
> > > function vchiq_platform_init().
> > > 
> > > Note: vchiq_platform_init() is only called once in vchiq_probe(), so
> > > presumably should be static function. However, making the function
> > > static breaks the build.
> > > 
> > 
> > That's weird.  I don't have an ARM cross compile set up.  How does the
> > build break?
> > 
> > regards,
> > dan carpenter
> >
> 
> Hi Dan,
> 
> Test building the driver in x86 I get the error pasted below.
> 
> However, now that you mention it, I made an ARM (64 bit) cross
> compilation: making the function static builds OK without the warning.
> I'll to do the same for a 32 bit setup.
> 
> So I suppose that making the function static is the right approach ?
> 
> FWIW, branch and cross-compiler:
> 
> - Remote: https://github.com/raspberrypi/linux.git
> - Branch: rpi-5.17.y
> - gcc: aarch64-linux-gnu-gcc (Debian 10.2.1-6) 10.2.1 20210110
> 
> 
> -> x86 build error:
> 
> CC [M]  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.o
> In function ‘free_pagelist’,
>     inlined from ‘vchiq_complete_bulk’ at drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:650:3:
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:434:4: error: argument 2 null where non-null expected [-Werror=nonnull]
>   434 |    memcpy((char *)kmap(pages[0]) +
>       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   435 |     pagelist->offset,
>       |     ~~~~~~~~~~~~~~~~~
>   436 |     fragments,
>       |     ~~~~~~~~~~
>   437 |     head_bytes);
>       |     ~~~~~~~~~~~
> In file included from ./arch/x86/include/asm/string.h:5,
>                  from ./include/linux/string.h:20,
>                  from ./include/linux/bitmap.h:11,
>                  from ./include/linux/cpumask.h:12,
>                  from ./arch/x86/include/asm/cpumask.h:5,
>                  from ./arch/x86/include/asm/msr.h:11,
>                  from ./arch/x86/include/asm/processor.h:22,
>                  from ./arch/x86/include/asm/timex.h:5,
>                  from ./include/linux/timex.h:65,
>                  from ./include/linux/time32.h:13,
>                  from ./include/linux/time.h:60,
>                  from ./include/linux/stat.h:19,
>                  from ./include/linux/module.h:13,
>                  from drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:8:
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: In function ‘vchiq_complete_bulk’:
> ./arch/x86/include/asm/string_64.h:14:14: note: in a call to function ‘memcpy’ declared here
>    14 | extern void *memcpy(void *to, const void *from, size_t len);
>       |              ^~~~~~
> cc1: all warnings being treated as errors
> make[1]: *** [scripts/Makefile.build:288: drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.o] Error 1
> make: *** [Makefile:1831: drivers/staging/vc04_services] Error 2

That's so weird.

1) The GCC warning is a false positive, but -Werror means we have to
   deal with it.
2) I can't explain the false positive.
3) My guess is that making the function static affects how it is inlined
   so it gives the compiler more visibility into how free_pagelist()?
   is called so that's why it gets triggered because of the patch?
4) This is an x86 config and it's an ARM driver, but nothing in the
   Kconfig file says that vchiq_arm needs ARM or raspberry pi.  Should
   there be?  And a COMPILE_TEST option?

Anyway, we don't paper over warnings like this so the patch is not the
correct thing.  Especially we don't paper over it without a giant
comment explaining how we filed a bug with GCC or whatever and a link
to bugzilla and all that other stuff.

regards,
dan carpenter