[PATCH 0/4] Small fixes and cleanups for tpm_crb

[PATCH 0/4] Small fixes and cleanups for tpm_crb

[Jarkko Sakkinen]

A set of small fixes and clean ups for tpm_crb.

Jarkko Sakkinen (4):
  tpm_crb: fix crb_req_canceled behavior
  tpm_crb: remove wmb()'s
  tpm_crb: refine the naming of constants
  tpm_crb: fix incorrect values of cmdReady and goIdle bits

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

-- 
2.7.4

[PATCH 1/4] tpm_crb: fix crb_req_canceled behavior

[Jarkko Sakkinen]

The req_canceled() callback is used by tpm_transmit() periodically to
check whether the request has been canceled while it is receiving a
response from the TPM.

The TPM_CRB_CTRL_CANCEL register was cleared already in the crb_cancel
callback, which has two consequences:

* Cancel might not happen.
* req_canceled() always returns zero.

A better place to clear the register is when starting to send a new
command. The behavior of TPM_CRB_CTRL_CANCEL is described in the
section 5.5.3.6 of the PTP specification.

CC: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 018c3825..1801f38 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -142,6 +142,11 @@ 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;
 
+	/* Zero the cancel register so that the next command will not get
+	 * canceled.
+	 */
+	iowrite32(0, &priv->cca->cancel);
+
 	if (len > ioread32(&priv->cca->cmd_size)) {
 		dev_err(&chip->dev,
 			"invalid command count value %x %zx\n",
@@ -175,8 +180,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
-
-	iowrite32(0, &priv->cca->cancel);
 }
 
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
-- 
2.7.4


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

[PATCH 2/4] tpm_crb: remove wmb()'s

[Jarkko Sakkinen]

wmb()'s are not needed as iowrite32() is used.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 1801f38..358c475 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -175,9 +175,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	iowrite32(cpu_to_le32(CRB_CANCEL_INVOKE), &priv->cca->cancel);
 
-	/* Make sure that cmd is populated before issuing cancel. */
-	wmb();
-
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
 }
-- 
2.7.4


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

[PATCH 3/4] tpm_crb: refine the naming of constants

[Jarkko Sakkinen]

Renamed CRB protocol specific constants to match the TCG PC Client
Platform TPM Profile (PTP) Specification and driver status constants
to be explicit that they are driver specific.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 358c475..c8b0d91 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,14 +34,14 @@ enum crb_defaults {
 	CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_ca_request {
-	CRB_CA_REQ_GO_IDLE	= BIT(0),
-	CRB_CA_REQ_CMD_READY	= BIT(1),
+enum crb_ctrl_req {
+	CRB_CTRL_REQ_GO_IDLE	= BIT(0),
+	CRB_CTRL_REQ_CMD_READY	= BIT(1),
 };
 
-enum crb_ca_status {
-	CRB_CA_STS_ERROR	= BIT(0),
-	CRB_CA_STS_TPM_IDLE	= BIT(1),
+enum crb_ctrl_sts {
+	CRB_CTRL_STS_ERROR	= BIT(0),
+	CRB_CTRL_STS_TPM_IDLE	= BIT(1),
 };
 
 enum crb_start {
@@ -67,7 +67,7 @@ struct crb_control_area {
 } __packed;
 
 enum crb_status {
-	CRB_STS_COMPLETE	= BIT(0),
+	CRB_DRV_STS_COMPLETE	= BIT(0),
 };
 
 enum crb_flags {
@@ -92,7 +92,7 @@ static u8 crb_status(struct tpm_chip *chip)
 
 	if ((ioread32(&priv->cca->start) & CRB_START_INVOKE) !=
 	    CRB_START_INVOKE)
-		sts |= CRB_STS_COMPLETE;
+		sts |= CRB_DRV_STS_COMPLETE;
 
 	return sts;
 }
@@ -106,7 +106,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	if (count < 6)
 		return -EIO;
 
-	if (ioread32(&priv->cca->sts) & CRB_CA_STS_ERROR)
+	if (ioread32(&priv->cca->sts) & CRB_CTRL_STS_ERROR)
 		return -EIO;
 
 	memcpy_fromio(buf, priv->rsp, 6);
@@ -194,8 +194,8 @@ static const struct tpm_class_ops tpm_crb = {
 	.send = crb_send,
 	.cancel = crb_cancel,
 	.req_canceled = crb_req_canceled,
-	.req_complete_mask = CRB_STS_COMPLETE,
-	.req_complete_val = CRB_STS_COMPLETE,
+	.req_complete_mask = CRB_DRV_STS_COMPLETE,
+	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
 
 static int crb_init(struct acpi_device *device, struct crb_priv *priv)
-- 
2.7.4


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

[PATCH 4/4] tpm_crb: fix incorrect values of cmdReady and goIdle bits

[Jarkko Sakkinen]

CRB_CTRL_CMD_READY and CRB_CTRL_GO_IDLE have incorrect values.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index c8b0d91..7f602dc 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -35,8 +35,8 @@ enum crb_defaults {
 };
 
 enum crb_ctrl_req {
-	CRB_CTRL_REQ_GO_IDLE	= BIT(0),
-	CRB_CTRL_REQ_CMD_READY	= BIT(1),
+	CRB_CTRL_REQ_CMD_READY	= BIT(0),
+	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
 };
 
 enum crb_ctrl_sts {
-- 
2.7.4


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

Re: [PATCH 4/4] tpm_crb: fix incorrect values of cmdReady and goIdle bits

[Jarkko Sakkinen]

On Fri, Sep 02, 2016 at 10:34:20PM +0300, Jarkko Sakkinen wrote:
> CRB_CTRL_CMD_READY and CRB_CTRL_GO_IDLE have incorrect values.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
Cc: stable@vger.kernel.org

/Jarkko

Re: [PATCH 0/4] Small fixes and cleanups for tpm_crb

[Jarkko Sakkinen]

On Fri, Sep 02, 2016 at 10:34:16PM +0300, Jarkko Sakkinen wrote:
> A set of small fixes and clean ups for tpm_crb.
> 
> Jarkko Sakkinen (4):
>   tpm_crb: fix crb_req_canceled behavior
>   tpm_crb: remove wmb()'s
>   tpm_crb: refine the naming of constants
>   tpm_crb: fix incorrect values of cmdReady and goIdle bits
> 
>  drivers/char/tpm/tpm_crb.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)

I guess these should be no-brainers?

/Jarkko

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

Re: [PATCH 0/4] Small fixes and cleanups for tpm_crb

[Jarkko Sakkinen]

On Sat, Sep 03, 2016 at 09:27:54AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2016 at 10:34:16PM +0300, Jarkko Sakkinen wrote:
> > A set of small fixes and clean ups for tpm_crb.
> > 
> > Jarkko Sakkinen (4):
> >   tpm_crb: fix crb_req_canceled behavior
> >   tpm_crb: remove wmb()'s
> >   tpm_crb: refine the naming of constants
> >   tpm_crb: fix incorrect values of cmdReady and goIdle bits
> > 
> >  drivers/char/tpm/tpm_crb.c | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> I guess these should be no-brainers?

Actually I'm picking these patches to my runtime PM patch set.
Still reviews are always welcome of course.

/Jarkko

Re: [PATCH 0/4] Small fixes and cleanups for tpm_crb

[Jarkko Sakkinen]

On Fri, Sep 02, 2016 at 10:34:16PM +0300, Jarkko Sakkinen wrote:
> A set of small fixes and clean ups for tpm_crb.

I will apply these fixes tomorrow because they are no brainers
basically. Tomas, I guess you can gives these tested-by since
you've tested runtime PM code on top of these?

> Jarkko Sakkinen (4):
>   tpm_crb: fix crb_req_canceled behavior
>   tpm_crb: remove wmb()'s
>   tpm_crb: refine the naming of constants
>   tpm_crb: fix incorrect values of cmdReady and goIdle bits
> 
>  drivers/char/tpm/tpm_crb.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> -- 
> 2.7.4

/Jarkko

Re: [PATCH 0/4] Small fixes and cleanups for tpm_crb

[Jarkko Sakkinen]

On Thu, Sep 08, 2016 at 01:27:59PM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2016 at 10:34:16PM +0300, Jarkko Sakkinen wrote:
> > A set of small fixes and clean ups for tpm_crb.
> 
> I will apply these fixes tomorrow because they are no brainers
> basically. Tomas, I guess you can gives these tested-by since
> you've tested runtime PM code on top of these?

I've applied these to my master branch.

/Jarkko

> > Jarkko Sakkinen (4):
> >   tpm_crb: fix crb_req_canceled behavior
> >   tpm_crb: remove wmb()'s
> >   tpm_crb: refine the naming of constants
> >   tpm_crb: fix incorrect values of cmdReady and goIdle bits
> > 
> >  drivers/char/tpm/tpm_crb.c | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > -- 
> > 2.7.4
> 
> /Jarkko

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