diff mbox series

crypto: ccp: Remove forward declaration

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

Commit Message

Nathan Chancellor Sept. 24, 2018, 5:26 p.m. UTC
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(-)

Comments

Gary R Hook Sept. 24, 2018, 7:18 p.m. UTC | #1
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",
>
Nathan Chancellor Sept. 24, 2018, 7:40 p.m. UTC | #2
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",
> > 
>
Nick Desaulniers Sept. 24, 2018, 8:24 p.m. UTC | #3
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",
> >
>
Gary R Hook Sept. 24, 2018, 9:22 p.m. UTC | #4
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.
Gary R Hook Sept. 24, 2018, 9:27 p.m. UTC | #5
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",
>>>
>>
> 
>
Nick Desaulniers Sept. 24, 2018, 9:42 p.m. UTC | #6
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.
Nick Desaulniers Sept. 24, 2018, 9:44 p.m. UTC | #7
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",
> >>>
> >>
> >
> >
>
Nathan Chancellor Sept. 24, 2018, 9:44 p.m. UTC | #8
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
Gary R Hook Sept. 24, 2018, 11:41 p.m. UTC | #9
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",
>>>>>
>>>>
>>>
>>>
>>
> 
>
Herbert Xu Oct. 5, 2018, 2:27 a.m. UTC | #10
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 mbox series

Patch

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",