[PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Arnd Bergmann]

When CONFIG_PM_SLEEP is disabled, we get a warning about unused functions:

drivers/char/tpm/tpm_crb.c:551:12: error: 'crb_pm_resume' defined but not used [-Werror=unused-function]
drivers/char/tpm/tpm_crb.c:540:12: error: 'crb_pm_suspend' defined but not used [-Werror=unused-function]

We could solve this with more sophistated #ifdefs, but a simpler and safer
way is to just mark them as __maybe_unused.

Fixes: 848efcfb560c ("tpm/tpm_crb: enter the low power state upon device suspend")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/tpm/tpm_crb.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 1dfc37e33c02..15f1118982a6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -519,8 +519,7 @@ static int crb_acpi_remove(struct acpi_device *device)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int crb_pm_runtime_suspend(struct device *dev)
+static __maybe_unused int crb_pm_runtime_suspend(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -528,7 +527,7 @@ static int crb_pm_runtime_suspend(struct device *dev)
 	return crb_go_idle(dev, priv);
 }
 
-static int crb_pm_runtime_resume(struct device *dev)
+static __maybe_unused int crb_pm_runtime_resume(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -536,7 +535,7 @@ static int crb_pm_runtime_resume(struct device *dev)
 	return crb_cmd_ready(dev, priv);
 }
 
-static int crb_pm_suspend(struct device *dev)
+static __maybe_unused int crb_pm_suspend(struct device *dev)
 {
 	int ret;
 
@@ -547,7 +546,7 @@ static int crb_pm_suspend(struct device *dev)
 	return crb_pm_runtime_suspend(dev);
 }
 
-static int crb_pm_resume(struct device *dev)
+static __maybe_unused int crb_pm_resume(struct device *dev)
 {
 	int ret;
 
@@ -558,8 +557,6 @@ static int crb_pm_resume(struct device *dev)
 	return tpm_pm_resume(dev);
 }
 
-#endif /* CONFIG_PM */
-
 static const struct dev_pm_ops crb_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(crb_pm_suspend, crb_pm_resume)
 	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
-- 
2.9.0

Re: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Winkler, Tomas]

> 
> When CONFIG_PM_SLEEP is disabled, we get a warning about unused
> functions:
> 
> drivers/char/tpm/tpm_crb.c:551:12: error: 'crb_pm_resume' defined but not
> used [-Werror=unused-function]
> drivers/char/tpm/tpm_crb.c:540:12: error: 'crb_pm_suspend' defined but not
> used [-Werror=unused-function]
>
Note that the runtime_pm functions are not affected by this issue the macro SET_RUNTIME_PM_OPS
is under CONFIG_PM. This patch does more than described. 

> We could solve this with more sophistated #ifdefs, but a simpler and safer way
> is to just mark them as __maybe_unused.

> 
> Fixes: 848efcfb560c ("tpm/tpm_crb: enter the low power state upon device
> suspend")
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_crb.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index
> 1dfc37e33c02..15f1118982a6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -519,8 +519,7 @@ static int crb_acpi_remove(struct acpi_device *device)
>  	return 0;
>  }
> 
> -#ifdef CONFIG_PM
> -static int crb_pm_runtime_suspend(struct device *dev)
> +static __maybe_unused int crb_pm_runtime_suspend(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev); @@ -528,7
> +527,7 @@ static int crb_pm_runtime_suspend(struct device *dev)
>  	return crb_go_idle(dev, priv);
>  }
> 
> -static int crb_pm_runtime_resume(struct device *dev)
> +static __maybe_unused int crb_pm_runtime_resume(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev); @@ -536,7
> +535,7 @@ static int crb_pm_runtime_resume(struct device *dev)
>  	return crb_cmd_ready(dev, priv);
>  }
> 
> -static int crb_pm_suspend(struct device *dev)
> +static __maybe_unused int crb_pm_suspend(struct device *dev)
>  {
>  	int ret;
> 
> @@ -547,7 +546,7 @@ static int crb_pm_suspend(struct device *dev)
>  	return crb_pm_runtime_suspend(dev);
>  }

It's enough to 
#endif /* CONFIG_PM */
#ifdef CONFIG_PM_SLEEP
> -static int crb_pm_resume(struct device *dev)
> +static __maybe_unused int crb_pm_resume(struct device *dev)
>  {
>  	int ret;
> 
> @@ -558,8 +557,6 @@ static int crb_pm_resume(struct device *dev)
>  	return tpm_pm_resume(dev);
>  }
> 
> -#endif /* CONFIG_PM */
And 
#endif CONFIG_PM_SLEEP

> -
>  static const struct dev_pm_ops crb_pm = {
>  	SET_SYSTEM_SLEEP_PM_OPS(crb_pm_suspend, crb_pm_resume)
>  	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend,
> crb_pm_runtime_resume, NULL)
> --
> 2.9.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 1:11 PM, Winkler, Tomas <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>>
>> When CONFIG_PM_SLEEP is disabled, we get a warning about unused
>> functions:
>>
>> drivers/char/tpm/tpm_crb.c:551:12: error: 'crb_pm_resume' defined but not
>> used [-Werror=unused-function]
>> drivers/char/tpm/tpm_crb.c:540:12: error: 'crb_pm_suspend' defined but not
>> used [-Werror=unused-function]
>>
> Note that the runtime_pm functions are not affected by this issue the macro
> SET_RUNTIME_PM_OPS is under CONFIG_PM. This patch does more than described.

Well, the problem is that there is an #ifdef that is wrong here as I
tried to indicate:

>> We could solve this with more sophistated #ifdefs, but a simpler and safer way
>> is to just mark them as __maybe_unused.

>> @@ -547,7 +546,7 @@ static int crb_pm_suspend(struct device *dev)
>>       return crb_pm_runtime_suspend(dev);
>>  }
>
> It's enough to
> #endif /* CONFIG_PM */
> #ifdef CONFIG_PM_SLEEP
>> -static int crb_pm_resume(struct device *dev)
>> +static __maybe_unused int crb_pm_resume(struct device *dev)
>>  {
>>       int ret;
>>
>> @@ -558,8 +557,6 @@ static int crb_pm_resume(struct device *dev)
>>       return tpm_pm_resume(dev);
>>  }
>>
>> -#endif /* CONFIG_PM */
> And
> #endif CONFIG_PM_SLEEP

This tends to cause other warnings half of the time, when both the
runtime-pm and pm-sleep variants call into another function that
becomes unused when both are disabled.

       Arnd

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Winkler, Tomas]

> On Mon, Mar 20, 2017 at 1:11 PM, Winkler, Tomas
> <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> When CONFIG_PM_SLEEP is disabled, we get a warning about unused
> >> functions:
> >>
> >> drivers/char/tpm/tpm_crb.c:551:12: error: 'crb_pm_resume' defined but
> >> not used [-Werror=unused-function]
> >> drivers/char/tpm/tpm_crb.c:540:12: error: 'crb_pm_suspend' defined
> >> but not used [-Werror=unused-function]
> >>
> > Note that the runtime_pm functions are not affected by this issue the
> > macro SET_RUNTIME_PM_OPS is under CONFIG_PM. This patch does more
> than described.
> 
> Well, the problem is that there is an #ifdef that is wrong here as I tried to
> indicate:

Yep, I've taken a bit easy path here and reused the runtime function inside pm callbacks, unaware of the change in the 'ifdefs'
If I use drictly go_idle/cmd_ready  functions this will unclutter it. 

> >> We could solve this with more sophistated #ifdefs, but a simpler and
> >> safer way is to just mark them as __maybe_unused.
> 
> >> @@ -547,7 +546,7 @@ static int crb_pm_suspend(struct device *dev)
> >>       return crb_pm_runtime_suspend(dev);  }
> >
> > It's enough to
> > #endif /* CONFIG_PM */
> > #ifdef CONFIG_PM_SLEEP
> >> -static int crb_pm_resume(struct device *dev)
> >> +static __maybe_unused int crb_pm_resume(struct device *dev)
> >>  {
> >>       int ret;
> >>
> >> @@ -558,8 +557,6 @@ static int crb_pm_resume(struct device *dev)
> >>       return tpm_pm_resume(dev);
> >>  }
> >>
> >> -#endif /* CONFIG_PM */
> > And
> > #endif CONFIG_PM_SLEEP
> 
> This tends to cause other warnings half of the time, when both the runtime-
> pm and pm-sleep variants call into another function that becomes unused
> when both are disabled.

Then usually such functions  should be also under 'ifdef', but I understand your point in
some cases it might be not so straight forward.
The only reason against marking the function __maybe_unsed is  maybe the binary size.

I believe that  in this case the #ifdefs can be done correctly quite easily, 
but now I'm not against your solution as well, just maybe put some of this info to the commit message. 

Thanks
Tomas



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Jason Gunthorpe]

On Mon, Mar 20, 2017 at 11:01:36PM +0000, Winkler, Tomas wrote:

> I believe that in this case the #ifdefs can be done correctly quite
> easily, but now I'm not against your solution as well, just maybe
> put some of this info to the commit message.

I perfer fewer ifdefs, it makes it more maintainable..

The compiler will remove unused static functions.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Winkler, Tomas]

> 
> On Mon, Mar 20, 2017 at 11:01:36PM +0000, Winkler, Tomas wrote:
> 
> > I believe that in this case the #ifdefs can be done correctly quite
> > easily, but now I'm not against your solution as well, just maybe put
> > some of this info to the commit message.
> 
> I perfer fewer ifdefs, it makes it more maintainable..

Sure,
> 
> The compiler will remove unused static functions.

I'm not sure if this goes away w/o --gc-sections, but it might. 
Will check, didn't looked at that for a while. 

Thanks
Tomas 

Tomas


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Arnd Bergmann]

On Tue, Mar 21, 2017 at 12:35 AM, Winkler, Tomas
<tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
>>
>> On Mon, Mar 20, 2017 at 11:01:36PM +0000, Winkler, Tomas wrote:
>>
>> > I believe that in this case the #ifdefs can be done correctly quite
>> > easily, but now I'm not against your solution as well, just maybe put
>> > some of this info to the commit message.
>>
>> I perfer fewer ifdefs, it makes it more maintainable..
>
> Sure,
>>
>> The compiler will remove unused static functions.
>
> I'm not sure if this goes away w/o --gc-sections, but it might.
> Will check, didn't looked at that for a while.

gcc-4.1 had a bug where code it failed to eliminate a dead function
if it was referenced through a function pointer in another unused
static function, but it would work correctly in this case (obviously
unused code) and compiler that people actually use don't have
this problem. Note that the kernel depends on dead code elimination
to work correctly in a lot of places, it wouldn't build at all if that
was broken.

      Arnd

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused

[Jarkko Sakkinen]

On Mon, Mar 20, 2017 at 10:17:19AM +0100, Arnd Bergmann wrote:
> When CONFIG_PM_SLEEP is disabled, we get a warning about unused functions:
> 
> drivers/char/tpm/tpm_crb.c:551:12: error: 'crb_pm_resume' defined but not used [-Werror=unused-function]
> drivers/char/tpm/tpm_crb.c:540:12: error: 'crb_pm_suspend' defined but not used [-Werror=unused-function]
> 
> We could solve this with more sophistated #ifdefs, but a simpler and safer
> way is to just mark them as __maybe_unused.
> 
> Fixes: 848efcfb560c ("tpm/tpm_crb: enter the low power state upon device suspend")
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Hi, I somehow missed this and applied very similar patch. Sorry about
that.

/Jarkko

> ---
>  drivers/char/tpm/tpm_crb.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 1dfc37e33c02..15f1118982a6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -519,8 +519,7 @@ static int crb_acpi_remove(struct acpi_device *device)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM
> -static int crb_pm_runtime_suspend(struct device *dev)
> +static __maybe_unused int crb_pm_runtime_suspend(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -528,7 +527,7 @@ static int crb_pm_runtime_suspend(struct device *dev)
>  	return crb_go_idle(dev, priv);
>  }
>  
> -static int crb_pm_runtime_resume(struct device *dev)
> +static __maybe_unused int crb_pm_runtime_resume(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -536,7 +535,7 @@ static int crb_pm_runtime_resume(struct device *dev)
>  	return crb_cmd_ready(dev, priv);
>  }
>  
> -static int crb_pm_suspend(struct device *dev)
> +static __maybe_unused int crb_pm_suspend(struct device *dev)
>  {
>  	int ret;
>  
> @@ -547,7 +546,7 @@ static int crb_pm_suspend(struct device *dev)
>  	return crb_pm_runtime_suspend(dev);
>  }
>  
> -static int crb_pm_resume(struct device *dev)
> +static __maybe_unused int crb_pm_resume(struct device *dev)
>  {
>  	int ret;
>  
> @@ -558,8 +557,6 @@ static int crb_pm_resume(struct device *dev)
>  	return tpm_pm_resume(dev);
>  }
>  
> -#endif /* CONFIG_PM */
> -
>  static const struct dev_pm_ops crb_pm = {
>  	SET_SYSTEM_SLEEP_PM_OPS(crb_pm_suspend, crb_pm_resume)
>  	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
> -- 
> 2.9.0
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot