| Message ID | 1473247953-24617-2-git-send-email-tomas.winkler@intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 07, 2016 at 02:32:31PM +0300, Tomas Winkler wrote: > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for > SW to indicate that the device can enter or should exit the idle state. > > The legacy ACPI-start (SMI + DMA) based devices do not support these > bits and the idle state management is not exposed to the host SW. > Thus, this functionality only is enabled only for a CRB start (MMIO) > based devices. > > We introduce two new callbacks for command ready and go idle for TPM CRB > device which are called across TPM transactions. Jarkko and I have been talking about higher level locking (eg to sequence a series of operations) it might make more sense to move the power management up to that layer. No sense in going to idle mode if we know another command is about to come. Are you sure this shouldn't be linked into some kind of core kernel pm scheme? Why is this different from the existing pm stuff? > +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) > +{ > +static int crb_go_idle(struct tpm_chip *chip) > +{ > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > + > + return __crb_go_idle(&chip->dev, priv); Hurm, these ugly wrappers should probably be part of the next patch, since that is what needs them. Jason ------------------------------------------------------------------------------
> On Wed, Sep 07, 2016 at 02:32:31PM +0300, Tomas Winkler wrote: > > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for > > SW to indicate that the device can enter or should exit the idle state. > > > > The legacy ACPI-start (SMI + DMA) based devices do not support these > > bits and the idle state management is not exposed to the host SW. > > Thus, this functionality only is enabled only for a CRB start (MMIO) > > based devices. > > > > We introduce two new callbacks for command ready and go idle for TPM > > CRB device which are called across TPM transactions. > > Jarkko and I have been talking about higher level locking (eg to sequence a > series of operations) it might make more sense to move the power > management up to that layer. I'm not sure this has much benefit, this would be much more error prone and would be unnecessary more complicated than just protecting the low API in one place. >No sense in going to idle mode if we know > another command is about to come. Yes you are right, this is of course one thing that comes immediately to mind, and this is indeed implemented in the FW already and also what would autosuspend feature in runtime_pm provide in via SW .... if we would use it. So shortly from the functional point this is covered, the idle state is not trashed. > Are you sure this shouldn't be linked into some kind of core kernel pm > scheme? Why is this different from the existing pm stuff? > Yes, of course I've considered runtime_pm but I've pulled from using it for now is that it has many side effects that need to be mapped first, second because of the HW bug we cannot safely detect if the device is in idle state so some more complicated book keeping would have to be implemented. Last we need a simple solution to backport. > > +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) { > > +static int crb_go_idle(struct tpm_chip *chip) { > > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > > + > > + return __crb_go_idle(&chip->dev, priv); > > Hurm, these ugly wrappers should probably be part of the next patch, since > that is what needs them. You are right it missing a bit context if you're looking into single patch, but the paradigm is used widely in the kernel. If this blocks this patch from merging I will move the code. Thanks Tomas ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 09:14:35PM +0000, Winkler, Tomas wrote: > > Jarkko and I have been talking about higher level locking (eg to sequence a > > series of operations) it might make more sense to move the power > > management up to that layer. > > I'm not sure this has much benefit, this would be much more error prone and > would be unnecessary more complicated than just protecting the low API in one place. Well, we will have the API, so it shouldn't be much risk. I guess it can change later > Yes, of course I've considered runtime_pm but I've pulled from using > it for now is that it has many side effects that need to be mapped > first, second because of the HW bug we cannot safely detect if the > device is in idle state so some more complicated book keeping would > have to be implemented. Last we need a simple solution to backport. So are you going to send a runtime_pm patch too? What is the downside to the idle state that requires it to be controlled by the host and not internally? > > > +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) { > > > +static int crb_go_idle(struct tpm_chip *chip) { > > > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > > > + > > > + return __crb_go_idle(&chip->dev, priv); > > > > Hurm, these ugly wrappers should probably be part of the next patch, since > > that is what needs them. > > You are right it missing a bit context if you're looking into single > patch, but the paradigm is used widely in the kernel. The widely used paradigm has the wrapper actually do something useful, eg lock/unlock. This does nothing useful until you realize it is needed because you don't have a chip yet in crb_map_io due to the work around. Which begs the question if this is even the right approach at all, or should be chip be allocated sooner (as tpm_tis does).. So I'd rather see it in the next patch, and I'd rather not see it at all, move the chip allocation up instead.. Jason ------------------------------------------------------------------------------
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] > Sent: Thursday, September 08, 2016 00:55 > To: Winkler, Tomas <tomas.winkler@intel.com> > Cc: tpmdd-devel@lists.sourceforge.net; Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> > Subject: Re: [tpmdd-devel] [PATCH 1/3] tpm/tpm_crb: implement tpm crb > idle state > > On Wed, Sep 07, 2016 at 09:14:35PM +0000, Winkler, Tomas wrote: > > > > Jarkko and I have been talking about higher level locking (eg to > > > sequence a series of operations) it might make more sense to move > > > the power management up to that layer. > > > > I'm not sure this has much benefit, this would be much more error > > prone and would be unnecessary more complicated than just protecting > the low API in one place. > > Well, we will have the API, so it shouldn't be much risk. I guess it can change > later Maybe I don't understand where are you heading here, anyhow I don't see siginficat benefit using ready/idle at more then one place if the hysteresis is working anyway. > > > Yes, of course I've considered runtime_pm but I've pulled from using > > it for now is that it has many side effects that need to be mapped > > first, second because of the HW bug we cannot safely detect if the > > device is in idle state so some more complicated book keeping would > > have to be implemented. Last we need a simple solution to backport. > > So are you going to send a runtime_pm patch too? What is the downside to > the idle state that requires it to be controlled by the host and not internally? > > > > > +static int __crb_go_idle(struct device *dev, struct crb_priv > > > > +*priv) { static int crb_go_idle(struct tpm_chip *chip) { > > > > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > > > > + > > > > + return __crb_go_idle(&chip->dev, priv); > > > > > > Hurm, these ugly wrappers should probably be part of the next patch, > > > since that is what needs them. > > > > You are right it missing a bit context if you're looking into single > > patch, but the paradigm is used widely in the kernel. > > The widely used paradigm has the wrapper actually do something useful, eg > lock/unlock. This does nothing useful until you realize it is needed because > you don't have a chip yet in crb_map_io due to the work around. Which begs > the question if this is even the right approach at all, or should be chip be > allocated sooner (as tpm_tis does).. Okay, not a bad idea, will require a bit more code movement. I think there is a memory leak in tpm_tis on the error path; maybe a missing call to dev_put, not sure I had just quick look. > So I'd rather see it in the next patch, and I'd rather not see it at all, move the > chip allocation up instead.. > > Jason ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 10:17:22PM +0000, Winkler, Tomas wrote: > I think there is a memory leak in tpm_tis on the error path; maybe a > missing call to dev_put, not sure I had just quick look. tpmm_alloc uses devm, so the put happens in a devm cleanup. Assuming everything still works the way it was intended :/ Jason ------------------------------------------------------------------------------
> > On Wed, Sep 07, 2016 at 10:17:22PM +0000, Winkler, Tomas wrote: > > > I think there is a memory leak in tpm_tis on the error path; maybe a > > missing call to dev_put, not sure I had just quick look. > > tpmm_alloc uses devm, so the put happens in a devm cleanup. > > Assuming everything still works the way it was intended :/ How this is triggered on the error path, if this didn't call yet to the device_add() Tomas ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 10:28:45PM +0000, Winkler, Tomas wrote: > > > > On Wed, Sep 07, 2016 at 10:17:22PM +0000, Winkler, Tomas wrote: > > > > > I think there is a memory leak in tpm_tis on the error path; maybe a > > > missing call to dev_put, not sure I had just quick look. > > > > tpmm_alloc uses devm, so the put happens in a devm cleanup. > > > > Assuming everything still works the way it was intended :/ > > How this is triggered on the error path, if this didn't call yet to > the device_add() In the chip code 'pdev' is the device passed in to the driver's probe function. devm_add_action is applied to the pdev, which is undone by devm error unwind when the driver's probe fails. Jason ------------------------------------------------------------------------------
> On Wed, Sep 07, 2016 at 10:28:45PM +0000, Winkler, Tomas wrote: > > > > > > On Wed, Sep 07, 2016 at 10:17:22PM +0000, Winkler, Tomas wrote: > > > > > > > I think there is a memory leak in tpm_tis on the error path; maybe > > > > a missing call to dev_put, not sure I had just quick look. > > > > > > tpmm_alloc uses devm, so the put happens in a devm cleanup. > > > > > > Assuming everything still works the way it was intended :/ > > > > How this is triggered on the error path, if this didn't call yet to > > the device_add() > > In the chip code 'pdev' is the device passed in to the driver's probe function. > devm_add_action is applied to the pdev, which is undone by devm error > unwind when the driver's probe fails. Okay, I see now. Tomas ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 10:15:48AM -0600, Jason Gunthorpe wrote: > On Wed, Sep 07, 2016 at 02:32:31PM +0300, Tomas Winkler wrote: > > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for > > SW to indicate that the device can enter or should exit the idle state. > > > > The legacy ACPI-start (SMI + DMA) based devices do not support these > > bits and the idle state management is not exposed to the host SW. > > Thus, this functionality only is enabled only for a CRB start (MMIO) > > based devices. > > > > We introduce two new callbacks for command ready and go idle for TPM CRB > > device which are called across TPM transactions. > > Jarkko and I have been talking about higher level locking (eg to > sequence a series of operations) it might make more sense to move the > power management up to that layer. No sense in going to idle mode if > we know another command is about to come. Maybe if TIS/FIFO driver have similar feature in future we can generalize but I generally like bottom up approach. Do things first at the low level and bump them up when it starts to make sense. I think right now what this patch set contains is sufficient from performance perspetice even if goToIdle is done for every transaction given the workloads. We can improve from this later on by using PM runtime framework and its autosuspend feature. Does this sound fair? /Jarkko ------------------------------------------------------------------------------
On Wed, Sep 07, 2016 at 02:32:31PM +0300, Tomas Winkler wrote: > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for > SW to indicate that the device can enter or should exit the idle state. > > The legacy ACPI-start (SMI + DMA) based devices do not support these > bits and the idle state management is not exposed to the host SW. > Thus, this functionality only is enabled only for a CRB start (MMIO) > based devices. > > We introduce two new callbacks for command ready and go idle for TPM CRB > device which are called across TPM transactions. > > Based on Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> oringal patch > 'tpm_crb: implement power tpm crb power management' > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > --- > drivers/char/tpm/tpm-interface.c | 21 +++++++++++ > drivers/char/tpm/tpm_crb.c | 77 ++++++++++++++++++++++++++++++++++++++++ > include/linux/tpm.h | 3 +- > 3 files changed, 100 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index fd863ff30f79..c78dca5ce7a6 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -327,6 +327,20 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, > } > EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); > > +static inline int tpm_go_idle(struct tpm_chip *chip) > +{ > + if (!chip->ops->idle) > + return 0; > + return chip->ops->idle(chip); > +} > + > +static inline int tpm_cmd_ready(struct tpm_chip *chip) > +{ > + if (!chip->ops->ready) > + return 0; > + return chip->ops->ready(chip); > +} > + > /* > * Internal kernel interface to transmit TPM commands > */ > @@ -353,6 +367,10 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > mutex_lock(&chip->tpm_mutex); > > + rc = tpm_cmd_ready(chip); > + if (rc) > + goto out; > + > rc = chip->ops->send(chip, (u8 *) buf, count); > if (rc < 0) { > dev_err(&chip->dev, > @@ -394,8 +412,11 @@ out_recv: > dev_err(&chip->dev, > "tpm_transmit: tpm_recv: error %zd\n", rc); > out: > + tpm_go_idle(chip); > + > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > mutex_unlock(&chip->tpm_mutex); > + > return rc; > } > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 82a3ccd52a3a..98a7fdfe9936 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -83,6 +83,81 @@ struct crb_priv { > u8 __iomem *rsp; > }; > > +/** > + * __crb_go_idle - write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ > + * The device should respond within TIMEOUT_C by clearing the bit. > + * Anyhow, we do not wait here as a consequent CMD_READY request > + * will be handled correctly even if idle was not completed. > + * > + * @dev: tpm device > + * @priv: crb private context > + * > + * Return: 0 always > + */ > +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) > +{ > + if (priv->flags & CRB_FL_ACPI_START) > + return 0; > + iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req); > + /* we don't really care when this settles */ > + > + return 0; > +} > + > +static int crb_go_idle(struct tpm_chip *chip) > +{ > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > + > + return __crb_go_idle(&chip->dev, priv); > +} > + > +/** > + * __crb_cmd_ready - write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ > + * and poll till the device acknowledge it by clearing the bit. > + * The device should respond within TIMEOUT_C. > + * > + * The function does nothing for devices with ACPI-start method > + * > + * @dev: tpm device > + * @priv: crb private context > + * > + * Return: 0 on success -ETIME on timeout; > + */ > +static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv) > +{ > + ktime_t stop, start; > + > + if (priv->flags & CRB_FL_ACPI_START) > + return 0; > + > + iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req); > + > + start = ktime_get(); > + stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > + do { > + if (!(ioread32(&priv->cca->req) & CRB_CTRL_REQ_CMD_READY)) { > + dev_dbg(dev, "cmdReady in %lld usecs\n", > + ktime_to_us(ktime_sub(ktime_get(), start))); > + return 0; > + } > + usleep_range(500, 1000); > + } while (ktime_before(ktime_get(), stop)); What's the problem of using wait_for_tpm_stat like: http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/7a1172b5b3cb38083ae931309db216db3c528efe /Jarkko ------------------------------------------------------------------------------
On Thu, Sep 08, 2016 at 02:11:15PM +0300, Jarkko Sakkinen wrote: > On Wed, Sep 07, 2016 at 02:32:31PM +0300, Tomas Winkler wrote: > > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for > > SW to indicate that the device can enter or should exit the idle state. > > > > The legacy ACPI-start (SMI + DMA) based devices do not support these > > bits and the idle state management is not exposed to the host SW. > > Thus, this functionality only is enabled only for a CRB start (MMIO) > > based devices. > > > > We introduce two new callbacks for command ready and go idle for TPM CRB > > device which are called across TPM transactions. > > > > Based on Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> oringal patch > > 'tpm_crb: implement power tpm crb power management' > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > --- > > drivers/char/tpm/tpm-interface.c | 21 +++++++++++ > > drivers/char/tpm/tpm_crb.c | 77 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/tpm.h | 3 +- > > 3 files changed, 100 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > index fd863ff30f79..c78dca5ce7a6 100644 > > --- a/drivers/char/tpm/tpm-interface.c > > +++ b/drivers/char/tpm/tpm-interface.c > > @@ -327,6 +327,20 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, > > } > > EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); > > > > +static inline int tpm_go_idle(struct tpm_chip *chip) > > +{ > > + if (!chip->ops->idle) > > + return 0; > > + return chip->ops->idle(chip); > > +} > > + > > +static inline int tpm_cmd_ready(struct tpm_chip *chip) > > +{ > > + if (!chip->ops->ready) > > + return 0; > > + return chip->ops->ready(chip); > > +} > > + > > /* > > * Internal kernel interface to transmit TPM commands > > */ > > @@ -353,6 +367,10 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, > > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > > mutex_lock(&chip->tpm_mutex); > > > > + rc = tpm_cmd_ready(chip); > > + if (rc) > > + goto out; > > + > > rc = chip->ops->send(chip, (u8 *) buf, count); > > if (rc < 0) { > > dev_err(&chip->dev, > > @@ -394,8 +412,11 @@ out_recv: > > dev_err(&chip->dev, > > "tpm_transmit: tpm_recv: error %zd\n", rc); > > out: > > + tpm_go_idle(chip); > > + > > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > > mutex_unlock(&chip->tpm_mutex); > > + > > return rc; > > } > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > index 82a3ccd52a3a..98a7fdfe9936 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -83,6 +83,81 @@ struct crb_priv { > > u8 __iomem *rsp; > > }; > > > > +/** > > + * __crb_go_idle - write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ > > + * The device should respond within TIMEOUT_C by clearing the bit. > > + * Anyhow, we do not wait here as a consequent CMD_READY request > > + * will be handled correctly even if idle was not completed. > > + * > > + * @dev: tpm device > > + * @priv: crb private context > > + * > > + * Return: 0 always > > + */ > > +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) > > +{ > > + if (priv->flags & CRB_FL_ACPI_START) > > + return 0; > > + iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req); > > + /* we don't really care when this settles */ > > + > > + return 0; > > +} > > + > > +static int crb_go_idle(struct tpm_chip *chip) > > +{ > > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > > + > > + return __crb_go_idle(&chip->dev, priv); > > +} > > + > > +/** > > + * __crb_cmd_ready - write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ > > + * and poll till the device acknowledge it by clearing the bit. > > + * The device should respond within TIMEOUT_C. > > + * > > + * The function does nothing for devices with ACPI-start method > > + * > > + * @dev: tpm device > > + * @priv: crb private context > > + * > > + * Return: 0 on success -ETIME on timeout; > > + */ > > +static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv) > > +{ > > + ktime_t stop, start; > > + > > + if (priv->flags & CRB_FL_ACPI_START) > > + return 0; > > + > > + iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req); > > + > > + start = ktime_get(); > > + stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > > + do { > > + if (!(ioread32(&priv->cca->req) & CRB_CTRL_REQ_CMD_READY)) { > > + dev_dbg(dev, "cmdReady in %lld usecs\n", > > + ktime_to_us(ktime_sub(ktime_get(), start))); > > + return 0; > > + } > > + usleep_range(500, 1000); > > + } while (ktime_before(ktime_get(), stop)); > > What's the problem of using wait_for_tpm_stat like: > > http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/7a1172b5b3cb38083ae931309db216db3c528efe I'm proponent for my version since it's less intrusive change and adds less ad-hoc code to the CRB driver. Would you mind if I would construct v3 with two patches from this patch set and my version with some tweaks from your code that are needed for the workaround? /Jarkko ------------------------------------------------------------------------------
> -----Original Message----- > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > Sent: Thursday, September 08, 2016 14:18 > To: Winkler, Tomas <tomas.winkler@intel.com> > Cc: tpmdd-devel@lists.sourceforge.net > Subject: Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state > > On Thu, Sep 08, 2016 at 02:11:15PM +0300, Jarkko Sakkinen wrote: > > On Wed, Sep 07, 2016 at 02:32:31PM +0300, Tomas Winkler wrote: > > > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady > > > for SW to indicate that the device can enter or should exit the idle state. > > > > > > The legacy ACPI-start (SMI + DMA) based devices do not support these > > > bits and the idle state management is not exposed to the host SW. > > > Thus, this functionality only is enabled only for a CRB start (MMIO) > > > based devices. > > > > > > We introduce two new callbacks for command ready and go idle for TPM > > > CRB device which are called across TPM transactions. > > > > > > Based on Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> oringal > > > patch > > > 'tpm_crb: implement power tpm crb power management' > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > --- > > > drivers/char/tpm/tpm-interface.c | 21 +++++++++++ > > > drivers/char/tpm/tpm_crb.c | 77 > ++++++++++++++++++++++++++++++++++++++++ > > > include/linux/tpm.h | 3 +- > > > 3 files changed, 100 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > b/drivers/char/tpm/tpm-interface.c > > > index fd863ff30f79..c78dca5ce7a6 100644 > > > --- a/drivers/char/tpm/tpm-interface.c > > > +++ b/drivers/char/tpm/tpm-interface.c > > > @@ -327,6 +327,20 @@ unsigned long tpm_calc_ordinal_duration(struct > > > tpm_chip *chip, } EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); > > > > > > +static inline int tpm_go_idle(struct tpm_chip *chip) { > > > + if (!chip->ops->idle) > > > + return 0; > > > + return chip->ops->idle(chip); > > > +} > > > + > > > +static inline int tpm_cmd_ready(struct tpm_chip *chip) { > > > + if (!chip->ops->ready) > > > + return 0; > > > + return chip->ops->ready(chip); > > > +} > > > + > > > /* > > > * Internal kernel interface to transmit TPM commands > > > */ > > > @@ -353,6 +367,10 @@ ssize_t tpm_transmit(struct tpm_chip *chip, > const u8 *buf, size_t bufsiz, > > > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > > > mutex_lock(&chip->tpm_mutex); > > > > > > + rc = tpm_cmd_ready(chip); > > > + if (rc) > > > + goto out; > > > + > > > rc = chip->ops->send(chip, (u8 *) buf, count); > > > if (rc < 0) { > > > dev_err(&chip->dev, > > > @@ -394,8 +412,11 @@ out_recv: > > > dev_err(&chip->dev, > > > "tpm_transmit: tpm_recv: error %zd\n", rc); > > > out: > > > + tpm_go_idle(chip); > > > + > > > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > > > mutex_unlock(&chip->tpm_mutex); > > > + > > > return rc; > > > } > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > > index 82a3ccd52a3a..98a7fdfe9936 100644 > > > --- a/drivers/char/tpm/tpm_crb.c > > > +++ b/drivers/char/tpm/tpm_crb.c > > > @@ -83,6 +83,81 @@ struct crb_priv { > > > u8 __iomem *rsp; > > > }; > > > > > > +/** > > > + * __crb_go_idle - write CRB_CTRL_REQ_GO_IDLE to > TPM_CRB_CTRL_REQ > > > + * The device should respond within TIMEOUT_C by clearing the bit. > > > + * Anyhow, we do not wait here as a consequent CMD_READY request > > > + * will be handled correctly even if idle was not completed. > > > + * > > > + * @dev: tpm device > > > + * @priv: crb private context > > > + * > > > + * Return: 0 always > > > + */ > > > +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) > > > +{ > > > + if (priv->flags & CRB_FL_ACPI_START) > > > + return 0; > > > + iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req); > > > + /* we don't really care when this settles */ > > > + > > > + return 0; > > > +} > > > + > > > +static int crb_go_idle(struct tpm_chip *chip) { > > > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > > > + > > > + return __crb_go_idle(&chip->dev, priv); } > > > + > > > +/** > > > + * __crb_cmd_ready - write CRB_CTRL_REQ_CMD_READY to > TPM_CRB_CTRL_REQ > > > + * and poll till the device acknowledge it by clearing the bit. > > > + * The device should respond within TIMEOUT_C. > > > + * > > > + * The function does nothing for devices with ACPI-start method > > > + * > > > + * @dev: tpm device > > > + * @priv: crb private context > > > + * > > > + * Return: 0 on success -ETIME on timeout; */ static int > > > +__crb_cmd_ready(struct device *dev, struct crb_priv *priv) { > > > + ktime_t stop, start; > > > + > > > + if (priv->flags & CRB_FL_ACPI_START) > > > + return 0; > > > + > > > + iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req); > > > + > > > + start = ktime_get(); > > > + stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > > > + do { > > > + if (!(ioread32(&priv->cca->req) & > CRB_CTRL_REQ_CMD_READY)) { > > > + dev_dbg(dev, "cmdReady in %lld usecs\n", > > > + ktime_to_us(ktime_sub(ktime_get(), start))); > > > + return 0; > > > + } > > > + usleep_range(500, 1000); > > > + } while (ktime_before(ktime_get(), stop)); > > > > What's the problem of using wait_for_tpm_stat like: > > > > http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/7a1172b5 > > b3cb38083ae931309db216db3c528efe > > I'm proponent for my version since it's less intrusive change and adds less > ad-hoc code to the CRB driver. I'm not sure what is adhoc about this code, this keeps the issue local to crb . I removed the use of wait_for_tpm_stat() for purpose, this just abusing this interface for something which is not 'stats' in addition you are setting rubbish from the CA_STATUS as its value is not retained and on the other side the values of CA_REQUEST is read from edge (from 1 to 0) so there will be rubbish collected when we are not in transition. Last we need to fine tune polling delay for the HW we cannot have it in the generic code. > > Would you mind if I would construct v3 with two patches from this patch set > and my version with some tweaks from your code that are needed for the > workaround? I'm already reworking the code according last night discussion with Jason, please hold on. Thanks Tomas ------------------------------------------------------------------------------
On Thu, Sep 08, 2016 at 12:35:48PM +0000, Winkler, Tomas wrote: > > > > -----Original Message----- > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > > Sent: Thursday, September 08, 2016 14:18 > > To: Winkler, Tomas <tomas.winkler@intel.com> > > Cc: tpmdd-devel@lists.sourceforge.net > > Subject: Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state > > > > On Thu, Sep 08, 2016 at 02:11:15PM +0300, Jarkko Sakkinen wrote: > > > On Wed, Sep 07, 2016 at 02:32:31PM +0300, Tomas Winkler wrote: > > > > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady > > > > for SW to indicate that the device can enter or should exit the idle state. > > > > > > > > The legacy ACPI-start (SMI + DMA) based devices do not support these > > > > bits and the idle state management is not exposed to the host SW. > > > > Thus, this functionality only is enabled only for a CRB start (MMIO) > > > > based devices. > > > > > > > > We introduce two new callbacks for command ready and go idle for TPM > > > > CRB device which are called across TPM transactions. > > > > > > > > Based on Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> oringal > > > > patch > > > > 'tpm_crb: implement power tpm crb power management' > > > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > > --- > > > > drivers/char/tpm/tpm-interface.c | 21 +++++++++++ > > > > drivers/char/tpm/tpm_crb.c | 77 > > ++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/tpm.h | 3 +- > > > > 3 files changed, 100 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > > b/drivers/char/tpm/tpm-interface.c > > > > index fd863ff30f79..c78dca5ce7a6 100644 > > > > --- a/drivers/char/tpm/tpm-interface.c > > > > +++ b/drivers/char/tpm/tpm-interface.c > > > > @@ -327,6 +327,20 @@ unsigned long tpm_calc_ordinal_duration(struct > > > > tpm_chip *chip, } EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); > > > > > > > > +static inline int tpm_go_idle(struct tpm_chip *chip) { > > > > + if (!chip->ops->idle) > > > > + return 0; > > > > + return chip->ops->idle(chip); > > > > +} > > > > + > > > > +static inline int tpm_cmd_ready(struct tpm_chip *chip) { > > > > + if (!chip->ops->ready) > > > > + return 0; > > > > + return chip->ops->ready(chip); > > > > +} > > > > + > > > > /* > > > > * Internal kernel interface to transmit TPM commands > > > > */ > > > > @@ -353,6 +367,10 @@ ssize_t tpm_transmit(struct tpm_chip *chip, > > const u8 *buf, size_t bufsiz, > > > > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > > > > mutex_lock(&chip->tpm_mutex); > > > > > > > > + rc = tpm_cmd_ready(chip); > > > > + if (rc) > > > > + goto out; > > > > + > > > > rc = chip->ops->send(chip, (u8 *) buf, count); > > > > if (rc < 0) { > > > > dev_err(&chip->dev, > > > > @@ -394,8 +412,11 @@ out_recv: > > > > dev_err(&chip->dev, > > > > "tpm_transmit: tpm_recv: error %zd\n", rc); > > > > out: > > > > + tpm_go_idle(chip); > > > > + > > > > if (!(flags & TPM_TRANSMIT_UNLOCKED)) > > > > mutex_unlock(&chip->tpm_mutex); > > > > + > > > > return rc; > > > > } > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > > > index 82a3ccd52a3a..98a7fdfe9936 100644 > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > @@ -83,6 +83,81 @@ struct crb_priv { > > > > u8 __iomem *rsp; > > > > }; > > > > > > > > +/** > > > > + * __crb_go_idle - write CRB_CTRL_REQ_GO_IDLE to > > TPM_CRB_CTRL_REQ > > > > + * The device should respond within TIMEOUT_C by clearing the bit. > > > > + * Anyhow, we do not wait here as a consequent CMD_READY request > > > > + * will be handled correctly even if idle was not completed. > > > > + * > > > > + * @dev: tpm device > > > > + * @priv: crb private context > > > > + * > > > > + * Return: 0 always > > > > + */ > > > > +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) > > > > +{ > > > > + if (priv->flags & CRB_FL_ACPI_START) > > > > + return 0; > > > > + iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req); > > > > + /* we don't really care when this settles */ > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int crb_go_idle(struct tpm_chip *chip) { > > > > + struct crb_priv *priv = dev_get_drvdata(&chip->dev); > > > > + > > > > + return __crb_go_idle(&chip->dev, priv); } > > > > + > > > > +/** > > > > + * __crb_cmd_ready - write CRB_CTRL_REQ_CMD_READY to > > TPM_CRB_CTRL_REQ > > > > + * and poll till the device acknowledge it by clearing the bit. > > > > + * The device should respond within TIMEOUT_C. > > > > + * > > > > + * The function does nothing for devices with ACPI-start method > > > > + * > > > > + * @dev: tpm device > > > > + * @priv: crb private context > > > > + * > > > > + * Return: 0 on success -ETIME on timeout; */ static int > > > > +__crb_cmd_ready(struct device *dev, struct crb_priv *priv) { > > > > + ktime_t stop, start; > > > > + > > > > + if (priv->flags & CRB_FL_ACPI_START) > > > > + return 0; > > > > + > > > > + iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req); > > > > + > > > > + start = ktime_get(); > > > > + stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > > > > + do { > > > > + if (!(ioread32(&priv->cca->req) & > > CRB_CTRL_REQ_CMD_READY)) { > > > > + dev_dbg(dev, "cmdReady in %lld usecs\n", > > > > + ktime_to_us(ktime_sub(ktime_get(), start))); > > > > + return 0; > > > > + } > > > > + usleep_range(500, 1000); > > > > + } while (ktime_before(ktime_get(), stop)); > > > > > > What's the problem of using wait_for_tpm_stat like: > > > > > > http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/7a1172b5 > > > b3cb38083ae931309db216db3c528efe > > > > I'm proponent for my version since it's less intrusive change and adds less > > ad-hoc code to the CRB driver. > > I'm not sure what is adhoc about this code, this keeps the issue local > to crb . I removed the use of wait_for_tpm_stat() for purpose, this > just abusing this interface for something which is not 'stats' in > addition you are setting rubbish from the CA_STATUS as its value is > not retained and on the other side the values of CA_REQUEST is read > from edge (from 1 to 0) so there will be rubbish collected when we > are not in transition. Last we need to fine tune polling delay for the > HW we cannot have it in the generic code. Well we anyway use a synthetized status value for the CRB driver so I don't see your point. And wait_for_tpm_stat() allows you to specify any delay. For me this looks just more duplicate code to maintain. /Jarkko ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index fd863ff30f79..c78dca5ce7a6 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -327,6 +327,20 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, } EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); +static inline int tpm_go_idle(struct tpm_chip *chip) +{ + if (!chip->ops->idle) + return 0; + return chip->ops->idle(chip); +} + +static inline int tpm_cmd_ready(struct tpm_chip *chip) +{ + if (!chip->ops->ready) + return 0; + return chip->ops->ready(chip); +} + /* * Internal kernel interface to transmit TPM commands */ @@ -353,6 +367,10 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, if (!(flags & TPM_TRANSMIT_UNLOCKED)) mutex_lock(&chip->tpm_mutex); + rc = tpm_cmd_ready(chip); + if (rc) + goto out; + rc = chip->ops->send(chip, (u8 *) buf, count); if (rc < 0) { dev_err(&chip->dev, @@ -394,8 +412,11 @@ out_recv: dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %zd\n", rc); out: + tpm_go_idle(chip); + if (!(flags & TPM_TRANSMIT_UNLOCKED)) mutex_unlock(&chip->tpm_mutex); + return rc; } diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 82a3ccd52a3a..98a7fdfe9936 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -83,6 +83,81 @@ struct crb_priv { u8 __iomem *rsp; }; +/** + * __crb_go_idle - write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ + * The device should respond within TIMEOUT_C by clearing the bit. + * Anyhow, we do not wait here as a consequent CMD_READY request + * will be handled correctly even if idle was not completed. + * + * @dev: tpm device + * @priv: crb private context + * + * Return: 0 always + */ +static int __crb_go_idle(struct device *dev, struct crb_priv *priv) +{ + if (priv->flags & CRB_FL_ACPI_START) + return 0; + iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req); + /* we don't really care when this settles */ + + return 0; +} + +static int crb_go_idle(struct tpm_chip *chip) +{ + struct crb_priv *priv = dev_get_drvdata(&chip->dev); + + return __crb_go_idle(&chip->dev, priv); +} + +/** + * __crb_cmd_ready - write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ + * and poll till the device acknowledge it by clearing the bit. + * The device should respond within TIMEOUT_C. + * + * The function does nothing for devices with ACPI-start method + * + * @dev: tpm device + * @priv: crb private context + * + * Return: 0 on success -ETIME on timeout; + */ +static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv) +{ + ktime_t stop, start; + + if (priv->flags & CRB_FL_ACPI_START) + return 0; + + iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req); + + start = ktime_get(); + stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); + do { + if (!(ioread32(&priv->cca->req) & CRB_CTRL_REQ_CMD_READY)) { + dev_dbg(dev, "cmdReady in %lld usecs\n", + ktime_to_us(ktime_sub(ktime_get(), start))); + return 0; + } + usleep_range(500, 1000); + } while (ktime_before(ktime_get(), stop)); + + if (ioread32(&priv->cca->req) & CRB_CTRL_REQ_CMD_READY) { + dev_warn(dev, "cmdReady timed out\n"); + return -ETIME; + } + + return 0; +} + +static int crb_cmd_ready(struct tpm_chip *chip) +{ + struct crb_priv *priv = dev_get_drvdata(&chip->dev); + + return __crb_cmd_ready(&chip->dev, priv); +} + static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume); static u8 crb_status(struct tpm_chip *chip) @@ -193,6 +268,8 @@ static const struct tpm_class_ops tpm_crb = { .recv = crb_recv, .send = crb_send, .cancel = crb_cancel, + .ready = crb_cmd_ready, + .idle = crb_go_idle, .req_canceled = crb_req_canceled, .req_complete_mask = CRB_DRV_STS_COMPLETE, .req_complete_val = CRB_DRV_STS_COMPLETE, diff --git a/include/linux/tpm.h b/include/linux/tpm.h index da158f06e0b2..1ed9ff6a5d48 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -48,7 +48,8 @@ struct tpm_class_ops { u8 (*status) (struct tpm_chip *chip); bool (*update_timeouts)(struct tpm_chip *chip, unsigned long *timeout_cap); - + int (*idle)(struct tpm_chip *chip); + int (*ready)(struct tpm_chip *chip); }; #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for SW to indicate that the device can enter or should exit the idle state. The legacy ACPI-start (SMI + DMA) based devices do not support these bits and the idle state management is not exposed to the host SW. Thus, this functionality only is enabled only for a CRB start (MMIO) based devices. We introduce two new callbacks for command ready and go idle for TPM CRB device which are called across TPM transactions. Based on Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> oringal patch 'tpm_crb: implement power tpm crb power management' Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> --- drivers/char/tpm/tpm-interface.c | 21 +++++++++++ drivers/char/tpm/tpm_crb.c | 77 ++++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 3 +- 3 files changed, 100 insertions(+), 1 deletion(-)