staging: vchiq: add 'static' to function definition

  This fixes the following sparse error:

    warning: symbol 'vchiq_platform_init' was not declared. Should it be static?

Signed-off-by: Scott J. Crouch <scottjcrouch@gmail.com>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Sat, Oct 22, 2022 at 03:35:48PM +1100, Scott J. Crouch wrote:
> This fixes the following sparse error:
> 
>     warning: symbol 'vchiq_platform_init' was not declared. Should it be static?
> 
> Signed-off-by: Scott J. Crouch <scottjcrouch@gmail.com>
> ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index dc33490ba7fb..ffa517077b80 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -462,7 +462,7 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>  	cleanup_pagelistinfo(instance, pagelistinfo);
>  }
>  
> -int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
> +static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
> -- 
> 2.37.3
> 
> 

Nice try, but this breaks the build in a very horrible and strange way
that no one has been able to figure out yet:

 CC [M]  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.o
In file included from drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:16:
In function ‘memcpy_to_page’,
    inlined from ‘free_pagelist’ at drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:433:4,
    inlined from ‘vchiq_complete_bulk’ at drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:648:3:
./include/linux/highmem.h:377:9: error: argument 2 null where non-null expected [-Werror=nonnull]
  377 |         memcpy(to + offset, from, len);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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:67,
                 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:19:14: note: in a call to function ‘memcpy’ declared ‘nonnull’
   19 | extern void *memcpy(void *to, const void *from, size_t len);
      |              ^~~~~~
In file included from drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:16:
In function ‘memcpy_to_page’,
    inlined from ‘free_pagelist’ at drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:440:4,
    inlined from ‘vchiq_complete_bulk’ at drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:648:3:
./include/linux/highmem.h:377:9: error: ‘memcpy’ offset 0 is out of the bounds [0, 0] [-Werror=array-bounds]
  377 |         memcpy(to + offset, from, len);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


So we can't take this patch.

Also remember to always test-build your patches before sending them.

thanks,

greg k-h
  

Hi Scott,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/intel-lab-lkp/linux/commits/Scott-J-Crouch/staging-vchiq-add-static-to-function-definition/20221022-123712
patch link:    https://lore.kernel.org/r/20221022043548.1671644-1-scottjcrouch%40gmail.com
patch subject: [PATCH] staging: vchiq: add 'static' to function definition
config: ia64-randconfig-s043-20221019
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/212f23023953af62f30dfa46cae6ed350f72e798
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Scott-J-Crouch/staging-vchiq-add-static-to-function-definition/20221022-123712
        git checkout 212f23023953af62f30dfa46cae6ed350f72e798
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/staging/vc04_services/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:16:
   In function 'memcpy_to_page',
       inlined from 'free_pagelist' at drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:433:4:
>> include/linux/highmem.h:377:9: warning: argument 2 null where non-null expected [-Wnonnull]
     377 |         memcpy(to + offset, from, len);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/string.h:20,
                    from include/linux/bitmap.h:11,
                    from include/linux/cpumask.h:12,
                    from include/linux/mm_types_task.h:14,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:8:
   arch/ia64/include/asm/string.h: In function 'free_pagelist':
   arch/ia64/include/asm/string.h:19:14: note: in a call to function 'memcpy' declared 'nonnull'
      19 | extern void *memcpy (void *, const void *, __kernel_size_t);
         |              ^~~~~~
   In function 'memcpy_to_page',
       inlined from 'free_pagelist' at drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:440:4:
>> include/linux/highmem.h:377:9: warning: 'memcpy' reading between 1 and 31 bytes from a region of size 0 [-Wstringop-overread]
     377 |         memcpy(to + offset, from, len);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +377 include/linux/highmem.h

bb90d4bc7b6a53 Ira Weiny         2021-02-09  370  
bb90d4bc7b6a53 Ira Weiny         2021-02-09  371  static inline void memcpy_to_page(struct page *page, size_t offset,
bb90d4bc7b6a53 Ira Weiny         2021-02-09  372  				  const char *from, size_t len)
bb90d4bc7b6a53 Ira Weiny         2021-02-09  373  {
61b205f579911a Ira Weiny         2021-02-09  374  	char *to = kmap_local_page(page);
bb90d4bc7b6a53 Ira Weiny         2021-02-09  375  
ca18f6ea012bf3 Ira Weiny         2021-02-10  376  	VM_BUG_ON(offset + len > PAGE_SIZE);
bb90d4bc7b6a53 Ira Weiny         2021-02-09 @377  	memcpy(to + offset, from, len);
8dad53a11f8d94 Christoph Hellwig 2021-07-23  378  	flush_dcache_page(page);
61b205f579911a Ira Weiny         2021-02-09  379  	kunmap_local(to);
bb90d4bc7b6a53 Ira Weiny         2021-02-09  380  }
bb90d4bc7b6a53 Ira Weiny         2021-02-09  381
  

On Sat, Oct 22, 2022, at 10:07, Greg Kroah-Hartman wrote:
> On Sat, Oct 22, 2022 at 03:35:48PM +1100, Scott J. Crouch wrote:

> Nice try, but this breaks the build in a very horrible and strange way
> that no one has been able to figure out yet:

I got curious and figured out what happens:

Without CONFIG_OF, of_match_node() always returns NULL, so
vchiq_probe() returns -EINVAL unconditionally before calling
vchiq_platform_init(). 

If vchiq_platform_init() is marked 'static', gcc's dead code
elimination then eliminates it, which in turn means that
'g_fragments_base' is never initialized and gets replaced
with a NULL pointer.

I think the easiest workaround is to take this otherwise correct
patch but at the same time add 'depends on OF' to BCM2835_VCHIQ.
This is a correct dependency as shown by gcc above, and it still
allows compile testing this driver on x86 allmodconfig, which
enables CONFIG_OF.

     Arnd
  

On 28/10/22 01:08, Arnd Bergmann wrote:
> On Sat, Oct 22, 2022, at 10:07, Greg Kroah-Hartman wrote:
>> On Sat, Oct 22, 2022 at 03:35:48PM +1100, Scott J. Crouch wrote:
> 
>> Nice try, but this breaks the build in a very horrible and strange way
>> that no one has been able to figure out yet:
> 
> I got curious and figured out what happens:
> 
> Without CONFIG_OF, of_match_node() always returns NULL, so
> vchiq_probe() returns -EINVAL unconditionally before calling
> vchiq_platform_init(). 
> 
> If vchiq_platform_init() is marked 'static', gcc's dead code
> elimination then eliminates it, which in turn means that
> 'g_fragments_base' is never initialized and gets replaced
> with a NULL pointer.

Good spotting.  Actually, I was clumsily learning how to run sparse on the
staging directory and wasn't sure what config to use that didn't involve
enabling everything manually.  But from what I can tell, it's OF_OVERLAY (or
something downstream of that) that gets rid of the warning.  BCM2835_VCHIQ
requires BCM_VIDEOCORE which already depends on OF.  I'm confused because I
don't actually know how to reproduce the gcc warning without manually enabling
the module such that OF is enabled but OF_OVERLAY isn't, since allmodconfig
enables both -- maybe there's a way to do that.

I'm also confused why devm_rpi_firmware_get() doesn't have the same problem as
of_match_node() just above it -- it returns NULL when RASPBERRYPI_FIRMWARE is
unset, but gcc still builds without the warning.

Scott.
  

On Sun, Oct 30, 2022, at 03:43, Scott J. Crouch wrote:
> On 28/10/22 01:08, Arnd Bergmann wrote:
>> On Sat, Oct 22, 2022, at 10:07, Greg Kroah-Hartman wrote:
>>> On Sat, Oct 22, 2022 at 03:35:48PM +1100, Scott J. Crouch wrote:
>> 
>>> Nice try, but this breaks the build in a very horrible and strange way
>>> that no one has been able to figure out yet:
>> 
>> I got curious and figured out what happens:
>> 
>> Without CONFIG_OF, of_match_node() always returns NULL, so
>> vchiq_probe() returns -EINVAL unconditionally before calling
>> vchiq_platform_init(). 
>> 
>> If vchiq_platform_init() is marked 'static', gcc's dead code
>> elimination then eliminates it, which in turn means that
>> 'g_fragments_base' is never initialized and gets replaced
>> with a NULL pointer.
>
> Good spotting.  Actually, I was clumsily learning how to run sparse on the
> staging directory and wasn't sure what config to use that didn't involve
> enabling everything manually.  But from what I can tell, it's OF_OVERLAY (or
> something downstream of that) that gets rid of the warning.  BCM2835_VCHIQ
> requires BCM_VIDEOCORE which already depends on OF. 

Ok, I see. My best guess would be OF_DYNAMIC in this case, but I don't
actually see how that changes anything in this file (I only looked
at the sources, did not build)

> I'm also confused why devm_rpi_firmware_get() doesn't have the same problem as
> of_match_node() just above it -- it returns NULL when RASPBERRYPI_FIRMWARE is
> unset, but gcc still builds without the warning.

Hmm, I see an of_node_put() between devm_rpi_firmware_get() and the
error check. With OF_DYNAMIC=y, this is an external function call, so
I guess gcc can no longer assume that drvdata->fw is NULL after that,
so it doesn't make this optimization.

This would mean you only get a warning when both RASPBERRYPI_FIRMWARE
and OF_DYNAMIC are disabled. If you can confirm that, adding a dependency
on RASPBERRYPI_FIRMWARE is still the correct fix.

        Arnd
  

On 30/10/22 21:46, Arnd Bergmann wrote:
> Hmm, I see an of_node_put() between devm_rpi_firmware_get() and the
> error check. With OF_DYNAMIC=y, this is an external function call, so
> I guess gcc can no longer assume that drvdata->fw is NULL after that,
> so it doesn't make this optimization.

Ah, of course, you're right.

> Ok, I see. My best guess would be OF_DYNAMIC

I checked and you are correct.

> but I don't actually see how that changes anything in this file

Yeah.  I had a look as well and I'm similarly puzzled.

> This would mean you only get a warning when both RASPBERRYPI_FIRMWARE
> and OF_DYNAMIC are disabled. If you can confirm that, adding a dependency
> on RASPBERRYPI_FIRMWARE is still the correct fix.

It builds ok as long as OF_DYNAMIC is set; RASPBERRYPI_FIRMWARE doesn't need
setting (maybe since devm_rpi_firmware_get() is a static inline it's optimized
differently?).  Perhaps RASPBERRYPI_FIRMWARE ought to be made a dependency in
any case, but since it depends on ARCH_BCM2835, doing so will mean one is
unable to test-compile/run sparse on x86.  Which is fine; I wouldn't expect to
be able to; I'm not really sure what accommodations (if any) are usually made
in this regard with respect to drivers.

Scott.