[PATCH 4.3 07/55] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Greg Kroah-Hartman]

4.3-stable review patch.  If anyone has any objections, please let me know.

--

From: Jarkko Sakkinen 

commit 399235dc6e95400a1322ae92073bc572f0c8 upstream.

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
Reviewed-by: Peter Huewe 
Signed-off-by: Peter Huewe 
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/char/tpm/tpm.h |7 +
 drivers/char/tpm/tpm_crb.c |   32 ++-
 drivers/char/tpm/tpm_tis.c |  192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 {
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -220,12 +214,6 @@ static int crb_acpi_add(struct acpi_devi
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -233,13 +221,15 @@ static int crb_acpi_add(struct acpi_devi
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_devi
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct tpm_info {
+   unsigned long start;
+   unsigned long len;
+   unsigned int irq;
+};
+
+static struct tpm_info tis_default_info = {
+   .start = TIS_MEM_BASE,
+   .len = TIS_MEM_LEN,
+   .irq = 0,
+};
 
 /* Some timeout values are needed before it is known whether the chip is
  * TPM 

[PATCH 4.1 01/43] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Greg Kroah-Hartman]

4.1-stable review patch.  If anyone has any objections, please let me know.

--

From: Jarkko Sakkinen 

commit 399235dc6e95400a1322ae92073bc572f0c8 upstream.

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
Reviewed-by: Peter Huewe 
Signed-off-by: Peter Huewe 
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/char/tpm/tpm.h |7 +
 drivers/char/tpm/tpm_crb.c |   32 ++-
 drivers/char/tpm/tpm_tis.c |  192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 {
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -220,12 +214,6 @@ static int crb_acpi_add(struct acpi_devi
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -233,13 +221,15 @@ static int crb_acpi_add(struct acpi_devi
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_devi
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct tpm_info {
+   unsigned long start;
+   unsigned long len;
+   unsigned int irq;
+};
+
+static struct tpm_info tis_default_info = {
+   .start = TIS_MEM_BASE,
+   .len = TIS_MEM_LEN,
+   .irq = 0,
+};
 
 /* Some timeout values are needed before it is known whether the chip is
  * TPM 

[PATCH 4.3 07/55] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Greg Kroah-Hartman]

4.3-stable review patch.  If anyone has any objections, please let me know.

--

From: Jarkko Sakkinen 

commit 399235dc6e95400a1322ae92073bc572f0c8 upstream.

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
Reviewed-by: Peter Huewe 
Signed-off-by: Peter Huewe 
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/char/tpm/tpm.h |7 +
 drivers/char/tpm/tpm_crb.c |   32 ++-
 drivers/char/tpm/tpm_tis.c |  192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 {
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -220,12 +214,6 @@ static int crb_acpi_add(struct acpi_devi
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -233,13 +221,15 @@ static int crb_acpi_add(struct acpi_devi
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_devi
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct 

[PATCH 4.1 01/43] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Greg Kroah-Hartman]

4.1-stable review patch.  If anyone has any objections, please let me know.

--

From: Jarkko Sakkinen 

commit 399235dc6e95400a1322ae92073bc572f0c8 upstream.

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
Reviewed-by: Peter Huewe 
Signed-off-by: Peter Huewe 
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/char/tpm/tpm.h |7 +
 drivers/char/tpm/tpm_crb.c |   32 ++-
 drivers/char/tpm/tpm_tis.c |  192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 {
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -220,12 +214,6 @@ static int crb_acpi_add(struct acpi_devi
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -233,13 +221,15 @@ static int crb_acpi_add(struct acpi_devi
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_devi
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct 

[PATCH 4.2.y-ckt 048/211] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Kamal Mostafa]

4.2.8-ckt1 -stable review patch.  If anyone has any objections, please let me 
know.

--

From: Jarkko Sakkinen 

commit 399235dc6e95400a1322ae92073bc572f0c8 upstream.

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
Reviewed-by: Peter Huewe 
Signed-off-by: Peter Huewe 
Signed-off-by: Kamal Mostafa 
---
 drivers/char/tpm/tpm.h |   7 ++
 drivers/char/tpm/tpm_crb.c |  32 +++-
 drivers/char/tpm/tpm_tis.c | 192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 83068fa..4bb9727 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -221,12 +215,6 @@ static int crb_acpi_add(struct acpi_device *device)
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -234,13 +222,15 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -260,11 +250,11 @@ static int crb_acpi_add(struct acpi_device *device)
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..696ef1d 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 

[PATCH 4.2.y-ckt 048/211] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Kamal Mostafa]

4.2.8-ckt1 -stable review patch.  If anyone has any objections, please let me 
know.

--

From: Jarkko Sakkinen 

commit 399235dc6e95400a1322ae92073bc572f0c8 upstream.

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
Reviewed-by: Peter Huewe 
Signed-off-by: Peter Huewe 
Signed-off-by: Kamal Mostafa 
---
 drivers/char/tpm/tpm.h |   7 ++
 drivers/char/tpm/tpm_crb.c |  32 +++-
 drivers/char/tpm/tpm_tis.c | 192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 83068fa..4bb9727 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -221,12 +215,6 @@ static int crb_acpi_add(struct acpi_device *device)
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -234,13 +222,15 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -260,11 +250,11 @@ static int crb_acpi_add(struct acpi_device *device)
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..696ef1d 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * 

Re: [tpmdd-devel] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Valentin Rothberg]

On Oct 21 '15 18:58, Jarkko Sakkinen wrote:
> On Tue, Oct 20, 2015 at 05:58:35PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 20, 2015 at 01:49:02PM +0200, Andreas Ziegler wrote:
> > > Hi Jarkko,
> > > 
> > > your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
> > > showed up as commit 399235dc6e95 in linux-next today (that is,
> > > next-20151020). I noticed it because we (a research group from
> > > Erlangen[0]) are running daily checks on linux-next.
> > > 
> > > Your commit creates the following structure of #ifdef blocks in
> > > drivers/char/tpm/tpm_tis.c following line 1088:
> > > 
> > >  #ifdef CONFIG_ACPI
> > >  ...
> > >  #ifdef CONFIG_PNP
> > >  ...
> > >  #endif
> > >  ...
> > >  #endif
> > > 
> > > Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
> > > 5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
> > > is always enabled if CONFIG_ACPI has been enabled.
> > > Thus, the inner #ifdef statement can never evaluate to 'false' if the
> > > outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
> > > hence, the #ifdef is unnecessary.
> > > 
> > > The same situation holds for the nested structure following line 1124,
> > > where the #ifdef CONFIG_PNP at line 1129 is unnecessary.
> > > 
> > > Is this correct or did we miss something?
> > 
> > Good catch. Shoud I send a separate fix for this? Thanks for pointing
> > this out.
> 
> In all I would cases do a separate fix and do not fixup the original
> patchs because I wouldn't consider this a regression.
> 
> The next question is: will it always be like this? Can I safely assume
> that ACPI will always select PNP unconditionally? This is so minor
> cosmetic glitch in the code that I'm getting second thoughts whether I
> should anything to this or not.

Any option has certain benefits.  In the current form, a reader might
(falsely) assume that the block is configurable.  However, if the
constraints of ACPI or PNP change, the situation might look different.

Best regards,
 Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [tpmdd-devel] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Valentin Rothberg]

On Oct 21 '15 18:58, Jarkko Sakkinen wrote:
> On Tue, Oct 20, 2015 at 05:58:35PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 20, 2015 at 01:49:02PM +0200, Andreas Ziegler wrote:
> > > Hi Jarkko,
> > > 
> > > your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
> > > showed up as commit 399235dc6e95 in linux-next today (that is,
> > > next-20151020). I noticed it because we (a research group from
> > > Erlangen[0]) are running daily checks on linux-next.
> > > 
> > > Your commit creates the following structure of #ifdef blocks in
> > > drivers/char/tpm/tpm_tis.c following line 1088:
> > > 
> > >  #ifdef CONFIG_ACPI
> > >  ...
> > >  #ifdef CONFIG_PNP
> > >  ...
> > >  #endif
> > >  ...
> > >  #endif
> > > 
> > > Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
> > > 5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
> > > is always enabled if CONFIG_ACPI has been enabled.
> > > Thus, the inner #ifdef statement can never evaluate to 'false' if the
> > > outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
> > > hence, the #ifdef is unnecessary.
> > > 
> > > The same situation holds for the nested structure following line 1124,
> > > where the #ifdef CONFIG_PNP at line 1129 is unnecessary.
> > > 
> > > Is this correct or did we miss something?
> > 
> > Good catch. Shoud I send a separate fix for this? Thanks for pointing
> > this out.
> 
> In all I would cases do a separate fix and do not fixup the original
> patchs because I wouldn't consider this a regression.
> 
> The next question is: will it always be like this? Can I safely assume
> that ACPI will always select PNP unconditionally? This is so minor
> cosmetic glitch in the code that I'm getting second thoughts whether I
> should anything to this or not.

Any option has certain benefits.  In the current form, a reader might
(falsely) assume that the block is configurable.  However, if the
constraints of ACPI or PNP change, the situation might look different.

Best regards,
 Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [tpmdd-devel] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Tue, Oct 20, 2015 at 05:58:35PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 20, 2015 at 01:49:02PM +0200, Andreas Ziegler wrote:
> > Hi Jarkko,
> > 
> > your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
> > showed up as commit 399235dc6e95 in linux-next today (that is,
> > next-20151020). I noticed it because we (a research group from
> > Erlangen[0]) are running daily checks on linux-next.
> > 
> > Your commit creates the following structure of #ifdef blocks in
> > drivers/char/tpm/tpm_tis.c following line 1088:
> > 
> >  #ifdef CONFIG_ACPI
> >  ...
> >  #ifdef CONFIG_PNP
> >  ...
> >  #endif
> >  ...
> >  #endif
> > 
> > Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
> > 5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
> > is always enabled if CONFIG_ACPI has been enabled.
> > Thus, the inner #ifdef statement can never evaluate to 'false' if the
> > outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
> > hence, the #ifdef is unnecessary.
> > 
> > The same situation holds for the nested structure following line 1124,
> > where the #ifdef CONFIG_PNP at line 1129 is unnecessary.
> > 
> > Is this correct or did we miss something?
> 
> Good catch. Shoud I send a separate fix for this? Thanks for pointing
> this out.

In all I would cases do a separate fix and do not fixup the original
patchs because I wouldn't consider this a regression.

The next question is: will it always be like this? Can I safely assume
that ACPI will always select PNP unconditionally? This is so minor
cosmetic glitch in the code that I'm getting second thoughts whether I
should anything to this or not.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [tpmdd-devel] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Tue, Oct 20, 2015 at 05:58:35PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 20, 2015 at 01:49:02PM +0200, Andreas Ziegler wrote:
> > Hi Jarkko,
> > 
> > your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
> > showed up as commit 399235dc6e95 in linux-next today (that is,
> > next-20151020). I noticed it because we (a research group from
> > Erlangen[0]) are running daily checks on linux-next.
> > 
> > Your commit creates the following structure of #ifdef blocks in
> > drivers/char/tpm/tpm_tis.c following line 1088:
> > 
> >  #ifdef CONFIG_ACPI
> >  ...
> >  #ifdef CONFIG_PNP
> >  ...
> >  #endif
> >  ...
> >  #endif
> > 
> > Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
> > 5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
> > is always enabled if CONFIG_ACPI has been enabled.
> > Thus, the inner #ifdef statement can never evaluate to 'false' if the
> > outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
> > hence, the #ifdef is unnecessary.
> > 
> > The same situation holds for the nested structure following line 1124,
> > where the #ifdef CONFIG_PNP at line 1129 is unnecessary.
> > 
> > Is this correct or did we miss something?
> 
> Good catch. Shoud I send a separate fix for this? Thanks for pointing
> this out.

In all I would cases do a separate fix and do not fixup the original
patchs because I wouldn't consider this a regression.

The next question is: will it always be like this? Can I safely assume
that ACPI will always select PNP unconditionally? This is so minor
cosmetic glitch in the code that I'm getting second thoughts whether I
should anything to this or not.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Tue, Oct 20, 2015 at 01:49:02PM +0200, Andreas Ziegler wrote:
> Hi Jarkko,
> 
> your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
> showed up as commit 399235dc6e95 in linux-next today (that is,
> next-20151020). I noticed it because we (a research group from
> Erlangen[0]) are running daily checks on linux-next.
> 
> Your commit creates the following structure of #ifdef blocks in
> drivers/char/tpm/tpm_tis.c following line 1088:
> 
>  #ifdef CONFIG_ACPI
>  ...
>  #ifdef CONFIG_PNP
>  ...
>  #endif
>  ...
>  #endif
> 
> Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
> 5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
> is always enabled if CONFIG_ACPI has been enabled.
> Thus, the inner #ifdef statement can never evaluate to 'false' if the
> outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
> hence, the #ifdef is unnecessary.
> 
> The same situation holds for the nested structure following line 1124,
> where the #ifdef CONFIG_PNP at line 1129 is unnecessary.
> 
> Is this correct or did we miss something?

Good catch. Shoud I send a separate fix for this? Thanks for pointing
this out.

> Regards,
> 
> Andreas
> 
> [0] https://cados.cs.fau.de

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Andreas Ziegler]

Hi Jarkko,

your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
showed up as commit 399235dc6e95 in linux-next today (that is,
next-20151020). I noticed it because we (a research group from
Erlangen[0]) are running daily checks on linux-next.

Your commit creates the following structure of #ifdef blocks in
drivers/char/tpm/tpm_tis.c following line 1088:

 #ifdef CONFIG_ACPI
 ...
 #ifdef CONFIG_PNP
 ...
 #endif
 ...
 #endif

Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
is always enabled if CONFIG_ACPI has been enabled.
Thus, the inner #ifdef statement can never evaluate to 'false' if the
outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
hence, the #ifdef is unnecessary.

The same situation holds for the nested structure following line 1124,
where the #ifdef CONFIG_PNP at line 1129 is unnecessary.

Is this correct or did we miss something?

Regards,

Andreas

[0] https://cados.cs.fau.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Andreas Ziegler]

Hi Jarkko,

your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
showed up as commit 399235dc6e95 in linux-next today (that is,
next-20151020). I noticed it because we (a research group from
Erlangen[0]) are running daily checks on linux-next.

Your commit creates the following structure of #ifdef blocks in
drivers/char/tpm/tpm_tis.c following line 1088:

 #ifdef CONFIG_ACPI
 ...
 #ifdef CONFIG_PNP
 ...
 #endif
 ...
 #endif

Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
is always enabled if CONFIG_ACPI has been enabled.
Thus, the inner #ifdef statement can never evaluate to 'false' if the
outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
hence, the #ifdef is unnecessary.

The same situation holds for the nested structure following line 1124,
where the #ifdef CONFIG_PNP at line 1129 is unnecessary.

Is this correct or did we miss something?

Regards,

Andreas

[0] https://cados.cs.fau.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Tue, Oct 20, 2015 at 01:49:02PM +0200, Andreas Ziegler wrote:
> Hi Jarkko,
> 
> your patch "tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0"
> showed up as commit 399235dc6e95 in linux-next today (that is,
> next-20151020). I noticed it because we (a research group from
> Erlangen[0]) are running daily checks on linux-next.
> 
> Your commit creates the following structure of #ifdef blocks in
> drivers/char/tpm/tpm_tis.c following line 1088:
> 
>  #ifdef CONFIG_ACPI
>  ...
>  #ifdef CONFIG_PNP
>  ...
>  #endif
>  ...
>  #endif
> 
> Looking at the definition of CONFIG_ACPI at drivers/acpi/Kconfig, line
> 5, we see that ACPI unconditionally selects PNP, meaning that CONFIG_PNP
> is always enabled if CONFIG_ACPI has been enabled.
> Thus, the inner #ifdef statement can never evaluate to 'false' if the
> outer #ifdef evaluates to true (i.e., CONFIG_ACPI is enabled), and
> hence, the #ifdef is unnecessary.
> 
> The same situation holds for the nested structure following line 1124,
> where the #ifdef CONFIG_PNP at line 1129 is unnecessary.
> 
> Is this correct or did we miss something?

Good catch. Shoud I send a separate fix for this? Thanks for pointing
this out.

> Regards,
> 
> Andreas
> 
> [0] https://cados.cs.fau.de

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 02/10] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
---
 drivers/char/tpm/tpm.h |   7 ++
 drivers/char/tpm/tpm_crb.c |  32 +++-
 drivers/char/tpm/tpm_tis.c | 192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 623fc0a..ded4899 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -221,12 +215,6 @@ static int crb_acpi_add(struct acpi_device *device)
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -234,13 +222,15 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -260,11 +250,11 @@ static int crb_acpi_add(struct acpi_device *device)
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..696ef1d 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct tpm_info {
+   unsigned long start;
+   unsigned long len;
+   unsigned int irq;
+};
+
+static struct tpm_info tis_default_info = {
+   .start = TIS_MEM_BASE,
+   .len = TIS_MEM_LEN,
+   .irq = 0,
+};
 
 /* Some timeout values are 

[PATCH 02/10] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs and acpi_driver only for MSFT0101 in
  order ensure backwards compatibility.

v4:

* Check for FIFO before doing *anything* in crb_acpi_add().
* There was return immediately after acpi_bus_unregister_driver() in
  cleanup_tis(). This caused pnp_unregister_driver() not to be called.

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
Tested-by: Michael Marley 
Tested-by: Mimi Zohar  (on TPM 1.2)
---
 drivers/char/tpm/tpm.h |   7 ++
 drivers/char/tpm/tpm_crb.c |  32 +++-
 drivers/char/tpm/tpm_tis.c | 192 ++---
 3 files changed, 181 insertions(+), 50 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 623fc0a..ded4899 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -221,12 +215,6 @@ static int crb_acpi_add(struct acpi_device *device)
u64 pa;
int rc;
 
-   chip = tpmm_chip_alloc(dev, _crb);
-   if (IS_ERR(chip))
-   return PTR_ERR(chip);
-
-   chip->flags = TPM_CHIP_FLAG_TPM2;
-
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) );
if (ACPI_FAILURE(status)) {
@@ -234,13 +222,15 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
+
+   chip = tpmm_chip_alloc(dev, _crb);
+   if (IS_ERR(chip))
+   return PTR_ERR(chip);
+
+   chip->flags = TPM_CHIP_FLAG_TPM2;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -260,11 +250,11 @@ static int crb_acpi_add(struct acpi_device *device)
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..696ef1d 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct tpm_info {
+   unsigned long start;
+   

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Matthew Garrett]

On Thu, Oct 08, 2015 at 06:04:27PM +0300, Jarkko Sakkinen wrote:
> Hi
> 
> This would need Tested-by's. I've tested it both PTT/fTPM based system
> and dTPM (2.0) based system. Thank you.

I'm on the road at the moment, but I'll be able to test this next week.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

Hi

This would need Tested-by's. I've tested it both PTT/fTPM based system
and dTPM (2.0) based system. Thank you.

/Jarkko

On Wed, Sep 30, 2015 at 06:19:28PM +0300, Jarkko Sakkinen wrote:
> Both for FIFO and CRB interface TCG has decided to use the same HID
> MSFT0101. They can be differentiated by looking at the start method from
> TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.
> 
> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders 
> Reported-by: Michael Marley 
> Reported-by: Jethro Beekman 
> Reported-by: Matthew Garrett 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  drivers/char/tpm/tpm.h |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190 
> ++---
>  3 files changed, 174 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
>   CRB_ACPI_START_INDEX = 1,
>  };
>  
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
>  struct acpi_tpm2 {
>   struct acpi_table_header hdr;
>   u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
>   return -ENODEV;
>   }
>  
> - /* At least some versions of AMI BIOS have a bug that TPM2 table has
> -  * zero address for the control area and therefore we must fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for the 
> control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>  
>   if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
>   dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
>* report only ACPI start but in practice seems to require both
>* ACPI start and CRB start.
>*/
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
>   !strcmp(acpi_device_hid(device), "MSFT0101"))
>   priv->flags |= CRB_FL_CRB_START;
>  
> - if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
>   priv->flags |= CRB_FL_ACPI_START;
>  
>   priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..6efdda1 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2005, 2006 IBM Corporation
> - * Copyright (C) 2014 Intel Corporation
> + * Copyright (C) 2014, 2015 Intel Corporation
>   *
>   * Authors:
>   * Leendert van Doorn 
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,17 @@ enum tis_defaults {
>   TIS_LONG_TIMEOUT = 2000,/* 2 sec */
>  };
>  
> +struct tpm_info {
> + unsigned long start;
> + unsigned long len;
> + unsigned int irq;
> +};
> +
> +static struct tpm_info tis_default_info = {
> + .start = TIS_MEM_BASE,
> + .len = TIS_MEM_LEN,
> + .irq = 0,
> +};
>  
>  /* Some timeout values are needed before it is known whether the chip is
>   * TPM 1.0 or TPM 2.0.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> - struct acpi_device *acpi = pnp_acpi_device(dev);
>   struct acpi_hardware_id *id;
>  
> - if (!acpi)
> - return 0;
> -
> - list_for_each_entry(id, >pnp.ids, list) {
> - if (!strcmp("INTC0102", id->id))
> + 

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

Hi

This would need Tested-by's. I've tested it both PTT/fTPM based system
and dTPM (2.0) based system. Thank you.

/Jarkko

On Wed, Sep 30, 2015 at 06:19:28PM +0300, Jarkko Sakkinen wrote:
> Both for FIFO and CRB interface TCG has decided to use the same HID
> MSFT0101. They can be differentiated by looking at the start method from
> TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.
> 
> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders 
> Reported-by: Michael Marley 
> Reported-by: Jethro Beekman 
> Reported-by: Matthew Garrett 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  drivers/char/tpm/tpm.h |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190 
> ++---
>  3 files changed, 174 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
>   CRB_ACPI_START_INDEX = 1,
>  };
>  
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
>  struct acpi_tpm2 {
>   struct acpi_table_header hdr;
>   u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
>   return -ENODEV;
>   }
>  
> - /* At least some versions of AMI BIOS have a bug that TPM2 table has
> -  * zero address for the control area and therefore we must fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for the 
> control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>  
>   if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
>   dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
>* report only ACPI start but in practice seems to require both
>* ACPI start and CRB start.
>*/
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
>   !strcmp(acpi_device_hid(device), "MSFT0101"))
>   priv->flags |= CRB_FL_CRB_START;
>  
> - if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
>   priv->flags |= CRB_FL_ACPI_START;
>  
>   priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..6efdda1 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2005, 2006 IBM Corporation
> - * Copyright (C) 2014 Intel Corporation
> + * Copyright (C) 2014, 2015 Intel Corporation
>   *
>   * Authors:
>   * Leendert van Doorn 
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,17 @@ enum tis_defaults {
>   TIS_LONG_TIMEOUT = 2000,/* 2 sec */
>  };
>  
> +struct tpm_info {
> + unsigned long start;
> + unsigned long len;
> + unsigned int irq;
> +};
> +
> +static struct tpm_info tis_default_info = {
> + .start = TIS_MEM_BASE,
> + .len = TIS_MEM_LEN,
> + .irq = 0,
> +};
>  
>  /* Some timeout values are needed before it is known whether the chip is
>   * TPM 1.0 or TPM 2.0.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> - struct acpi_device *acpi = pnp_acpi_device(dev);
>   struct acpi_hardware_id *id;
>  
> -   

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Matthew Garrett]

On Thu, Oct 08, 2015 at 06:04:27PM +0300, Jarkko Sakkinen wrote:
> Hi
> 
> This would need Tested-by's. I've tested it both PTT/fTPM based system
> and dTPM (2.0) based system. Thank you.

I'm on the road at the moment, but I'll be able to test this next week.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Wed, Sep 30, 2015 at 06:19:28PM +0300, Jarkko Sakkinen wrote:
> Both for FIFO and CRB interface TCG has decided to use the same HID
> MSFT0101. They can be differentiated by looking at the start method from
> TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.
> 
> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders 
> Reported-by: Michael Marley 
> Reported-by: Jethro Beekman 
> Reported-by: Matthew Garrett 
> Signed-off-by: Jarkko Sakkinen 

Tested-by: Michael Marley  ?

/Jarkko

> ---
>  drivers/char/tpm/tpm.h |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190 
> ++---
>  3 files changed, 174 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
>   CRB_ACPI_START_INDEX = 1,
>  };
>  
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
>  struct acpi_tpm2 {
>   struct acpi_table_header hdr;
>   u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
>   return -ENODEV;
>   }
>  
> - /* At least some versions of AMI BIOS have a bug that TPM2 table has
> -  * zero address for the control area and therefore we must fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for the 
> control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>  
>   if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
>   dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
>* report only ACPI start but in practice seems to require both
>* ACPI start and CRB start.
>*/
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
>   !strcmp(acpi_device_hid(device), "MSFT0101"))
>   priv->flags |= CRB_FL_CRB_START;
>  
> - if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
>   priv->flags |= CRB_FL_ACPI_START;
>  
>   priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..6efdda1 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2005, 2006 IBM Corporation
> - * Copyright (C) 2014 Intel Corporation
> + * Copyright (C) 2014, 2015 Intel Corporation
>   *
>   * Authors:
>   * Leendert van Doorn 
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,17 @@ enum tis_defaults {
>   TIS_LONG_TIMEOUT = 2000,/* 2 sec */
>  };
>  
> +struct tpm_info {
> + unsigned long start;
> + unsigned long len;
> + unsigned int irq;
> +};
> +
> +static struct tpm_info tis_default_info = {
> + .start = TIS_MEM_BASE,
> + .len = TIS_MEM_LEN,
> + .irq = 0,
> +};
>  
>  /* Some timeout values are needed before it is known whether the chip is
>   * TPM 1.0 or TPM 2.0.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> - struct acpi_device *acpi = pnp_acpi_device(dev);
>   struct acpi_hardware_id *id;
>  
> - if (!acpi)
> - return 0;
> -
> - list_for_each_entry(id, >pnp.ids, list) {
> - if (!strcmp("INTC0102", id->id))
> + list_for_each_entry(id, >pnp.ids, list)
> + if (!strcmp(hid, id->id))
>   

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Wed, 2015-09-30 at 18:19 +0300, Jarkko Sakkinen wrote:
> Both for FIFO and CRB interface TCG has decided to use the same HID
> MSFT0101. They can be differentiated by looking at the start method
> from
> TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.

This would require testing with TPM 1.2. I don't have a machine/env
right now for 1.2 (I'm going to setup one with old Dell laptop as 
soon as I have the time). Is there anyone who could verify this with
1.2?

/Jarkko

> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders 
> Reported-by: Michael Marley 
> Reported-by: Jethro Beekman 
> Reported-by: Matthew Garrett 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  drivers/char/tpm/tpm.h |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190
> ++---
>  3 files changed, 174 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
>   CRB_ACPI_START_INDEX = 1,
>  };
>  
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
>  struct acpi_tpm2 {
>   struct acpi_table_header hdr;
>   u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device
> *device)
>   return -ENODEV;
>   }
>  
> - /* At least some versions of AMI BIOS have a bug that TPM2
> table has
> -  * zero address for the control area and therefore we must
> fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for
> the control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>  
>   if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
>   dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device
> *device)
>* report only ACPI start but in practice seems to require
> both
>* ACPI start and CRB start.
>*/
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
>   !strcmp(acpi_device_hid(device), "MSFT0101"))
>   priv->flags |= CRB_FL_CRB_START;
>  
> - if (sm == CRB_SM_ACPI_START || sm ==
> CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
>   priv->flags |= CRB_FL_ACPI_START;
>  
>   priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..6efdda1 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2005, 2006 IBM Corporation
> - * Copyright (C) 2014 Intel Corporation
> + * Copyright (C) 2014, 2015 Intel Corporation
>   *
>   * Authors:
>   * Leendert van Doorn 
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,17 @@ enum tis_defaults {
>   TIS_LONG_TIMEOUT = 2000,/* 2 sec */
>  };
>  
> +struct tpm_info {
> + unsigned long start;
> + unsigned long len;
> + unsigned int irq;
> +};
> +
> +static struct tpm_info tis_default_info = {
> + .start = TIS_MEM_BASE,
> + .len = TIS_MEM_LEN,
> + .irq = 0,
> +};
>  
>  /* Some timeout values are needed before it is known whether the
> chip is
>   * TPM 1.0 or TPM 2.0.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> - struct acpi_device *acpi = pnp_acpi_device(dev);
>   struct acpi_hardware_id *id;
>  
> - if (!acpi)
> - return 0;
> -
> - 

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Wed, 2015-09-30 at 18:19 +0300, Jarkko Sakkinen wrote:
> Both for FIFO and CRB interface TCG has decided to use the same HID
> MSFT0101. They can be differentiated by looking at the start method
> from
> TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.

This would require testing with TPM 1.2. I don't have a machine/env
right now for 1.2 (I'm going to setup one with old Dell laptop as 
soon as I have the time). Is there anyone who could verify this with
1.2?

/Jarkko

> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders 
> Reported-by: Michael Marley 
> Reported-by: Jethro Beekman 
> Reported-by: Matthew Garrett 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  drivers/char/tpm/tpm.h |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190
> ++---
>  3 files changed, 174 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
>   CRB_ACPI_START_INDEX = 1,
>  };
>  
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
>  struct acpi_tpm2 {
>   struct acpi_table_header hdr;
>   u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device
> *device)
>   return -ENODEV;
>   }
>  
> - /* At least some versions of AMI BIOS have a bug that TPM2
> table has
> -  * zero address for the control area and therefore we must
> fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for
> the control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>  
>   if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
>   dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device
> *device)
>* report only ACPI start but in practice seems to require
> both
>* ACPI start and CRB start.
>*/
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
>   !strcmp(acpi_device_hid(device), "MSFT0101"))
>   priv->flags |= CRB_FL_CRB_START;
>  
> - if (sm == CRB_SM_ACPI_START || sm ==
> CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
>   priv->flags |= CRB_FL_ACPI_START;
>  
>   priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..6efdda1 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2005, 2006 IBM Corporation
> - * Copyright (C) 2014 Intel Corporation
> + * Copyright (C) 2014, 2015 Intel Corporation
>   *
>   * Authors:
>   * Leendert van Doorn 
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,17 @@ enum tis_defaults {
>   TIS_LONG_TIMEOUT = 2000,/* 2 sec */
>  };
>  
> +struct tpm_info {
> + unsigned long start;
> + unsigned long len;
> + unsigned int irq;
> +};
> +
> +static struct tpm_info tis_default_info = {
> + .start = TIS_MEM_BASE,
> + .len = TIS_MEM_LEN,
> + .irq = 0,
> +};
>  
>  /* Some timeout values are needed before it is known whether the
> chip is
>   * TPM 1.0 or TPM 2.0.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> -  

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Wed, Sep 30, 2015 at 06:19:28PM +0300, Jarkko Sakkinen wrote:
> Both for FIFO and CRB interface TCG has decided to use the same HID
> MSFT0101. They can be differentiated by looking at the start method from
> TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.
> 
> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders 
> Reported-by: Michael Marley 
> Reported-by: Jethro Beekman 
> Reported-by: Matthew Garrett 
> Signed-off-by: Jarkko Sakkinen 

Tested-by: Michael Marley  ?

/Jarkko

> ---
>  drivers/char/tpm/tpm.h |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190 
> ++---
>  3 files changed, 174 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
>   CRB_ACPI_START_INDEX = 1,
>  };
>  
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
>  struct acpi_tpm2 {
>   struct acpi_table_header hdr;
>   u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
>   return -ENODEV;
>   }
>  
> - /* At least some versions of AMI BIOS have a bug that TPM2 table has
> -  * zero address for the control area and therefore we must fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for the 
> control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>  
>   if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
>   dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
>* report only ACPI start but in practice seems to require both
>* ACPI start and CRB start.
>*/
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
>   !strcmp(acpi_device_hid(device), "MSFT0101"))
>   priv->flags |= CRB_FL_CRB_START;
>  
> - if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
>   priv->flags |= CRB_FL_ACPI_START;
>  
>   priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..6efdda1 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2005, 2006 IBM Corporation
> - * Copyright (C) 2014 Intel Corporation
> + * Copyright (C) 2014, 2015 Intel Corporation
>   *
>   * Authors:
>   * Leendert van Doorn 
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,17 @@ enum tis_defaults {
>   TIS_LONG_TIMEOUT = 2000,/* 2 sec */
>  };
>  
> +struct tpm_info {
> + unsigned long start;
> + unsigned long len;
> + unsigned int irq;
> +};
> +
> +static struct tpm_info tis_default_info = {
> + .start = TIS_MEM_BASE,
> + .len = TIS_MEM_LEN,
> + .irq = 0,
> +};
>  
>  /* Some timeout values are needed before it is known whether the chip is
>   * TPM 1.0 or TPM 2.0.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> - struct acpi_device *acpi = pnp_acpi_device(dev);
>   struct acpi_hardware_id *id;
>  
> - if (!acpi)
> - return 0;
> -
> - 

[PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm.h |   7 ++
 drivers/char/tpm/tpm_crb.c |  20 ++---
 drivers/char/tpm/tpm_tis.c | 190 ++---
 3 files changed, 174 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..6efdda1 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct tpm_info {
+   unsigned long start;
+   unsigned long len;
+   unsigned int irq;
+};
+
+static struct tpm_info tis_default_info = {
+   .start = TIS_MEM_BASE,
+   .len = TIS_MEM_LEN,
+   .irq = 0,
+};
 
 /* Some timeout values are needed before it is known whether the chip is
  * TPM 1.0 or TPM 2.0.
@@ -91,26 +103,54 @@ struct priv_data {
 };
 
 #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
-static int is_itpm(struct pnp_dev *dev)
+static int has_hid(struct acpi_device *dev, const char *hid)
 {
-   struct acpi_device *acpi = pnp_acpi_device(dev);
struct acpi_hardware_id *id;
 
-   if (!acpi)
-   return 0;
-
-   list_for_each_entry(id, >pnp.ids, list) {
-   if (!strcmp("INTC0102", id->id))
+   list_for_each_entry(id, >pnp.ids, list)
+   if (!strcmp(hid, id->id))
return 1;
-   }
 
return 0;
 }
+
+static inline int is_itpm(struct acpi_device *dev)
+{
+   return has_hid(dev, "INTC0102");
+}
+
+static inline int is_fifo(struct acpi_device *dev)
+{
+   struct acpi_table_tpm2 *tbl;
+   acpi_status st;
+
+   /* TPM 1.2 FIFO */

[PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method from
TPM2 ACPI table. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs

Reported-by: Michael Saunders 
Reported-by: Michael Marley 
Reported-by: Jethro Beekman 
Reported-by: Matthew Garrett 
Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm.h |   7 ++
 drivers/char/tpm/tpm_crb.c |  20 ++---
 drivers/char/tpm/tpm_tis.c | 190 ++---
 3 files changed, 174 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
 };
 
-enum crb_start_method {
-   CRB_SM_ACPI_START = 2,
-   CRB_SM_CRB = 7,
-   CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
 struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
 
-   /* At least some versions of AMI BIOS have a bug that TPM2 table has
-* zero address for the control area and therefore we must fail.
-   */
-   if (!buf->control_area_pa) {
-   dev_err(dev, "TPM2 ACPI table has a zero address for the 
control area\n");
-   return -EINVAL;
-   }
+   /* Should the FIFO driver handle this? */
+   if (buf->start_method == TPM2_START_FIFO)
+   return -ENODEV;
 
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
 * report only ACPI start but in practice seems to require both
 * ACPI start and CRB start.
 */
-   if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+   if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
 
-   if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+   if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
 
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..6efdda1 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn 
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
 };
 
+struct tpm_info {
+   unsigned long start;
+   unsigned long len;
+   unsigned int irq;
+};
+
+static struct tpm_info tis_default_info = {
+   .start = TIS_MEM_BASE,
+   .len = TIS_MEM_LEN,
+   .irq = 0,
+};
 
 /* Some timeout values are needed before it is known whether the chip is
  * TPM 1.0 or TPM 2.0.
@@ -91,26 +103,54 @@ struct priv_data {
 };
 
 #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
-static int is_itpm(struct pnp_dev *dev)
+static int has_hid(struct acpi_device *dev, const char *hid)
 {
-   struct acpi_device *acpi = pnp_acpi_device(dev);
struct acpi_hardware_id *id;
 
-   if (!acpi)
-   return 0;
-
-   list_for_each_entry(id, >pnp.ids, list) {
-   if (!strcmp("INTC0102", id->id))
+   list_for_each_entry(id, >pnp.ids, list)
+   if (!strcmp(hid, id->id))
return 1;
-   }
 
return 0;
 }
+
+static inline int is_itpm(struct acpi_device *dev)
+{
+   return has_hid(dev, "INTC0102");