| Message ID | 20180924172615.25591-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: ccp: Remove forward declaration | expand |
On 09/24/2018 12:26 PM, Nathan Chancellor wrote: > Clang emits a warning about this construct: > > drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array > definition assumed to have one element > static const struct acpi_device_id sp_acpi_match[]; > ^ > 1 warning generated. > > Just remove the forward declarations and move the initializations up > so that they can be used in sp_get_of_version and sp_get_acpi_version. I'm not going to out and out object to this just yet. I am not a clang expert. Can you please provide a make command that would explain how you precipitated this complaint? > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- > 1 file changed, 25 insertions(+), 28 deletions(-) > > diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c > index 71734f254fd1..b75dc7db2d4a 100644 > --- a/drivers/crypto/ccp/sp-platform.c > +++ b/drivers/crypto/ccp/sp-platform.c > @@ -33,8 +33,31 @@ struct sp_platform { > unsigned int irq_count; > }; > > -static const struct acpi_device_id sp_acpi_match[]; > -static const struct of_device_id sp_of_match[]; > +static const struct sp_dev_vdata dev_vdata[] = { > + { > + .bar = 0, > +#ifdef CONFIG_CRYPTO_DEV_SP_CCP > + .ccp_vdata = &ccpv3_platform, > +#endif > + }, > +}; > + > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id sp_acpi_match[] = { > + { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > +#endif > + > +#ifdef CONFIG_OF > +static const struct of_device_id sp_of_match[] = { > + { .compatible = "amd,ccp-seattle-v1a", > + .data = (const void *)&dev_vdata[0] }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, sp_of_match); > +#endif > > static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev) > { > @@ -201,32 +224,6 @@ static int sp_platform_resume(struct platform_device *pdev) > } > #endif > > -static const struct sp_dev_vdata dev_vdata[] = { > - { > - .bar = 0, > -#ifdef CONFIG_CRYPTO_DEV_SP_CCP > - .ccp_vdata = &ccpv3_platform, > -#endif > - }, > -}; > - > -#ifdef CONFIG_ACPI > -static const struct acpi_device_id sp_acpi_match[] = { > - { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > - { }, > -}; > -MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > -#endif > - > -#ifdef CONFIG_OF > -static const struct of_device_id sp_of_match[] = { > - { .compatible = "amd,ccp-seattle-v1a", > - .data = (const void *)&dev_vdata[0] }, > - { }, > -}; > -MODULE_DEVICE_TABLE(of, sp_of_match); > -#endif > - > static struct platform_driver sp_platform_driver = { > .driver = { > .name = "ccp", >
On Mon, Sep 24, 2018 at 07:18:23PM +0000, Gary R Hook wrote: > On 09/24/2018 12:26 PM, Nathan Chancellor wrote: > > Clang emits a warning about this construct: > > > > drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array > > definition assumed to have one element > > static const struct acpi_device_id sp_acpi_match[]; > > ^ > > 1 warning generated. > > > > Just remove the forward declarations and move the initializations up > > so that they can be used in sp_get_of_version and sp_get_acpi_version. > > I'm not going to out and out object to this just yet. > > I am not a clang expert. Can you please provide a make command that > would explain how you precipitated this complaint? > Hi Gary, I can produce the warning with Clang 6.0 using the following set of commands: make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- allyesconfig ./scripts/config -d CONFIG_CPU_BIG_ENDIAN make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- olddefconfig make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- drivers/crypto/ccp/sp-platform.o Nathan > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- > > 1 file changed, 25 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c > > index 71734f254fd1..b75dc7db2d4a 100644 > > --- a/drivers/crypto/ccp/sp-platform.c > > +++ b/drivers/crypto/ccp/sp-platform.c > > @@ -33,8 +33,31 @@ struct sp_platform { > > unsigned int irq_count; > > }; > > > > -static const struct acpi_device_id sp_acpi_match[]; > > -static const struct of_device_id sp_of_match[]; > > +static const struct sp_dev_vdata dev_vdata[] = { > > + { > > + .bar = 0, > > +#ifdef CONFIG_CRYPTO_DEV_SP_CCP > > + .ccp_vdata = &ccpv3_platform, > > +#endif > > + }, > > +}; > > + > > +#ifdef CONFIG_ACPI > > +static const struct acpi_device_id sp_acpi_match[] = { > > + { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > > +#endif > > + > > +#ifdef CONFIG_OF > > +static const struct of_device_id sp_of_match[] = { > > + { .compatible = "amd,ccp-seattle-v1a", > > + .data = (const void *)&dev_vdata[0] }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, sp_of_match); > > +#endif > > > > static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev) > > { > > @@ -201,32 +224,6 @@ static int sp_platform_resume(struct platform_device *pdev) > > } > > #endif > > > > -static const struct sp_dev_vdata dev_vdata[] = { > > - { > > - .bar = 0, > > -#ifdef CONFIG_CRYPTO_DEV_SP_CCP > > - .ccp_vdata = &ccpv3_platform, > > -#endif > > - }, > > -}; > > - > > -#ifdef CONFIG_ACPI > > -static const struct acpi_device_id sp_acpi_match[] = { > > - { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > > - { }, > > -}; > > -MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > > -#endif > > - > > -#ifdef CONFIG_OF > > -static const struct of_device_id sp_of_match[] = { > > - { .compatible = "amd,ccp-seattle-v1a", > > - .data = (const void *)&dev_vdata[0] }, > > - { }, > > -}; > > -MODULE_DEVICE_TABLE(of, sp_of_match); > > -#endif > > - > > static struct platform_driver sp_platform_driver = { > > .driver = { > > .name = "ccp", > > >
On Mon, Sep 24, 2018 at 12:18 PM Gary R Hook <ghook@amd.com> wrote: > > On 09/24/2018 12:26 PM, Nathan Chancellor wrote: > > Clang emits a warning about this construct: > > > > drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array > > definition assumed to have one element > > static const struct acpi_device_id sp_acpi_match[]; > > ^ > > 1 warning generated. > > > > Just remove the forward declarations and move the initializations up > > so that they can be used in sp_get_of_version and sp_get_acpi_version. > > I'm not going to out and out object to this just yet. Tentative array definitions need to have the correct length specified; it should either be forward declared as the correct length, or as this patch does, skip the forward declare and move up the definition. Thanks for this patch Nathan. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > I am not a clang expert. Can you please provide a make command that > would explain how you precipitated this complaint? > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- > > 1 file changed, 25 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c > > index 71734f254fd1..b75dc7db2d4a 100644 > > --- a/drivers/crypto/ccp/sp-platform.c > > +++ b/drivers/crypto/ccp/sp-platform.c > > @@ -33,8 +33,31 @@ struct sp_platform { > > unsigned int irq_count; > > }; > > > > -static const struct acpi_device_id sp_acpi_match[]; > > -static const struct of_device_id sp_of_match[]; > > +static const struct sp_dev_vdata dev_vdata[] = { > > + { > > + .bar = 0, > > +#ifdef CONFIG_CRYPTO_DEV_SP_CCP > > + .ccp_vdata = &ccpv3_platform, > > +#endif > > + }, > > +}; > > + > > +#ifdef CONFIG_ACPI > > +static const struct acpi_device_id sp_acpi_match[] = { > > + { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > > +#endif > > + > > +#ifdef CONFIG_OF > > +static const struct of_device_id sp_of_match[] = { > > + { .compatible = "amd,ccp-seattle-v1a", > > + .data = (const void *)&dev_vdata[0] }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, sp_of_match); > > +#endif > > > > static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev) > > { > > @@ -201,32 +224,6 @@ static int sp_platform_resume(struct platform_device *pdev) > > } > > #endif > > > > -static const struct sp_dev_vdata dev_vdata[] = { > > - { > > - .bar = 0, > > -#ifdef CONFIG_CRYPTO_DEV_SP_CCP > > - .ccp_vdata = &ccpv3_platform, > > -#endif > > - }, > > -}; > > - > > -#ifdef CONFIG_ACPI > > -static const struct acpi_device_id sp_acpi_match[] = { > > - { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > > - { }, > > -}; > > -MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > > -#endif > > - > > -#ifdef CONFIG_OF > > -static const struct of_device_id sp_of_match[] = { > > - { .compatible = "amd,ccp-seattle-v1a", > > - .data = (const void *)&dev_vdata[0] }, > > - { }, > > -}; > > -MODULE_DEVICE_TABLE(of, sp_of_match); > > -#endif > > - > > static struct platform_driver sp_platform_driver = { > > .driver = { > > .name = "ccp", > > >
On 09/24/2018 02:40 PM, Nathan Chancellor wrote: > On Mon, Sep 24, 2018 at 07:18:23PM +0000, Gary R Hook wrote: >> On 09/24/2018 12:26 PM, Nathan Chancellor wrote: >>> Clang emits a warning about this construct: >>> >>> drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array >>> definition assumed to have one element >>> static const struct acpi_device_id sp_acpi_match[]; >>> ^ >>> 1 warning generated. >>> >>> Just remove the forward declarations and move the initializations up >>> so that they can be used in sp_get_of_version and sp_get_acpi_version. >> >> I'm not going to out and out object to this just yet. >> >> I am not a clang expert. Can you please provide a make command that >> would explain how you precipitated this complaint? >> > > Hi Gary, > > I can produce the warning with Clang 6.0 using the following set of > commands: > > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- allyesconfig > ./scripts/config -d CONFIG_CPU_BIG_ENDIAN > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- olddefconfig > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- drivers/crypto/ccp/sp-platform.o No, I"m not getting a warning on my system. I get this: ghook@taos:~/src/cryptodev-2.6/src$ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- CFLAGS=-v arch/arm64/Makefile:27: ld does not support --fix-cortex-a53-843419; kernel may be susceptible to erratum arch/arm64/Makefile:40: LSE atomics not supported by binutils arch/arm64/Makefile:48: Detected assembler with broken .inst; disassembly will be unreliable CALL scripts/checksyscalls.sh VDSOA arch/arm64/kernel/vdso/gettimeofday.o arch/arm64/kernel/vdso/gettimeofday.S: Assembler messages: arch/arm64/kernel/vdso/gettimeofday.S:28: Error: no such instruction: `vdso_data .req x6' arch/arm64/kernel/vdso/gettimeofday.S:29: Error: no such instruction: `seqcnt .req w7' arch/arm64/kernel/vdso/gettimeofday.S:30: Error: no such instruction: `w_tmp .req w8' ... The only reason I bring this up is that it would be helpful to be able to recreate results. I figure I'm not set up for this. That said... please see my response to Nick.
On 09/24/2018 03:24 PM, Nick Desaulniers wrote: > On Mon, Sep 24, 2018 at 12:18 PM Gary R Hook <ghook@amd.com> wrote: >> >> On 09/24/2018 12:26 PM, Nathan Chancellor wrote: >>> Clang emits a warning about this construct: >>> >>> drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array >>> definition assumed to have one element >>> static const struct acpi_device_id sp_acpi_match[]; >>> ^ >>> 1 warning generated. >>> >>> Just remove the forward declarations and move the initializations up >>> so that they can be used in sp_get_of_version and sp_get_acpi_version. >> >> I'm not going to out and out object to this just yet. > > Tentative array definitions need to have the correct length specified; > it should either be forward declared as the correct length, or as this > patch does, skip the forward declare and move up the definition. > Thanks for this patch Nathan. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Checked my C99, and using both static and [] to create a declaration (because it's really a definition) apparently isn't valid. I should have known that. Since I'm old school... Acked-by: Gary R Hook <gary.hook@amd.com> > >> >> I am not a clang expert. Can you please provide a make command that >> would explain how you precipitated this complaint? >> >> >>> Reported-by: Nick Desaulniers <ndesaulniers@google.com> >>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >>> --- >>> drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- >>> 1 file changed, 25 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c >>> index 71734f254fd1..b75dc7db2d4a 100644 >>> --- a/drivers/crypto/ccp/sp-platform.c >>> +++ b/drivers/crypto/ccp/sp-platform.c >>> @@ -33,8 +33,31 @@ struct sp_platform { >>> unsigned int irq_count; >>> }; >>> >>> -static const struct acpi_device_id sp_acpi_match[]; >>> -static const struct of_device_id sp_of_match[]; >>> +static const struct sp_dev_vdata dev_vdata[] = { >>> + { >>> + .bar = 0, >>> +#ifdef CONFIG_CRYPTO_DEV_SP_CCP >>> + .ccp_vdata = &ccpv3_platform, >>> +#endif >>> + }, >>> +}; >>> + >>> +#ifdef CONFIG_ACPI >>> +static const struct acpi_device_id sp_acpi_match[] = { >>> + { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, >>> + { }, >>> +}; >>> +MODULE_DEVICE_TABLE(acpi, sp_acpi_match); >>> +#endif >>> + >>> +#ifdef CONFIG_OF >>> +static const struct of_device_id sp_of_match[] = { >>> + { .compatible = "amd,ccp-seattle-v1a", >>> + .data = (const void *)&dev_vdata[0] }, >>> + { }, >>> +}; >>> +MODULE_DEVICE_TABLE(of, sp_of_match); >>> +#endif >>> >>> static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev) >>> { >>> @@ -201,32 +224,6 @@ static int sp_platform_resume(struct platform_device *pdev) >>> } >>> #endif >>> >>> -static const struct sp_dev_vdata dev_vdata[] = { >>> - { >>> - .bar = 0, >>> -#ifdef CONFIG_CRYPTO_DEV_SP_CCP >>> - .ccp_vdata = &ccpv3_platform, >>> -#endif >>> - }, >>> -}; >>> - >>> -#ifdef CONFIG_ACPI >>> -static const struct acpi_device_id sp_acpi_match[] = { >>> - { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, >>> - { }, >>> -}; >>> -MODULE_DEVICE_TABLE(acpi, sp_acpi_match); >>> -#endif >>> - >>> -#ifdef CONFIG_OF >>> -static const struct of_device_id sp_of_match[] = { >>> - { .compatible = "amd,ccp-seattle-v1a", >>> - .data = (const void *)&dev_vdata[0] }, >>> - { }, >>> -}; >>> -MODULE_DEVICE_TABLE(of, sp_of_match); >>> -#endif >>> - >>> static struct platform_driver sp_platform_driver = { >>> .driver = { >>> .name = "ccp", >>> >> > >
On Mon, Sep 24, 2018 at 2:22 PM Gary R Hook <ghook@amd.com> wrote: > > On 09/24/2018 02:40 PM, Nathan Chancellor wrote: > > On Mon, Sep 24, 2018 at 07:18:23PM +0000, Gary R Hook wrote: > >> On 09/24/2018 12:26 PM, Nathan Chancellor wrote: > >>> Clang emits a warning about this construct: > >>> > >>> drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array > >>> definition assumed to have one element > >>> static const struct acpi_device_id sp_acpi_match[]; > >>> ^ > >>> 1 warning generated. > >>> > >>> Just remove the forward declarations and move the initializations up > >>> so that they can be used in sp_get_of_version and sp_get_acpi_version. > >> > >> I'm not going to out and out object to this just yet. > >> > >> I am not a clang expert. Can you please provide a make command that > >> would explain how you precipitated this complaint? > >> > > > > Hi Gary, > > > > I can produce the warning with Clang 6.0 using the following set of > > commands: > > > > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- allyesconfig > > ./scripts/config -d CONFIG_CPU_BIG_ENDIAN > > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- olddefconfig > > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- drivers/crypto/ccp/sp-platform.o > > No, I"m not getting a warning on my system. I get this: > > ghook@taos:~/src/cryptodev-2.6/src$ make ARCH=arm64 CC=clang > CROSS_COMPILE=aarch64-linux-gnu- CFLAGS=-v > arch/arm64/Makefile:27: ld does not support --fix-cortex-a53-843419; > kernel may be susceptible to erratum > arch/arm64/Makefile:40: LSE atomics not supported by binutils ./scripts/config -d CONFIG_ARM64_LSE_ATOMICS > arch/arm64/Makefile:48: Detected assembler with broken .inst; > disassembly will be unreliable > CALL scripts/checksyscalls.sh > VDSOA arch/arm64/kernel/vdso/gettimeofday.o > arch/arm64/kernel/vdso/gettimeofday.S: Assembler messages: > arch/arm64/kernel/vdso/gettimeofday.S:28: Error: no such instruction: > `vdso_data .req x6' > arch/arm64/kernel/vdso/gettimeofday.S:29: Error: no such instruction: > `seqcnt .req w7' > arch/arm64/kernel/vdso/gettimeofday.S:30: Error: no such instruction: > `w_tmp .req w8' > ... > > The only reason I bring this up is that it would be helpful to be able > to recreate results. I figure I'm not set up for this. > > That said... please see my response to Nick.
On Mon, Sep 24, 2018 at 2:27 PM Gary R Hook <ghook@amd.com> wrote: > > On 09/24/2018 03:24 PM, Nick Desaulniers wrote: > > On Mon, Sep 24, 2018 at 12:18 PM Gary R Hook <ghook@amd.com> wrote: > >> > >> On 09/24/2018 12:26 PM, Nathan Chancellor wrote: > >>> Clang emits a warning about this construct: > >>> > >>> drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array > >>> definition assumed to have one element > >>> static const struct acpi_device_id sp_acpi_match[]; > >>> ^ > >>> 1 warning generated. > >>> > >>> Just remove the forward declarations and move the initializations up > >>> so that they can be used in sp_get_of_version and sp_get_acpi_version. > >> > >> I'm not going to out and out object to this just yet. > > > > Tentative array definitions need to have the correct length specified; > > it should either be forward declared as the correct length, or as this > > patch does, skip the forward declare and move up the definition. > > Thanks for this patch Nathan. > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > Checked my C99, and using both static and [] to create a declaration Note that the kernel uses gnu89, which is ISO C90 + gnu extensions (IIUC). > (because it's really a definition) apparently isn't valid. I should have > known that. Since I'm old school... > > Acked-by: Gary R Hook <gary.hook@amd.com> > > > > > >> > >> I am not a clang expert. Can you please provide a make command that > >> would explain how you precipitated this complaint? > >> > >> > >>> Reported-by: Nick Desaulniers <ndesaulniers@google.com> > >>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > >>> --- > >>> drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- > >>> 1 file changed, 25 insertions(+), 28 deletions(-) > >>> > >>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c > >>> index 71734f254fd1..b75dc7db2d4a 100644 > >>> --- a/drivers/crypto/ccp/sp-platform.c > >>> +++ b/drivers/crypto/ccp/sp-platform.c > >>> @@ -33,8 +33,31 @@ struct sp_platform { > >>> unsigned int irq_count; > >>> }; > >>> > >>> -static const struct acpi_device_id sp_acpi_match[]; > >>> -static const struct of_device_id sp_of_match[]; > >>> +static const struct sp_dev_vdata dev_vdata[] = { > >>> + { > >>> + .bar = 0, > >>> +#ifdef CONFIG_CRYPTO_DEV_SP_CCP > >>> + .ccp_vdata = &ccpv3_platform, > >>> +#endif > >>> + }, > >>> +}; > >>> + > >>> +#ifdef CONFIG_ACPI > >>> +static const struct acpi_device_id sp_acpi_match[] = { > >>> + { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > >>> + { }, > >>> +}; > >>> +MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > >>> +#endif > >>> + > >>> +#ifdef CONFIG_OF > >>> +static const struct of_device_id sp_of_match[] = { > >>> + { .compatible = "amd,ccp-seattle-v1a", > >>> + .data = (const void *)&dev_vdata[0] }, > >>> + { }, > >>> +}; > >>> +MODULE_DEVICE_TABLE(of, sp_of_match); > >>> +#endif > >>> > >>> static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev) > >>> { > >>> @@ -201,32 +224,6 @@ static int sp_platform_resume(struct platform_device *pdev) > >>> } > >>> #endif > >>> > >>> -static const struct sp_dev_vdata dev_vdata[] = { > >>> - { > >>> - .bar = 0, > >>> -#ifdef CONFIG_CRYPTO_DEV_SP_CCP > >>> - .ccp_vdata = &ccpv3_platform, > >>> -#endif > >>> - }, > >>> -}; > >>> - > >>> -#ifdef CONFIG_ACPI > >>> -static const struct acpi_device_id sp_acpi_match[] = { > >>> - { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, > >>> - { }, > >>> -}; > >>> -MODULE_DEVICE_TABLE(acpi, sp_acpi_match); > >>> -#endif > >>> - > >>> -#ifdef CONFIG_OF > >>> -static const struct of_device_id sp_of_match[] = { > >>> - { .compatible = "amd,ccp-seattle-v1a", > >>> - .data = (const void *)&dev_vdata[0] }, > >>> - { }, > >>> -}; > >>> -MODULE_DEVICE_TABLE(of, sp_of_match); > >>> -#endif > >>> - > >>> static struct platform_driver sp_platform_driver = { > >>> .driver = { > >>> .name = "ccp", > >>> > >> > > > > >
On Mon, Sep 24, 2018 at 02:42:56PM -0700, Nick Desaulniers wrote: > On Mon, Sep 24, 2018 at 2:22 PM Gary R Hook <ghook@amd.com> wrote: > > > > On 09/24/2018 02:40 PM, Nathan Chancellor wrote: > > > On Mon, Sep 24, 2018 at 07:18:23PM +0000, Gary R Hook wrote: > > >> On 09/24/2018 12:26 PM, Nathan Chancellor wrote: > > >>> Clang emits a warning about this construct: > > >>> > > >>> drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array > > >>> definition assumed to have one element > > >>> static const struct acpi_device_id sp_acpi_match[]; > > >>> ^ > > >>> 1 warning generated. > > >>> > > >>> Just remove the forward declarations and move the initializations up > > >>> so that they can be used in sp_get_of_version and sp_get_acpi_version. > > >> > > >> I'm not going to out and out object to this just yet. > > >> > > >> I am not a clang expert. Can you please provide a make command that > > >> would explain how you precipitated this complaint? > > >> > > > > > > Hi Gary, > > > > > > I can produce the warning with Clang 6.0 using the following set of > > > commands: > > > > > > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- allyesconfig > > > ./scripts/config -d CONFIG_CPU_BIG_ENDIAN > > > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- olddefconfig > > > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- drivers/crypto/ccp/sp-platform.o > > > > No, I"m not getting a warning on my system. I get this: > > > > ghook@taos:~/src/cryptodev-2.6/src$ make ARCH=arm64 CC=clang > > CROSS_COMPILE=aarch64-linux-gnu- CFLAGS=-v > > arch/arm64/Makefile:27: ld does not support --fix-cortex-a53-843419; > > kernel may be susceptible to erratum > > arch/arm64/Makefile:40: LSE atomics not supported by binutils > > ./scripts/config -d CONFIG_ARM64_LSE_ATOMICS > D'oh, I keep forgetting to update my gist with my commands... Thanks! > > arch/arm64/Makefile:48: Detected assembler with broken .inst; > > disassembly will be unreliable > > CALL scripts/checksyscalls.sh > > VDSOA arch/arm64/kernel/vdso/gettimeofday.o > > arch/arm64/kernel/vdso/gettimeofday.S: Assembler messages: > > arch/arm64/kernel/vdso/gettimeofday.S:28: Error: no such instruction: > > `vdso_data .req x6' > > arch/arm64/kernel/vdso/gettimeofday.S:29: Error: no such instruction: > > `seqcnt .req w7' > > arch/arm64/kernel/vdso/gettimeofday.S:30: Error: no such instruction: > > `w_tmp .req w8' > > ... > > > > The only reason I bring this up is that it would be helpful to be able > > to recreate results. I figure I'm not set up for this. > > > > That said... please see my response to Nick. > > > > -- > Thanks, > ~Nick Desaulniers
On 09/24/2018 04:44 PM, Nick Desaulniers wrote: > On Mon, Sep 24, 2018 at 2:27 PM Gary R Hook <ghook@amd.com> wrote: >> >> On 09/24/2018 03:24 PM, Nick Desaulniers wrote: >>> On Mon, Sep 24, 2018 at 12:18 PM Gary R Hook <ghook@amd.com> wrote: >>>> >>>> On 09/24/2018 12:26 PM, Nathan Chancellor wrote: >>>>> Clang emits a warning about this construct: >>>>> >>>>> drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array >>>>> definition assumed to have one element >>>>> static const struct acpi_device_id sp_acpi_match[]; >>>>> ^ >>>>> 1 warning generated. >>>>> >>>>> Just remove the forward declarations and move the initializations up >>>>> so that they can be used in sp_get_of_version and sp_get_acpi_version. >>>> >>>> I'm not going to out and out object to this just yet. >>> >>> Tentative array definitions need to have the correct length specified; >>> it should either be forward declared as the correct length, or as this >>> patch does, skip the forward declare and move up the definition. >>> Thanks for this patch Nathan. >>> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> >> >> >> Checked my C99, and using both static and [] to create a declaration > > Note that the kernel uses gnu89, which is ISO C90 + gnu extensions (IIUC). I understand that (and appreciate that ubiquitousness of the dialect) but in this case it sounds like we are interested in the widest possible compatibility. Which makes the original patch problematic, and the fix suggested herein suitable. > >> (because it's really a definition) apparently isn't valid. I should have >> known that. Since I'm old school... >> >> Acked-by: Gary R Hook <gary.hook@amd.com> >> >> >>> >>>> >>>> I am not a clang expert. Can you please provide a make command that >>>> would explain how you precipitated this complaint? >>>> >>>> >>>>> Reported-by: Nick Desaulniers <ndesaulniers@google.com> >>>>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >>>>> --- >>>>> drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- >>>>> 1 file changed, 25 insertions(+), 28 deletions(-) >>>>> >>>>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c >>>>> index 71734f254fd1..b75dc7db2d4a 100644 >>>>> --- a/drivers/crypto/ccp/sp-platform.c >>>>> +++ b/drivers/crypto/ccp/sp-platform.c >>>>> @@ -33,8 +33,31 @@ struct sp_platform { >>>>> unsigned int irq_count; >>>>> }; >>>>> >>>>> -static const struct acpi_device_id sp_acpi_match[]; >>>>> -static const struct of_device_id sp_of_match[]; >>>>> +static const struct sp_dev_vdata dev_vdata[] = { >>>>> + { >>>>> + .bar = 0, >>>>> +#ifdef CONFIG_CRYPTO_DEV_SP_CCP >>>>> + .ccp_vdata = &ccpv3_platform, >>>>> +#endif >>>>> + }, >>>>> +}; >>>>> + >>>>> +#ifdef CONFIG_ACPI >>>>> +static const struct acpi_device_id sp_acpi_match[] = { >>>>> + { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, >>>>> + { }, >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(acpi, sp_acpi_match); >>>>> +#endif >>>>> + >>>>> +#ifdef CONFIG_OF >>>>> +static const struct of_device_id sp_of_match[] = { >>>>> + { .compatible = "amd,ccp-seattle-v1a", >>>>> + .data = (const void *)&dev_vdata[0] }, >>>>> + { }, >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(of, sp_of_match); >>>>> +#endif >>>>> >>>>> static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev) >>>>> { >>>>> @@ -201,32 +224,6 @@ static int sp_platform_resume(struct platform_device *pdev) >>>>> } >>>>> #endif >>>>> >>>>> -static const struct sp_dev_vdata dev_vdata[] = { >>>>> - { >>>>> - .bar = 0, >>>>> -#ifdef CONFIG_CRYPTO_DEV_SP_CCP >>>>> - .ccp_vdata = &ccpv3_platform, >>>>> -#endif >>>>> - }, >>>>> -}; >>>>> - >>>>> -#ifdef CONFIG_ACPI >>>>> -static const struct acpi_device_id sp_acpi_match[] = { >>>>> - { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, >>>>> - { }, >>>>> -}; >>>>> -MODULE_DEVICE_TABLE(acpi, sp_acpi_match); >>>>> -#endif >>>>> - >>>>> -#ifdef CONFIG_OF >>>>> -static const struct of_device_id sp_of_match[] = { >>>>> - { .compatible = "amd,ccp-seattle-v1a", >>>>> - .data = (const void *)&dev_vdata[0] }, >>>>> - { }, >>>>> -}; >>>>> -MODULE_DEVICE_TABLE(of, sp_of_match); >>>>> -#endif >>>>> - >>>>> static struct platform_driver sp_platform_driver = { >>>>> .driver = { >>>>> .name = "ccp", >>>>> >>>> >>> >>> >> > >
On Mon, Sep 24, 2018 at 10:26:15AM -0700, Nathan Chancellor wrote: > Clang emits a warning about this construct: > > drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array > definition assumed to have one element > static const struct acpi_device_id sp_acpi_match[]; > ^ > 1 warning generated. > > Just remove the forward declarations and move the initializations up > so that they can be used in sp_get_of_version and sp_get_acpi_version. > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- > 1 file changed, 25 insertions(+), 28 deletions(-) Patch applied. Thanks.
diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c index 71734f254fd1..b75dc7db2d4a 100644 --- a/drivers/crypto/ccp/sp-platform.c +++ b/drivers/crypto/ccp/sp-platform.c @@ -33,8 +33,31 @@ struct sp_platform { unsigned int irq_count; }; -static const struct acpi_device_id sp_acpi_match[]; -static const struct of_device_id sp_of_match[]; +static const struct sp_dev_vdata dev_vdata[] = { + { + .bar = 0, +#ifdef CONFIG_CRYPTO_DEV_SP_CCP + .ccp_vdata = &ccpv3_platform, +#endif + }, +}; + +#ifdef CONFIG_ACPI +static const struct acpi_device_id sp_acpi_match[] = { + { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, sp_acpi_match); +#endif + +#ifdef CONFIG_OF +static const struct of_device_id sp_of_match[] = { + { .compatible = "amd,ccp-seattle-v1a", + .data = (const void *)&dev_vdata[0] }, + { }, +}; +MODULE_DEVICE_TABLE(of, sp_of_match); +#endif static struct sp_dev_vdata *sp_get_of_version(struct platform_device *pdev) { @@ -201,32 +224,6 @@ static int sp_platform_resume(struct platform_device *pdev) } #endif -static const struct sp_dev_vdata dev_vdata[] = { - { - .bar = 0, -#ifdef CONFIG_CRYPTO_DEV_SP_CCP - .ccp_vdata = &ccpv3_platform, -#endif - }, -}; - -#ifdef CONFIG_ACPI -static const struct acpi_device_id sp_acpi_match[] = { - { "AMDI0C00", (kernel_ulong_t)&dev_vdata[0] }, - { }, -}; -MODULE_DEVICE_TABLE(acpi, sp_acpi_match); -#endif - -#ifdef CONFIG_OF -static const struct of_device_id sp_of_match[] = { - { .compatible = "amd,ccp-seattle-v1a", - .data = (const void *)&dev_vdata[0] }, - { }, -}; -MODULE_DEVICE_TABLE(of, sp_of_match); -#endif - static struct platform_driver sp_platform_driver = { .driver = { .name = "ccp",
Clang emits a warning about this construct: drivers/crypto/ccp/sp-platform.c:36:36: warning: tentative array definition assumed to have one element static const struct acpi_device_id sp_acpi_match[]; ^ 1 warning generated. Just remove the forward declarations and move the initializations up so that they can be used in sp_get_of_version and sp_get_acpi_version. Reported-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/crypto/ccp/sp-platform.c | 53 +++++++++++++++----------------- 1 file changed, 25 insertions(+), 28 deletions(-)