[PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Wei Yongjun]

Fixes the following sparse warning:

drivers/char/hw_random/optee-rng.c:265:35: warning:
 symbol 'optee_rng_id_table' was not declared. Should it be static?

Fixes: 5fe8b1cc6a03 ("hwrng: add OP-TEE based rng driver")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/char/hw_random/optee-rng.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c
index 2b9fc8ac5500..3a4dd6fc9ff4 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c
@@ -262,7 +262,7 @@ static int optee_rng_remove(struct device *dev)
 	return 0;
 }
 
-const struct tee_client_device_id optee_rng_id_table[] = {
+static const struct tee_client_device_id optee_rng_id_table[] = {
 	{UUID_INIT(0xab7a617c, 0xb8e7, 0x4d8f,
 		   0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64)},
 	{}

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Sumit Garg]

On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
>
> Fixes the following sparse warning:
>
> drivers/char/hw_random/optee-rng.c:265:35: warning:
>  symbol 'optee_rng_id_table' was not declared. Should it be static?
>

I haven't observed this warning during my normal Linux build using
gcc. Is there any specific configuration you are using?

-Sumit

> Fixes: 5fe8b1cc6a03 ("hwrng: add OP-TEE based rng driver")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>  drivers/char/hw_random/optee-rng.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c
> index 2b9fc8ac5500..3a4dd6fc9ff4 100644
> --- a/drivers/char/hw_random/optee-rng.c
> +++ b/drivers/char/hw_random/optee-rng.c
> @@ -262,7 +262,7 @@ static int optee_rng_remove(struct device *dev)
>         return 0;
>  }
>
> -const struct tee_client_device_id optee_rng_id_table[] = {
> +static const struct tee_client_device_id optee_rng_id_table[] = {
>         {UUID_INIT(0xab7a617c, 0xb8e7, 0x4d8f,
>                    0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64)},
>         {}
>
>
>

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Ard Biesheuvel]

On Wed, 20 Feb 2019 at 11:34, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
> >
> > Fixes the following sparse warning:
> >
> > drivers/char/hw_random/optee-rng.c:265:35: warning:
> >  symbol 'optee_rng_id_table' was not declared. Should it be static?
> >
>
> I haven't observed this warning during my normal Linux build using
> gcc. Is there any specific configuration you are using?
>

This is a sparse warning, not GCC. You need to install it separately
and build with C=1 (iirc)


> > Fixes: 5fe8b1cc6a03 ("hwrng: add OP-TEE based rng driver")
> > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > ---
> >  drivers/char/hw_random/optee-rng.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c
> > index 2b9fc8ac5500..3a4dd6fc9ff4 100644
> > --- a/drivers/char/hw_random/optee-rng.c
> > +++ b/drivers/char/hw_random/optee-rng.c
> > @@ -262,7 +262,7 @@ static int optee_rng_remove(struct device *dev)
> >         return 0;
> >  }
> >
> > -const struct tee_client_device_id optee_rng_id_table[] = {
> > +static const struct tee_client_device_id optee_rng_id_table[] = {
> >         {UUID_INIT(0xab7a617c, 0xb8e7, 0x4d8f,
> >                    0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64)},
> >         {}
> >
> >
> >

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Colin Ian King]

On 20/02/2019 10:37, Ard Biesheuvel wrote:
> On Wed, 20 Feb 2019 at 11:34, Sumit Garg <sumit.garg@linaro.org> wrote:
>>
>> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
>>>
>>> Fixes the following sparse warning:
>>>
>>> drivers/char/hw_random/optee-rng.c:265:35: warning:
>>>  symbol 'optee_rng_id_table' was not declared. Should it be static?
>>>
>>
>> I haven't observed this warning during my normal Linux build using
>> gcc. Is there any specific configuration you are using?
>>
> 
> This is a sparse warning, not GCC. You need to install it separately
> and build with C=1 (iirc)
> 
It's useful to may these symbols static just to reduce the scope and
there is on-going work to fix these symbols up across the entire kernel

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Sumit Garg]

On Wed, 20 Feb 2019 at 16:19, Colin Ian King <colin.king@canonical.com> wrote:
>
> On 20/02/2019 10:37, Ard Biesheuvel wrote:
> > On Wed, 20 Feb 2019 at 11:34, Sumit Garg <sumit.garg@linaro.org> wrote:
> >>
> >> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
> >>>
> >>> Fixes the following sparse warning:
> >>>
> >>> drivers/char/hw_random/optee-rng.c:265:35: warning:
> >>>  symbol 'optee_rng_id_table' was not declared. Should it be static?
> >>>
> >>
> >> I haven't observed this warning during my normal Linux build using
> >> gcc. Is there any specific configuration you are using?
> >>
> >
> > This is a sparse warning, not GCC. You need to install it separately
> > and build with C=1 (iirc)
> >

TBH, I wasn't aware about this sparse tool. I did install sparse and
build with C=1 option. But I could only get following such
errors/warnings for drivers/char/hw_random/optee-rng.c:

./arch/arm64/include/asm/lse.h:18:37: warning: Unknown escape 'l'
./arch/arm64/include/asm/alternative.h:213:28: warning: Unknown escape 'o'
./include/linux/compiler.h:194:8: error: attribute '__gnu_inline__':
unknown attribute

But not the one mentioned in this patch.

> It's useful to may these symbols static just to reduce the scope and
> there is on-going work to fix these symbols up across the entire kernel
>

Agree with this patch to make optee_rng_id_table symbol static.
Actually I was curious to know the approach used to get these type of
warnings so that they could be fixed beforehand.

-Sumit

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Arnd Bergmann]

On Wed, Feb 20, 2019 at 12:17 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 20 Feb 2019 at 16:19, Colin Ian King <colin.king@canonical.com> wrote:
> >
> > On 20/02/2019 10:37, Ard Biesheuvel wrote:
> > > On Wed, 20 Feb 2019 at 11:34, Sumit Garg <sumit.garg@linaro.org> wrote:
> > >>
> > >> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
> > >>>
> > >>> Fixes the following sparse warning:
> > >>>
> > >>> drivers/char/hw_random/optee-rng.c:265:35: warning:
> > >>>  symbol 'optee_rng_id_table' was not declared. Should it be static?
> > >>>
> > >>
> > >> I haven't observed this warning during my normal Linux build using
> > >> gcc. Is there any specific configuration you are using?
> > >>
> > >
> > > This is a sparse warning, not GCC. You need to install it separately
> > > and build with C=1 (iirc)
> > >
>
> TBH, I wasn't aware about this sparse tool. I did install sparse and
> build with C=1 option. But I could only get following such
> errors/warnings for drivers/char/hw_random/optee-rng.c:
>
> ./arch/arm64/include/asm/lse.h:18:37: warning: Unknown escape 'l'
> ./arch/arm64/include/asm/alternative.h:213:28: warning: Unknown escape 'o'
> ./include/linux/compiler.h:194:8: error: attribute '__gnu_inline__':
> unknown attribute
>
> But not the one mentioned in this patch.

Not sure what went wrong, I see the same warning as the others.
Maybe you have an outdated version of sparse that runs into unrelated
issues?

> > It's useful to may these symbols static just to reduce the scope and
> > there is on-going work to fix these symbols up across the entire kernel
> >
>
> Agree with this patch to make optee_rng_id_table symbol static.
> Actually I was curious to know the approach used to get these type of
> warnings so that they could be fixed beforehand.

If you provide an 'Acked-by' or 'Reviewed-by' tag, I can apply this one
on top of the other two fixes I already took.

I also recommend building with 'make W=1', which enables additional
warnings and found this bug in your driver:

drivers/char/hw_random/optee-rng.c: In function 'get_optee_rng_data':
drivers/char/hw_random/optee-rng.c:94:11: error: comparison of
unsigned expression < 0 is always false [-Werror=type-limits]
  if ((ret < 0) || (inv_arg.ret != 0)) {
           ^
drivers/char/hw_random/optee-rng.c: In function 'get_optee_rng_info':
drivers/char/hw_random/optee-rng.c:188:11: error: comparison of
unsigned expression < 0 is always false [-Werror=type-limits]
  if ((ret < 0) || (inv_arg.ret != 0)) {
           ^

Here, you probably need to make 'ret' an 'int', and you should drop
the extraneous zero-initialization for a couple of variables like that one,
so gcc is able to do its job of warning about uninitialized variable

      Arnd

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Sumit Garg]

Hi Arnd,

On Wed, 20 Feb 2019 at 21:04, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Feb 20, 2019 at 12:17 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Wed, 20 Feb 2019 at 16:19, Colin Ian King <colin.king@canonical.com> wrote:
> > >
> > > On 20/02/2019 10:37, Ard Biesheuvel wrote:
> > > > On Wed, 20 Feb 2019 at 11:34, Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >>
> > > >> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
> > > >>>
> > > >>> Fixes the following sparse warning:
> > > >>>
> > > >>> drivers/char/hw_random/optee-rng.c:265:35: warning:
> > > >>>  symbol 'optee_rng_id_table' was not declared. Should it be static?
> > > >>>
> > > >>
> > > >> I haven't observed this warning during my normal Linux build using
> > > >> gcc. Is there any specific configuration you are using?
> > > >>
> > > >
> > > > This is a sparse warning, not GCC. You need to install it separately
> > > > and build with C=1 (iirc)
> > > >
> >
> > TBH, I wasn't aware about this sparse tool. I did install sparse and
> > build with C=1 option. But I could only get following such
> > errors/warnings for drivers/char/hw_random/optee-rng.c:
> >
> > ./arch/arm64/include/asm/lse.h:18:37: warning: Unknown escape 'l'
> > ./arch/arm64/include/asm/alternative.h:213:28: warning: Unknown escape 'o'
> > ./include/linux/compiler.h:194:8: error: attribute '__gnu_inline__':
> > unknown attribute
> >
> > But not the one mentioned in this patch.
>
> Not sure what went wrong, I see the same warning as the others.
> Maybe you have an outdated version of sparse that runs into unrelated
> issues?
>

$ apt list sparse
Listing... Done
sparse/xenial,now 0.5.0-1build1 amd64 [installed]

> > > It's useful to may these symbols static just to reduce the scope and
> > > there is on-going work to fix these symbols up across the entire kernel
> > >
> >
> > Agree with this patch to make optee_rng_id_table symbol static.
> > Actually I was curious to know the approach used to get these type of
> > warnings so that they could be fixed beforehand.
>
> If you provide an 'Acked-by' or 'Reviewed-by' tag, I can apply this one
> on top of the other two fixes I already took.
>

Sure.

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>


> I also recommend building with 'make W=1',

Thanks for your suggestion.

> which enables additional
> warnings and found this bug in your driver:
>
> drivers/char/hw_random/optee-rng.c: In function 'get_optee_rng_data':
> drivers/char/hw_random/optee-rng.c:94:11: error: comparison of
> unsigned expression < 0 is always false [-Werror=type-limits]
>   if ((ret < 0) || (inv_arg.ret != 0)) {
>            ^
> drivers/char/hw_random/optee-rng.c: In function 'get_optee_rng_info':
> drivers/char/hw_random/optee-rng.c:188:11: error: comparison of
> unsigned expression < 0 is always false [-Werror=type-limits]
>   if ((ret < 0) || (inv_arg.ret != 0)) {
>            ^
>
> Here, you probably need to make 'ret' an 'int', and you should drop
> the extraneous zero-initialization for a couple of variables like that one,
> so gcc is able to do its job of warning about uninitialized variable
>

There are already fixes sent in upstream for these. Maybe you could
pick those too.

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1935826.html
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1935849.html

One more reported by Dan Carpenter here:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1936881.html

Thanks,
Sumit

>       Arnd

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Greg Kroah-Hartman]

On Wed, Feb 20, 2019 at 09:55:50PM +0530, Sumit Garg wrote:
> Hi Arnd,
> 
> On Wed, 20 Feb 2019 at 21:04, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Wed, Feb 20, 2019 at 12:17 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On Wed, 20 Feb 2019 at 16:19, Colin Ian King <colin.king@canonical.com> wrote:
> > > >
> > > > On 20/02/2019 10:37, Ard Biesheuvel wrote:
> > > > > On Wed, 20 Feb 2019 at 11:34, Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > >>
> > > > >> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
> > > > >>>
> > > > >>> Fixes the following sparse warning:
> > > > >>>
> > > > >>> drivers/char/hw_random/optee-rng.c:265:35: warning:
> > > > >>>  symbol 'optee_rng_id_table' was not declared. Should it be static?
> > > > >>>
> > > > >>
> > > > >> I haven't observed this warning during my normal Linux build using
> > > > >> gcc. Is there any specific configuration you are using?
> > > > >>
> > > > >
> > > > > This is a sparse warning, not GCC. You need to install it separately
> > > > > and build with C=1 (iirc)
> > > > >
> > >
> > > TBH, I wasn't aware about this sparse tool. I did install sparse and
> > > build with C=1 option. But I could only get following such
> > > errors/warnings for drivers/char/hw_random/optee-rng.c:
> > >
> > > ./arch/arm64/include/asm/lse.h:18:37: warning: Unknown escape 'l'
> > > ./arch/arm64/include/asm/alternative.h:213:28: warning: Unknown escape 'o'
> > > ./include/linux/compiler.h:194:8: error: attribute '__gnu_inline__':
> > > unknown attribute
> > >
> > > But not the one mentioned in this patch.
> >
> > Not sure what went wrong, I see the same warning as the others.
> > Maybe you have an outdated version of sparse that runs into unrelated
> > issues?
> >
> 
> $ apt list sparse
> Listing... Done
> sparse/xenial,now 0.5.0-1build1 amd64 [installed]

Ick, really obsolete, please use the sparse version on kernel.org for
kernel stuff:
	git://git.kernel.org/pub/scm/devel/sparse/sparse.git

Loads of things have been fixed and resolved from 0.5.0 which is very
old.

thanks,

greg k-h

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Sumit Garg]

On Wed, 20 Feb 2019 at 22:03, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Feb 20, 2019 at 09:55:50PM +0530, Sumit Garg wrote:
> > Hi Arnd,
> >
> > On Wed, 20 Feb 2019 at 21:04, Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Wed, Feb 20, 2019 at 12:17 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > On Wed, 20 Feb 2019 at 16:19, Colin Ian King <colin.king@canonical.com> wrote:
> > > > >
> > > > > On 20/02/2019 10:37, Ard Biesheuvel wrote:
> > > > > > On Wed, 20 Feb 2019 at 11:34, Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > > >>
> > > > > >> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
> > > > > >>>
> > > > > >>> Fixes the following sparse warning:
> > > > > >>>
> > > > > >>> drivers/char/hw_random/optee-rng.c:265:35: warning:
> > > > > >>>  symbol 'optee_rng_id_table' was not declared. Should it be static?
> > > > > >>>
> > > > > >>
> > > > > >> I haven't observed this warning during my normal Linux build using
> > > > > >> gcc. Is there any specific configuration you are using?
> > > > > >>
> > > > > >
> > > > > > This is a sparse warning, not GCC. You need to install it separately
> > > > > > and build with C=1 (iirc)
> > > > > >
> > > >
> > > > TBH, I wasn't aware about this sparse tool. I did install sparse and
> > > > build with C=1 option. But I could only get following such
> > > > errors/warnings for drivers/char/hw_random/optee-rng.c:
> > > >
> > > > ./arch/arm64/include/asm/lse.h:18:37: warning: Unknown escape 'l'
> > > > ./arch/arm64/include/asm/alternative.h:213:28: warning: Unknown escape 'o'
> > > > ./include/linux/compiler.h:194:8: error: attribute '__gnu_inline__':
> > > > unknown attribute
> > > >
> > > > But not the one mentioned in this patch.
> > >
> > > Not sure what went wrong, I see the same warning as the others.
> > > Maybe you have an outdated version of sparse that runs into unrelated
> > > issues?
> > >
> >
> > $ apt list sparse
> > Listing... Done
> > sparse/xenial,now 0.5.0-1build1 amd64 [installed]
>
> Ick, really obsolete, please use the sparse version on kernel.org for
> kernel stuff:
>         git://git.kernel.org/pub/scm/devel/sparse/sparse.git
>
> Loads of things have been fixed and resolved from 0.5.0 which is very
> old.
>

Thanks for this info. Now it works pretty well.

-Sumit

> thanks,
>
> greg k-h

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Arnd Bergmann]

On Wed, Feb 20, 2019 at 5:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> On Wed, 20 Feb 2019 at 21:04, Arnd Bergmann <arnd@arndb.de> wrote:

> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
>
> There are already fixes sent in upstream for these. Maybe you could
> pick those too.
>
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1935826.html
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1935849.html
>
> One more reported by Dan Carpenter here:
>
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1936881.html
>

Applied all for now, thanks a lot!

      Arnd

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Dan Carpenter]

On Wed, Feb 20, 2019 at 04:04:17PM +0530, Sumit Garg wrote:
> On Wed, 20 Feb 2019 at 14:51, Wei Yongjun <weiyongjun1@huawei.com> wrote:
> >
> > Fixes the following sparse warning:
                        ^^^^^^

> >
> > drivers/char/hw_random/optee-rng.c:265:35: warning:
> >  symbol 'optee_rng_id_table' was not declared. Should it be static?
> >
> 
> I haven't observed this warning during my normal Linux build using
> gcc. Is there any specific configuration you are using?
> 

It's from the Sparse tool.

regards,
dan carpenter

Re: [PATCH -next] hwrng: make symbol 'optee_rng_id_table' static

[Jeffrey Walton]

On Wed, Feb 20, 2019 at 4:23 AM Wei Yongjun <weiyongjun1@huawei.com> wrote:
>
> Fixes the following sparse warning:
>
> drivers/char/hw_random/optee-rng.c:265:35: warning:
>  symbol 'optee_rng_id_table' was not declared. Should it be static?

Static limits visibility to the current translation unit. Static is
like private visibility.

Maybe you are thinking if it should be declared extern so other
translation units can find the symbol. extern is like public
visibility.

Jeff