[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


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

[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


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

[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

[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


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

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


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

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

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