RE: [PATCH 4.14 020/140] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

From: Shaikh, Azhar
Date: Tue Mar 13 2018 - 20:42:18 EST

Next message: Shaikh, Azhar: "RE: [PATCH 4.15 118/146] tpm_tis: Move ilb_base_addr to tpm_tis_data"
Previous message: Pavel Tatashin: "Re: [PATCH v2 1/2] mm: uninitialized struct page poisoning sanity checking"
In reply to: Greg Kroah-Hartman: "[PATCH 4.14 020/140] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()"
Next in thread: Greg Kroah-Hartman: "[PATCH 4.14 022/140] tpm: remove unused variables"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

No objections from my side.

Please merge.

Regards,
Azhar Shaikh

>-----Original Message-----
>From: Greg Kroah-Hartman [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
>Sent: Tuesday, March 13, 2018 8:24 AM
>To: linux-kernel@xxxxxxxxxxxxxxx
>Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>;
>stable@xxxxxxxxxxxxxxx; Shaikh, Azhar <azhar.shaikh@xxxxxxxxx>; Jarkko
>Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
>Subject: [PATCH 4.14 020/140] tpm: Keep CLKRUN enabled throughout the
>duration of transmit_cmd()
>
>4.14-stable review patch. If anyone has any objections, please let me know.
>
>------------------
>
>From: Azhar Shaikh <azhar.shaikh@xxxxxxxxx>
>
>commit b3e958ce4c585bf666de249dc794971ebc62d2d3 upstream.
>
>Commit 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
>systems") disabled CLKRUN protocol during TPM transactions and re-enabled
>once the transaction is completed. But there were still some corner cases
>observed where, reading of TPM header failed for savestate command while
>going to suspend, which resulted in suspend failure.
>To fix this issue keep the CLKRUN protocol disabled for the entire duration of
>a single TPM command and not disabling and re-enabling again for every TPM
>transaction. For the other TPM accesses outside TPM command flow, add a
>higher level of disabling and re-enabling the CLKRUN protocol, instead of
>doing for every TPM transaction.
>
>Fixes: 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
>systems")
>Signed-off-by: Azhar Shaikh <azhar.shaikh@xxxxxxxxx>
>Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
>Tested-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
>Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
>---
> drivers/char/tpm/tpm-interface.c | 6 ++
> drivers/char/tpm/tpm_tis.c | 92 +++------------------------------
> drivers/char/tpm/tpm_tis_core.c | 108
>+++++++++++++++++++++++++++++++++++----
> drivers/char/tpm/tpm_tis_core.h | 4 +
> include/linux/tpm.h | 1
> 5 files changed, 119 insertions(+), 92 deletions(-)
>
>--- a/drivers/char/tpm/tpm-interface.c
>+++ b/drivers/char/tpm/tpm-interface.c
>@@ -413,6 +413,9 @@ ssize_t tpm_transmit(struct tpm_chip *ch
> if (chip->dev.parent)
> pm_runtime_get_sync(chip->dev.parent);
>
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, true);
>+
> /* Store the decision as chip->locality will be changed. */
> need_locality = chip->locality == -1;
>
>@@ -489,6 +492,9 @@ out:
> chip->locality = -1;
> }
> out_no_locality:
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, false);
>+
> if (chip->dev.parent)
> pm_runtime_put_sync(chip->dev.parent);
>
>--- a/drivers/char/tpm/tpm_tis.c
>+++ b/drivers/char/tpm/tpm_tis.c
>@@ -132,79 +132,17 @@ static int check_acpi_tpm2(struct device } #endif
>
>-#ifdef CONFIG_X86
>-#define LPC_CNTRL_OFFSET 0x84
>-#define LPC_CLKRUN_EN (1 << 2)
>-
>-/**
>- * tpm_platform_begin_xfer() - clear LPC CLKRUN_EN i.e. clocks will be
>running
>- */
>-static void tpm_platform_begin_xfer(struct tpm_tis_data *data) -{
>- u32 clkrun_val;
>-
>- if (!is_bsw())
>- return;
>-
>- clkrun_val = ioread32(data->ilb_base_addr + LPC_CNTRL_OFFSET);
>-
>- /* Disable LPC CLKRUN# */
>- clkrun_val &= ~LPC_CLKRUN_EN;
>- iowrite32(clkrun_val, data->ilb_base_addr + LPC_CNTRL_OFFSET);
>-
>- /*
>- * Write any random value on port 0x80 which is on LPC, to make
>- * sure LPC clock is running before sending any TPM command.
>- */
>- outb(0xCC, 0x80);
>-
>-}
>-
>-/**
>- * tpm_platform_end_xfer() - set LPC CLKRUN_EN i.e. clocks can be turned
>off
>- */
>-static void tpm_platform_end_xfer(struct tpm_tis_data *data) -{
>- u32 clkrun_val;
>-
>- if (!is_bsw())
>- return;
>-
>- clkrun_val = ioread32(data->ilb_base_addr + LPC_CNTRL_OFFSET);
>-
>- /* Enable LPC CLKRUN# */
>- clkrun_val |= LPC_CLKRUN_EN;
>- iowrite32(clkrun_val, data->ilb_base_addr + LPC_CNTRL_OFFSET);
>-
>- /*
>- * Write any random value on port 0x80 which is on LPC, to make
>- * sure LPC clock is running before sending any TPM command.
>- */
>- outb(0xCC, 0x80);
>-
>-}
>-#else
>-static void tpm_platform_begin_xfer(struct tpm_tis_data *data) -{ -}
>-
>-static void tpm_platform_end_xfer(struct tpm_tis_data *data) -{ -} -#endif
>-
> static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
> u8 *result)
> {
> struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>- tpm_platform_begin_xfer(data);
>+ if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>+ WARN(1, "CLKRUN not enabled!\n");
>
> while (len--)
> *result++ = ioread8(phy->iobase + addr);
>
>- tpm_platform_end_xfer(data);
>-
> return 0;
> }
>
>@@ -213,13 +151,12 @@ static int tpm_tcg_write_bytes(struct tp {
> struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>- tpm_platform_begin_xfer(data);
>+ if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>+ WARN(1, "CLKRUN not enabled!\n");
>
> while (len--)
> iowrite8(*value++, phy->iobase + addr);
>
>- tpm_platform_end_xfer(data);
>-
> return 0;
> }
>
>@@ -227,12 +164,11 @@ static int tpm_tcg_read16(struct tpm_tis {
> struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>- tpm_platform_begin_xfer(data);
>+ if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>+ WARN(1, "CLKRUN not enabled!\n");
>
> *result = ioread16(phy->iobase + addr);
>
>- tpm_platform_end_xfer(data);
>-
> return 0;
> }
>
>@@ -240,12 +176,11 @@ static int tpm_tcg_read32(struct tpm_tis {
> struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>- tpm_platform_begin_xfer(data);
>+ if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>+ WARN(1, "CLKRUN not enabled!\n");
>
> *result = ioread32(phy->iobase + addr);
>
>- tpm_platform_end_xfer(data);
>-
> return 0;
> }
>
>@@ -253,12 +188,11 @@ static int tpm_tcg_write32(struct tpm_ti {
> struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>- tpm_platform_begin_xfer(data);
>+ if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>+ WARN(1, "CLKRUN not enabled!\n");
>
> iowrite32(value, phy->iobase + addr);
>
>- tpm_platform_end_xfer(data);
>-
> return 0;
> }
>
>@@ -340,9 +274,6 @@ static void tpm_tis_pnp_remove(struct pn
>
> tpm_chip_unregister(chip);
> tpm_tis_remove(chip);
>- if (is_bsw())
>- iounmap(priv->ilb_base_addr);
>-
> }
>
> static struct pnp_driver tis_pnp_driver = { @@ -394,9 +325,6 @@ static int
>tpm_tis_plat_remove(struct pl
> tpm_chip_unregister(chip);
> tpm_tis_remove(chip);
>
>- if (is_bsw())
>- iounmap(priv->ilb_base_addr);
>-
> return 0;
> }
>
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -31,6 +31,8 @@
> #include "tpm.h"
> #include "tpm_tis_core.h"
>
>+static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
>+
> /* Before we attempt to access the TPM we must see that the valid bit is set.
> * The specification says that this bit is 0 at reset and remains 0 until the
> * 'TPM has gone through its self test and initialization and has established
>@@ -422,19 +424,28 @@ static bool tpm_tis_update_timeouts(stru
> int i, rc;
> u32 did_vid;
>
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, true);
>+
> rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid);
> if (rc < 0)
>- return rc;
>+ goto out;
>
> for (i = 0; i != ARRAY_SIZE(vendor_timeout_overrides); i++) {
> if (vendor_timeout_overrides[i].did_vid != did_vid)
> continue;
> memcpy(timeout_cap,
>vendor_timeout_overrides[i].timeout_us,
> sizeof(vendor_timeout_overrides[i].timeout_us));
>- return true;
>+ rc = true;
> }
>
>- return false;
>+ rc = false;
>+
>+out:
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, false);
>+
>+ return rc;
> }
>
> /*
>@@ -654,14 +665,74 @@ void tpm_tis_remove(struct tpm_chip *chi
> u32 interrupt;
> int rc;
>
>+ tpm_tis_clkrun_enable(chip, true);
>+
> rc = tpm_tis_read32(priv, reg, &interrupt);
> if (rc < 0)
> interrupt = 0;
>
> tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
>+
>+ tpm_tis_clkrun_enable(chip, false);
>+
>+ if (priv->ilb_base_addr)
>+ iounmap(priv->ilb_base_addr);
> }
> EXPORT_SYMBOL_GPL(tpm_tis_remove);
>
>+/**
>+ * tpm_tis_clkrun_enable() - Keep clkrun protocol disabled for entire
>duration
>+ * of a single TPM command
>+ * @chip: TPM chip to use
>+ * @value: 1 - Disable CLKRUN protocol, so that clocks are free running
>+ * 0 - Enable CLKRUN protocol
>+ * Call this function directly in tpm_tis_remove() in error or driver
>+removal
>+ * path, since the chip->ops is set to NULL in tpm_chip_unregister().
>+ */
>+static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) {
>+ struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
>+ u32 clkrun_val;
>+
>+ if (!IS_ENABLED(CONFIG_X86) || !is_bsw())
>+ return;
>+
>+ if (value) {
>+ data->flags |= TPM_TIS_CLK_ENABLE;
>+ data->clkrun_enabled++;
>+ if (data->clkrun_enabled > 1)
>+ return;
>+ clkrun_val = ioread32(data->ilb_base_addr +
>LPC_CNTRL_OFFSET);
>+
>+ /* Disable LPC CLKRUN# */
>+ clkrun_val &= ~LPC_CLKRUN_EN;
>+ iowrite32(clkrun_val, data->ilb_base_addr +
>LPC_CNTRL_OFFSET);
>+
>+ /*
>+ * Write any random value on port 0x80 which is on LPC, to
>make
>+ * sure LPC clock is running before sending any TPM
>command.
>+ */
>+ outb(0xCC, 0x80);
>+ } else {
>+ data->clkrun_enabled--;
>+ if (data->clkrun_enabled)
>+ return;
>+
>+ clkrun_val = ioread32(data->ilb_base_addr +
>LPC_CNTRL_OFFSET);
>+
>+ /* Enable LPC CLKRUN# */
>+ clkrun_val |= LPC_CLKRUN_EN;
>+ iowrite32(clkrun_val, data->ilb_base_addr +
>LPC_CNTRL_OFFSET);
>+
>+ /*
>+ * Write any random value on port 0x80 which is on LPC, to
>make
>+ * sure LPC clock is running before sending any TPM
>command.
>+ */
>+ outb(0xCC, 0x80);
>+ data->flags &= ~TPM_TIS_CLK_ENABLE;
>+ }
>+}
>+
> static const struct tpm_class_ops tpm_tis = {
> .flags = TPM_OPS_AUTO_STARTUP,
> .status = tpm_tis_status,
>@@ -674,6 +745,7 @@ static const struct tpm_class_ops tpm_ti
> .req_canceled = tpm_tis_req_canceled,
> .request_locality = request_locality,
> .relinquish_locality = release_locality,
>+ .clk_enable = tpm_tis_clkrun_enable,
> };
>
> int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>@@ -708,6 +780,9 @@ int tpm_tis_core_init(struct device *dev
> return -ENOMEM;
> }
>
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, true);
>+
> if (wait_startup(chip, 0) != 0) {
> rc = -ENODEV;
> goto out_err;
>@@ -799,14 +874,18 @@ int tpm_tis_core_init(struct device *dev
> }
>
> rc = tpm_chip_register(chip);
>- if (rc && is_bsw())
>- iounmap(priv->ilb_base_addr);
>+ if (rc)
>+ goto out_err;
>
>- return rc;
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, false);
>+
>+ return 0;
> out_err:
>+ if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL))
>+ chip->ops->clk_enable(chip, false);
>+
> tpm_tis_remove(chip);
>- if (is_bsw())
>- iounmap(priv->ilb_base_addr);
>
> return rc;
> }
>@@ -819,22 +898,31 @@ static void tpm_tis_reenable_interrupts(
> u32 intmask;
> int rc;
>
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, true);
>+
> /* reenable interrupts that device may have lost or
> * BIOS/firmware may have disabled
> */
> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), priv->irq);
> if (rc < 0)
>- return;
>+ goto out;
>
> rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality),
>&intmask);
> if (rc < 0)
>- return;
>+ goto out;
>
> intmask |= TPM_INTF_CMD_READY_INT
> | TPM_INTF_LOCALITY_CHANGE_INT |
>TPM_INTF_DATA_AVAIL_INT
> | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
>
> tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>+
>+out:
>+ if (chip->ops->clk_enable != NULL)
>+ chip->ops->clk_enable(chip, false);
>+
>+ return;
> }
>
> int tpm_tis_resume(struct device *dev)
>--- a/drivers/char/tpm/tpm_tis_core.h
>+++ b/drivers/char/tpm/tpm_tis_core.h
>@@ -79,11 +79,14 @@ enum tis_defaults {
> #define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
> #define TPM_RID(l) (0x0F04 | ((l) << 12))
>
>+#define LPC_CNTRL_OFFSET 0x84
>+#define LPC_CLKRUN_EN (1 << 2)
> #define INTEL_LEGACY_BLK_BASE_ADDR 0xFED08000
> #define ILB_REMAP_SIZE 0x100
>
> enum tpm_tis_flags {
> TPM_TIS_ITPM_WORKAROUND = BIT(0),
>+ TPM_TIS_CLK_ENABLE = BIT(1),
> };
>
> struct tpm_tis_data {
>@@ -93,6 +96,7 @@ struct tpm_tis_data {
> bool irq_tested;
> unsigned int flags;
> void __iomem *ilb_base_addr;
>+ u16 clkrun_enabled;
> wait_queue_head_t int_queue;
> wait_queue_head_t read_queue;
> const struct tpm_tis_phy_ops *phy_ops;
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -50,6 +50,7 @@ struct tpm_class_ops {
> unsigned long *timeout_cap);
> int (*request_locality)(struct tpm_chip *chip, int loc);
> void (*relinquish_locality)(struct tpm_chip *chip, int loc);
>+ void (*clk_enable)(struct tpm_chip *chip, bool value);
> };
>
> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
>

Next message: Shaikh, Azhar: "RE: [PATCH 4.15 118/146] tpm_tis: Move ilb_base_addr to tpm_tis_data"
Previous message: Pavel Tatashin: "Re: [PATCH v2 1/2] mm: uninitialized struct page poisoning sanity checking"
In reply to: Greg Kroah-Hartman: "[PATCH 4.14 020/140] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()"
Next in thread: Greg Kroah-Hartman: "[PATCH 4.14 022/140] tpm: remove unused variables"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]