The Intel 0-DAY CI Kernel Test Service reports an unused variable
warning when compiling with clang for PowerPC:
>> drivers/mfd/kempld-core.c:556:36: warning: unused variable 'kempld_acpi_table' [-Wunused-const-variable]
static const struct acpi_device_id kempld_acpi_table[] = {
The issue can be fixed by marking kempld_acpi_table as __maybe_unused.
Fixes: e8299c7313af ("[PATCH] mfd: Add ACPI support to Kontron PLD driver")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Michael Brunner <[email protected]>
---
drivers/mfd/kempld-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
index 1dfe556df038..273481dfaad4 100644
--- a/drivers/mfd/kempld-core.c
+++ b/drivers/mfd/kempld-core.c
@@ -553,7 +553,7 @@ static int kempld_remove(struct platform_device *pdev)
return 0;
}
-static const struct acpi_device_id kempld_acpi_table[] = {
+static const struct acpi_device_id __maybe_unused kempld_acpi_table[] = {
{ "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic },
{}
};
--
2.25.1
On Thu, 01 Oct 2020, Michael Brunner wrote:
> The Intel 0-DAY CI Kernel Test Service reports an unused variable
> warning when compiling with clang for PowerPC:
>
> >> drivers/mfd/kempld-core.c:556:36: warning: unused variable 'kempld_acpi_table' [-Wunused-const-variable]
> static const struct acpi_device_id kempld_acpi_table[] = {
>
> The issue can be fixed by marking kempld_acpi_table as __maybe_unused.
>
> Fixes: e8299c7313af ("[PATCH] mfd: Add ACPI support to Kontron PLD driver")
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Michael Brunner <[email protected]>
> ---
> drivers/mfd/kempld-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> index 1dfe556df038..273481dfaad4 100644
> --- a/drivers/mfd/kempld-core.c
> +++ b/drivers/mfd/kempld-core.c
> @@ -553,7 +553,7 @@ static int kempld_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct acpi_device_id kempld_acpi_table[] = {
> +static const struct acpi_device_id __maybe_unused kempld_acpi_table[] = {
> { "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic },
> {}
> };
This is not the right fix. Better just to compile it out completely
in these circumstances. I already have a fix for this in soak.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 2020-10-02 at 08:01 +0100, Lee Jones wrote:
> On Thu, 01 Oct 2020, Michael Brunner wrote:
>
> > The Intel 0-DAY CI Kernel Test Service reports an unused variable
> > warning when compiling with clang for PowerPC:
> >
> > > > drivers/mfd/kempld-core.c:556:36: warning: unused variable
> > > > 'kempld_acpi_table' [-Wunused-const-variable]
> > static const struct acpi_device_id kempld_acpi_table[] = {
> >
> > The issue can be fixed by marking kempld_acpi_table as
> > __maybe_unused.
> >
> > Fixes: e8299c7313af ("[PATCH] mfd: Add ACPI support to Kontron PLD
> > driver")
> >
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Michael Brunner <[email protected]>
> > ---
> > drivers/mfd/kempld-core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> > index 1dfe556df038..273481dfaad4 100644
> > --- a/drivers/mfd/kempld-core.c
> > +++ b/drivers/mfd/kempld-core.c
> > @@ -553,7 +553,7 @@ static int kempld_remove(struct platform_device
> > *pdev)
> > return 0;
> > }
> >
> > -static const struct acpi_device_id kempld_acpi_table[] = {
> > +static const struct acpi_device_id __maybe_unused
> > kempld_acpi_table[] = {
> > { "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic },
> > {}
> > };
>
> This is not the right fix. Better just to compile it out completely
> in these circumstances. I already have a fix for this in soak.
Ok - thank you for the other fix you submitted!
But just out of curiosity - in process/coding-style.rst is written that
__maybe_unused should be preferred over wrapping in preprocessor
conditionals, if a function or variable may potentially go unused in a
particular configuration. So why is my patch not the right one here? At
least in my tests it seemed to solve the issue.
Thanks,
Michael
On Mon, 05 Oct 2020, Michael Brunner wrote:
> On Fri, 2020-10-02 at 08:01 +0100, Lee Jones wrote:
> > On Thu, 01 Oct 2020, Michael Brunner wrote:
> >
> > > The Intel 0-DAY CI Kernel Test Service reports an unused variable
> > > warning when compiling with clang for PowerPC:
> > >
> > > > > drivers/mfd/kempld-core.c:556:36: warning: unused variable
> > > > > 'kempld_acpi_table' [-Wunused-const-variable]
> > > static const struct acpi_device_id kempld_acpi_table[] = {
> > >
> > > The issue can be fixed by marking kempld_acpi_table as
> > > __maybe_unused.
> > >
> > > Fixes: e8299c7313af ("[PATCH] mfd: Add ACPI support to Kontron PLD
> > > driver")
> > >
> > > Reported-by: kernel test robot <[email protected]>
> > > Signed-off-by: Michael Brunner <[email protected]>
> > > ---
> > > drivers/mfd/kempld-core.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> > > index 1dfe556df038..273481dfaad4 100644
> > > --- a/drivers/mfd/kempld-core.c
> > > +++ b/drivers/mfd/kempld-core.c
> > > @@ -553,7 +553,7 @@ static int kempld_remove(struct platform_device
> > > *pdev)
> > > return 0;
> > > }
> > >
> > > -static const struct acpi_device_id kempld_acpi_table[] = {
> > > +static const struct acpi_device_id __maybe_unused
> > > kempld_acpi_table[] = {
> > > { "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic },
> > > {}
> > > };
> >
> > This is not the right fix. Better just to compile it out completely
> > in these circumstances. I already have a fix for this in soak.
>
> Ok - thank you for the other fix you submitted!
>
> But just out of curiosity - in process/coding-style.rst is written that
> __maybe_unused should be preferred over wrapping in preprocessor
> conditionals, if a function or variable may potentially go unused in a
> particular configuration. So why is my patch not the right one here? At
> least in my tests it seemed to solve the issue.
It's a bone of contention for sure. In these kinds of scenarios
(i.e. ACPI and OF tables) it is way more common to wrap them:
$ git grep -B3 'acpi_device_id\|of_device_id' | grep 'CONFIG_ACPI\|CONFIG_OF' | wc -l
596
$ git grep -B3 'acpi_device_id\|of_device_id' | grep __maybe_unused | wc -l
63
Parsing them out completely, also has the benefit of saving space,
reducing the size of the finalised binary.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 2020-10-06 at 07:53 +0100, Lee Jones wrote:
> On Mon, 05 Oct 2020, Michael Brunner wrote:
>
> > On Fri, 2020-10-02 at 08:01 +0100, Lee Jones wrote:
> > > On Thu, 01 Oct 2020, Michael Brunner wrote:
> > >
> > > > The Intel 0-DAY CI Kernel Test Service reports an unused variable
> > > > warning when compiling with clang for PowerPC:
> > > >
> > > > > > drivers/mfd/kempld-core.c:556:36: warning: unused variable
> > > > > > 'kempld_acpi_table' [-Wunused-const-variable]
> > > > static const struct acpi_device_id kempld_acpi_table[] = {
> > > >
> > > > The issue can be fixed by marking kempld_acpi_table as
> > > > __maybe_unused.
> > > >
> > > > Fixes: e8299c7313af ("[PATCH] mfd: Add ACPI support to Kontron PLD
> > > > driver")
> > > >
> > > > Reported-by: kernel test robot <[email protected]>
> > > > Signed-off-by: Michael Brunner <[email protected]>
> > > > ---
> > > > drivers/mfd/kempld-core.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> > > > index 1dfe556df038..273481dfaad4 100644
> > > > --- a/drivers/mfd/kempld-core.c
> > > > +++ b/drivers/mfd/kempld-core.c
> > > > @@ -553,7 +553,7 @@ static int kempld_remove(struct platform_device
> > > > *pdev)
> > > > return 0;
> > > > }
> > > >
> > > > -static const struct acpi_device_id kempld_acpi_table[] = {
> > > > +static const struct acpi_device_id __maybe_unused
> > > > kempld_acpi_table[] = {
> > > > { "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic },
> > > > {}
> > > > };
> > >
> > > This is not the right fix. Better just to compile it out completely
> > > in these circumstances. I already have a fix for this in soak.
> >
> > Ok - thank you for the other fix you submitted!
> >
> > But just out of curiosity - in process/coding-style.rst is written that
> > __maybe_unused should be preferred over wrapping in preprocessor
> > conditionals, if a function or variable may potentially go unused in a
> > particular configuration. So why is my patch not the right one here? At
> > least in my tests it seemed to solve the issue.
>
> It's a bone of contention for sure. In these kinds of scenarios
> (i.e. ACPI and OF tables) it is way more common to wrap them:
>
> $ git grep -B3 'acpi_device_id\|of_device_id' | grep 'CONFIG_ACPI\|CONFIG_OF' | wc -l
> 596
> $ git grep -B3 'acpi_device_id\|of_device_id' | grep __maybe_unused | wc -l
> 63
>
> Parsing them out completely, also has the benefit of saving space,
> reducing the size of the finalised binary.
Doesn't the compiler remove it anyway? At least in my test I didn't see
a difference in the resulting object files.
Doing a crosscheck, by adding __attribute__((used)) to the definition
of kempld_acpi_table, the object file size increased and
kempld_acpi_table showed up in the symbol table.
Nevertheless, I don't want to start a discussion. I am fine with using
the preprocessor. Just wanted to make sure I understand the technical
implications of both solutions.
Thank you for your time!
Best regards,
Michael
On Tue, 06 Oct 2020, Michael Brunner wrote:
> On Tue, 2020-10-06 at 07:53 +0100, Lee Jones wrote:
> > On Mon, 05 Oct 2020, Michael Brunner wrote:
> >
> > > On Fri, 2020-10-02 at 08:01 +0100, Lee Jones wrote:
> > > > On Thu, 01 Oct 2020, Michael Brunner wrote:
> > > >
> > > > > The Intel 0-DAY CI Kernel Test Service reports an unused variable
> > > > > warning when compiling with clang for PowerPC:
> > > > >
> > > > > > > drivers/mfd/kempld-core.c:556:36: warning: unused variable
> > > > > > > 'kempld_acpi_table' [-Wunused-const-variable]
> > > > > static const struct acpi_device_id kempld_acpi_table[] = {
> > > > >
> > > > > The issue can be fixed by marking kempld_acpi_table as
> > > > > __maybe_unused.
> > > > >
> > > > > Fixes: e8299c7313af ("[PATCH] mfd: Add ACPI support to Kontron PLD
> > > > > driver")
> > > > >
> > > > > Reported-by: kernel test robot <[email protected]>
> > > > > Signed-off-by: Michael Brunner <[email protected]>
> > > > > ---
> > > > > drivers/mfd/kempld-core.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> > > > > index 1dfe556df038..273481dfaad4 100644
> > > > > --- a/drivers/mfd/kempld-core.c
> > > > > +++ b/drivers/mfd/kempld-core.c
> > > > > @@ -553,7 +553,7 @@ static int kempld_remove(struct platform_device
> > > > > *pdev)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static const struct acpi_device_id kempld_acpi_table[] = {
> > > > > +static const struct acpi_device_id __maybe_unused
> > > > > kempld_acpi_table[] = {
> > > > > { "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic },
> > > > > {}
> > > > > };
> > > >
> > > > This is not the right fix. Better just to compile it out completely
> > > > in these circumstances. I already have a fix for this in soak.
> > >
> > > Ok - thank you for the other fix you submitted!
> > >
> > > But just out of curiosity - in process/coding-style.rst is written that
> > > __maybe_unused should be preferred over wrapping in preprocessor
> > > conditionals, if a function or variable may potentially go unused in a
> > > particular configuration. So why is my patch not the right one here? At
> > > least in my tests it seemed to solve the issue.
> >
> > It's a bone of contention for sure. In these kinds of scenarios
> > (i.e. ACPI and OF tables) it is way more common to wrap them:
> >
> > $ git grep -B3 'acpi_device_id\|of_device_id' | grep 'CONFIG_ACPI\|CONFIG_OF' | wc -l
> > 596
> > $ git grep -B3 'acpi_device_id\|of_device_id' | grep __maybe_unused | wc -l
> > 63
> >
> > Parsing them out completely, also has the benefit of saving space,
> > reducing the size of the finalised binary.
>
> Doesn't the compiler remove it anyway? At least in my test I didn't see
> a difference in the resulting object files.
> Doing a crosscheck, by adding __attribute__((used)) to the definition
> of kempld_acpi_table, the object file size increased and
> kempld_acpi_table showed up in the symbol table.
>
> Nevertheless, I don't want to start a discussion. I am fine with using
> the preprocessor. Just wanted to make sure I understand the technical
> implications of both solutions.
This is what happened last time I submitted a patch using
__maybe_unused:
https://lkml.org/lkml/2020/8/17/1704
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog