| Message ID | 20170320091755.1043811-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
> > 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@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); > } 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
On Mon, Mar 20, 2017 at 1:11 PM, Winkler, Tomas <tomas.winkler@intel.com> 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
> On Mon, Mar 20, 2017 at 1:11 PM, Winkler, Tomas > <tomas.winkler@intel.com> 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
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
> > 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
On Tue, Mar 21, 2017 at 12:35 AM, Winkler, Tomas <tomas.winkler@intel.com> 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
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@arndb.de> 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
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)
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(-)