[PATCH] staging: vchiq: add 'static' to function definition

[PATCH] staging: vchiq: add 'static' to function definition

[Scott J. Crouch]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: add 'static' to function definition

[Greg Kroah-Hartman]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: add 'static' to function definition

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: add 'static' to function definition

[Scott J. Crouch]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: add 'static' to function definition

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: add 'static' to function definition

[Scott J. Crouch]

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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel