From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751840AbcACRJN (ORCPT ); Sun, 3 Jan 2016 12:09:13 -0500 Received: from mga03.intel.com ([134.134.136.65]:3564 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751117AbcACRJK (ORCPT ); Sun, 3 Jan 2016 12:09:10 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,516,1444719600"; d="scan'208";a="852701988" Date: Sun, 3 Jan 2016 19:09:06 +0200 From: Jarkko Sakkinen To: Jason Gunthorpe Cc: tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, Martin Wilck , Peter Huewe , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Subject: Re: [PATCH v3 1/7] tpm_crb: Use the common ACPI definition of struct acpi_tpm2 Message-ID: <20160103170906.GA4155@intel.com> References: <1450376600-6970-1-git-send-email-jgunthorpe@obsidianresearch.com> <1450376600-6970-2-git-send-email-jgunthorpe@obsidianresearch.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1450376600-6970-2-git-send-email-jgunthorpe@obsidianresearch.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 17, 2015 at 11:23:14AM -0700, Jason Gunthorpe wrote: > include/acpi/actbl2.h is the proper place for these definitions > and the needed TPM2 ones have been there since > commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table") > > This also drops a couple of le32_to_cpu's for members of this table, > the existing swapping was not done consistently, and the definitions > in actbl2.h do not have endianness annotations, declaring that no swap > is required. Note that the TPM ACPI spec defines all of these > values to be little endian, both in crb2 and ppi. I think this patch mixes two separate changes to the driver: removing l32_to_cpu's and moving to common headers and that is not right. Even if they should be removed I think what you say about actbl2.h is wrong. Annotations are probably missing because it is imported code (I'm happy to be corrected if this is not the case). /Jarkko > Signed-off-by: Jason Gunthorpe > Tested-by: Wilck, Martin > Tested-by: Jarkko Sakkinen > --- > drivers/char/tpm/tpm.h | 7 ------- > drivers/char/tpm/tpm_crb.c | 31 +++++++++---------------------- > drivers/char/tpm/tpm_tis.c | 2 +- > 3 files changed, 10 insertions(+), 30 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 542a80cbfd9c..28b477e8da6a 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -128,13 +128,6 @@ enum tpm2_startup_types { > TPM2_SU_STATE = 0x0001, > }; > > -enum tpm2_start_method { > - TPM2_START_ACPI = 2, > - TPM2_START_FIFO = 6, > - TPM2_START_CRB = 7, > - TPM2_START_CRB_WITH_ACPI = 8, > -}; > - > struct tpm_chip; > > struct tpm_vendor_specific { > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 8342cf51ffdc..8dd70696ebe8 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -34,14 +34,6 @@ enum crb_defaults { > CRB_ACPI_START_INDEX = 1, > }; > > -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), > @@ -207,7 +199,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; > @@ -217,13 +209,14 @@ static int crb_acpi_add(struct acpi_device *device) > > status = acpi_get_table(ACPI_SIG_TPM2, 1, > (struct acpi_table_header **) &buf); > - if (ACPI_FAILURE(status)) { > - dev_err(dev, "failed to get TPM2 ACPI table\n"); > + if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) { > + dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n"); > return -ENODEV; > } > > /* Should the FIFO driver handle this? */ > - if (buf->start_method == TPM2_START_FIFO) > + sm = buf->start_method; > + if (sm == ACPI_TPM2_MEMORY_MAPPED) > return -ENODEV; > > chip = tpmm_chip_alloc(dev, &tpm_crb); > @@ -232,11 +225,6 @@ static int crb_acpi_add(struct acpi_device *device) > > chip->flags = TPM_CHIP_FLAG_TPM2; > > - if (buf->hdr.length < sizeof(struct acpi_tpm2)) { > - dev_err(dev, "TPM2 ACPI table has wrong size"); > - return -EINVAL; > - } > - > priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv), > GFP_KERNEL); > if (!priv) { > @@ -244,21 +232,20 @@ static int crb_acpi_add(struct acpi_device *device) > return -ENOMEM; > } > > - sm = le32_to_cpu(buf->start_method); > - > /* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs > * report only ACPI start but in practice seems to require both > * ACPI start and CRB start. > */ > - if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO || > + if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED || > !strcmp(acpi_device_hid(device), "MSFT0101")) > priv->flags |= CRB_FL_CRB_START; > > - if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI) > + if (sm == ACPI_TPM2_START_METHOD || > + sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) > priv->flags |= CRB_FL_ACPI_START; > > priv->cca = (struct crb_control_area __iomem *) > - devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000); > + devm_ioremap_nocache(dev, buf->control_address, 0x1000); > if (!priv->cca) { > dev_err(dev, "ioremap of the control area failed\n"); > return -ENOMEM; > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 8a3509cb10da..304323bdcaaa 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -135,7 +135,7 @@ static inline int is_fifo(struct acpi_device *dev) > return 0; > } > > - if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO) > + if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED) > return 0; > > /* TPM 2.0 FIFO */ > -- > 2.1.4 >