* [PATCH 1/2] staging/vc04_services: initialize cache line size properly @ 2017-03-01 22:50 ` Arnd Bergmann 0 siblings, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2017-03-01 22:50 UTC (permalink / raw) To: linux-arm-kernel While debugging another problem I noticed that g_cache_line_size gets set to sizeof(CACHE_LINE_SIZE), which is sizeof(int) or 4, while presumably CACHE_LINE_SIZE (e.g. 32) was meant. This initializes it the way it was meant. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c index 3aeffcb9c87e..b0e9eb6ff73f 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c @@ -78,7 +78,7 @@ struct vchiq_pagelist_info { }; static void __iomem *g_regs; -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE); +static unsigned int g_cache_line_size = CACHE_LINE_SIZE; static unsigned int g_fragments_size; static char *g_fragments_base; static char *g_free_fragments; -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/2] staging/vc04_services: initialize cache line size properly @ 2017-03-01 22:50 ` Arnd Bergmann 0 siblings, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2017-03-01 22:50 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Arnd Bergmann, Stephen Warren, Lee Jones, Eric Anholt, Michael Zoran, Stefan Wahren, Daniel Stone, popcornmix, linux-rpi-kernel, linux-arm-kernel, devel, linux-kernel While debugging another problem I noticed that g_cache_line_size gets set to sizeof(CACHE_LINE_SIZE), which is sizeof(int) or 4, while presumably CACHE_LINE_SIZE (e.g. 32) was meant. This initializes it the way it was meant. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c index 3aeffcb9c87e..b0e9eb6ff73f 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c @@ -78,7 +78,7 @@ struct vchiq_pagelist_info { }; static void __iomem *g_regs; -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE); +static unsigned int g_cache_line_size = CACHE_LINE_SIZE; static unsigned int g_fragments_size; static char *g_fragments_base; static char *g_free_fragments; -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] staging/vc04_services: add CONFIG_OF dependency 2017-03-01 22:50 ` Arnd Bergmann @ 2017-03-01 22:50 ` Arnd Bergmann -1 siblings, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2017-03-01 22:50 UTC (permalink / raw) To: linux-arm-kernel After several hours of debugging this obviously bogus but elaborate gcc-7.0.1 warning, drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c: In function 'vchiq_complete_bulk': drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:603:4: error: argument 2 null where non-null expected [-Werror=nonnull] memcpy((char *)page_address(pages[0]) + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pagelist->offset, ~~~~~~~~~~~~~~~~~ fragments, ~~~~~~~~~~ head_bytes); ~~~~~~~~~~~ In file included from include/linux/string.h:18:0, from include/linux/bitmap.h:8, from include/linux/cpumask.h:11, from include/linux/interrupt.h:9, from drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:37: arch/arm/include/asm/string.h:16:15: note: in a call to function 'memcpy' declared here extern void * memcpy(void *, const void *, __kernel_size_t) __nocapture(2); ^~~~~~ I have concluded that gcc was technically right in the first place: vchiq_complete_bulk is an externally visible function that calls free_pagelist(), which in turn derives a pointer from the global g_fragments_base variable. g_fragments_base is initialized in vchiq_platform_init(), but we only get there if of_property_read_u32() successfully reads the cache line size. When CONFIG_OF is disabled, this always fails, and g_fragments_base is guaranteed to be NULL when vchiq_complete_bulk() gets called. This adds a CONFIG_OF Kconfig dependency, which is also technically correct but nonobvious, and thus seems like a good fit for the warning. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/staging/vc04_services/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/vc04_services/Kconfig b/drivers/staging/vc04_services/Kconfig index e61e4ca064a8..74094fff4367 100644 --- a/drivers/staging/vc04_services/Kconfig +++ b/drivers/staging/vc04_services/Kconfig @@ -1,6 +1,7 @@ config BCM2835_VCHIQ tristate "Videocore VCHIQ" depends on HAS_DMA + depends on OF depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE) default y help -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] staging/vc04_services: add CONFIG_OF dependency @ 2017-03-01 22:50 ` Arnd Bergmann 0 siblings, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2017-03-01 22:50 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Arnd Bergmann, Stephen Warren, Lee Jones, Eric Anholt, Daniel Stone, Noralf Trønnes, Colin Ian King, popcornmix, Stefan Wahren, linux-rpi-kernel, linux-arm-kernel, devel, linux-kernel After several hours of debugging this obviously bogus but elaborate gcc-7.0.1 warning, drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c: In function 'vchiq_complete_bulk': drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:603:4: error: argument 2 null where non-null expected [-Werror=nonnull] memcpy((char *)page_address(pages[0]) + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pagelist->offset, ~~~~~~~~~~~~~~~~~ fragments, ~~~~~~~~~~ head_bytes); ~~~~~~~~~~~ In file included from include/linux/string.h:18:0, from include/linux/bitmap.h:8, from include/linux/cpumask.h:11, from include/linux/interrupt.h:9, from drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:37: arch/arm/include/asm/string.h:16:15: note: in a call to function 'memcpy' declared here extern void * memcpy(void *, const void *, __kernel_size_t) __nocapture(2); ^~~~~~ I have concluded that gcc was technically right in the first place: vchiq_complete_bulk is an externally visible function that calls free_pagelist(), which in turn derives a pointer from the global g_fragments_base variable. g_fragments_base is initialized in vchiq_platform_init(), but we only get there if of_property_read_u32() successfully reads the cache line size. When CONFIG_OF is disabled, this always fails, and g_fragments_base is guaranteed to be NULL when vchiq_complete_bulk() gets called. This adds a CONFIG_OF Kconfig dependency, which is also technically correct but nonobvious, and thus seems like a good fit for the warning. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/staging/vc04_services/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/vc04_services/Kconfig b/drivers/staging/vc04_services/Kconfig index e61e4ca064a8..74094fff4367 100644 --- a/drivers/staging/vc04_services/Kconfig +++ b/drivers/staging/vc04_services/Kconfig @@ -1,6 +1,7 @@ config BCM2835_VCHIQ tristate "Videocore VCHIQ" depends on HAS_DMA + depends on OF depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE) default y help -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/2] staging/vc04_services: initialize cache line size properly 2017-03-01 22:50 ` Arnd Bergmann @ 2017-03-02 1:59 ` Michael Zoran -1 siblings, 0 replies; 8+ messages in thread From: Michael Zoran @ 2017-03-02 1:59 UTC (permalink / raw) To: linux-arm-kernel Hi Arnd, I submitted a change which is in Linux-next now that makes the whole CACHE_LINE_SIZE macro meaningless. It now always reads the size from the DT and errors out with -ENODEV if the property is missing. I was going to submit a change to delete the macro completely, just never got to it. https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit /drivers/staging/vc04_services/interface?id=6cf1bf636a067eb308cb3a8322b 9d6b1844a075d On Wed, 2017-03-01 at 23:50 +0100, Arnd Bergmann wrote: > While debugging another problem I noticed that g_cache_line_size gets > set > to sizeof(CACHE_LINE_SIZE), which is sizeof(int) or 4, while > presumably > CACHE_LINE_SIZE (e.g. 32) was meant. > > This initializes it the way it was meant. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > ?drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | > 2 +- > ?1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > index 3aeffcb9c87e..b0e9eb6ff73f 100644 > --- > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > +++ > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > @@ -78,7 +78,7 @@ struct vchiq_pagelist_info { > ?}; > ? > ?static void __iomem *g_regs; > -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE); > +static unsigned int g_cache_line_size = CACHE_LINE_SIZE; > ?static unsigned int g_fragments_size; > ?static char *g_fragments_base; > ?static char *g_free_fragments; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] staging/vc04_services: initialize cache line size properly @ 2017-03-02 1:59 ` Michael Zoran 0 siblings, 0 replies; 8+ messages in thread From: Michael Zoran @ 2017-03-02 1:59 UTC (permalink / raw) To: Arnd Bergmann, Greg Kroah-Hartman Cc: Stephen Warren, Lee Jones, Eric Anholt, Stefan Wahren, Daniel Stone, popcornmix, linux-rpi-kernel, linux-arm-kernel, devel, linux-kernel Hi Arnd, I submitted a change which is in Linux-next now that makes the whole CACHE_LINE_SIZE macro meaningless. It now always reads the size from the DT and errors out with -ENODEV if the property is missing. I was going to submit a change to delete the macro completely, just never got to it. https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit /drivers/staging/vc04_services/interface?id=6cf1bf636a067eb308cb3a8322b 9d6b1844a075d On Wed, 2017-03-01 at 23:50 +0100, Arnd Bergmann wrote: > While debugging another problem I noticed that g_cache_line_size gets > set > to sizeof(CACHE_LINE_SIZE), which is sizeof(int) or 4, while > presumably > CACHE_LINE_SIZE (e.g. 32) was meant. > > This initializes it the way it was meant. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | > 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > index 3aeffcb9c87e..b0e9eb6ff73f 100644 > --- > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > +++ > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c > @@ -78,7 +78,7 @@ struct vchiq_pagelist_info { > }; > > static void __iomem *g_regs; > -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE); > +static unsigned int g_cache_line_size = CACHE_LINE_SIZE; > static unsigned int g_fragments_size; > static char *g_fragments_base; > static char *g_free_fragments; ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] staging/vc04_services: initialize cache line size properly 2017-03-02 1:59 ` Michael Zoran @ 2017-03-06 12:53 ` Greg Kroah-Hartman -1 siblings, 0 replies; 8+ messages in thread From: Greg Kroah-Hartman @ 2017-03-06 12:53 UTC (permalink / raw) To: linux-arm-kernel On Wed, Mar 01, 2017 at 05:59:38PM -0800, Michael Zoran wrote: > Hi Arnd, > > I submitted a change which is in Linux-next now that makes the whole > CACHE_LINE_SIZE macro meaningless. It now always reads the size from > the DT and errors out with -ENODEV if the property is missing. > > I was going to submit a change to delete the macro completely, just > never got to it. > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit > /drivers/staging/vc04_services/interface?id=6cf1bf636a067eb308cb3a8322b > 9d6b1844a075d This should be in 4.11-rc1. thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] staging/vc04_services: initialize cache line size properly @ 2017-03-06 12:53 ` Greg Kroah-Hartman 0 siblings, 0 replies; 8+ messages in thread From: Greg Kroah-Hartman @ 2017-03-06 12:53 UTC (permalink / raw) To: Michael Zoran Cc: Arnd Bergmann, Stefan Wahren, devel, Daniel Stone, Stephen Warren, Lee Jones, linux-kernel, Eric Anholt, linux-rpi-kernel, popcornmix, linux-arm-kernel On Wed, Mar 01, 2017 at 05:59:38PM -0800, Michael Zoran wrote: > Hi Arnd, > > I submitted a change which is in Linux-next now that makes the whole > CACHE_LINE_SIZE macro meaningless. It now always reads the size from > the DT and errors out with -ENODEV if the property is missing. > > I was going to submit a change to delete the macro completely, just > never got to it. > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit > /drivers/staging/vc04_services/interface?id=6cf1bf636a067eb308cb3a8322b > 9d6b1844a075d This should be in 4.11-rc1. thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-03-06 12:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-01 22:50 [PATCH 1/2] staging/vc04_services: initialize cache line size properly Arnd Bergmann 2017-03-01 22:50 ` Arnd Bergmann 2017-03-01 22:50 ` [PATCH 2/2] staging/vc04_services: add CONFIG_OF dependency Arnd Bergmann 2017-03-01 22:50 ` Arnd Bergmann 2017-03-02 1:59 ` [PATCH 1/2] staging/vc04_services: initialize cache line size properly Michael Zoran 2017-03-02 1:59 ` Michael Zoran 2017-03-06 12:53 ` Greg Kroah-Hartman 2017-03-06 12:53 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.