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

["Shaikh, Azhar" <azhar.shaikh@intel.com>]

No objections from my side.

Please merge.

Regards,
Azhar Shaikh

>-----Original Message-----
>From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
>Sent: Tuesday, March 13, 2018 8:24 AM
>To: linux-kernel@vger.kernel.org
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>;
>stable@vger.kernel.org; Shaikh, Azhar <azhar.shaikh@intel.com>; Jarkko
>Sakkinen <jarkko.sakkinen@linux.intel.com>
>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@intel.com>
>
>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@intel.com>
>Reviewed-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
>Tested-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
>Signed-off-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
>Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
>---
> 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)
>