| Message ID | 1432915058-5598-1-git-send-email-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
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;
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(-)