[PATCH 0/3] tpm/tpm_crb: implement power management.

[PATCH 0/3] tpm/tpm_crb: implement power management.

[Tomas Winkler]

The overall platform ability to enter a low power state is also
conditioned on the ability of a tpm device to go to idle state.
This series should provide this feature. 

Unfortunately, there is a HW bug on Intel PTT devices on Skylake,
Kabylake, and Broxton devices, where certain registers lost retention
during TPM idle state. Hence this implementation takes this into
consideration and implement the feature based only on access to
registers that retain their state. This still conforms to the spec
and should be correct also on non Intle devices.

This path series should be applied on top of the series:
'Small fixes and cleanups for tpm_crb'

Tomas Winkler (3):
  tpm/tpm_crb: implement tpm crb idle state
  tmp/tpm_crb: fix Intel PTT hw bug during idle state
  tpm/tpm_crb: cache cmd_size register value.

 drivers/char/tpm/tpm-interface.c |  21 +++++++
 drivers/char/tpm/tpm_crb.c       | 124 ++++++++++++++++++++++++++++++++++-----
 include/linux/tpm.h              |   3 +-
 3 files changed, 132 insertions(+), 16 deletions(-)

-- 
2.7.4


------------------------------------------------------------------------------

[PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Tomas Winkler]

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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal patch
'tpm_crb: implement power tpm crb power management'

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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));
+
+	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)
-- 
2.7.4


------------------------------------------------------------------------------

[PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Tomas Winkler]

There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT device,
where most of the registers in the control area except START, REQUEST,
CANCEL, and LOC_CTRL lost retention when the device is in the idle state.
Hence we need to bring the device to ready state before accessing the
other registers. The fix brings device to ready state before trying to read
command and response buffer addresses in order to remap the for access.

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 98a7fdfe9936..cf9aab698dfe 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -328,6 +328,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	struct list_head resources;
 	struct resource io_res;
 	struct device *dev = &device->dev;
+	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
 	u64 rsp_pa;
@@ -335,15 +336,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	int ret;
 
 	INIT_LIST_HEAD(&resources);
-	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
-				     &io_res);
+	ret = acpi_dev_get_resources(device, &resources,
+				     crb_check_resource, &io_res);
 	if (ret < 0)
 		return ret;
 	acpi_dev_free_resource_list(&resources);
 
 	if (resource_type(&io_res) != IORESOURCE_MEM) {
-		dev_err(dev,
-			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
+		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
 		return -EINVAL;
 	}
 
@@ -356,12 +356,24 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (IS_ERR(priv->cca))
 		return PTR_ERR(priv->cca);
 
-	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
-		  (u64) ioread32(&priv->cca->cmd_pa_low);
+	/*
+	 * PTT HW bug w/a: wake up the device to access
+	 * possibly not retained registers.
+	 */
+	ret = __crb_cmd_ready(dev, priv);
+	if (ret)
+		return ret;
+
+	pa_high = ioread32(&priv->cca->cmd_pa_high);
+	pa_low  = ioread32(&priv->cca->cmd_pa_low);
+	cmd_pa = ((u64)pa_high << 32) | pa_low;
 	cmd_size = ioread32(&priv->cca->cmd_size);
+
 	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
-	if (IS_ERR(priv->cmd))
-		return PTR_ERR(priv->cmd);
+	if (IS_ERR(priv->cmd)) {
+		ret = PTR_ERR(priv->cmd);
+		goto out;
+	}
 
 	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
 	rsp_pa = le64_to_cpu(rsp_pa);
@@ -369,7 +381,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	if (cmd_pa != rsp_pa) {
 		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
-		return PTR_ERR_OR_ZERO(priv->rsp);
+		ret = PTR_ERR_OR_ZERO(priv->rsp);
+		goto out;
 	}
 
 	/* According to the PTP specification, overlapping command and response
@@ -377,10 +390,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 */
 	if (cmd_size != rsp_size) {
 		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	priv->rsp = priv->cmd;
+
+out:
+	__crb_go_idle(dev, priv);
 	return 0;
 }
 
-- 
2.7.4


------------------------------------------------------------------------------

[PATCH 3/3] tpm/tpm_crb: cache cmd_size register value.

[Tomas Winkler]

Instead of expensive register access on retrieving cmd_size
on each send, save the value during initialization in the private
context. The value doesn't change.

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index cf9aab698dfe..f8c9d587029b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -81,6 +81,7 @@ struct crb_priv {
 	struct crb_control_area __iomem *cca;
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
+	u32 cmd_size;
 };
 
 /**
@@ -217,11 +218,9 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	int rc = 0;
 
-	if (len > ioread32(&priv->cca->cmd_size)) {
-		dev_err(&chip->dev,
-			"invalid command count value %x %zx\n",
-			(unsigned int) len,
-			(size_t) ioread32(&priv->cca->cmd_size));
+	if (len > priv->cmd_size) {
+		dev_err(&chip->dev, "invalid command count value %zd %d\n",
+			len, priv->cmd_size);
 		return -E2BIG;
 	}
 
@@ -374,6 +373,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		ret = PTR_ERR(priv->cmd);
 		goto out;
 	}
+	priv->cmd_size = cmd_size;
 
 	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
 	rsp_pa = le64_to_cpu(rsp_pa);
-- 
2.7.4


------------------------------------------------------------------------------

Re: [PATCH 0/3] tpm/tpm_crb: implement power management.

[Jarkko Sakkinen]

On Wed, Sep 07, 2016 at 02:32:30PM +0300, Tomas Winkler wrote:
> The overall platform ability to enter a low power state is also
> conditioned on the ability of a tpm device to go to idle state.
> This series should provide this feature. 
> 
> Unfortunately, there is a HW bug on Intel PTT devices on Skylake,
> Kabylake, and Broxton devices, where certain registers lost retention
> during TPM idle state. Hence this implementation takes this into
> consideration and implement the feature based only on access to
> registers that retain their state. This still conforms to the spec
> and should be correct also on non Intle devices.
> 
> This path series should be applied on top of the series:
> 'Small fixes and cleanups for tpm_crb'

Thanks Tomas. I'll head on to testing tomorrow.

> Tomas Winkler (3):
>   tpm/tpm_crb: implement tpm crb idle state
>   tmp/tpm_crb: fix Intel PTT hw bug during idle state
>   tpm/tpm_crb: cache cmd_size register value.
> 
>  drivers/char/tpm/tpm-interface.c |  21 +++++++
>  drivers/char/tpm/tpm_crb.c       | 124 ++++++++++++++++++++++++++++++++++-----
>  include/linux/tpm.h              |   3 +-
>  3 files changed, 132 insertions(+), 16 deletions(-)
> 
> -- 
> 2.7.4

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jason Gunthorpe]

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

------------------------------------------------------------------------------

Re: [PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Jason Gunthorpe]

On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote:
>  	INIT_LIST_HEAD(&resources);
> -	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
> -				     &io_res);
> +	ret = acpi_dev_get_resources(device, &resources,
> +				     crb_check_resource, &io_res);

Do not randomly reflow unrelated text in patches

> +	/*
> +	 * PTT HW bug w/a: wake up the device to access
> +	 * possibly not retained registers.
> +	 */
> +	ret = __crb_cmd_ready(dev, priv);
> +	if (ret)
> +		return ret;
> +
> +	pa_high = ioread32(&priv->cca->cmd_pa_high);
> +	pa_low  = ioread32(&priv->cca->cmd_pa_low);
> +	cmd_pa = ((u64)pa_high << 32) | pa_low;

Why change from the original hunk?

 -	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
 -		  (u64) ioread32(&priv->cca->cmd_pa_low);

>  	priv->rsp = priv->cmd;
> +
> +out:
> +	__crb_go_idle(dev, priv);
>  	return 0;

Nope on the return 0.. return ret I guess?

Jason

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> 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


------------------------------------------------------------------------------

Re: [PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Winkler, Tomas]

> 
> On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote:
> >  	INIT_LIST_HEAD(&resources);
> > -	ret = acpi_dev_get_resources(device, &resources,
> crb_check_resource,
> > -				     &io_res);
> > +	ret = acpi_dev_get_resources(device, &resources,
> > +				     crb_check_resource, &io_res);
> 
> Do not randomly reflow unrelated text in patches

It wasn't random, who breaks code like that... 
 
> > +	/*
> > +	 * PTT HW bug w/a: wake up the device to access
> > +	 * possibly not retained registers.
> > +	 */
> > +	ret = __crb_cmd_ready(dev, priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pa_high = ioread32(&priv->cca->cmd_pa_high);
> > +	pa_low  = ioread32(&priv->cca->cmd_pa_low);
> > +	cmd_pa = ((u64)pa_high << 32) | pa_low;
> 
> Why change from the original hunk?


This is where the bug is visible... I'll put the debug print back it might be useful. 
  
>  -	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
>  -		  (u64) ioread32(&priv->cca->cmd_pa_low);
> 
> >  	priv->rsp = priv->cmd;
> > +
> > +out:
> > +	__crb_go_idle(dev, priv);
> >  	return 0;
> 
> Nope on the return 0.. return ret I guess?

Right, good catch.



------------------------------------------------------------------------------

Re: [PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Jason Gunthorpe]

On Wed, Sep 07, 2016 at 09:21:34PM +0000, Winkler, Tomas wrote:
> 
> > 
> > On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote:
> > >  	INIT_LIST_HEAD(&resources);
> > > -	ret = acpi_dev_get_resources(device, &resources,
> > crb_check_resource,
> > > -				     &io_res);
> > > +	ret = acpi_dev_get_resources(device, &resources,
> > > +				     crb_check_resource, &io_res);
> > 
> > Do not randomly reflow unrelated text in patches
> 
> It wasn't random, who breaks code like that...

..  it has nothing to do with this patch.

., and I have no idea why you think that is better, the original is what
clang-format produces and it computes an optimal break point using
Knuth's algorithm...

> > > +	/*
> > > +	 * PTT HW bug w/a: wake up the device to access
> > > +	 * possibly not retained registers.
> > > +	 */
> > > +	ret = __crb_cmd_ready(dev, priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	pa_high = ioread32(&priv->cca->cmd_pa_high);
> > > +	pa_low  = ioread32(&priv->cca->cmd_pa_low);
> > > +	cmd_pa = ((u64)pa_high << 32) | pa_low;
> > 
> > Why change from the original hunk?
> 
> This is where the bug is visible... I'll put the debug print back it might be useful. 

I don't get it, what is the difference? read ordering?

> >  -	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> >  -		  (u64) ioread32(&priv->cca->cmd_pa_low);

Jason

------------------------------------------------------------------------------

Re: [PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Winkler, Tomas]

> 
> On Wed, Sep 07, 2016 at 09:21:34PM +0000, Winkler, Tomas wrote:
> >
> > >
> > > On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote:
> > > >  	INIT_LIST_HEAD(&resources);
> > > > -	ret = acpi_dev_get_resources(device, &resources,
> > > crb_check_resource,
> > > > -				     &io_res);
> > > > +	ret = acpi_dev_get_resources(device, &resources,
> > > > +				     crb_check_resource, &io_res);
> > >
> > > Do not randomly reflow unrelated text in patches
> >
> > It wasn't random, who breaks code like that...
> 
> ..  it has nothing to do with this patch.
> 
> ., and I have no idea why you think that is better, the original is what clang-
> format produces and it computes an optimal break point using Knuth's
> algorithm...

For me it lacks some visual symmetry, but what I can expect from an optimal algorithm :)
I've revered it already :)

> 
> > > > +	/*
> > > > +	 * PTT HW bug w/a: wake up the device to access
> > > > +	 * possibly not retained registers.
> > > > +	 */
> > > > +	ret = __crb_cmd_ready(dev, priv);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	pa_high = ioread32(&priv->cca->cmd_pa_high);
> > > > +	pa_low  = ioread32(&priv->cca->cmd_pa_low);
> > > > +	cmd_pa = ((u64)pa_high << 32) | pa_low;
> > >
> > > Why change from the original hunk?
> >
> > This is where the bug is visible... I'll put the debug print back it might be
> useful.
> 
> I don't get it, what is the difference? read ordering?

No, read order is not relevant   here, just wanted to have each register printed

dev_dbg(dev, "cmd_hi = 0x%X cmd_low = 0x%X cmd_size %d\n",
                pa_high, pa_low, cmd_size);

Tomas

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jason Gunthorpe]

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

------------------------------------------------------------------------------

Re: [PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Jason Gunthorpe]

On Wed, Sep 07, 2016 at 09:52:38PM +0000, Winkler, Tomas wrote:
> > I don't get it, what is the difference? read ordering?
> 
> No, read order is not relevant   here, just wanted to have each register printed
> 
> dev_dbg(dev, "cmd_hi = 0x%X cmd_low = 0x%X cmd_size %d\n",
>                 pa_high, pa_low, cmd_size);

Oh, well if you've dropped the dev_dbg then drop the needless change
too.

Jason

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Thursday, September 08, 2016 00:55
> To: Winkler, Tomas <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Jarkko Sakkinen
> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 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

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jason Gunthorpe]

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

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> 
> 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



------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jason Gunthorpe]

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

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Winkler, 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.

Okay,  I see now.
Tomas 

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

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

------------------------------------------------------------------------------

Re: [PATCH 3/3] tpm/tpm_crb: cache cmd_size register value.

[Jarkko Sakkinen]

On Wed, Sep 07, 2016 at 02:32:33PM +0300, Tomas Winkler wrote:
> Instead of expensive register access on retrieving cmd_size
> on each send, save the value during initialization in the private
> context. The value doesn't change.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

> ---
>  drivers/char/tpm/tpm_crb.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index cf9aab698dfe..f8c9d587029b 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -81,6 +81,7 @@ struct crb_priv {
>  	struct crb_control_area __iomem *cca;
>  	u8 __iomem *cmd;
>  	u8 __iomem *rsp;
> +	u32 cmd_size;
>  };
>  
>  /**
> @@ -217,11 +218,9 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>  	int rc = 0;
>  
> -	if (len > ioread32(&priv->cca->cmd_size)) {
> -		dev_err(&chip->dev,
> -			"invalid command count value %x %zx\n",
> -			(unsigned int) len,
> -			(size_t) ioread32(&priv->cca->cmd_size));
> +	if (len > priv->cmd_size) {
> +		dev_err(&chip->dev, "invalid command count value %zd %d\n",
> +			len, priv->cmd_size);
>  		return -E2BIG;
>  	}
>  
> @@ -374,6 +373,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  		ret = PTR_ERR(priv->cmd);
>  		goto out;
>  	}
> +	priv->cmd_size = cmd_size;
>  
>  	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
>  	rsp_pa = le64_to_cpu(rsp_pa);
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal patch
> 'tpm_crb: implement power tpm crb power management'
> 
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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

------------------------------------------------------------------------------

Re: [PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Jarkko Sakkinen]

On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote:
> There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT device,
> where most of the registers in the control area except START, REQUEST,
> CANCEL, and LOC_CTRL lost retention when the device is in the idle state.
> Hence we need to bring the device to ready state before accessing the
> other registers. The fix brings device to ready state before trying to read
> command and response buffer addresses in order to remap the for access.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

I think I would add the dev_dbg line permanently with it that you have
in responses in this thread. There's been some unrelated bugs before
where this information in klog would have been very handy.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

> ---
>  drivers/char/tpm/tpm_crb.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 98a7fdfe9936..cf9aab698dfe 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -328,6 +328,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	struct list_head resources;
>  	struct resource io_res;
>  	struct device *dev = &device->dev;
> +	u32 pa_high, pa_low;
>  	u64 cmd_pa;
>  	u32 cmd_size;
>  	u64 rsp_pa;
> @@ -335,15 +336,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	int ret;
>  
>  	INIT_LIST_HEAD(&resources);
> -	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
> -				     &io_res);
> +	ret = acpi_dev_get_resources(device, &resources,
> +				     crb_check_resource, &io_res);
>  	if (ret < 0)
>  		return ret;
>  	acpi_dev_free_resource_list(&resources);
>  
>  	if (resource_type(&io_res) != IORESOURCE_MEM) {
> -		dev_err(dev,
> -			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> +		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
>  		return -EINVAL;
>  	}
>  
> @@ -356,12 +356,24 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	if (IS_ERR(priv->cca))
>  		return PTR_ERR(priv->cca);
>  
> -	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
> -		  (u64) ioread32(&priv->cca->cmd_pa_low);
> +	/*
> +	 * PTT HW bug w/a: wake up the device to access
> +	 * possibly not retained registers.
> +	 */
> +	ret = __crb_cmd_ready(dev, priv);
> +	if (ret)
> +		return ret;
> +
> +	pa_high = ioread32(&priv->cca->cmd_pa_high);
> +	pa_low  = ioread32(&priv->cca->cmd_pa_low);
> +	cmd_pa = ((u64)pa_high << 32) | pa_low;
>  	cmd_size = ioread32(&priv->cca->cmd_size);
> +
>  	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
> -	if (IS_ERR(priv->cmd))
> -		return PTR_ERR(priv->cmd);
> +	if (IS_ERR(priv->cmd)) {
> +		ret = PTR_ERR(priv->cmd);
> +		goto out;
> +	}
>  
>  	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
>  	rsp_pa = le64_to_cpu(rsp_pa);
> @@ -369,7 +381,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  
>  	if (cmd_pa != rsp_pa) {
>  		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
> -		return PTR_ERR_OR_ZERO(priv->rsp);
> +		ret = PTR_ERR_OR_ZERO(priv->rsp);
> +		goto out;
>  	}
>  
>  	/* According to the PTP specification, overlapping command and response
> @@ -377,10 +390,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	 */
>  	if (cmd_size != rsp_size) {
>  		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out;
>  	}
>  
>  	priv->rsp = priv->cmd;
> +
> +out:
> +	__crb_go_idle(dev, priv);
>  	return 0;
>  }
>  
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal patch
> > 'tpm_crb: implement power tpm crb power management'
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  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

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org]
> Sent: Thursday, September 08, 2016 14:18
> To: Winkler, Tomas <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal
> > > patch
> > > 'tpm_crb: implement power tpm crb power management'
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  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


------------------------------------------------------------------------------

Re: [PATCH 3/3] tpm/tpm_crb: cache cmd_size register value.

[Jarkko Sakkinen]

On Thu, Sep 08, 2016 at 02:00:34PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 07, 2016 at 02:32:33PM +0300, Tomas Winkler wrote:
> > Instead of expensive register access on retrieving cmd_size
> > on each send, save the value during initialization in the private
> > context. The value doesn't change.
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

(with Broxton)

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Jarkko Sakkinen]

On Thu, Sep 08, 2016 at 02:14:58PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 07, 2016 at 02:32:32PM +0300, Tomas Winkler wrote:
> > There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT device,
> > where most of the registers in the control area except START, REQUEST,
> > CANCEL, and LOC_CTRL lost retention when the device is in the idle state.
> > Hence we need to bring the device to ready state before accessing the
> > other registers. The fix brings device to ready state before trying to read
> > command and response buffer addresses in order to remap the for access.
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> I think I would add the dev_dbg line permanently with it that you have
> in responses in this thread. There's been some unrelated bugs before
> where this information in klog would have been very handy.
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

(with Broxton PCH)

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH 1/3] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

On Thu, Sep 08, 2016 at 12:35:48PM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org]
> > Sent: Thursday, September 08, 2016 14:18
> > To: Winkler, Tomas <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> > 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal
> > > > patch
> > > > 'tpm_crb: implement power tpm crb power management'
> > > >
> > > > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  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

------------------------------------------------------------------------------

[PATCH 2/3] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Tomas Winkler]

There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT device,
where most of the registers in the control area except START, REQUEST,
CANCEL, and LOC_CTRL lost retention when the device is in the idle state.
Hence we need to bring the device to ready state before accessing the
other registers. The fix brings device to ready state before trying to read
command and response buffer addresses in order to remap the for access.

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 98a7fdfe9936..cf9aab698dfe 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -328,6 +328,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	struct list_head resources;
 	struct resource io_res;
 	struct device *dev = &device->dev;
+	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
 	u64 rsp_pa;
@@ -335,15 +336,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	int ret;
 
 	INIT_LIST_HEAD(&resources);
-	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
-				     &io_res);
+	ret = acpi_dev_get_resources(device, &resources,
+				     crb_check_resource, &io_res);
 	if (ret < 0)
 		return ret;
 	acpi_dev_free_resource_list(&resources);
 
 	if (resource_type(&io_res) != IORESOURCE_MEM) {
-		dev_err(dev,
-			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
+		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
 		return -EINVAL;
 	}
 
@@ -356,12 +356,24 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (IS_ERR(priv->cca))
 		return PTR_ERR(priv->cca);
 
-	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
-		  (u64) ioread32(&priv->cca->cmd_pa_low);
+	/*
+	 * PTT HW bug w/a: wake up the device to access
+	 * possibly not retained registers.
+	 */
+	ret = __crb_cmd_ready(dev, priv);
+	if (ret)
+		return ret;
+
+	pa_high = ioread32(&priv->cca->cmd_pa_high);
+	pa_low  = ioread32(&priv->cca->cmd_pa_low);
+	cmd_pa = ((u64)pa_high << 32) | pa_low;
 	cmd_size = ioread32(&priv->cca->cmd_size);
+
 	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
-	if (IS_ERR(priv->cmd))
-		return PTR_ERR(priv->cmd);
+	if (IS_ERR(priv->cmd)) {
+		ret = PTR_ERR(priv->cmd);
+		goto out;
+	}
 
 	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
 	rsp_pa = le64_to_cpu(rsp_pa);
@@ -369,7 +381,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	if (cmd_pa != rsp_pa) {
 		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
-		return PTR_ERR_OR_ZERO(priv->rsp);
+		ret = PTR_ERR_OR_ZERO(priv->rsp);
+		goto out;
 	}
 
 	/* According to the PTP specification, overlapping command and response
@@ -377,10 +390,14 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 */
 	if (cmd_size != rsp_size) {
 		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	priv->rsp = priv->cmd;
+
+out:
+	__crb_go_idle(dev, priv);
 	return 0;
 }
 
-- 
2.7.4


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev