[PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control

[PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control

[Jarkko Sakkinen]

Migrate to struct acpi_table_tpm2 and struct acpi_tpm2_control defined
in include/acpi/actbl3.h from the internal structures.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 64 +++++++++++++++-------------------------------
 1 file changed, 21 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index b26ceee..0cbc944 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
@@ -20,10 +20,9 @@
 #include <linux/rculist.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <acpi/actbl3.h>
 #include "tpm.h"
 
-#define ACPI_SIG_TPM2 "TPM2"
-
 static const u8 CRB_ACPI_START_UUID[] = {
 	/* 0000 */ 0xAB, 0x6C, 0xBF, 0x6B, 0x63, 0x54, 0x14, 0x47,
 	/* 0008 */ 0xB7, 0xCD, 0xF0, 0x20, 0x3C, 0x03, 0x68, 0xD4
@@ -40,14 +39,6 @@ enum crb_start_method {
 	CRB_SM_CRB_WITH_ACPI_START = 8,
 };
 
-struct acpi_tpm2 {
-	struct acpi_table_header hdr;
-	u16 platform_class;
-	u16 reserved;
-	u64 control_area_pa;
-	u32 start_method;
-} __packed;
-
 enum crb_ca_request {
 	CRB_CA_REQ_GO_IDLE	= BIT(0),
 	CRB_CA_REQ_CMD_READY	= BIT(1),
@@ -66,19 +57,6 @@ enum crb_cancel {
 	CRB_CANCEL_INVOKE	= BIT(0),
 };
 
-struct crb_control_area {
-	u32 req;
-	u32 sts;
-	u32 cancel;
-	u32 start;
-	u32 int_enable;
-	u32 int_sts;
-	u32 cmd_size;
-	u64 cmd_pa;
-	u32 rsp_size;
-	u64 rsp_pa;
-} __packed;
-
 enum crb_status {
 	CRB_STS_COMPLETE	= BIT(0),
 };
@@ -90,7 +68,7 @@ enum crb_flags {
 
 struct crb_priv {
 	unsigned int flags;
-	struct crb_control_area __iomem *cca;
+	struct acpi_tpm2_control __iomem *ctl;
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
 };
@@ -102,7 +80,7 @@ static u8 crb_status(struct tpm_chip *chip)
 	struct crb_priv *priv = chip->vendor.priv;
 	u8 sts = 0;
 
-	if ((le32_to_cpu(ioread32(&priv->cca->start)) & CRB_START_INVOKE) !=
+	if ((le32_to_cpu(ioread32(&priv->ctl->start)) & CRB_START_INVOKE) !=
 	    CRB_START_INVOKE)
 		sts |= CRB_STS_COMPLETE;
 
@@ -118,7 +96,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	if (count < 6)
 		return -EIO;
 
-	if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
+	if (le32_to_cpu(ioread32(&priv->ctl->error)) & CRB_CA_STS_ERROR)
 		return -EIO;
 
 	memcpy_fromio(buf, priv->rsp, 6);
@@ -152,13 +130,13 @@ static int crb_do_acpi_start(struct tpm_chip *chip)
 static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 {
 	struct crb_priv *priv = chip->vendor.priv;
+	u32 cmd_size = le32_to_cpu(ioread32(&priv->ctl->command_size));
 	int rc = 0;
 
-	if (len > le32_to_cpu(ioread32(&priv->cca->cmd_size))) {
+	if (len > cmd_size) {
 		dev_err(&chip->dev,
 			"invalid command count value %x %zx\n",
-			(unsigned int) len,
-			(size_t) le32_to_cpu(ioread32(&priv->cca->cmd_size)));
+			(unsigned int) len, (size_t) cmd_size);
 		return -E2BIG;
 	}
 
@@ -168,7 +146,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	wmb();
 
 	if (priv->flags & CRB_FL_CRB_START)
-		iowrite32(cpu_to_le32(CRB_START_INVOKE), &priv->cca->start);
+		iowrite32(cpu_to_le32(CRB_START_INVOKE), &priv->ctl->start);
 
 	if (priv->flags & CRB_FL_ACPI_START)
 		rc = crb_do_acpi_start(chip);
@@ -180,7 +158,7 @@ static void crb_cancel(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = chip->vendor.priv;
 
-	iowrite32(cpu_to_le32(CRB_CANCEL_INVOKE), &priv->cca->cancel);
+	iowrite32(cpu_to_le32(CRB_CANCEL_INVOKE), &priv->ctl->cancel);
 
 	/* Make sure that cmd is populated before issuing cancel. */
 	wmb();
@@ -188,13 +166,13 @@ 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);
+	iowrite32(0, &priv->ctl->cancel);
 }
 
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	struct crb_priv *priv = chip->vendor.priv;
-	u32 cancel = le32_to_cpu(ioread32(&priv->cca->cancel));
+	u32 cancel = le32_to_cpu(ioread32(&priv->ctl->cancel));
 
 	return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE;
 }
@@ -212,7 +190,7 @@ static const struct tpm_class_ops tpm_crb = {
 static int crb_acpi_add(struct acpi_device *device)
 {
 	struct tpm_chip *chip;
-	struct acpi_tpm2 *buf;
+	struct acpi_table_tpm2 *buf;
 	struct crb_priv *priv;
 	struct device *dev = &device->dev;
 	acpi_status status;
@@ -233,7 +211,7 @@ static int crb_acpi_add(struct acpi_device *device)
 		return -ENODEV;
 	}
 
-	if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
+	if (buf->header.length < sizeof(struct acpi_table_tpm2)) {
 		dev_err(dev, "TPM2 ACPI table has wrong size");
 		return -EINVAL;
 	}
@@ -258,26 +236,26 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
 		priv->flags |= CRB_FL_ACPI_START;
 
-	priv->cca = (struct crb_control_area __iomem *)
-		devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
-	if (!priv->cca) {
+	priv->ctl = (struct acpi_tpm2_control __iomem *)
+		devm_ioremap_nocache(dev, buf->control_address, PAGE_SIZE);
+	if (!priv->ctl) {
 		dev_err(dev, "ioremap of the control area failed\n");
 		return -ENOMEM;
 	}
 
-	memcpy_fromio(&pa, &priv->cca->cmd_pa, 8);
+	memcpy_fromio(&pa, &priv->ctl->command_address, 8);
 	pa = le64_to_cpu(pa);
 	priv->cmd = devm_ioremap_nocache(dev, le64_to_cpu(pa),
-					 ioread32(&priv->cca->cmd_size));
+					 ioread32(&priv->ctl->command_size));
 	if (!priv->cmd) {
 		dev_err(dev, "ioremap of the command buffer failed\n");
 		return -ENOMEM;
 	}
 
-	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
+	memcpy_fromio(&pa, &priv->ctl->response_address, 8);
 	pa = le64_to_cpu(pa);
 	priv->rsp = devm_ioremap_nocache(dev, le64_to_cpu(pa),
-					 ioread32(&priv->cca->rsp_size));
+					 ioread32(&priv->ctl->response_size));
 	if (!priv->rsp) {
 		dev_err(dev, "ioremap of the response buffer failed\n");
 		return -ENOMEM;
-- 
2.1.4

Aw: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control

[Peter Huewe]

Hi
>Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control
> Migrate to struct acpi_table_tpm2 and struct acpi_tpm2_control defined
> in include/acpi/actbl3.h from the internal structures.

I definitely do like the idea! Thanks for spotting this!

However one small remark
> -struct crb_control_area {
> - u32 req;
> - u32 sts;
> - u32 cancel;
> - u32 start;
> - u32 int_enable;
> - u32 int_sts;
> - u32 cmd_size;
> - u64 cmd_pa;
> - u32 rsp_size;
> - u64 rsp_pa;
> -} __packed;
> -
> 
> - if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
> + if (le32_to_cpu(ioread32(&priv->ctl->error)) & CRB_CA_STS_ERROR)
> return -EIO;

I know the fields are described in include/acpi/actbl3.h as 
+struct acpi_tpm2_control {
+	u32 reserved;
+	u32 error;
+	u32 cancel;
+	u32 start;
+	u64 interrupt_control;
+	u32 command_size;
+	u64 command_address;
+	u32 response_size;
+	u64 response_address;
+};

but are the names there still correct? Isn't this information outdated?
The acpi spec refers to the MS spec which is not present anymore, and MS refers to the TCG -- and in the PTP your names are used.

---> We should update the ACPI header? 
At least the naming for reserved and error.
What do you think?

Thanks,
Peter

Re: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control

[Jarkko Sakkinen]

Hi

I somehow missed your reply to this last week.

On Tue, Jun 02, 2015 at 04:00:37PM +0200, Peter Huewe wrote:
> Hi
> >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control
> > Migrate to struct acpi_table_tpm2 and struct acpi_tpm2_control defined
> > in include/acpi/actbl3.h from the internal structures.
> 
> I definitely do like the idea! Thanks for spotting this!
> 
> However one small remark
> > -struct crb_control_area {
> > - u32 req;
> > - u32 sts;
> > - u32 cancel;
> > - u32 start;
> > - u32 int_enable;
> > - u32 int_sts;
> > - u32 cmd_size;
> > - u64 cmd_pa;
> > - u32 rsp_size;
> > - u64 rsp_pa;
> > -} __packed;
> > -
> > 
> > - if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
> > + if (le32_to_cpu(ioread32(&priv->ctl->error)) & CRB_CA_STS_ERROR)
> > return -EIO;
> 
> I know the fields are described in include/acpi/actbl3.h as 
> +struct acpi_tpm2_control {
> +	u32 reserved;
> +	u32 error;
> +	u32 cancel;
> +	u32 start;
> +	u64 interrupt_control;
> +	u32 command_size;
> +	u64 command_address;
> +	u32 response_size;
> +	u64 response_address;
> +};
> 
> but are the names there still correct? Isn't this information outdated?
> The acpi spec refers to the MS spec which is not present anymore, and MS refers to the TCG -- and in the PTP your names are used.
> 
> ---> We should update the ACPI header? 
> At least the naming for reserved and error.
> What do you think?

I think you are right. It does not make sense to degrade here. I'll
prepare "CRB fixes" patch set and also include a workaround for this
bug:

https://bugzilla.kernel.org/show_bug.cgi?id=98181

See my last comment.

> Thanks,
> Peter

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control

[Jarkko Sakkinen]

On Mon, Jun 08, 2015 at 02:54:35PM +0300, Jarkko Sakkinen wrote:
> Hi
> 
> I somehow missed your reply to this last week.
> 
> On Tue, Jun 02, 2015 at 04:00:37PM +0200, Peter Huewe wrote:
> > Hi
> > >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control
> > > Migrate to struct acpi_table_tpm2 and struct acpi_tpm2_control defined
> > > in include/acpi/actbl3.h from the internal structures.
> > 
> > I definitely do like the idea! Thanks for spotting this!
> > 
> > However one small remark
> > > -struct crb_control_area {
> > > - u32 req;
> > > - u32 sts;
> > > - u32 cancel;
> > > - u32 start;
> > > - u32 int_enable;
> > > - u32 int_sts;
> > > - u32 cmd_size;
> > > - u64 cmd_pa;
> > > - u32 rsp_size;
> > > - u64 rsp_pa;
> > > -} __packed;
> > > -
> > > 
> > > - if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
> > > + if (le32_to_cpu(ioread32(&priv->ctl->error)) & CRB_CA_STS_ERROR)
> > > return -EIO;
> > 
> > I know the fields are described in include/acpi/actbl3.h as 
> > +struct acpi_tpm2_control {
> > +	u32 reserved;
> > +	u32 error;
> > +	u32 cancel;
> > +	u32 start;
> > +	u64 interrupt_control;
> > +	u32 command_size;
> > +	u64 command_address;
> > +	u32 response_size;
> > +	u64 response_address;
> > +};
> > 
> > but are the names there still correct? Isn't this information outdated?
> > The acpi spec refers to the MS spec which is not present anymore, and MS refers to the TCG -- and in the PTP your names are used.
> > 
> > ---> We should update the ACPI header? 
> > At least the naming for reserved and error.
> > What do you think?
> 
> I think you are right. It does not make sense to degrade here. I'll
> prepare "CRB fixes" patch set and also include a workaround for this
> bug:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=98181
> 
> See my last comment.

Some rethinking after sending this email.

I implement and submit the bug as a separate item as these are
unrelated issues.

I think the control area should not be in the ACPI headers in the
first place as it is not defined TCG ACPI Specification.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control

[Peter Hüwe]

Hi Jarkko

> > > >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and
> > > >acpi_tpm2_control
> > > but are the names there still correct? Isn't this information outdated?
> > > The acpi spec refers to the MS spec which is not present anymore, and
> > > MS refers to the TCG -- and in the PTP your names are used.
> > > 
> > > ---> We should update the ACPI header?
> > > At least the naming for reserved and error.
> > > What do you think?
> > 
> > I think you are right. It does not make sense to degrade here. 

so I'm waiting on the new version depending on the updated acpi header?

Thanks,
Peter

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and acpi_tpm2_control

[Jarkko Sakkinen]

On Tue, Jun 16, 2015 at 10:46:50PM +0200, Peter Hüwe wrote:
> Hi Jarkko
> 
> > > > >Betreff: [PATCH] tpm, tpm_crb: migrate to struct acpi_table_tpm2 and
> > > > >acpi_tpm2_control
> > > > but are the names there still correct? Isn't this information outdated?
> > > > The acpi spec refers to the MS spec which is not present anymore, and
> > > > MS refers to the TCG -- and in the PTP your names are used.
> > > > 
> > > > ---> We should update the ACPI header?
> > > > At least the naming for reserved and error.
> > > > What do you think?
> > > 
> > > I think you are right. It does not make sense to degrade here. 
> 
> so I'm waiting on the new version depending on the updated acpi header?

Yes. I'll submit a new patch later on when new arrives come from ACPICA.
You can ignore this until then.

> Thanks,
> Peter

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/