[PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[Nathan Chancellor]

Clang emits the following warning:

drivers/staging/rtl8723bs/os_dep/sdio_intf.c:25:36: warning: variable
'acpi_ids' is not needed and will not be emitted
[-Wunneeded-internal-declaration]
static const struct acpi_device_id acpi_ids[] = {
                                   ^
1 warning generated.

Mark acpi_ids with the attribute __used, which makes it clear to Clang
that we don't want this warning while not inhibiting Clang's dead code
elimination from removing the unreferenced internal symbol when moving
the data to the globally available symbol with MODULE_DEVICE_TABLE.

$ nm -S drivers/staging/rtl8723bs/os_dep/sdio_intf.o | grep acpi
0000000000000000 0000000000000040 R __mod_acpi__acpi_ids_device_table

Link: https://github.com/ClangBuiltLinux/linux/issues/169
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index 6d02904de63f..7c03b69b8ed3 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -22,7 +22,7 @@ static const struct sdio_device_id sdio_ids[] =
 	{ SDIO_DEVICE(0x024c, 0xb723), },
 	{ /* end: all zeroes */				},
 };
-static const struct acpi_device_id acpi_ids[] = {
+static const struct acpi_device_id acpi_ids[] __used = {
 	{"OBDA8723", 0x0000},
 	{}
 };
-- 
2.19.0

Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[Greg Kroah-Hartman]

On Wed, Sep 26, 2018 at 04:20:55PM -0700, Nathan Chancellor wrote:
> Clang emits the following warning:
> 
> drivers/staging/rtl8723bs/os_dep/sdio_intf.c:25:36: warning: variable
> 'acpi_ids' is not needed and will not be emitted
> [-Wunneeded-internal-declaration]
> static const struct acpi_device_id acpi_ids[] = {
>                                    ^
> 1 warning generated.
> 
> Mark acpi_ids with the attribute __used, which makes it clear to Clang
> that we don't want this warning while not inhibiting Clang's dead code
> elimination from removing the unreferenced internal symbol when moving
> the data to the globally available symbol with MODULE_DEVICE_TABLE.
> 
> $ nm -S drivers/staging/rtl8723bs/os_dep/sdio_intf.o | grep acpi
> 0000000000000000 0000000000000040 R __mod_acpi__acpi_ids_device_table
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/169
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> index 6d02904de63f..7c03b69b8ed3 100644
> --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> @@ -22,7 +22,7 @@ static const struct sdio_device_id sdio_ids[] =
>  	{ SDIO_DEVICE(0x024c, 0xb723), },
>  	{ /* end: all zeroes */				},
>  };
> -static const struct acpi_device_id acpi_ids[] = {
> +static const struct acpi_device_id acpi_ids[] __used = {

Can't we put __used somewhere in the MODULE_DEVICE_TABLE() macro so we
don't have to go adding it to all individual entries?

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[Nick Desaulniers]

On Wed, Sep 26, 2018 at 10:16 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 26, 2018 at 04:20:55PM -0700, Nathan Chancellor wrote:
> > Clang emits the following warning:
> >
> > drivers/staging/rtl8723bs/os_dep/sdio_intf.c:25:36: warning: variable
> > 'acpi_ids' is not needed and will not be emitted
> > [-Wunneeded-internal-declaration]
> > static const struct acpi_device_id acpi_ids[] = {
> >                                    ^
> > 1 warning generated.
> >
> > Mark acpi_ids with the attribute __used, which makes it clear to Clang
> > that we don't want this warning while not inhibiting Clang's dead code
> > elimination from removing the unreferenced internal symbol when moving
> > the data to the globally available symbol with MODULE_DEVICE_TABLE.
> >
> > $ nm -S drivers/staging/rtl8723bs/os_dep/sdio_intf.o | grep acpi
> > 0000000000000000 0000000000000040 R __mod_acpi__acpi_ids_device_table
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/169
> > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > index 6d02904de63f..7c03b69b8ed3 100644
> > --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > @@ -22,7 +22,7 @@ static const struct sdio_device_id sdio_ids[] =
> >       { SDIO_DEVICE(0x024c, 0xb723), },
> >       { /* end: all zeroes */                         },
> >  };
> > -static const struct acpi_device_id acpi_ids[] = {
> > +static const struct acpi_device_id acpi_ids[] __used = {
>
> Can't we put __used somewhere in the MODULE_DEVICE_TABLE() macro so we
> don't have to go adding it to all individual entries?

Maybe; I'm still trying to think of what we could do in that macro to
make its arg appear to be used while still being no functional change.

This is not a million-little-commits problem; more like 2 drivers in
the whole tree have this problem.

Note that this change that Nathan has DOES NOT need to applied to
every driver that uses MODULE_DEVICE_TABLE.  Only when the struct has
no other references other than JUST being passed to
MODULE_DEVICE_TABLE() is this a problem.  For an allyesconfig, I only
see 2 instances of this case in the whole tree:

https://github.com/ClangBuiltLinux/linux/issues/169 <- this patch is addressing
https://github.com/ClangBuiltLinux/linux/issues/178 <- waiting on the
results of this patch

If you're ok with this change to two drivers, then:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[Nick Desaulniers]

On Thu, Sep 27, 2018 at 1:05 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Sep 26, 2018 at 10:16 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Sep 26, 2018 at 04:20:55PM -0700, Nathan Chancellor wrote:
> > > Clang emits the following warning:
> > >
> > > drivers/staging/rtl8723bs/os_dep/sdio_intf.c:25:36: warning: variable
> > > 'acpi_ids' is not needed and will not be emitted
> > > [-Wunneeded-internal-declaration]
> > > static const struct acpi_device_id acpi_ids[] = {
> > >                                    ^
> > > 1 warning generated.
> > >
> > > Mark acpi_ids with the attribute __used, which makes it clear to Clang
> > > that we don't want this warning while not inhibiting Clang's dead code
> > > elimination from removing the unreferenced internal symbol when moving
> > > the data to the globally available symbol with MODULE_DEVICE_TABLE.
> > >
> > > $ nm -S drivers/staging/rtl8723bs/os_dep/sdio_intf.o | grep acpi
> > > 0000000000000000 0000000000000040 R __mod_acpi__acpi_ids_device_table
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/169
> > > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > index 6d02904de63f..7c03b69b8ed3 100644
> > > --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> > > @@ -22,7 +22,7 @@ static const struct sdio_device_id sdio_ids[] =
> > >       { SDIO_DEVICE(0x024c, 0xb723), },
> > >       { /* end: all zeroes */                         },
> > >  };
> > > -static const struct acpi_device_id acpi_ids[] = {
> > > +static const struct acpi_device_id acpi_ids[] __used = {
> >
> > Can't we put __used somewhere in the MODULE_DEVICE_TABLE() macro so we
> > don't have to go adding it to all individual entries?
>
> Maybe; I'm still trying to think of what we could do in that macro to
> make its arg appear to be used while still being no functional change.

Sorry, I meant to say:
No, the __used needs to go on the declaration of the struct array, not
the alias being created within MODULE_DEVICE_TABLE().  I'm still
trying to think of what we can do *in* MODULE_DEVICE_TABLE() to
prevent this problem tree-wide, without having to modify any call
sites.

>
> This is not a million-little-commits problem; more like 2 drivers in
> the whole tree have this problem.
>
> Note that this change that Nathan has DOES NOT need to applied to
> every driver that uses MODULE_DEVICE_TABLE.  Only when the struct has
> no other references other than JUST being passed to
> MODULE_DEVICE_TABLE() is this a problem.  For an allyesconfig, I only
> see 2 instances of this case in the whole tree:
>
> https://github.com/ClangBuiltLinux/linux/issues/169 <- this patch is addressing
> https://github.com/ClangBuiltLinux/linux/issues/178 <- waiting on the
> results of this patch
>
> If you're ok with this change to two drivers, then:
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[Nick Desaulniers]

Digging up an old thread to point out I was wrong:

On Thu, Sep 27, 2018 at 1:08 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> > This is not a million-little-commits problem; more like 2 drivers in
> > the whole tree have this problem.
> >
> > Note that this change that Nathan has DOES NOT need to applied to
> > every driver that uses MODULE_DEVICE_TABLE.  Only when the struct has
> > no other references other than JUST being passed to
> > MODULE_DEVICE_TABLE() is this a problem.  For an allyesconfig, I only
> > see 2 instances of this case in the whole tree:
> >
> > https://github.com/ClangBuiltLinux/linux/issues/169 <- this patch is addressing
> > https://github.com/ClangBuiltLinux/linux/issues/178 <- waiting on the
> > results of this patch
> >
> > If you're ok with this change to two drivers, then:
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I just landed a fix for this in Clang
(https://reviews.llvm.org/rL350776) which will ship soon in Clang 8.
Sedat helped test this patch and reported that it removed 41
false-negative warnings
https://github.com/ClangBuiltLinux/linux/issues/232#issuecomment-440006399
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[Greg KH]

On Wed, Jan 09, 2019 at 04:19:30PM -0800, Nick Desaulniers wrote:
> Digging up an old thread to point out I was wrong:
> 
> On Thu, Sep 27, 2018 at 1:08 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> > > This is not a million-little-commits problem; more like 2 drivers in
> > > the whole tree have this problem.
> > >
> > > Note that this change that Nathan has DOES NOT need to applied to
> > > every driver that uses MODULE_DEVICE_TABLE.  Only when the struct has
> > > no other references other than JUST being passed to
> > > MODULE_DEVICE_TABLE() is this a problem.  For an allyesconfig, I only
> > > see 2 instances of this case in the whole tree:
> > >
> > > https://github.com/ClangBuiltLinux/linux/issues/169 <- this patch is addressing
> > > https://github.com/ClangBuiltLinux/linux/issues/178 <- waiting on the
> > > results of this patch
> > >
> > > If you're ok with this change to two drivers, then:
> > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> I just landed a fix for this in Clang
> (https://reviews.llvm.org/rL350776) which will ship soon in Clang 8.
> Sedat helped test this patch and reported that it removed 41
> false-negative warnings
> https://github.com/ClangBuiltLinux/linux/issues/232#issuecomment-440006399

Great, thanks for letting me know.  Can I revert this patch?

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: Mark ACPI table declaration as used

[Nick Desaulniers]

On Wed, Jan 9, 2019 at 11:01 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 09, 2019 at 04:19:30PM -0800, Nick Desaulniers wrote:
> > Digging up an old thread to point out I was wrong:
> >
> > On Thu, Sep 27, 2018 at 1:08 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > > This is not a million-little-commits problem; more like 2 drivers in
> > > > the whole tree have this problem.
> > > >
> > > > Note that this change that Nathan has DOES NOT need to applied to
> > > > every driver that uses MODULE_DEVICE_TABLE.  Only when the struct has
> > > > no other references other than JUST being passed to
> > > > MODULE_DEVICE_TABLE() is this a problem.  For an allyesconfig, I only
> > > > see 2 instances of this case in the whole tree:
> > > >
> > > > https://github.com/ClangBuiltLinux/linux/issues/169 <- this patch is addressing
> > > > https://github.com/ClangBuiltLinux/linux/issues/178 <- waiting on the
> > > > results of this patch
> > > >
> > > > If you're ok with this change to two drivers, then:
> > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > I just landed a fix for this in Clang
> > (https://reviews.llvm.org/rL350776) which will ship soon in Clang 8.
> > Sedat helped test this patch and reported that it removed 41
> > false-negative warnings
> > https://github.com/ClangBuiltLinux/linux/issues/232#issuecomment-440006399
>
> Great, thanks for letting me know.  Can I revert this patch?

Go for it.  I think my mistake was that we had 2 instances of this
listed in our bug tracker, but an allyesconfig had many many more
instances than I was aware of.
-- 
Thanks,
~Nick Desaulniers