diff mbox series

[09/13] cpufreq: acpi-cpufreq: Remove unused ID structs

Message ID 20200714145049.2496163-10-lee.jones@linaro.org (mailing list archive)
State New, archived
Headers show
Series Rid W=1 warnings in CPUFreq | expand

Commit Message

Lee Jones July 14, 2020, 2:50 p.m. UTC
Can't see them being used anywhere and the compiler doesn't complain
that they're missing, so ...

Fixes the following W=1 kernel build warning(s):

 drivers/cpufreq/acpi-cpufreq.c:1004:36: warning: ‘processor_device_ids’ defined but not used [-Wunused-const-variable=]
 997 | static const struct x86_cpu_id acpi_cpufreq_ids[] = {
 | ^~~~~~~~~~~~~~~~
 drivers/cpufreq/acpi-cpufreq.c:997:32: warning: ‘acpi_cpufreq_ids’ defined but not used [-Wunused-const-variable=]
 619 | static const struct acpi_device_id processor_device_ids[] = {
 | ^~~~~~~~~~~~~~~~~~~~

Cc: Andy Grover <andrew.grover@intel.com>
Cc: Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
Cc: Dominik Brodowski <linux@brodo.de>
Cc: Denis Sadykov <denis.m.sadykov@intel.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c | 14 --------------
 1 file changed, 14 deletions(-)

Comments

Rafael J. Wysocki July 14, 2020, 3:58 p.m. UTC | #1
On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> Can't see them being used anywhere and the compiler doesn't complain
> that they're missing, so ...

Aren't they needed for automatic module loading in certain configurations?

> Fixes the following W=1 kernel build warning(s):
>
>  drivers/cpufreq/acpi-cpufreq.c:1004:36: warning: ‘processor_device_ids’ defined but not used [-Wunused-const-variable=]
>  997 | static const struct x86_cpu_id acpi_cpufreq_ids[] = {
>  | ^~~~~~~~~~~~~~~~
>  drivers/cpufreq/acpi-cpufreq.c:997:32: warning: ‘acpi_cpufreq_ids’ defined but not used [-Wunused-const-variable=]
>  619 | static const struct acpi_device_id processor_device_ids[] = {
>  | ^~~~~~~~~~~~~~~~~~~~
>
> Cc: Andy Grover <andrew.grover@intel.com>
> Cc: Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
> Cc: Dominik Brodowski <linux@brodo.de>
> Cc: Denis Sadykov <denis.m.sadykov@intel.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index d38a693b48e03..fc68f13352695 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -995,18 +995,4 @@ MODULE_PARM_DESC(acpi_pstate_strict,
>  late_initcall(acpi_cpufreq_init);
>  module_exit(acpi_cpufreq_exit);
>
> -static const struct x86_cpu_id acpi_cpufreq_ids[] = {
> -       X86_MATCH_FEATURE(X86_FEATURE_ACPI, NULL),
> -       X86_MATCH_FEATURE(X86_FEATURE_HW_PSTATE, NULL),
> -       {}
> -};
> -MODULE_DEVICE_TABLE(x86cpu, acpi_cpufreq_ids);
> -
> -static const struct acpi_device_id processor_device_ids[] = {
> -       {ACPI_PROCESSOR_OBJECT_HID, },
> -       {ACPI_PROCESSOR_DEVICE_HID, },
> -       {},
> -};
> -MODULE_DEVICE_TABLE(acpi, processor_device_ids);
> -
>  MODULE_ALIAS("acpi");
> --
> 2.25.1
>
Lee Jones July 14, 2020, 9:03 p.m. UTC | #2
On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:

> On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > Can't see them being used anywhere and the compiler doesn't complain
> > that they're missing, so ...
> 
> Aren't they needed for automatic module loading in certain configurations?

Any idea how that works, or where the code is for that?

> > Fixes the following W=1 kernel build warning(s):
> >
> >  drivers/cpufreq/acpi-cpufreq.c:1004:36: warning: ‘processor_device_ids’ defined but not used [-Wunused-const-variable=]
> >  997 | static const struct x86_cpu_id acpi_cpufreq_ids[] = {
> >  | ^~~~~~~~~~~~~~~~
> >  drivers/cpufreq/acpi-cpufreq.c:997:32: warning: ‘acpi_cpufreq_ids’ defined but not used [-Wunused-const-variable=]
> >  619 | static const struct acpi_device_id processor_device_ids[] = {
> >  | ^~~~~~~~~~~~~~~~~~~~
> >
> > Cc: Andy Grover <andrew.grover@intel.com>
> > Cc: Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
> > Cc: Dominik Brodowski <linux@brodo.de>
> > Cc: Denis Sadykov <denis.m.sadykov@intel.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/cpufreq/acpi-cpufreq.c | 14 --------------
> >  1 file changed, 14 deletions(-)
> >
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > index d38a693b48e03..fc68f13352695 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -995,18 +995,4 @@ MODULE_PARM_DESC(acpi_pstate_strict,
> >  late_initcall(acpi_cpufreq_init);
> >  module_exit(acpi_cpufreq_exit);
> >
> > -static const struct x86_cpu_id acpi_cpufreq_ids[] = {
> > -       X86_MATCH_FEATURE(X86_FEATURE_ACPI, NULL),
> > -       X86_MATCH_FEATURE(X86_FEATURE_HW_PSTATE, NULL),
> > -       {}
> > -};
> > -MODULE_DEVICE_TABLE(x86cpu, acpi_cpufreq_ids);
> > -
> > -static const struct acpi_device_id processor_device_ids[] = {
> > -       {ACPI_PROCESSOR_OBJECT_HID, },
> > -       {ACPI_PROCESSOR_DEVICE_HID, },
> > -       {},
> > -};
> > -MODULE_DEVICE_TABLE(acpi, processor_device_ids);
> > -
> >  MODULE_ALIAS("acpi");
> >
Viresh Kumar July 15, 2020, 3:24 a.m. UTC | #3
On 14-07-20, 22:03, Lee Jones wrote:
> On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> 
> > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > Can't see them being used anywhere and the compiler doesn't complain
> > > that they're missing, so ...
> > 
> > Aren't they needed for automatic module loading in certain configurations?
> 
> Any idea how that works, or where the code is for that?

The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
product-id that the kernel keeps after boot (and so there is no static
reference of it for the compiler), later when a device is hotplugged
into the kernel it refers to the map to find the related driver for it
and loads it if it isn't already loaded.

This has some of it, search for MODULE_DEVICE_TABLE() in it.
Documentation/driver-api/usb/hotplug.rst
Viresh Kumar July 15, 2020, 3:27 a.m. UTC | #4
On 15-07-20, 08:54, Viresh Kumar wrote:
> On 14-07-20, 22:03, Lee Jones wrote:
> > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > 
> > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > that they're missing, so ...
> > > 
> > > Aren't they needed for automatic module loading in certain configurations?
> > 
> > Any idea how that works, or where the code is for that?
> 
> The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> product-id that the kernel keeps after boot (and so there is no static
> reference of it for the compiler), later when a device is hotplugged
> into the kernel it refers to the map to find the related driver for it
> and loads it if it isn't already loaded.
> 
> This has some of it, search for MODULE_DEVICE_TABLE() in it.
> Documentation/driver-api/usb/hotplug.rst

And you just need to add __maybe_unused to them to suppress the
warning.
Lee Jones July 15, 2020, 6:37 a.m. UTC | #5
On Wed, 15 Jul 2020, Viresh Kumar wrote:

> On 15-07-20, 08:54, Viresh Kumar wrote:
> > On 14-07-20, 22:03, Lee Jones wrote:
> > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > 
> > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > that they're missing, so ...
> > > > 
> > > > Aren't they needed for automatic module loading in certain configurations?
> > > 
> > > Any idea how that works, or where the code is for that?
> > 
> > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > product-id that the kernel keeps after boot (and so there is no static
> > reference of it for the compiler), later when a device is hotplugged
> > into the kernel it refers to the map to find the related driver for it
> > and loads it if it isn't already loaded.
> > 
> > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > Documentation/driver-api/usb/hotplug.rst
> 
> And you just need to add __maybe_unused to them to suppress the
> warning.

Right.  That's Plan B.

Will fix.
Rafael J. Wysocki July 15, 2020, 11:27 a.m. UTC | #6
On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 15-07-20, 08:54, Viresh Kumar wrote:
> > On 14-07-20, 22:03, Lee Jones wrote:
> > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > >
> > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > that they're missing, so ...
> > > >
> > > > Aren't they needed for automatic module loading in certain configurations?
> > >
> > > Any idea how that works, or where the code is for that?
> >
> > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > product-id that the kernel keeps after boot (and so there is no static
> > reference of it for the compiler), later when a device is hotplugged
> > into the kernel it refers to the map to find the related driver for it
> > and loads it if it isn't already loaded.
> >
> > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > Documentation/driver-api/usb/hotplug.rst
>
> And you just need to add __maybe_unused to them to suppress the
> warning.

Wouldn't that cause the compiler to optimize them away if it doesn't
see any users?
Lee Jones July 15, 2020, 11:34 a.m. UTC | #7
On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:

> On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > >
> > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > that they're missing, so ...
> > > > >
> > > > > Aren't they needed for automatic module loading in certain configurations?
> > > >
> > > > Any idea how that works, or where the code is for that?
> > >
> > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > product-id that the kernel keeps after boot (and so there is no static
> > > reference of it for the compiler), later when a device is hotplugged
> > > into the kernel it refers to the map to find the related driver for it
> > > and loads it if it isn't already loaded.
> > >
> > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > Documentation/driver-api/usb/hotplug.rst
> >
> > And you just need to add __maybe_unused to them to suppress the
> > warning.
> 
> Wouldn't that cause the compiler to optimize them away if it doesn't
> see any users?

It looks like they're only unused when !MODULE, in which case
optimising them away would be the correct thing to do, no?
Rafael J. Wysocki July 15, 2020, 11:44 a.m. UTC | #8
On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
>
> > On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > > >
> > > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >
> > > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > > that they're missing, so ...
> > > > > >
> > > > > > Aren't they needed for automatic module loading in certain configurations?
> > > > >
> > > > > Any idea how that works, or where the code is for that?
> > > >
> > > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > > product-id that the kernel keeps after boot (and so there is no static
> > > > reference of it for the compiler), later when a device is hotplugged
> > > > into the kernel it refers to the map to find the related driver for it
> > > > and loads it if it isn't already loaded.
> > > >
> > > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > > Documentation/driver-api/usb/hotplug.rst
> > >
> > > And you just need to add __maybe_unused to them to suppress the
> > > warning.
> >
> > Wouldn't that cause the compiler to optimize them away if it doesn't
> > see any users?
>
> It looks like they're only unused when !MODULE,

OK

> in which case optimising them away would be the correct thing to do, no?
Lee Jones July 15, 2020, 11:50 a.m. UTC | #9
On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:

> On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> >
> > > On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > > > >
> > > > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > > > that they're missing, so ...
> > > > > > >
> > > > > > > Aren't they needed for automatic module loading in certain configurations?
> > > > > >
> > > > > > Any idea how that works, or where the code is for that?
> > > > >
> > > > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > > > product-id that the kernel keeps after boot (and so there is no static
> > > > > reference of it for the compiler), later when a device is hotplugged
> > > > > into the kernel it refers to the map to find the related driver for it
> > > > > and loads it if it isn't already loaded.
> > > > >
> > > > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > > > Documentation/driver-api/usb/hotplug.rst
> > > >
> > > > And you just need to add __maybe_unused to them to suppress the
> > > > warning.
> > >
> > > Wouldn't that cause the compiler to optimize them away if it doesn't
> > > see any users?
> >
> > It looks like they're only unused when !MODULE,
> 
> OK
> 
> > in which case optimising them away would be the correct thing to do, no?

It would be good if someone with a little more knowledge could provide
a second opinion though.  I would think (hope) that the compiler would
be smart enough to see when its actually in use.  After all, it is the
compiler that places the information into the device table.

If that is not the case, then the MODULE_DEVICE_TABLE() magic is
broken and will need fixing.  Removing boiler-plate is good, but not
at the expense of obfuscation.
Lee Jones July 15, 2020, 12:07 p.m. UTC | #10
On Wed, 15 Jul 2020, Lee Jones wrote:

> On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> 
> > On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> > >
> > > > On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > >
> > > > > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > > > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > > > > >
> > > > > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > > > > that they're missing, so ...
> > > > > > > >
> > > > > > > > Aren't they needed for automatic module loading in certain configurations?
> > > > > > >
> > > > > > > Any idea how that works, or where the code is for that?
> > > > > >
> > > > > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > > > > product-id that the kernel keeps after boot (and so there is no static
> > > > > > reference of it for the compiler), later when a device is hotplugged
> > > > > > into the kernel it refers to the map to find the related driver for it
> > > > > > and loads it if it isn't already loaded.
> > > > > >
> > > > > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > > > > Documentation/driver-api/usb/hotplug.rst
> > > > >
> > > > > And you just need to add __maybe_unused to them to suppress the
> > > > > warning.
> > > >
> > > > Wouldn't that cause the compiler to optimize them away if it doesn't
> > > > see any users?
> > >
> > > It looks like they're only unused when !MODULE,
> > 
> > OK
> > 
> > > in which case optimising them away would be the correct thing to do, no?
> 
> It would be good if someone with a little more knowledge could provide
> a second opinion though.  I would think (hope) that the compiler would
> be smart enough to see when its actually in use.  After all, it is the
> compiler that places the information into the device table.
> 
> If that is not the case, then the MODULE_DEVICE_TABLE() magic is
> broken and will need fixing.  Removing boiler-plate is good, but not
> at the expense of obfuscation.

Okay, I'm satisfied.  This test build is without __maybe_unused:

# All configs built as modules (MODULE) - the compiler knows to use the tables

 $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86 allmodconfig
 $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86  W=1  drivers/cpufreq/
 [...]
   CC [M]  drivers/cpufreq/pcc-cpufreq.o

# All configs built-in (!MODULE) - the compiler sees that they are unused

 $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86 allyesconfig
 $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86  W=1  drivers/cpufreq/
   CC      drivers/cpufreq/pcc-cpufreq.o
  drivers/cpufreq/pcc-cpufreq.c:619:36: warning: ‘processor_device_ids’ defined but not used [-Wunused-const-variable=]
  619 | static const struct acpi_device_id processor_device_ids[] = {
      |                                    ^~~~~~~~~~~~~~~~~~~~
Rafael J. Wysocki July 15, 2020, 12:09 p.m. UTC | #11
On Wed, Jul 15, 2020 at 1:50 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
>
> > On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> > >
> > > > On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > >
> > > > > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > > > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > > > > >
> > > > > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > > > > that they're missing, so ...
> > > > > > > >
> > > > > > > > Aren't they needed for automatic module loading in certain configurations?
> > > > > > >
> > > > > > > Any idea how that works, or where the code is for that?
> > > > > >
> > > > > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > > > > product-id that the kernel keeps after boot (and so there is no static
> > > > > > reference of it for the compiler), later when a device is hotplugged
> > > > > > into the kernel it refers to the map to find the related driver for it
> > > > > > and loads it if it isn't already loaded.
> > > > > >
> > > > > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > > > > Documentation/driver-api/usb/hotplug.rst
> > > > >
> > > > > And you just need to add __maybe_unused to them to suppress the
> > > > > warning.
> > > >
> > > > Wouldn't that cause the compiler to optimize them away if it doesn't
> > > > see any users?
> > >
> > > It looks like they're only unused when !MODULE,
> >
> > OK
> >
> > > in which case optimising them away would be the correct thing to do, no?
>
> It would be good if someone with a little more knowledge could provide
> a second opinion though.  I would think (hope) that the compiler would
> be smart enough to see when its actually in use.  After all, it is the
> compiler that places the information into the device table.
>
> If that is not the case, then the MODULE_DEVICE_TABLE() magic is
> broken and will need fixing.

I'm not sure why that would be the case?

> Removing boiler-plate is good, but not at the expense of obfuscation.

I'm not following you here to be honest.

BTW, I'm wondering if removing the "static" modifier from the
definitions of the structures in question makes the warnings you want
to get rid of go away.
Rafael J. Wysocki July 15, 2020, 12:11 p.m. UTC | #12
On Wed, Jul 15, 2020 at 2:07 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 15 Jul 2020, Lee Jones wrote:
>
> > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> >
> > > On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> > > >
> > > > > On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > > >
> > > > > > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > > > > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > > > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > > > > > >
> > > > > > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > > > > > that they're missing, so ...
> > > > > > > > >
> > > > > > > > > Aren't they needed for automatic module loading in certain configurations?
> > > > > > > >
> > > > > > > > Any idea how that works, or where the code is for that?
> > > > > > >
> > > > > > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > > > > > product-id that the kernel keeps after boot (and so there is no static
> > > > > > > reference of it for the compiler), later when a device is hotplugged
> > > > > > > into the kernel it refers to the map to find the related driver for it
> > > > > > > and loads it if it isn't already loaded.
> > > > > > >
> > > > > > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > > > > > Documentation/driver-api/usb/hotplug.rst
> > > > > >
> > > > > > And you just need to add __maybe_unused to them to suppress the
> > > > > > warning.
> > > > >
> > > > > Wouldn't that cause the compiler to optimize them away if it doesn't
> > > > > see any users?
> > > >
> > > > It looks like they're only unused when !MODULE,
> > >
> > > OK
> > >
> > > > in which case optimising them away would be the correct thing to do, no?
> >
> > It would be good if someone with a little more knowledge could provide
> > a second opinion though.  I would think (hope) that the compiler would
> > be smart enough to see when its actually in use.  After all, it is the
> > compiler that places the information into the device table.
> >
> > If that is not the case, then the MODULE_DEVICE_TABLE() magic is
> > broken and will need fixing.  Removing boiler-plate is good, but not
> > at the expense of obfuscation.
>
> Okay, I'm satisfied.  This test build is without __maybe_unused:
>
> # All configs built as modules (MODULE) - the compiler knows to use the tables
>
>  $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86 allmodconfig
>  $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86  W=1  drivers/cpufreq/
>  [...]
>    CC [M]  drivers/cpufreq/pcc-cpufreq.o
>
> # All configs built-in (!MODULE) - the compiler sees that they are unused
>
>  $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86 allyesconfig
>  $ ccache make -f Makefile -j24 KBUILD_OUTPUT=../builds/build-x86  W=1  drivers/cpufreq/
>    CC      drivers/cpufreq/pcc-cpufreq.o
>   drivers/cpufreq/pcc-cpufreq.c:619:36: warning: ‘processor_device_ids’ defined but not used [-Wunused-const-variable=]
>   619 | static const struct acpi_device_id processor_device_ids[] = {
>       |                                    ^~~~~~~~~~~~~~~~~~~~
>

OK

I thought that this would be the case. :-)
Lee Jones July 15, 2020, 12:16 p.m. UTC | #13
On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:

> On Wed, Jul 15, 2020 at 1:50 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> >
> > > On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> > > >
> > > > > On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > > >
> > > > > > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > > > > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > > > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > > > > > >
> > > > > > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > > > > > that they're missing, so ...
> > > > > > > > >
> > > > > > > > > Aren't they needed for automatic module loading in certain configurations?
> > > > > > > >
> > > > > > > > Any idea how that works, or where the code is for that?
> > > > > > >
> > > > > > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > > > > > product-id that the kernel keeps after boot (and so there is no static
> > > > > > > reference of it for the compiler), later when a device is hotplugged
> > > > > > > into the kernel it refers to the map to find the related driver for it
> > > > > > > and loads it if it isn't already loaded.
> > > > > > >
> > > > > > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > > > > > Documentation/driver-api/usb/hotplug.rst
> > > > > >
> > > > > > And you just need to add __maybe_unused to them to suppress the
> > > > > > warning.
> > > > >
> > > > > Wouldn't that cause the compiler to optimize them away if it doesn't
> > > > > see any users?
> > > >
> > > > It looks like they're only unused when !MODULE,
> > >
> > > OK
> > >
> > > > in which case optimising them away would be the correct thing to do, no?
> >
> > It would be good if someone with a little more knowledge could provide
> > a second opinion though.  I would think (hope) that the compiler would
> > be smart enough to see when its actually in use.  After all, it is the
> > compiler that places the information into the device table.
> >
> If that is not the case, then the MODULE_DEVICE_TABLE() magic is
> > broken and will need fixing.
> 
> I'm not sure why that would be the case?

Nor me.  In fact, take a look at my latest email.  I think I just
proved out that it's not broken.  The warning is valid and
MODULE_DEVICE_TABLE() appears to work just as it should.

> > Removing boiler-plate is good, but not at the expense of obfuscation.
> 
> I'm not following you here to be honest.

Never mind.  It's no longer important.

> BTW, I'm wondering if removing the "static" modifier from the
> definitions of the structures in question makes the warnings you want
> to get rid of go away.

I'm sure that it would.  But that just alludes to the fact that the
tables may be in use elsewhere, which in the case of !MODULE is
untrue.  That's probably more of a hack than using __maybe_unused.
Robin Murphy July 15, 2020, 12:30 p.m. UTC | #14
On 2020-07-15 13:16, Lee Jones wrote:
> On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> 
>> On Wed, Jul 15, 2020 at 1:50 PM Lee Jones <lee.jones@linaro.org> wrote:
>>>
>>> On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
>>>
>>>> On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
>>>>>
>>>>> On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
>>>>>
>>>>>> On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>>>>>
>>>>>>> On 15-07-20, 08:54, Viresh Kumar wrote:
>>>>>>>> On 14-07-20, 22:03, Lee Jones wrote:
>>>>>>>>> On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>>> On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Can't see them being used anywhere and the compiler doesn't complain
>>>>>>>>>>> that they're missing, so ...
>>>>>>>>>>
>>>>>>>>>> Aren't they needed for automatic module loading in certain configurations?
>>>>>>>>>
>>>>>>>>> Any idea how that works, or where the code is for that?
>>>>>>>>
>>>>>>>> The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
>>>>>>>> product-id that the kernel keeps after boot (and so there is no static
>>>>>>>> reference of it for the compiler), later when a device is hotplugged
>>>>>>>> into the kernel it refers to the map to find the related driver for it
>>>>>>>> and loads it if it isn't already loaded.
>>>>>>>>
>>>>>>>> This has some of it, search for MODULE_DEVICE_TABLE() in it.
>>>>>>>> Documentation/driver-api/usb/hotplug.rst
>>>>>>>
>>>>>>> And you just need to add __maybe_unused to them to suppress the
>>>>>>> warning.
>>>>>>
>>>>>> Wouldn't that cause the compiler to optimize them away if it doesn't
>>>>>> see any users?
>>>>>
>>>>> It looks like they're only unused when !MODULE,
>>>>
>>>> OK
>>>>
>>>>> in which case optimising them away would be the correct thing to do, no?
>>>
>>> It would be good if someone with a little more knowledge could provide
>>> a second opinion though.  I would think (hope) that the compiler would
>>> be smart enough to see when its actually in use.  After all, it is the
>>> compiler that places the information into the device table.
>>>
>> If that is not the case, then the MODULE_DEVICE_TABLE() magic is
>>> broken and will need fixing.
>>
>> I'm not sure why that would be the case?
> 
> Nor me.  In fact, take a look at my latest email.  I think I just
> proved out that it's not broken.  The warning is valid and
> MODULE_DEVICE_TABLE() appears to work just as it should.

I won't claim to be an expert at all, but...

For !MODULE, MODULE_DEVICE_TABLE() expands to nothing, so after 
preprocessing the static variable is literally unreferenced.

Otherwise, MODULE_DEVICE_TABLE() emits an extern declaration of another 
variable which is defined via the "alias" attribute to refer to the 
symbol of the static variable. Thus the compiler presumably has to treat 
it as potentially accessible from other compilation units such that it 
can't be optimised away.

>>> Removing boiler-plate is good, but not at the expense of obfuscation.
>>
>> I'm not following you here to be honest.
> 
> Never mind.  It's no longer important.
> 
>> BTW, I'm wondering if removing the "static" modifier from the
>> definitions of the structures in question makes the warnings you want
>> to get rid of go away.
> 
> I'm sure that it would.  But that just alludes to the fact that the
> tables may be in use elsewhere, which in the case of !MODULE is
> untrue.  That's probably more of a hack than using __maybe_unused.

Right, that just ends up with someone sending another patch changing it 
back to shut up "variable foo was not declared, should it be static?" 
warnings from Sparse ;)

Robin.
Lee Jones July 15, 2020, 12:38 p.m. UTC | #15
On Wed, 15 Jul 2020, Robin Murphy wrote:

> On 2020-07-15 13:16, Lee Jones wrote:
> > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> > 
> > > On Wed, Jul 15, 2020 at 1:50 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > 
> > > > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> > > > 
> > > > > On Wed, Jul 15, 2020 at 1:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > 
> > > > > > On Wed, 15 Jul 2020, Rafael J. Wysocki wrote:
> > > > > > 
> > > > > > > On Wed, Jul 15, 2020 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > > > > > 
> > > > > > > > On 15-07-20, 08:54, Viresh Kumar wrote:
> > > > > > > > > On 14-07-20, 22:03, Lee Jones wrote:
> > > > > > > > > > On Tue, 14 Jul 2020, Rafael J. Wysocki wrote:
> > > > > > > > > > 
> > > > > > > > > > > On Tue, Jul 14, 2020 at 4:51 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > Can't see them being used anywhere and the compiler doesn't complain
> > > > > > > > > > > > that they're missing, so ...
> > > > > > > > > > > 
> > > > > > > > > > > Aren't they needed for automatic module loading in certain configurations?
> > > > > > > > > > 
> > > > > > > > > > Any idea how that works, or where the code is for that?
> > > > > > > > > 
> > > > > > > > > The MODULE_DEVICE_TABLE() thingy creates a map of vendor-id,
> > > > > > > > > product-id that the kernel keeps after boot (and so there is no static
> > > > > > > > > reference of it for the compiler), later when a device is hotplugged
> > > > > > > > > into the kernel it refers to the map to find the related driver for it
> > > > > > > > > and loads it if it isn't already loaded.
> > > > > > > > > 
> > > > > > > > > This has some of it, search for MODULE_DEVICE_TABLE() in it.
> > > > > > > > > Documentation/driver-api/usb/hotplug.rst
> > > > > > > > 
> > > > > > > > And you just need to add __maybe_unused to them to suppress the
> > > > > > > > warning.
> > > > > > > 
> > > > > > > Wouldn't that cause the compiler to optimize them away if it doesn't
> > > > > > > see any users?
> > > > > > 
> > > > > > It looks like they're only unused when !MODULE,
> > > > > 
> > > > > OK
> > > > > 
> > > > > > in which case optimising them away would be the correct thing to do, no?
> > > > 
> > > > It would be good if someone with a little more knowledge could provide
> > > > a second opinion though.  I would think (hope) that the compiler would
> > > > be smart enough to see when its actually in use.  After all, it is the
> > > > compiler that places the information into the device table.
> > > > 
> > > If that is not the case, then the MODULE_DEVICE_TABLE() magic is
> > > > broken and will need fixing.
> > > 
> > > I'm not sure why that would be the case?
> > 
> > Nor me.  In fact, take a look at my latest email.  I think I just
> > proved out that it's not broken.  The warning is valid and
> > MODULE_DEVICE_TABLE() appears to work just as it should.
> 
> I won't claim to be an expert at all, but...
> 
> For !MODULE, MODULE_DEVICE_TABLE() expands to nothing, so after
> preprocessing the static variable is literally unreferenced.
> 
> Otherwise, MODULE_DEVICE_TABLE() emits an extern declaration of another
> variable which is defined via the "alias" attribute to refer to the symbol
> of the static variable. Thus the compiler presumably has to treat it as
> potentially accessible from other compilation units such that it can't be
> optimised away.

Right.  That's essentially how I'm reading it.

> > > > Removing boiler-plate is good, but not at the expense of obfuscation.
> > > 
> > > I'm not following you here to be honest.
> > 
> > Never mind.  It's no longer important.
> > 
> > > BTW, I'm wondering if removing the "static" modifier from the
> > > definitions of the structures in question makes the warnings you want
> > > to get rid of go away.
> > 
> > I'm sure that it would.  But that just alludes to the fact that the
> > tables may be in use elsewhere, which in the case of !MODULE is
> > untrue.  That's probably more of a hack than using __maybe_unused.
> 
> Right, that just ends up with someone sending another patch changing it back
> to shut up "variable foo was not declared, should it be static?" warnings
> from Sparse ;)

Exactly.
diff mbox series

Patch

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index d38a693b48e03..fc68f13352695 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -995,18 +995,4 @@  MODULE_PARM_DESC(acpi_pstate_strict,
 late_initcall(acpi_cpufreq_init);
 module_exit(acpi_cpufreq_exit);
 
-static const struct x86_cpu_id acpi_cpufreq_ids[] = {
-	X86_MATCH_FEATURE(X86_FEATURE_ACPI, NULL),
-	X86_MATCH_FEATURE(X86_FEATURE_HW_PSTATE, NULL),
-	{}
-};
-MODULE_DEVICE_TABLE(x86cpu, acpi_cpufreq_ids);
-
-static const struct acpi_device_id processor_device_ids[] = {
-	{ACPI_PROCESSOR_OBJECT_HID, },
-	{ACPI_PROCESSOR_DEVICE_HID, },
-	{},
-};
-MODULE_DEVICE_TABLE(acpi, processor_device_ids);
-
 MODULE_ALIAS("acpi");