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

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

[Tomas Winkler]

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

V2: Utilize runtime_pm for driving tpm crb idle states.
V3. fix lower case corruption in the first patch
V4. Fix unbalanced runtime pm get and put

Tomas Winkler (4):
  tpm/tpm_crb: implement tpm crb idle state
  tmp/tpm_crb: fix Intel PTT hw bug during idle state
  tpm/tpm_crb: open code the crb_init into acpi_add
  tmp/tpm_crb: implement runtime pm for tpm_crb

 drivers/char/tpm/tpm-interface.c |   5 ++
 drivers/char/tpm/tpm_crb.c       | 166 +++++++++++++++++++++++++++++++++------
 2 files changed, 147 insertions(+), 24 deletions(-)

-- 
2.7.4

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

[Tomas Winkler]

From: "Winkler, Tomas" <tomas.winkler@intel.com>

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.

Based on Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
original patch:
'tpm_crb: implement power tpm crb power management'

To keep the implementation local to the hw we don't use wait_for_tpm_stat
for polling the TPM_CRB_CTRL_REQ.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
V2: do not export the functions via tpm ops
V3: fix lower case corruption; adjust function documentation
V4: resend

 drivers/char/tpm/tpm_crb.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index a7c870af916c..0f3b3f3d12d3 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -83,6 +83,76 @@ struct crb_priv {
 	u32 cmd_size;
 };
 
+/**
+ * crb_go_idle - request tpm crb device to go the idle state
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ *
+ * 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.
+ *
+ * The function does nothing for devices with ACPI-start method.
+ *
+ * Return: 0 always
+ */
+static int __maybe_unused 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 */
+	dev_dbg(dev, "goIdle\n");
+
+	return 0;
+}
+
+/**
+ * crb_cmd_ready - request tpm crb device to enter ready state
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ *
+ * 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
+ *
+ * Return: 0 on success -ETIME on timeout;
+ */
+static int __maybe_unused 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(50, 100);
+	} 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 SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
 
 static u8 crb_status(struct tpm_chip *chip)
-- 
2.7.4

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

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 02:59:36PM +0300, Tomas Winkler wrote:
> From: "Winkler, Tomas" <tomas.winkler@intel.com>
> 
> 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.
> 
> Based on Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> original patch:
> 'tpm_crb: implement power tpm crb power management'
> 
> To keep the implementation local to the hw we don't use wait_for_tpm_stat
> for polling the TPM_CRB_CTRL_REQ.

I just realized that the HW specific argument does not hold at all.  The
tpm_tis_core heavily uses wait_for_tpm_stat and return value of status
is synthetized.

It's upper layer for FIFO implemetations but still the arguement still
holds.

Lets keep it the way it is for now because this is fairly well test but
I might consider sending patch  that migrates to wait_for_tpm_stat unless
there is no real reason not to do so.

/Jarkko

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

[Tomas Winkler]

From: "Winkler, Tomas" <tomas.winkler@intel.com>

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@intel.com>
---
V2: cmd read need to be called also before crb_init as this will run
 self test.
V3: resend.
V4: add Kabylake to the list of effected platforms

 drivers/char/tpm/tpm_crb.c | 47 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 0f3b3f3d12d3..4eb96b85c653 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -319,6 +319,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;
@@ -346,12 +347,27 @@ 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);
+
+	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
+		pa_high, pa_low, 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);
@@ -359,7 +375,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
@@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
 
 	priv->rsp = priv->cmd;
-	return 0;
+
+out:
+	crb_go_idle(dev, priv);
+
+	return ret;
 }
 
 static int crb_acpi_add(struct acpi_device *device)
@@ -416,7 +439,15 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
-	return crb_init(device, priv);
+	rc  = crb_cmd_ready(dev, priv);
+	if (rc)
+		return rc;
+
+	rc = crb_init(device, priv);
+	if (rc)
+		crb_go_idle(dev, priv);
+
+	return rc;
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
-- 
2.7.4

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

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote:
> From: "Winkler, Tomas" <tomas.winkler@intel.com>
> 
> 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@intel.com>
> ---
> V2: cmd read need to be called also before crb_init as this will run
>  self test.
> V3: resend.
> V4: add Kabylake to the list of effected platforms
> 
>  drivers/char/tpm/tpm_crb.c | 47 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 0f3b3f3d12d3..4eb96b85c653 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -319,6 +319,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;
> @@ -346,12 +347,27 @@ 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);
> +
> +	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
> +		pa_high, pa_low, 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);
> @@ -359,7 +375,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
> @@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
>  
>  	priv->rsp = priv->cmd;
> -	return 0;
> +
> +out:
> +	crb_go_idle(dev, priv);
> +
> +	return ret;
>  }
>  
>  static int crb_acpi_add(struct acpi_device *device)
> @@ -416,7 +439,15 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (rc)
>  		return rc;
>  
> -	return crb_init(device, priv);
> +	rc  = crb_cmd_ready(dev, priv);
> +	if (rc)
> +		return rc;

I cannot find any valid reason why crb_map_io calls crb_go_idle in the
except in the case of a failure. This is something I complained earlier.

A minor thing but the extra crb_cmd_ready is basically clutter to the
initialization.

/Jarkko

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

[Winkler, Tomas]

> 
> On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote:
> > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> >
> > 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@intel.com>
> > ---
> > V2: cmd read need to be called also before crb_init as this will run
> > self test.
> > V3: resend.
> > V4: add Kabylake to the list of effected platforms
> >
> >  drivers/char/tpm/tpm_crb.c | 47
> > ++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 39 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 0f3b3f3d12d3..4eb96b85c653 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -319,6 +319,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;
> > @@ -346,12 +347,27 @@ 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);
> > +
> > +	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
> > +		pa_high, pa_low, 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);
> > @@ -359,7 +375,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 @@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
> >
> >  	priv->rsp = priv->cmd;
> > -	return 0;
> > +
> > +out:
> > +	crb_go_idle(dev, priv);
> > +
> > +	return ret;
> >  }
> >
> >  static int crb_acpi_add(struct acpi_device *device) @@ -416,7 +439,15
> > @@ static int crb_acpi_add(struct acpi_device *device)
> >  	if (rc)
> >  		return rc;
> >
> > -	return crb_init(device, priv);
> > +	rc  = crb_cmd_ready(dev, priv);
> > +	if (rc)
> > +		return rc;
> 
> I cannot find any valid reason why crb_map_io calls crb_go_idle in the except
> in the case of a failure. This is something I complained earlier.

Haven't I've already explained that?  Each flow has to be enclosed by cmdReady and goIdle.  There is nothing different here from the other flows in that matter. 
The assumption here is that we are starting in the idle state bug because of the HW bug we cannot access the registers. So the whole w/o is enclosed in the crb_map_io. 
After that we are starting from new, from the idle state. 


> A minor thing but the extra crb_cmd_ready is basically clutter to the
> initialization.

The extra cmdReady is here in case runtime pm is not compiled into the kernel and assumption here is that we are starting in the idle state.
Please remember that unfortunately we cannot detect whether we are in ready on idle state as the status register is not retained, so everything has to be ordered properly. 

Thanks
Tomas

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

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote:
> > 
> > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote:
> > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > >
> > > 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@intel.com>
> > > ---
> > > V2: cmd read need to be called also before crb_init as this will run
> > > self test.
> > > V3: resend.
> > > V4: add Kabylake to the list of effected platforms
> > >
> > >  drivers/char/tpm/tpm_crb.c | 47
> > > ++++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 39 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index 0f3b3f3d12d3..4eb96b85c653 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -319,6 +319,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;
> > > @@ -346,12 +347,27 @@ 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);
> > > +
> > > +	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
> > > +		pa_high, pa_low, 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);
> > > @@ -359,7 +375,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 @@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
> > >
> > >  	priv->rsp = priv->cmd;
> > > -	return 0;
> > > +
> > > +out:
> > > +	crb_go_idle(dev, priv);
> > > +
> > > +	return ret;
> > >  }
> > >
> > >  static int crb_acpi_add(struct acpi_device *device) @@ -416,7 +439,15
> > > @@ static int crb_acpi_add(struct acpi_device *device)
> > >  	if (rc)
> > >  		return rc;
> > >
> > > -	return crb_init(device, priv);
> > > +	rc  = crb_cmd_ready(dev, priv);
> > > +	if (rc)
> > > +		return rc;
> > 
> > I cannot find any valid reason why crb_map_io calls crb_go_idle in the except
> > in the case of a failure. This is something I complained earlier.
> 
> Haven't I've already explained that?  Each flow has to be enclosed by
> cmdReady and goIdle.  There is nothing different here from the other
> flows in that matter.  The assumption here is that we are starting in
> the idle state bug because of the HW bug we cannot access the
> registers. So the whole w/o is enclosed in the crb_map_io.  After that
> we are starting from new, from the idle state. 

But why you have to do crb_go_idle upon returning from crb_map_io()?

> > A minor thing but the extra crb_cmd_ready is basically clutter to the
> > initialization.
> 
> The extra cmdReady is here in case runtime pm is not compiled into the
> kernel and assumption here is that we are starting in the idle state.
> Please remember that unfortunately we cannot detect whether we are in
> ready on idle state as the status register is not retained, so
> everything has to be ordered properly. 

Rather you it would be better not to use cmdReady/goIdle at all if
kernel is not compiled with CONFIG_PM.

> Thanks
> Tomas

/Jarkko

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

[Jason Gunthorpe]

On Sat, Oct 08, 2016 at 07:00:06PM +0300, Jarkko Sakkinen wrote:

> Rather you it would be better not to use cmdReady/goIdle at all if
> kernel is not compiled with CONFIG_PM.

It would be best practice to ensure the tpm is powered up no matter
how the kernel is compiled.. The firmware may have left it in idle or
something

FWIW, I broadly agree there is no need to put the device into idle on
the error paths. It probably wasn't idle when the driver attached to
it.

Jason

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

[Winkler, Tomas]

> On Sat, Oct 08, 2016 at 07:00:06PM +0300, Jarkko Sakkinen wrote:
> 
> > Rather you it would be better not to use cmdReady/goIdle at all if
> > kernel is not compiled with CONFIG_PM.
> 
> It would be best practice to ensure the tpm is powered up no matter how the
> kernel is compiled.. The firmware may have left it in idle or something
> 
> FWIW, I broadly agree there is no need to put the device into idle on the error
> paths. It probably wasn't idle when the driver attached to it.

Actually, currently it is more common to have the device in the idle state, so in case a driver is not loaded for any reason  by the kernel, the platform won't leak power.
Thanks
Tomas

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

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 10:42:03AM -0600, Jason Gunthorpe wrote:
> On Sat, Oct 08, 2016 at 07:00:06PM +0300, Jarkko Sakkinen wrote:
> 
> > Rather you it would be better not to use cmdReady/goIdle at all if
> > kernel is not compiled with CONFIG_PM.
> 
> It would be best practice to ensure the tpm is powered up no matter
> how the kernel is compiled.. The firmware may have left it in idle or
> something
> 
> FWIW, I broadly agree there is no need to put the device into idle on
> the error paths. It probably wasn't idle when the driver attached to
> it.

I'll keep the current commit because it's anyway tested code.

If I want to change this I'll send a separate patch for review.

> Jason

/Jarkko

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

[Winkler, Tomas]

> 
> On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote:
> > >
> > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote:
> > > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > > >
> > > > 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@intel.com>
> > > > ---
> > > > V2: cmd read need to be called also before crb_init as this will
> > > > run self test.
> > > > V3: resend.
> > > > V4: add Kabylake to the list of effected platforms
> > > >
> > > >  drivers/char/tpm/tpm_crb.c | 47
> > > > ++++++++++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 39 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > b/drivers/char/tpm/tpm_crb.c index 0f3b3f3d12d3..4eb96b85c653
> > > > 100644
> > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > @@ -319,6 +319,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;
> > > > @@ -346,12 +347,27 @@ 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);
> > > > +
> > > > +	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
> > > > +		pa_high, pa_low, 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);
> > > > @@ -359,7 +375,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 @@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
> > > >
> > > >  	priv->rsp = priv->cmd;
> > > > -	return 0;
> > > > +
> > > > +out:
> > > > +	crb_go_idle(dev, priv);
> > > > +
> > > > +	return ret;
> > > >  }
> > > >
> > > >  static int crb_acpi_add(struct acpi_device *device) @@ -416,7
> > > > +439,15 @@ static int crb_acpi_add(struct acpi_device *device)
> > > >  	if (rc)
> > > >  		return rc;
> > > >
> > > > -	return crb_init(device, priv);
> > > > +	rc  = crb_cmd_ready(dev, priv);
> > > > +	if (rc)
> > > > +		return rc;
> > >
> > > I cannot find any valid reason why crb_map_io calls crb_go_idle in
> > > the except in the case of a failure. This is something I complained earlier.
> >
> > Haven't I've already explained that?  Each flow has to be enclosed by
> > cmdReady and goIdle.  There is nothing different here from the other
> > flows in that matter.  The assumption here is that we are starting in
> > the idle state bug because of the HW bug we cannot access the
> > registers. So the whole w/o is enclosed in the crb_map_io.  After that
> > we are starting from new, from the idle state.
> 
> But why you have to do crb_go_idle upon returning from crb_map_io()?

As written above it's is closed code... cmdReady  <perform the task>   goIdle. 

> 
> > > A minor thing but the extra crb_cmd_ready is basically clutter to
> > > the initialization.
> >
> > The extra cmdReady is here in case runtime pm is not compiled into the
> > kernel and assumption here is that we are starting in the idle state.
> > Please remember that unfortunately we cannot detect whether we are in
> > ready on idle state as the status register is not retained, so
> > everything has to be ordered properly.
> 
> Rather you it would be better not to use cmdReady/goIdle at all if kernel is not
> compiled with CONFIG_PM.

????. You have to do the initial cmdReady, and that's what the code does, otherwise your driver won't work at all. 
CONFIG_PM is the software thing, it doesn't magically match the HW power state. So you need to do cmdReady. 

Thanks
Tomas

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

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 04:56:39PM +0000, Winkler, Tomas wrote:
> 
> > 
> > On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote:
> > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > > > >
> > > > > 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@intel.com>
> > > > > ---
> > > > > V2: cmd read need to be called also before crb_init as this will
> > > > > run self test.
> > > > > V3: resend.
> > > > > V4: add Kabylake to the list of effected platforms
> > > > >
> > > > >  drivers/char/tpm/tpm_crb.c | 47
> > > > > ++++++++++++++++++++++++++++++++++++++--------
> > > > >  1 file changed, 39 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > b/drivers/char/tpm/tpm_crb.c index 0f3b3f3d12d3..4eb96b85c653
> > > > > 100644
> > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > @@ -319,6 +319,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;
> > > > > @@ -346,12 +347,27 @@ 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);
> > > > > +
> > > > > +	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
> > > > > +		pa_high, pa_low, 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);
> > > > > @@ -359,7 +375,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 @@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
> > > > >
> > > > >  	priv->rsp = priv->cmd;
> > > > > -	return 0;
> > > > > +
> > > > > +out:
> > > > > +	crb_go_idle(dev, priv);
> > > > > +
> > > > > +	return ret;
> > > > >  }
> > > > >
> > > > >  static int crb_acpi_add(struct acpi_device *device) @@ -416,7
> > > > > +439,15 @@ static int crb_acpi_add(struct acpi_device *device)
> > > > >  	if (rc)
> > > > >  		return rc;
> > > > >
> > > > > -	return crb_init(device, priv);
> > > > > +	rc  = crb_cmd_ready(dev, priv);
> > > > > +	if (rc)
> > > > > +		return rc;
> > > >
> > > > I cannot find any valid reason why crb_map_io calls crb_go_idle in
> > > > the except in the case of a failure. This is something I complained earlier.
> > >
> > > Haven't I've already explained that?  Each flow has to be enclosed by
> > > cmdReady and goIdle.  There is nothing different here from the other
> > > flows in that matter.  The assumption here is that we are starting in
> > > the idle state bug because of the HW bug we cannot access the
> > > registers. So the whole w/o is enclosed in the crb_map_io.  After that
> > > we are starting from new, from the idle state.
> > 
> > But why you have to do crb_go_idle upon returning from crb_map_io()?
> 
> As written above it's is closed code... cmdReady  <perform the task>   goIdle. 

It does extra cmdReady/goIdle. I don't really get your explanation
here. crb_map_io() is not a utility function used after the driver
has initialized. 

crb_map_io() could just leave it ready so you don't have to do an
explicit call to crb_cmd_ready() in crb_acpi_add. I think this is
quite obvious.

> > 
> > > > A minor thing but the extra crb_cmd_ready is basically clutter to
> > > > the initialization.
> > >
> > > The extra cmdReady is here in case runtime pm is not compiled into the
> > > kernel and assumption here is that we are starting in the idle state.
> > > Please remember that unfortunately we cannot detect whether we are in
> > > ready on idle state as the status register is not retained, so
> > > everything has to be ordered properly.
> > 
> > Rather you it would be better not to use cmdReady/goIdle at all if kernel is not
> > compiled with CONFIG_PM.
> 
> ????. You have to do the initial cmdReady, and that's what the code
> does, otherwise your driver won't work at all.  CONFIG_PM is the
> software thing, it doesn't magically match the HW power state. So you
> need to do cmdReady. 

In every machine that I've tried so far the PTT has worked if
cmdReady/goIdle haven't been used at all by the OS during a
power cycle.

> Thanks
> Tomas

I'll keep the current commit (v4 version) because it is tested
quite well, no worries about that, but this is something where a
cleanup would make sense later on.

/Jarkko

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

[Winkler, Tomas]

> Subject: Re: [PATCH 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle state
> 
> On Sat, Oct 08, 2016 at 04:56:39PM +0000, Winkler, Tomas wrote:
> >
> > >
> > > On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote:
> > > > >
> > > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote:
> > > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > > > > >
> > > > > > 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@intel.com>
> > > > > > ---
> > > > > > V2: cmd read need to be called also before crb_init as this
> > > > > > will run self test.
> > > > > > V3: resend.
> > > > > > V4: add Kabylake to the list of effected platforms
> > > > > >
> > > > > >  drivers/char/tpm/tpm_crb.c | 47
> > > > > > ++++++++++++++++++++++++++++++++++++++--------
> > > > > >  1 file changed, 39 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > > b/drivers/char/tpm/tpm_crb.c index 0f3b3f3d12d3..4eb96b85c653
> > > > > > 100644
> > > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > > @@ -319,6 +319,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;
> > > > > > @@ -346,12 +347,27 @@ 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);
> > > > > > +
> > > > > > +	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
> > > > > > +		pa_high, pa_low, 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); @@ -359,7 +375,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 @@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
> > > > > >
> > > > > >  	priv->rsp = priv->cmd;
> > > > > > -	return 0;
> > > > > > +
> > > > > > +out:
> > > > > > +	crb_go_idle(dev, priv);
> > > > > > +
> > > > > > +	return ret;
> > > > > >  }
> > > > > >
> > > > > >  static int crb_acpi_add(struct acpi_device *device) @@ -416,7
> > > > > > +439,15 @@ static int crb_acpi_add(struct acpi_device *device)
> > > > > >  	if (rc)
> > > > > >  		return rc;
> > > > > >
> > > > > > -	return crb_init(device, priv);
> > > > > > +	rc  = crb_cmd_ready(dev, priv);
> > > > > > +	if (rc)
> > > > > > +		return rc;
> > > > >
> > > > > I cannot find any valid reason why crb_map_io calls crb_go_idle
> > > > > in the except in the case of a failure. This is something I complained
> earlier.
> > > >
> > > > Haven't I've already explained that?  Each flow has to be enclosed
> > > > by cmdReady and goIdle.  There is nothing different here from the
> > > > other flows in that matter.  The assumption here is that we are
> > > > starting in the idle state bug because of the HW bug we cannot
> > > > access the registers. So the whole w/o is enclosed in the
> > > > crb_map_io.  After that we are starting from new, from the idle state.
> > >
> > > But why you have to do crb_go_idle upon returning from crb_map_io()?
> >
> > As written above it's is closed code... cmdReady  <perform the task>   goIdle.
> 
> It does extra cmdReady/goIdle. I don't really get your explanation here.
> crb_map_io() is not a utility function used after the driver has initialized.
> 
> crb_map_io() could just leave it ready so you don't have to do an explicit call
> to crb_cmd_ready() in crb_acpi_add. I think this is quite obvious.
> 
> > >
> > > > > A minor thing but the extra crb_cmd_ready is basically clutter
> > > > > to the initialization.
> > > >
> > > > The extra cmdReady is here in case runtime pm is not compiled into
> > > > the kernel and assumption here is that we are starting in the idle state.
> > > > Please remember that unfortunately we cannot detect whether we are
> > > > in ready on idle state as the status register is not retained, so
> > > > everything has to be ordered properly.
> > >
> > > Rather you it would be better not to use cmdReady/goIdle at all if
> > > kernel is not compiled with CONFIG_PM.
> >
> > ????. You have to do the initial cmdReady, and that's what the code
> > does, otherwise your driver won't work at all.  CONFIG_PM is the
> > software thing, it doesn't magically match the HW power state. So you
> > need to do cmdReady.
> 
> In every machine that I've tried so far the PTT has worked if cmdReady/goIdle
> haven't been used at all by the OS during a power cycle.

You mean BXT and SKL, right, but that's because you've been blessed with a specific BIOS that keeps it up.

> 
> I'll keep the current commit (v4 version) because it is tested quite well, no
> worries about that, but this is something where a cleanup would make sense
> later on.

No problem, though so far I believe the flow is correct.
Tomas

RE: [tpmdd-devel] [PATCH 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Winkler, Tomas]

> 
> > Subject: Re: [PATCH 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle
> > state
> >
> > On Sat, Oct 08, 2016 at 04:56:39PM +0000, Winkler, Tomas wrote:
> > >
> > > >
> > > > On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote:
> > > > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > > > > > >
> > > > > > > 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@intel.com>
> > > > > > > ---
> > > > > > > V2: cmd read need to be called also before crb_init as this
> > > > > > > will run self test.
> > > > > > > V3: resend.
> > > > > > > V4: add Kabylake to the list of effected platforms
> > > > > > >
> > > > > > >  drivers/char/tpm/tpm_crb.c | 47
> > > > > > > ++++++++++++++++++++++++++++++++++++++--------
> > > > > > >  1 file changed, 39 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > > > b/drivers/char/tpm/tpm_crb.c index
> > > > > > > 0f3b3f3d12d3..4eb96b85c653
> > > > > > > 100644
> > > > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > > > @@ -319,6 +319,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;
> > > > > > > @@ -346,12 +347,27 @@ 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);
> > > > > > > +
> > > > > > > +	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
> > > > > > > +		pa_high, pa_low, 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); @@ -359,7 +375,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 @@ -367,12 +384,18 @@ 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->cmd_size = cmd_size;
> > > > > > >
> > > > > > >  	priv->rsp = priv->cmd;
> > > > > > > -	return 0;
> > > > > > > +
> > > > > > > +out:
> > > > > > > +	crb_go_idle(dev, priv);
> > > > > > > +
> > > > > > > +	return ret;
> > > > > > >  }
> > > > > > >
> > > > > > >  static int crb_acpi_add(struct acpi_device *device) @@
> > > > > > > -416,7
> > > > > > > +439,15 @@ static int crb_acpi_add(struct acpi_device
> > > > > > > +*device)
> > > > > > >  	if (rc)
> > > > > > >  		return rc;
> > > > > > >
> > > > > > > -	return crb_init(device, priv);
> > > > > > > +	rc  = crb_cmd_ready(dev, priv);
> > > > > > > +	if (rc)
> > > > > > > +		return rc;
> > > > > >
> > > > > > I cannot find any valid reason why crb_map_io calls
> > > > > > crb_go_idle in the except in the case of a failure. This is
> > > > > > something I complained
> > earlier.
> > > > >
> > > > > Haven't I've already explained that?  Each flow has to be
> > > > > enclosed by cmdReady and goIdle.  There is nothing different
> > > > > here from the other flows in that matter.  The assumption here
> > > > > is that we are starting in the idle state bug because of the HW
> > > > > bug we cannot access the registers. So the whole w/o is enclosed
> > > > > in the crb_map_io.  After that we are starting from new, from the idle
> state.
> > > >
> > > > But why you have to do crb_go_idle upon returning from crb_map_io()?
> > >
> > > As written above it's is closed code... cmdReady  <perform the task>
> goIdle.
> >
> > It does extra cmdReady/goIdle. I don't really get your explanation here.
> > crb_map_io() is not a utility function used after the driver has initialized.
> >
> > crb_map_io() could just leave it ready so you don't have to do an
> > explicit call to crb_cmd_ready() in crb_acpi_add. I think this is quite obvious.
> >
> > > >
> > > > > > A minor thing but the extra crb_cmd_ready is basically clutter
> > > > > > to the initialization.
> > > > >
> > > > > The extra cmdReady is here in case runtime pm is not compiled
> > > > > into the kernel and assumption here is that we are starting in the idle
> state.
> > > > > Please remember that unfortunately we cannot detect whether we
> > > > > are in ready on idle state as the status register is not
> > > > > retained, so everything has to be ordered properly.
> > > >
> > > > Rather you it would be better not to use cmdReady/goIdle at all if
> > > > kernel is not compiled with CONFIG_PM.
> > >
> > > ????. You have to do the initial cmdReady, and that's what the code
> > > does, otherwise your driver won't work at all.  CONFIG_PM is the
> > > software thing, it doesn't magically match the HW power state. So
> > > you need to do cmdReady.
> >
> > In every machine that I've tried so far the PTT has worked if
> > cmdReady/goIdle haven't been used at all by the OS during a power cycle.
> 
> You mean BXT and SKL, right, but that's because you've been blessed with a
> specific BIOS that keeps it up.

Hmm, now I understand why you have a bit different angle on the problem. 
Tomas

[PATCH 3/4] tpm/tpm_crb: open code the crb_init into acpi_add

[Tomas Winkler]

From: "Winkler, Tomas" <tomas.winkler@intel.com>

This is preparation step for implementing tpm crb
runtime pm. We need to have tpm chip allocated
and populated before we access the runtime handlers.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
V2: new in the series
V3: resend
V4: resend

 drivers/char/tpm/tpm_crb.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 4eb96b85c653..def50b7d47b2 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -266,21 +266,6 @@ static const struct tpm_class_ops tpm_crb = {
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
 
-static int crb_init(struct acpi_device *device, struct crb_priv *priv)
-{
-	struct tpm_chip *chip;
-
-	chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
-	if (IS_ERR(chip))
-		return PTR_ERR(chip);
-
-	dev_set_drvdata(&chip->dev, priv);
-	chip->acpi_dev_handle = device->handle;
-	chip->flags = TPM_CHIP_FLAG_TPM2;
-
-	return tpm_chip_register(chip);
-}
-
 static int crb_check_resource(struct acpi_resource *ares, void *data)
 {
 	struct resource *io_res = data;
@@ -402,6 +387,7 @@ static int crb_acpi_add(struct acpi_device *device)
 {
 	struct acpi_table_tpm2 *buf;
 	struct crb_priv *priv;
+	struct tpm_chip *chip;
 	struct device *dev = &device->dev;
 	acpi_status status;
 	u32 sm;
@@ -439,11 +425,19 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
+	chip = tpmm_chip_alloc(dev, &tpm_crb);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	dev_set_drvdata(&chip->dev, priv);
+	chip->acpi_dev_handle = device->handle;
+	chip->flags = TPM_CHIP_FLAG_TPM2;
+
 	rc  = crb_cmd_ready(dev, priv);
 	if (rc)
 		return rc;
 
-	rc = crb_init(device, priv);
+	rc = tpm_chip_register(chip);
 	if (rc)
 		crb_go_idle(dev, priv);
 
-- 
2.7.4

[PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Tomas Winkler]

From: "Winkler, Tomas" <tomas.winkler@intel.com>

Utilize runtime_pm for driving tpm crb idle states.
The framework calls cmd_ready from the pm_runtime_resume handler
and go idle from the pm_runtime_suspend handler.
The TPM framework should wake the device before transmit and receive.
In case the runtime_pm framework is not compiled in or enabled, the device
will be in the permanent ready state.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: new in the series
V3: resend
V4: fix unbalanced runtime pm put and get

 drivers/char/tpm/tpm-interface.c |  5 +++++
 drivers/char/tpm/tpm_crb.c       | 42 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 8de61876f633..7743e8a21b82 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -29,6 +29,7 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/freezer.h>
+#include <linux/pm_runtime.h>
 
 #include "tpm.h"
 #include "tpm_eventlog.h"
@@ -356,6 +357,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_lock(&chip->tpm_mutex);
 
+	pm_runtime_get_sync(chip->dev.parent);
+
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
 		dev_err(&chip->dev,
@@ -397,6 +400,8 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
+	pm_runtime_put_sync(chip->dev.parent);
+
 	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 def50b7d47b2..eccd82af6215 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -19,6 +19,7 @@
 #include <linux/highmem.h>
 #include <linux/rculist.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
@@ -153,8 +154,6 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
-
 static u8 crb_status(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -437,11 +436,21 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	rc = tpm_chip_register(chip);
-	if (rc)
+	if (rc) {
 		crb_go_idle(dev, priv);
+		pm_runtime_put_noidle(dev);
+		pm_runtime_disable(dev);
+		return rc;
+	}
 
-	return rc;
+	pm_runtime_put(dev);
+
+	return 0;
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
@@ -451,9 +460,34 @@ static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
+	pm_runtime_disable(dev);
+
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int crb_pm_runtime_suspend(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return crb_go_idle(dev, priv);
+}
+
+static int crb_pm_runtime_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return crb_cmd_ready(dev, priv);
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops crb_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
+	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
+};
+
 static struct acpi_device_id crb_device_ids[] = {
 	{"MSFT0101", 0},
 	{"", 0},
-- 
2.7.4

Re: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> From: "Winkler, Tomas" <tomas.winkler@intel.com>
> 
> Utilize runtime_pm for driving tpm crb idle states.
> The framework calls cmd_ready from the pm_runtime_resume handler
> and go idle from the pm_runtime_suspend handler.
> The TPM framework should wake the device before transmit and receive.
> In case the runtime_pm framework is not compiled in or enabled, the device
> will be in the permanent ready state.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

I would rather want the fix as a separate patch to make review + testing
easier (without and with). Thanks.

/Jarkko

> ---
> V2: new in the series
> V3: resend
> V4: fix unbalanced runtime pm put and get
> 
>  drivers/char/tpm/tpm-interface.c |  5 +++++
>  drivers/char/tpm/tpm_crb.c       | 42 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 8de61876f633..7743e8a21b82 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -29,6 +29,7 @@
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
>  #include <linux/freezer.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
> @@ -356,6 +357,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
>  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
>  		mutex_lock(&chip->tpm_mutex);
>  
> +	pm_runtime_get_sync(chip->dev.parent);
> +
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
>  		dev_err(&chip->dev,
> @@ -397,6 +400,8 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> +	pm_runtime_put_sync(chip->dev.parent);
> +
>  	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 def50b7d47b2..eccd82af6215 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -19,6 +19,7 @@
>  #include <linux/highmem.h>
>  #include <linux/rculist.h>
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  #include "tpm.h"
>  
>  #define ACPI_SIG_TPM2 "TPM2"
> @@ -153,8 +154,6 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> -
>  static u8 crb_status(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -437,11 +436,21 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (rc)
>  		return rc;
>  
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	rc = tpm_chip_register(chip);
> -	if (rc)
> +	if (rc) {
>  		crb_go_idle(dev, priv);
> +		pm_runtime_put_noidle(dev);
> +		pm_runtime_disable(dev);
> +		return rc;
> +	}
>  
> -	return rc;
> +	pm_runtime_put(dev);
> +
> +	return 0;
>  }
>  
>  static int crb_acpi_remove(struct acpi_device *device)
> @@ -451,9 +460,34 @@ static int crb_acpi_remove(struct acpi_device *device)
>  
>  	tpm_chip_unregister(chip);
>  
> +	pm_runtime_disable(dev);
> +
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int crb_pm_runtime_suspend(struct device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	return crb_go_idle(dev, priv);
> +}
> +
> +static int crb_pm_runtime_resume(struct device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	return crb_cmd_ready(dev, priv);
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops crb_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> +	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
> +};
> +
>  static struct acpi_device_id crb_device_ids[] = {
>  	{"MSFT0101", 0},
>  	{"", 0},
> -- 
> 2.7.4
> 

RE: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Winkler, Tomas]

> 
> On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> >
> > Utilize runtime_pm for driving tpm crb idle states.
> > The framework calls cmd_ready from the pm_runtime_resume handler and
> > go idle from the pm_runtime_suspend handler.
> > The TPM framework should wake the device before transmit and receive.
> > In case the runtime_pm framework is not compiled in or enabled, the
> > device will be in the permanent ready state.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> I would rather want the fix as a separate patch to make review + testing easier
> (without and with). Thanks.

But you've dropped the patches, so I've resent them. 
Can you do the diff yourself, just branch it off.  it's really just few lines 

Anyhow here is output of your smoke tests 
sudo python -m unittest -v tpm2_smoke

test_seal_with_auth (tpm2_smoke.SmokeTest) ... ok
test_seal_with_policy (tpm2_smoke.SmokeTest) ... ok
test_seal_with_too_long_auth (tpm2_smoke.SmokeTest) ... ok
test_unseal_with_wrong_auth (tpm2_smoke.SmokeTest) ... ok
test_unseal_with_wrong_policy (tpm2_smoke.SmokeTest) ... ok

----------------------------------------------------------------------
Ran 5 tests in 26.186s

OK
Tomas 

> 
> /Jarkko
> 
> > ---
> > V2: new in the series
> > V3: resend
> > V4: fix unbalanced runtime pm put and get
> >
> >  drivers/char/tpm/tpm-interface.c |  5 +++++
> >  drivers/char/tpm/tpm_crb.c       | 42
> ++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 8de61876f633..7743e8a21b82 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/freezer.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #include "tpm.h"
> >  #include "tpm_eventlog.h"
> > @@ -356,6 +357,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8
> *buf, size_t bufsiz,
> >  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
> >  		mutex_lock(&chip->tpm_mutex);
> >
> > +	pm_runtime_get_sync(chip->dev.parent);
> > +
> >  	rc = chip->ops->send(chip, (u8 *) buf, count);
> >  	if (rc < 0) {
> >  		dev_err(&chip->dev,
> > @@ -397,6 +400,8 @@ out_recv:
> >  		dev_err(&chip->dev,
> >  			"tpm_transmit: tpm_recv: error %zd\n", rc);
> >  out:
> > +	pm_runtime_put_sync(chip->dev.parent);
> > +
> >  	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 def50b7d47b2..eccd82af6215 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/highmem.h>
> >  #include <linux/rculist.h>
> >  #include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> >  #include "tpm.h"
> >
> >  #define ACPI_SIG_TPM2 "TPM2"
> > @@ -153,8 +154,6 @@ static int __maybe_unused crb_cmd_ready(struct
> device *dev,
> >  	return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> > -
> >  static u8 crb_status(struct tpm_chip *chip)  {
> >  	struct crb_priv *priv = dev_get_drvdata(&chip->dev); @@ -437,11
> > +436,21 @@ static int crb_acpi_add(struct acpi_device *device)
> >  	if (rc)
> >  		return rc;
> >
> > +	pm_runtime_get_noresume(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +
> >  	rc = tpm_chip_register(chip);
> > -	if (rc)
> > +	if (rc) {
> >  		crb_go_idle(dev, priv);
> > +		pm_runtime_put_noidle(dev);
> > +		pm_runtime_disable(dev);
> > +		return rc;
> > +	}
> >
> > -	return rc;
> > +	pm_runtime_put(dev);
> > +
> > +	return 0;
> >  }
> >
> >  static int crb_acpi_remove(struct acpi_device *device) @@ -451,9
> > +460,34 @@ static int crb_acpi_remove(struct acpi_device *device)
> >
> >  	tpm_chip_unregister(chip);
> >
> > +	pm_runtime_disable(dev);
> > +
> >  	return 0;
> >  }
> >
> > +#ifdef CONFIG_PM
> > +static int crb_pm_runtime_suspend(struct device *dev) {
> > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +
> > +	return crb_go_idle(dev, priv);
> > +}
> > +
> > +static int crb_pm_runtime_resume(struct device *dev) {
> > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +
> > +	return crb_cmd_ready(dev, priv);
> > +}
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops crb_pm = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> > +	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend,
> crb_pm_runtime_resume,
> > +NULL) };
> > +
> >  static struct acpi_device_id crb_device_ids[] = {
> >  	{"MSFT0101", 0},
> >  	{"", 0},
> > --
> > 2.7.4
> >

Re: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 01:37:31PM +0000, Winkler, Tomas wrote:
> > 
> > On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > >
> > > Utilize runtime_pm for driving tpm crb idle states.
> > > The framework calls cmd_ready from the pm_runtime_resume handler and
> > > go idle from the pm_runtime_suspend handler.
> > > The TPM framework should wake the device before transmit and receive.
> > > In case the runtime_pm framework is not compiled in or enabled, the
> > > device will be in the permanent ready state.
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > 
> > I would rather want the fix as a separate patch to make review + testing easier
> > (without and with). Thanks.
> 
> But you've dropped the patches, so I've resent them. 
> Can you do the diff yourself, just branch it off.  it's really just few lines 

Nope. I have only dropped the workaround.

/Jarkko

RE: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Saturday, October 08, 2016 19:01
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: tpmdd-devel@lists.sourceforge.net; Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb
> 
> On Sat, Oct 08, 2016 at 01:37:31PM +0000, Winkler, Tomas wrote:
> > >
> > > On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > > >
> > > > Utilize runtime_pm for driving tpm crb idle states.
> > > > The framework calls cmd_ready from the pm_runtime_resume handler
> > > > and go idle from the pm_runtime_suspend handler.
> > > > The TPM framework should wake the device before transmit and receive.
> > > > In case the runtime_pm framework is not compiled in or enabled,
> > > > the device will be in the permanent ready state.
> > > >
> > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > >
> > > I would rather want the fix as a separate patch to make review +
> > > testing easier (without and with). Thanks.
> >
> > But you've dropped the patches, so I've resent them.
> > Can you do the diff yourself, just branch it off.  it's really just
> > few lines
> 
> Nope. I have only dropped the workaround.

This is what is current in the linus tree:

git log --oneline linux/master -- drivers/char/tpm/

324152502b0e Revert "tpm/tpm_crb: implement tpm crb idle state"
cfa188220363 Revert "tmp/tpm_crb: fix Intel PTT hw bug during idle state"
2b7926ae1cd4 Revert "tpm/tpm_crb: open code the crb_init into acpi_add"
4886cd80cb8e Revert "tmp/tpm_crb: implement runtime pm for tpm_crb"
e350e24694e4 tmp/tpm_crb: implement runtime pm for tpm_crb
0c22db435bf7 tpm/tpm_crb: open code the crb_init into acpi_add
9514ff1961c6 tmp/tpm_crb: fix Intel PTT hw bug during idle state
e17acbbb69d3 tpm/tpm_crb: implement tpm crb idle state

Re: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 06:18:36PM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > Sent: Saturday, October 08, 2016 19:01
> > To: Winkler, Tomas <tomas.winkler@intel.com>
> > Cc: tpmdd-devel@lists.sourceforge.net; Jason Gunthorpe
> > <jgunthorpe@obsidianresearch.com>; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb
> > 
> > On Sat, Oct 08, 2016 at 01:37:31PM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Sat, Oct 08, 2016 at 02:59:39PM +0300, Tomas Winkler wrote:
> > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com>
> > > > >
> > > > > Utilize runtime_pm for driving tpm crb idle states.
> > > > > The framework calls cmd_ready from the pm_runtime_resume handler
> > > > > and go idle from the pm_runtime_suspend handler.
> > > > > The TPM framework should wake the device before transmit and receive.
> > > > > In case the runtime_pm framework is not compiled in or enabled,
> > > > > the device will be in the permanent ready state.
> > > > >
> > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > >
> > > > I would rather want the fix as a separate patch to make review +
> > > > testing easier (without and with). Thanks.
> > >
> > > But you've dropped the patches, so I've resent them.
> > > Can you do the diff yourself, just branch it off.  it's really just
> > > few lines
> > 
> > Nope. I have only dropped the workaround.
> 
> This is what is current in the linus tree:
> 
> git log --oneline linux/master -- drivers/char/tpm/
> 
> 324152502b0e Revert "tpm/tpm_crb: implement tpm crb idle state"
> cfa188220363 Revert "tmp/tpm_crb: fix Intel PTT hw bug during idle state"
> 2b7926ae1cd4 Revert "tpm/tpm_crb: open code the crb_init into acpi_add"
> 4886cd80cb8e Revert "tmp/tpm_crb: implement runtime pm for tpm_crb"
> e350e24694e4 tmp/tpm_crb: implement runtime pm for tpm_crb
> 0c22db435bf7 tpm/tpm_crb: open code the crb_init into acpi_add
> 9514ff1961c6 tmp/tpm_crb: fix Intel PTT hw bug during idle state
> e17acbbb69d3 tpm/tpm_crb: implement tpm crb idle state

That's unrelated: https://lkml.org/lkml/2016/9/27/107

Anyway, I applied this because the change is fairly obvious but it would
be nicer to get the fix as a separate patch for a series that is already
applied. If you rely on "do the diff yourself", it is quite ineffective
way to crowdsource :)

/Jarkko