linux-kernel - RE: [PATCH] tpm/tpm_crb: mark PM functions as __maybe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Mar 2017 23:01:36 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     Peter Huewe <peterhuewe@....de>,
        Marcel Selhorst <tpmdd@...horst.net>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
        Jerry Snitselaar <jsnitsel@...hat.com>,
        "tpmdd-devel@...ts.sourceforge.net" 
        <tpmdd-devel@...ts.sourceforge.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] tpm/tpm_crb: mark PM functions as __maybe_unused


> On Mon, Mar 20, 2017 at 1:11 PM, Winkler, Tomas
> <tomas.winkler@...el.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