| Message ID | 20180516194600.28189-1-tomas.winkler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote: > New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to > streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is abused to > resolve tpm spaces recursive calls to tpm_transmit(). This looks good and all but I don't think we want to abuse anything in the driver code, do we? In other words, either 1. New flag is to be added. 2. Rename the existing flag to something else than UNLOCKED (perhaps SPACE). /Jarkko
> > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote: > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to > > streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is > abused > > to resolve tpm spaces recursive calls to tpm_transmit(). > > This looks good and all but I don't think we want to abuse anything in the > driver code, do we? It's not abuse just the flag UNLOCKED is not really named correctly I think this has to be backported so wanted to do less invasive change. > > In other words, either > > 1. New flag is to be added. No > 2. Rename the existing flag to something else than UNLOCKED (perhaps > SPACE). Currently both TPM_TRANSMIT_UNLOCKED and TPM_TRANSMIT_RAW servers the same purpose, to resolve the recursion in tpm tranmit. What I believe should be done is to split the function into two to eliminate recursive call instead of using flags. Please also there is clock_enable which is not protected from double call. Thanks Tomas
On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote: > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote: > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to > > > streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is > > abused > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > This looks good and all but I don't think we want to abuse anything in the > > driver code, do we? > > It's not abuse just the flag UNLOCKED is not really named correctly > I think this has to be backported so wanted to do less invasive change. It should be renamed anyway and possible merge conflicts are not hard to sort out in this change. Can you rename it as SPACE? Right, and even without rename this will probably cause merge conflicts at least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much gain not do the rename :-) /Jarkko
> On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote: > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote: > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() > wrappers > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag > is > > > abused > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > This looks good and all but I don't think we want to abuse anything > > > in the driver code, do we? > > > > It's not abuse just the flag UNLOCKED is not really named correctly I > > think this has to be backported so wanted to do less invasive change. > > It should be renamed anyway and possible merge conflicts are not hard to > sort out in this change. Can you rename it as SPACE? Not sure, I believe UNLOCKED is still better name than SPACE, I'm not sure this is Do you also want to remove TPM_TRANSMIT_RAW? clk_enable is handling its own anti recursion counter 'data->clkrun_enabled' but it should be all handled under one flag I guess. > Right, and even without rename this will probably cause merge conflicts at > least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much gain not > do the rename :-) I belive we should do minimal change and the big cleanup after that. Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't the original intention. No the SPACE is the issue, but any recursion call into tpm_transmit. A bigger change is needed and rename to SPACE would be just another intermediat change. Please reconsider. Thanks Tomas
On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote: > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote: > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote: > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() > > wrappers > > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag > > is > > > > abused > > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > > > This looks good and all but I don't think we want to abuse anything > > > > in the driver code, do we? > > > > > > It's not abuse just the flag UNLOCKED is not really named correctly I > > > think this has to be backported so wanted to do less invasive change. > > > > It should be renamed anyway and possible merge conflicts are not hard to > > sort out in this change. Can you rename it as SPACE? > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm not sure this is > Do you also want to remove TPM_TRANSMIT_RAW? > clk_enable is handling its own anti recursion counter 'data->clkrun_enabled' > but it should be all handled under one flag I guess. > > > Right, and even without rename this will probably cause merge conflicts at > > least in v4.4 an v4.9 since in-kernel RM landed in v4.12, so not much gain not > > do the rename :-) > > I belive we should do minimal change and the big cleanup after that. > Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't the original intention. > No the SPACE is the issue, but any recursion call into tpm_transmit. A bigger change is needed > and rename to SPACE would be just another intermediat change. > > Please reconsider. > > Thanks > Tomas Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
> > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote: > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote: > > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote: > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() > > > wrappers > > > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED > > > > > > flag > > > is > > > > > abused > > > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > > > > > This looks good and all but I don't think we want to abuse > > > > > anything in the driver code, do we? > > > > > > > > It's not abuse just the flag UNLOCKED is not really named > > > > correctly I think this has to be backported so wanted to do less invasive > change. > > > > > > It should be renamed anyway and possible merge conflicts are not > > > hard to sort out in this change. Can you rename it as SPACE? > > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm not > > sure this is Do you also want to remove TPM_TRANSMIT_RAW? > > clk_enable is handling its own anti recursion counter 'data- > >clkrun_enabled' > > but it should be all handled under one flag I guess. > > > > > Right, and even without rename this will probably cause merge > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in > > > v4.12, so not much gain not do the rename :-) > > > > I belive we should do minimal change and the big cleanup after that. > > Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't > the original intention. > > No the SPACE is the issue, but any recursion call into tpm_transmit. A > > bigger change is needed and rename to SPACE would be just another > intermediat change. > > > > Please reconsider. > > > > Thanks > > Tomas > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Does it mean you're Okay with the patch now? Thanks Tomas
On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote: > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote: > > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler wrote: > > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() > > > > wrappers > > > > > > > to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED > > > > > > > flag > > > > is > > > > > > abused > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > > > > > > > This looks good and all but I don't think we want to abuse > > > > > > anything in the driver code, do we? > > > > > > > > > > It's not abuse just the flag UNLOCKED is not really named > > > > > correctly I think this has to be backported so wanted to do less invasive > > change. > > > > > > > > It should be renamed anyway and possible merge conflicts are not > > > > hard to sort out in this change. Can you rename it as SPACE? > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm not > > > sure this is Do you also want to remove TPM_TRANSMIT_RAW? > > > clk_enable is handling its own anti recursion counter 'data- > > >clkrun_enabled' > > > but it should be all handled under one flag I guess. > > > > > > > Right, and even without rename this will probably cause merge > > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in > > > > v4.12, so not much gain not do the rename :-) > > > > > > I belive we should do minimal change and the big cleanup after that. > > > Not sure, I believe UNLOCKED is still better name than SPACE even it wasn't > > the original intention. > > > No the SPACE is the issue, but any recursion call into tpm_transmit. A > > > bigger change is needed and rename to SPACE would be just another > > intermediat change. > > > > > > Please reconsider. > > > > > > Thanks > > > Tomas > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > Does it mean you're Okay with the patch now? > Thanks > Tomas The change looks good but I'll have to test it. /Jarkko
> > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote: > > > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote: > > > > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler > wrote: > > > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() > > > > > wrappers > > > > > > > > to streamline tpm_try_transmit code. > TPM_TRANSMIT_UNLOCKED > > > > > > > > flag > > > > > is > > > > > > > abused > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > > > > > > > > > This looks good and all but I don't think we want to abuse > > > > > > > anything in the driver code, do we? > > > > > > > > > > > > It's not abuse just the flag UNLOCKED is not really named > > > > > > correctly I think this has to be backported so wanted to do > > > > > > less invasive > > > change. > > > > > > > > > > It should be renamed anyway and possible merge conflicts are not > > > > > hard to sort out in this change. Can you rename it as SPACE? > > > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm > > > >not sure this is Do you also want to remove TPM_TRANSMIT_RAW? > > > > clk_enable is handling its own anti recursion counter 'data- > > > >clkrun_enabled' > > > > but it should be all handled under one flag I guess. > > > > > > > > > Right, and even without rename this will probably cause merge > > > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in > > > > > v4.12, so not much gain not do the rename :-) > > > > > > > > I belive we should do minimal change and the big cleanup after that. > > > > Not sure, I believe UNLOCKED is still better name than SPACE even > > > > it wasn't > > > the original intention. > > > > No the SPACE is the issue, but any recursion call into > > > > tpm_transmit. A bigger change is needed and rename to SPACE would > > > > be just another > > > intermediat change. > > > > > > > > Please reconsider. > > > > > > > > Thanks > > > > Tomas > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > Does it mean you're Okay with the patch now? > > Thanks > > Tomas > > The change looks good but I'll have to test it. Any updates? Thanks
On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote: > > > > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote: > > > > > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote: > > > > > > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler > > wrote: > > > > > > > > > New wrappers are added tpm_cmd_ready() and tpm_go_idle() > > > > > > wrappers > > > > > > > > > to streamline tpm_try_transmit code. > > TPM_TRANSMIT_UNLOCKED > > > > > > > > > flag > > > > > > is > > > > > > > > abused > > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > > > > > > > > > > > This looks good and all but I don't think we want to abuse > > > > > > > > anything in the driver code, do we? > > > > > > > > > > > > > > It's not abuse just the flag UNLOCKED is not really named > > > > > > > correctly I think this has to be backported so wanted to do > > > > > > > less invasive > > > > change. > > > > > > > > > > > > It should be renamed anyway and possible merge conflicts are not > > > > > > hard to sort out in this change. Can you rename it as SPACE? > > > > > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE, I'm > > > > >not sure this is Do you also want to remove TPM_TRANSMIT_RAW? > > > > > clk_enable is handling its own anti recursion counter 'data- > > > > >clkrun_enabled' > > > > > but it should be all handled under one flag I guess. > > > > > > > > > > > Right, and even without rename this will probably cause merge > > > > > > conflicts at least in v4.4 an v4.9 since in-kernel RM landed in > > > > > > v4.12, so not much gain not do the rename :-) > > > > > > > > > > I belive we should do minimal change and the big cleanup after that. > > > > > Not sure, I believe UNLOCKED is still better name than SPACE even > > > > > it wasn't > > > > the original intention. > > > > > No the SPACE is the issue, but any recursion call into > > > > > tpm_transmit. A bigger change is needed and rename to SPACE would > > > > > be just another > > > > intermediat change. > > > > > > > > > > Please reconsider. > > > > > > > > > > Thanks > > > > > Tomas > > > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > > > > Does it mean you're Okay with the patch now? > > > Thanks > > > Tomas > > > > The change looks good but I'll have to test it. > Any updates? > Thanks Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
> -----Original Message----- > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > Sent: Thursday, June 07, 2018 13:25 > To: Winkler, Tomas <tomas.winkler@intel.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander > <alexander.usyskin@intel.com>; linux-integrity@vger.kernel.org; linux- > security-module@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm > > On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote: > > > > > > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote: > > > > > > > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas > wrote: > > > > > > > > > > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler > > > wrote: > > > > > > > > > > New wrappers are added tpm_cmd_ready() and > > > > > > > > > > tpm_go_idle() > > > > > > > wrappers > > > > > > > > > > to streamline tpm_try_transmit code. > > > TPM_TRANSMIT_UNLOCKED > > > > > > > > > > flag > > > > > > > is > > > > > > > > > abused > > > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > > > > > > > > > > > > > This looks good and all but I don't think we want to > > > > > > > > > abuse anything in the driver code, do we? > > > > > > > > > > > > > > > > It's not abuse just the flag UNLOCKED is not really named > > > > > > > > correctly I think this has to be backported so wanted to > > > > > > > > do less invasive > > > > > change. > > > > > > > > > > > > > > It should be renamed anyway and possible merge conflicts are > > > > > > > not hard to sort out in this change. Can you rename it as SPACE? > > > > > > > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE, > > > > > >I'm not sure this is Do you also want to remove > TPM_TRANSMIT_RAW? > > > > > > clk_enable is handling its own anti recursion counter 'data- > > > > > >clkrun_enabled' > > > > > > but it should be all handled under one flag I guess. > > > > > > > > > > > > > Right, and even without rename this will probably cause > > > > > > > merge conflicts at least in v4.4 an v4.9 since in-kernel RM > > > > > > > landed in v4.12, so not much gain not do the rename :-) > > > > > > > > > > > > I belive we should do minimal change and the big cleanup after > that. > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE > > > > > > even it wasn't > > > > > the original intention. > > > > > > No the SPACE is the issue, but any recursion call into > > > > > > tpm_transmit. A bigger change is needed and rename to SPACE > > > > > > would be just another > > > > > intermediat change. > > > > > > > > > > > > Please reconsider. > > > > > > > > > > > > Thanks > > > > > > Tomas > > > > > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > > > > > > > Does it mean you're Okay with the patch now? > > > > Thanks > > > > Tomas > > > > > > The change looks good but I'll have to test it. > > Any updates? > > Thanks > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> I've just realized we have issue in tpm_unseal_trusted() As TPM_TRANSMIT_UNLOCKED is used really just in 'locking' sense of the flow, it's not nested. Any of testing flows doesn't covers it. It's used only from by security/keys/trusted.c only Then I don't have a short fix for this issue. Will use TPM_TRANSMIT_RAW, maybe calling it TPM_TRANSMIT_NESTED. Thanks Tomas > > /Jarkko
On Thu, Jun 07, 2018 at 11:03:50AM +0000, Winkler, Tomas wrote: > > > > -----Original Message----- > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > > Sent: Thursday, June 07, 2018 13:25 > > To: Winkler, Tomas <tomas.winkler@intel.com> > > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander > > <alexander.usyskin@intel.com>; linux-integrity@vger.kernel.org; linux- > > security-module@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH] tpm: separate cmd_ready/go_idle from runtime_pm > > > > On Wed, Jun 06, 2018 at 11:01:42AM +0000, Winkler, Tomas wrote: > > > > > > > > On Wed, May 30, 2018 at 10:52:28AM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > On Wed, May 23, 2018 at 01:48:17PM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > > > On Tue, May 22, 2018 at 09:27:46AM +0000, Winkler, Tomas > > wrote: > > > > > > > > > > > > > > > > > > > > On Wed, May 16, 2018 at 10:46:00PM +0300, Tomas Winkler > > > > wrote: > > > > > > > > > > > New wrappers are added tpm_cmd_ready() and > > > > > > > > > > > tpm_go_idle() > > > > > > > > wrappers > > > > > > > > > > > to streamline tpm_try_transmit code. > > > > TPM_TRANSMIT_UNLOCKED > > > > > > > > > > > flag > > > > > > > > is > > > > > > > > > > abused > > > > > > > > > > > to resolve tpm spaces recursive calls to tpm_transmit(). > > > > > > > > > > > > > > > > > > > > This looks good and all but I don't think we want to > > > > > > > > > > abuse anything in the driver code, do we? > > > > > > > > > > > > > > > > > > It's not abuse just the flag UNLOCKED is not really named > > > > > > > > > correctly I think this has to be backported so wanted to > > > > > > > > > do less invasive > > > > > > change. > > > > > > > > > > > > > > > > It should be renamed anyway and possible merge conflicts are > > > > > > > > not hard to sort out in this change. Can you rename it as SPACE? > > > > > > > > > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE, > > > > > > >I'm not sure this is Do you also want to remove > > TPM_TRANSMIT_RAW? > > > > > > > clk_enable is handling its own anti recursion counter 'data- > > > > > > >clkrun_enabled' > > > > > > > but it should be all handled under one flag I guess. > > > > > > > > > > > > > > > Right, and even without rename this will probably cause > > > > > > > > merge conflicts at least in v4.4 an v4.9 since in-kernel RM > > > > > > > > landed in v4.12, so not much gain not do the rename :-) > > > > > > > > > > > > > > I belive we should do minimal change and the big cleanup after > > that. > > > > > > > Not sure, I believe UNLOCKED is still better name than SPACE > > > > > > > even it wasn't > > > > > > the original intention. > > > > > > > No the SPACE is the issue, but any recursion call into > > > > > > > tpm_transmit. A bigger change is needed and rename to SPACE > > > > > > > would be just another > > > > > > intermediat change. > > > > > > > > > > > > > > Please reconsider. > > > > > > > > > > > > > > Thanks > > > > > > > Tomas > > > > > > > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > > > > > > > > > > Does it mean you're Okay with the patch now? > > > > > Thanks > > > > > Tomas > > > > > > > > The change looks good but I'll have to test it. > > > Any updates? > > > Thanks > > > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > I've just realized we have issue in tpm_unseal_trusted() > As TPM_TRANSMIT_UNLOCKED is used really just in 'locking' sense of the flow, it's not nested. > Any of testing flows doesn't covers it. It's used only from by security/keys/trusted.c only > > Then I don't have a short fix for this issue. Will use TPM_TRANSMIT_RAW, > maybe calling it TPM_TRANSMIT_NESTED. Ah, nested would a good name for that. /Jarkko
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index e32f6e85dc6d..f802d4ee918c 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -29,7 +29,6 @@ #include <linux/mutex.h> #include <linux/spinlock.h> #include <linux/freezer.h> -#include <linux/pm_runtime.h> #include <linux/tpm_eventlog.h> #include "tpm.h" @@ -399,6 +398,28 @@ static void tpm_relinquish_locality(struct tpm_chip *chip) chip->locality = -1; } +static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags) +{ + if (flags & TPM_TRANSMIT_UNLOCKED) + return 0; + + if (!chip->ops->cmd_ready) + return 0; + + return chip->ops->cmd_ready(chip); +} + +static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags) +{ + if (flags & TPM_TRANSMIT_UNLOCKED) + return 0; + + if (!chip->ops->go_idle) + return 0; + + return chip->ops->go_idle(chip); +} + static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space, u8 *buf, size_t bufsiz, @@ -455,8 +476,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, goto out_no_locality; } - if (chip->dev.parent) - pm_runtime_get_sync(chip->dev.parent); + rc = tpm_cmd_ready(chip, flags); + if (rc) + goto out; rc = tpm2_prepare_space(chip, space, ordinal, buf); if (rc) @@ -518,8 +540,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, rc = tpm2_commit_space(chip, space, ordinal, buf, &len); out: - if (chip->dev.parent) - pm_runtime_put_sync(chip->dev.parent); + rc = tpm_go_idle(chip, flags); + if (rc) + goto out; if (need_locality) tpm_relinquish_locality(chip); diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 34fbc6cb097b..36952ef98f90 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -132,7 +132,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, } /** - * crb_go_idle - request tpm crb device to go the idle state + * __crb_go_idle - request tpm crb device to go the idle state * * @dev: crb device * @priv: crb private data @@ -147,7 +147,7 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, * * Return: 0 always */ -static int crb_go_idle(struct device *dev, struct crb_priv *priv) +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) { if ((priv->sm == ACPI_TPM2_START_METHOD) || (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) || @@ -163,11 +163,20 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv) dev_warn(dev, "goIdle timed out\n"); return -ETIME; } + return 0; } +static int crb_go_idle(struct tpm_chip *chip) +{ + struct device *dev = &chip->dev; + struct crb_priv *priv = dev_get_drvdata(dev); + + return __crb_go_idle(dev, priv); +} + /** - * crb_cmd_ready - request tpm crb device to enter ready state + * __crb_cmd_ready - request tpm crb device to enter ready state * * @dev: crb device * @priv: crb private data @@ -181,7 +190,7 @@ static int crb_go_idle(struct device *dev, struct crb_priv *priv) * * Return: 0 on success -ETIME on timeout; */ -static int crb_cmd_ready(struct device *dev, struct crb_priv *priv) +static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv) { if ((priv->sm == ACPI_TPM2_START_METHOD) || (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) || @@ -200,6 +209,14 @@ static int crb_cmd_ready(struct device *dev, struct crb_priv *priv) return 0; } +static int crb_cmd_ready(struct tpm_chip *chip) +{ + struct device *dev = &chip->dev; + struct crb_priv *priv = dev_get_drvdata(dev); + + return __crb_cmd_ready(dev, priv); +} + static int __crb_request_locality(struct device *dev, struct crb_priv *priv, int loc) { @@ -401,6 +418,8 @@ static const struct tpm_class_ops tpm_crb = { .send = crb_send, .cancel = crb_cancel, .req_canceled = crb_req_canceled, + .go_idle = crb_go_idle, + .cmd_ready = crb_cmd_ready, .request_locality = crb_request_locality, .relinquish_locality = crb_relinquish_locality, .req_complete_mask = CRB_DRV_STS_COMPLETE, @@ -520,7 +539,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, * PTT HW bug w/a: wake up the device to access * possibly not retained registers. */ - ret = crb_cmd_ready(dev, priv); + ret = __crb_cmd_ready(dev, priv); if (ret) goto out_relinquish_locality; @@ -565,7 +584,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, if (!ret) priv->cmd_size = cmd_size; - crb_go_idle(dev, priv); + __crb_go_idle(dev, priv); out_relinquish_locality: @@ -628,32 +647,7 @@ static int crb_acpi_add(struct acpi_device *device) chip->acpi_dev_handle = device->handle; chip->flags = TPM_CHIP_FLAG_TPM2; - rc = __crb_request_locality(dev, priv, 0); - if (rc) - return rc; - - rc = crb_cmd_ready(dev, priv); - if (rc) - goto out; - - pm_runtime_get_noresume(dev); - pm_runtime_set_active(dev); - pm_runtime_enable(dev); - - rc = tpm_chip_register(chip); - if (rc) { - crb_go_idle(dev, priv); - pm_runtime_put_noidle(dev); - pm_runtime_disable(dev); - goto out; - } - - pm_runtime_put_sync(dev); - -out: - __crb_relinquish_locality(dev, priv, 0); - - return rc; + return tpm_chip_register(chip); } static int crb_acpi_remove(struct acpi_device *device) @@ -663,52 +657,11 @@ static int crb_acpi_remove(struct acpi_device *device) tpm_chip_unregister(chip); - pm_runtime_disable(dev); - return 0; } -static int __maybe_unused crb_pm_runtime_suspend(struct device *dev) -{ - struct tpm_chip *chip = dev_get_drvdata(dev); - struct crb_priv *priv = dev_get_drvdata(&chip->dev); - - return crb_go_idle(dev, priv); -} - -static int __maybe_unused crb_pm_runtime_resume(struct device *dev) -{ - struct tpm_chip *chip = dev_get_drvdata(dev); - struct crb_priv *priv = dev_get_drvdata(&chip->dev); - - return crb_cmd_ready(dev, priv); -} - -static int __maybe_unused crb_pm_suspend(struct device *dev) -{ - int ret; - - ret = tpm_pm_suspend(dev); - if (ret) - return ret; - - return crb_pm_runtime_suspend(dev); -} - -static int __maybe_unused crb_pm_resume(struct device *dev) -{ - int ret; - - ret = crb_pm_runtime_resume(dev); - if (ret) - return ret; - - return tpm_pm_resume(dev); -} - 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) + SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume) }; static const struct acpi_device_id crb_device_ids[] = { diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 06639fb6ab85..8eb5e5ebe136 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -43,6 +43,8 @@ struct tpm_class_ops { u8 (*status) (struct tpm_chip *chip); bool (*update_timeouts)(struct tpm_chip *chip, unsigned long *timeout_cap); + int (*go_idle)(struct tpm_chip *chip); + int (*cmd_ready)(struct tpm_chip *chip); int (*request_locality)(struct tpm_chip *chip, int loc); int (*relinquish_locality)(struct tpm_chip *chip, int loc); void (*clk_enable)(struct tpm_chip *chip, bool value);
We cannot use go_idle cmd_ready commands via runtime_pm handles as with the introduction of localities this is no longer an optional feature, while runtime pm can be not enabled. Though cmd_ready/go_idle provides a power saving, it's also a part of TPM2 protocol and should be called explicitly. This patch exposes cmd_read/go_idle via tpm class ops and removes runtime pm support as it is not used by any driver. New wrappers are added tpm_cmd_ready() and tpm_go_idle() wrappers to streamline tpm_try_transmit code. TPM_TRANSMIT_UNLOCKED flag is abused to resolve tpm spaces recursive calls to tpm_transmit(). tpm_crb no longer needs own power saving functions and can drop using tpm_pm_suspend/resume. This patch cannot be really spearated from the locality fix. Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality) Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality) Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> --- V2:resent V3:resent V4: 1. Use tpm_pm_suspend/resume in tpm_crb 2. Drop cmd_ready/go_idle around tpm_chip_register, as there is no usage counter like in runtime_pm. V5: 1. add tpm_cmd_ready and tpm_go_idle wrappers. 2. Abuse TPM_TRANSMIT_UNLOCKED flag to resolve tpm space recursion. drivers/char/tpm/tpm-interface.c | 33 +++++++++++-- drivers/char/tpm/tpm_crb.c | 101 +++++++++++---------------------------- include/linux/tpm.h | 2 + 3 files changed, 57 insertions(+), 79 deletions(-)