[PATCH RFC v3 0/2] Fix corner cases with disabling CLKRUN in tpm_tis

[PATCH RFC v3 0/2] Fix corner cases with disabling CLKRUN in tpm_tis

[Azhar Shaikh]

Changes from v1:
- Patch 1: "tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()"
  - Add NULL checks before calling clk_toggle callback
  - Use IS_ENABLED instead of ifdef in tpm_tis_clkrun_toggle()
  - Do not call tpm_platform_begin_xfer() and tpm_platform_end_xfer()
    from tpm_tis_clkrun_toggle(). Make them static again.

- Patch 2: "tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy"
  - This is a new patch in this series as per suggestion from Jason.
  - Is the current implementation ok or I should move the code in tpm_tis_pnp_remove()
    and tpm_tis_plat_remove() inside tpm_tis_remove(). That way all the unmapping
    can be done in one place, instead of 3 different places now. Also the unmapping
    in tpm_tis_init() can be moved to tpm_tis_remove(), since in case of error
    tpm_tis_core_init() calls tpm_tis_remove(). Kindly suggest.

Changes from v2:
- Patch 1: "tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()"
  - No changes

- Patch 2: "tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy"
  - Updated is_bsw() function to have the #ifdef CONFIG_X86 check within the function
    itself. Also removed the #ifdef CONFIG_X86 from all other places around is_bsw()

Azhar Shaikh (2):
  tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
  tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

 drivers/char/tpm/tpm-interface.c |   6 +++
 drivers/char/tpm/tpm_tis.c       | 111 ++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm_tis_core.c  |  21 ++++++++
 drivers/char/tpm/tpm_tis_core.h  |   1 +
 include/linux/tpm.h              |   1 +
 5 files changed, 93 insertions(+), 47 deletions(-)

-- 
1.9.1

[PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Azhar Shaikh]

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.

Fixes: 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell systems")

Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
---
 drivers/char/tpm/tpm-interface.c |  6 ++++++
 drivers/char/tpm/tpm_tis.c       | 44 ++++++++++++++++++++++++----------------
 drivers/char/tpm/tpm_tis_core.c  | 21 +++++++++++++++++++
 drivers/char/tpm/tpm_tis_core.h  |  1 +
 include/linux/tpm.h              |  1 +
 5 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1d6729be4cd6..17f0d468e3e4 100644
--- 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 *chip, struct tpm_space *space,
 	if (chip->dev.parent)
 		pm_runtime_get_sync(chip->dev.parent);
 
+	if (chip->ops->clk_toggle != NULL)
+		chip->ops->clk_toggle(chip, true);
+
 	/* Store the decision as chip->locality will be changed. */
 	need_locality = chip->locality == -1;
 
@@ -489,6 +492,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		chip->locality = -1;
 	}
 out_no_locality:
+	if (chip->ops->clk_toggle != NULL)
+		chip->ops->clk_toggle(chip, false);
+
 	if (chip->dev.parent)
 		pm_runtime_put_sync(chip->dev.parent);
 
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index e2d1055fb814..76a7b64195c8 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -46,6 +46,7 @@ struct tpm_info {
 struct tpm_tis_tcg_phy {
 	struct tpm_tis_data priv;
 	void __iomem *iobase;
+	bool begin_xfer_done;
 };
 
 static inline struct tpm_tis_tcg_phy *to_tpm_tis_tcg_phy(struct tpm_tis_data *data)
@@ -148,12 +149,15 @@ static inline bool is_bsw(void)
 
 /**
  * tpm_platform_begin_xfer() - clear LPC CLKRUN_EN i.e. clocks will be running
+ * @data:	struct tpm_tis_data instance
  */
-static void tpm_platform_begin_xfer(void)
+static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
 {
 	u32 clkrun_val;
+	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
 
-	if (!is_bsw())
+	if (!is_bsw() || ((data->flags & TPM_TIS_CLK_ENABLE) &&
+					phy->begin_xfer_done))
 		return;
 
 	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
@@ -168,16 +172,21 @@ static void tpm_platform_begin_xfer(void)
 	 */
 	outb(0xCC, 0x80);
 
+	if (!(data->flags & TPM_TIS_CLK_ENABLE))
+		phy->begin_xfer_done = false;
+	else
+		phy->begin_xfer_done = true;
 }
 
 /**
  * tpm_platform_end_xfer() - set LPC CLKRUN_EN i.e. clocks can be turned off
+ * @data:	struct tpm_tis_data instance
  */
-static void tpm_platform_end_xfer(void)
+static void tpm_platform_end_xfer(struct tpm_tis_data *data)
 {
 	u32 clkrun_val;
 
-	if (!is_bsw())
+	if (!is_bsw() || (data->flags & TPM_TIS_CLK_ENABLE))
 		return;
 
 	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
@@ -193,17 +202,16 @@ static void tpm_platform_end_xfer(void)
 	outb(0xCC, 0x80);
 
 }
+
 #else
 static inline bool is_bsw(void)
 {
 	return false;
 }
-
-static void tpm_platform_begin_xfer(void)
+static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
 {
 }
-
-static void tpm_platform_end_xfer(void)
+static void tpm_platform_end_xfer(struct tpm_tis_data *data)
 {
 }
 #endif
@@ -213,12 +221,12 @@ static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
 {
 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
 
-	tpm_platform_begin_xfer();
+	tpm_platform_begin_xfer(data);
 
 	while (len--)
 		*result++ = ioread8(phy->iobase + addr);
 
-	tpm_platform_end_xfer();
+	tpm_platform_end_xfer(data);
 
 	return 0;
 }
@@ -228,12 +236,12 @@ static int tpm_tcg_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
 {
 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
 
-	tpm_platform_begin_xfer();
+	tpm_platform_begin_xfer(data);
 
 	while (len--)
 		iowrite8(*value++, phy->iobase + addr);
 
-	tpm_platform_end_xfer();
+	tpm_platform_end_xfer(data);
 
 	return 0;
 }
@@ -242,11 +250,11 @@ static int tpm_tcg_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
 {
 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
 
-	tpm_platform_begin_xfer();
+	tpm_platform_begin_xfer(data);
 
 	*result = ioread16(phy->iobase + addr);
 
-	tpm_platform_end_xfer();
+	tpm_platform_end_xfer(data);
 
 	return 0;
 }
@@ -255,11 +263,11 @@ static int tpm_tcg_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
 {
 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
 
-	tpm_platform_begin_xfer();
+	tpm_platform_begin_xfer(data);
 
 	*result = ioread32(phy->iobase + addr);
 
-	tpm_platform_end_xfer();
+	tpm_platform_end_xfer(data);
 
 	return 0;
 }
@@ -268,11 +276,11 @@ static int tpm_tcg_write32(struct tpm_tis_data *data, u32 addr, u32 value)
 {
 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
 
-	tpm_platform_begin_xfer();
+	tpm_platform_begin_xfer(data);
 
 	iowrite32(value, phy->iobase + addr);
 
-	tpm_platform_end_xfer();
+	tpm_platform_end_xfer(data);
 
 	return 0;
 }
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fdde971bc810..673d4fe2e87e 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -661,6 +661,26 @@ void tpm_tis_remove(struct tpm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(tpm_tis_remove);
 
+/**
+ * tpm_tis_clkrun_toggle() - 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
+ */
+static void tpm_tis_clkrun_toggle(struct tpm_chip *chip, bool value)
+{
+	struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
+
+	if (!IS_ENABLED(CONFIG_X86))
+		return;
+
+	if (value)
+		data->flags |= TPM_TIS_CLK_ENABLE;
+	else
+		data->flags &= ~TPM_TIS_CLK_ENABLE;
+}
+
 static const struct tpm_class_ops tpm_tis = {
 	.flags = TPM_OPS_AUTO_STARTUP,
 	.status = tpm_tis_status,
@@ -673,6 +693,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
 	.req_canceled = tpm_tis_req_canceled,
 	.request_locality = request_locality,
 	.relinquish_locality = release_locality,
+	.clk_toggle = tpm_tis_clkrun_toggle,
 };
 
 int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 6bbac319ff3b..281f744a1a44 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -81,6 +81,7 @@ enum tis_defaults {
 
 enum tpm_tis_flags {
 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
+	TPM_TIS_CLK_ENABLE		= BIT(1),
 };
 
 struct tpm_tis_data {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 5a090f5ab335..10753ec56d0a 100644
--- 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_toggle)(struct tpm_chip *chip, bool value);
 };
 
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
-- 
1.9.1

[PATCH RFC v3 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Azhar Shaikh]

Move the static variable ilb_base_addr to tpm_tis_tcg_phy.

Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
---
 drivers/char/tpm/tpm_tis.c | 67 ++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 76a7b64195c8..d87b37c5404b 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -46,6 +46,7 @@ struct tpm_info {
 struct tpm_tis_tcg_phy {
 	struct tpm_tis_data priv;
 	void __iomem *iobase;
+	void __iomem *ilb_base_addr;
 	bool begin_xfer_done;
 };
 
@@ -134,19 +135,22 @@ static int check_acpi_tpm2(struct device *dev)
 }
 #endif
 
+static inline bool is_bsw(void)
+{
 #ifdef CONFIG_X86
+	return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1 : 0);
+#else
+	return false;
+#endif
+}
+
 #define INTEL_LEGACY_BLK_BASE_ADDR      0xFED08000
 #define ILB_REMAP_SIZE			0x100
+
+#ifdef CONFIG_X86
 #define LPC_CNTRL_REG_OFFSET            0x84
 #define LPC_CLKRUN_EN                   (1 << 2)
 
-static void __iomem *ilb_base_addr;
-
-static inline bool is_bsw(void)
-{
-	return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1 : 0);
-}
-
 /**
  * tpm_platform_begin_xfer() - clear LPC CLKRUN_EN i.e. clocks will be running
  * @data:	struct tpm_tis_data instance
@@ -160,11 +164,11 @@ static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
 					phy->begin_xfer_done))
 		return;
 
-	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	clkrun_val = ioread32(phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
 
 	/* Disable LPC CLKRUN# */
 	clkrun_val &= ~LPC_CLKRUN_EN;
-	iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	iowrite32(clkrun_val, phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
 
 	/*
 	 * Write any random value on port 0x80 which is on LPC, to make
@@ -185,15 +189,16 @@ static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
 static void tpm_platform_end_xfer(struct tpm_tis_data *data)
 {
 	u32 clkrun_val;
+	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
 
 	if (!is_bsw() || (data->flags & TPM_TIS_CLK_ENABLE))
 		return;
 
-	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	clkrun_val = ioread32(phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
 
 	/* Enable LPC CLKRUN# */
 	clkrun_val |= LPC_CLKRUN_EN;
-	iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	iowrite32(clkrun_val, phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
 
 	/*
 	 * Write any random value on port 0x80 which is on LPC, to make
@@ -204,10 +209,6 @@ static void tpm_platform_end_xfer(struct tpm_tis_data *data)
 }
 
 #else
-static inline bool is_bsw(void)
-{
-	return false;
-}
 static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
 {
 }
@@ -311,14 +312,25 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
 	if (IS_ERR(phy->iobase))
 		return PTR_ERR(phy->iobase);
 
+	if (is_bsw()) {
+		phy->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
+					ILB_REMAP_SIZE);
+		if (!phy->ilb_base_addr)
+			return -ENOMEM;
+	}
+
 	if (interrupts)
 		irq = tpm_info->irq;
 
 	if (itpm || is_itpm(ACPI_COMPANION(dev)))
 		phy->priv.flags |= TPM_TIS_ITPM_WORKAROUND;
 
-	return tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
+	rc = tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
 				 ACPI_HANDLE(dev));
+	if (rc && is_bsw())
+		iounmap(phy->ilb_base_addr);
+
+	return rc;
 }
 
 static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
@@ -359,9 +371,14 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 static void tpm_tis_pnp_remove(struct pnp_dev *dev)
 {
 	struct tpm_chip *chip = pnp_get_drvdata(dev);
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(priv);
 
 	tpm_chip_unregister(chip);
 	tpm_tis_remove(chip);
+
+	if (is_bsw())
+		iounmap(phy->ilb_base_addr);
 }
 
 static struct pnp_driver tis_pnp_driver = {
@@ -408,10 +425,15 @@ static int tpm_tis_plat_probe(struct platform_device *pdev)
 static int tpm_tis_plat_remove(struct platform_device *pdev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(priv);
 
 	tpm_chip_unregister(chip);
 	tpm_tis_remove(chip);
 
+	if (is_bsw())
+		iounmap(phy->ilb_base_addr);
+
 	return 0;
 }
 
@@ -469,11 +491,6 @@ static int __init init_tis(void)
 	if (rc)
 		goto err_force;
 
-#ifdef CONFIG_X86
-	if (is_bsw())
-		ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
-					ILB_REMAP_SIZE);
-#endif
 	rc = platform_driver_register(&tis_drv);
 	if (rc)
 		goto err_platform;
@@ -492,10 +509,6 @@ static int __init init_tis(void)
 err_platform:
 	if (force_pdev)
 		platform_device_unregister(force_pdev);
-#ifdef CONFIG_X86
-	if (is_bsw())
-		iounmap(ilb_base_addr);
-#endif
 err_force:
 	return rc;
 }
@@ -505,10 +518,6 @@ static void __exit cleanup_tis(void)
 	pnp_unregister_driver(&tis_pnp_driver);
 	platform_driver_unregister(&tis_drv);
 
-#ifdef CONFIG_X86
-	if (is_bsw())
-		iounmap(ilb_base_addr);
-#endif
 	if (force_pdev)
 		platform_device_unregister(force_pdev);
 }
-- 
1.9.1

RE: [PATCH RFC v3 0/2] Fix corner cases with disabling CLKRUN in tpm_tis

[Shaikh, Azhar]

Hi Jarkko, Jason,

Could you please provide your review comments on this patchset.

Thank you!

Regards,
Azhar Shaikh

>-----Original Message-----
>From: Shaikh, Azhar
>Sent: Wednesday, November 15, 2017 2:07 PM
>To: jarkko.sakkinen@linux.intel.com; jgunthorpe@obsidianresearch.com;
>peterhuewe@gmx.de; linux-integrity@vger.kernel.org
>Cc: Shaikh, Azhar <azhar.shaikh@intel.com>
>Subject: [PATCH RFC v3 0/2] Fix corner cases with disabling CLKRUN in tpm_tis
>
>Changes from v1:
>- Patch 1: "tpm: Keep CLKRUN enabled throughout the duration of
>transmit_cmd()"
>  - Add NULL checks before calling clk_toggle callback
>  - Use IS_ENABLED instead of ifdef in tpm_tis_clkrun_toggle()
>  - Do not call tpm_platform_begin_xfer() and tpm_platform_end_xfer()
>    from tpm_tis_clkrun_toggle(). Make them static again.
>
>- Patch 2: "tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy"
>  - This is a new patch in this series as per suggestion from Jason.
>  - Is the current implementation ok or I should move the code in
>tpm_tis_pnp_remove()
>    and tpm_tis_plat_remove() inside tpm_tis_remove(). That way all the
>unmapping
>    can be done in one place, instead of 3 different places now. Also the
>unmapping
>    in tpm_tis_init() can be moved to tpm_tis_remove(), since in case of error
>    tpm_tis_core_init() calls tpm_tis_remove(). Kindly suggest.
>
>Changes from v2:
>- Patch 1: "tpm: Keep CLKRUN enabled throughout the duration of
>transmit_cmd()"
>  - No changes
>
>- Patch 2: "tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy"
>  - Updated is_bsw() function to have the #ifdef CONFIG_X86 check within the
>function
>    itself. Also removed the #ifdef CONFIG_X86 from all other places around
>is_bsw()
>
>Azhar Shaikh (2):
>  tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
>  tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy
>
> drivers/char/tpm/tpm-interface.c |   6 +++
> drivers/char/tpm/tpm_tis.c       | 111 ++++++++++++++++++++++-----------------
> drivers/char/tpm/tpm_tis_core.c  |  21 ++++++++
> drivers/char/tpm/tpm_tis_core.h  |   1 +
> include/linux/tpm.h              |   1 +
> 5 files changed, 93 insertions(+), 47 deletions(-)
>
>--
>1.9.1

Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Jason Gunthorpe]

On Wed, Nov 15, 2017 at 02:07:11PM -0800, Azhar Shaikh wrote:

> -static void tpm_platform_begin_xfer(void)
> +static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
>  {
>  	u32 clkrun_val;
> +	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>  
> -	if (!is_bsw())
> +	if (!is_bsw() || ((data->flags & TPM_TIS_CLK_ENABLE) &&
> +					phy->begin_xfer_done))
>  		return;

I think everything looks OK now, but I was reading over the series
again, and I admit I don't quite get it..

Why do we continue to have tpm_platform_begin_xfer after you added
clk_toggle?

Why not just directly enable CLK_RUN in clk_toggle and get rid of
tpm_platform_begin_xfer/etc ??

Is there some reason we still need to to per transfer stuff???

clk_enable or device_enable would also be a better name than
clock_toggle.

Jason

RE: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Shaikh, Azhar]

>-----Original Message-----
>From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>Sent: Monday, November 20, 2017 10:35 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>
>Cc: jarkko.sakkinen@linux.intel.com; peterhuewe@gmx.de; linux-
>integrity@vger.kernel.org
>Subject: Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the
>duration of transmit_cmd()
>
>On Wed, Nov 15, 2017 at 02:07:11PM -0800, Azhar Shaikh wrote:
>
>> -static void tpm_platform_begin_xfer(void)
>> +static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
>>  {
>>  	u32 clkrun_val;
>> +	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>>
>> -	if (!is_bsw())
>> +	if (!is_bsw() || ((data->flags & TPM_TIS_CLK_ENABLE) &&
>> +					phy->begin_xfer_done))
>>  		return;
>
>I think everything looks OK now, but I was reading over the series again, and I
>admit I don't quite get it..
>
>Why do we continue to have tpm_platform_begin_xfer after you added
>clk_toggle?
>

We want to have the CLKRUN disabled for any/all TPM transactions.
The clk_toggle handles only the case while a TPM command is being sent and received.
We have to take into consideration other places too where TPM access is happening outside the TPM command flow. For eg: request_locality, check_locality, release_locality, wait_startup which might be called outside the flow of a TPM command.

>Why not just directly enable CLK_RUN in clk_toggle and get rid of
>tpm_platform_begin_xfer/etc ??
>
>Is there some reason we still need to to per transfer stuff???
>
>clk_enable or device_enable would also be a better name than clock_toggle.
>

Will change it to clk_enable.
Should I then upload the next patch for review and remove the "RFC" tag now? And if so, should I retain the change history of the patch versions?

>Jason

Regards,
Azhar Shaikh

Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Jason Gunthorpe]

On Mon, Nov 20, 2017 at 06:52:13PM +0000, Shaikh, Azhar wrote:

> We want to have the CLKRUN disabled for any/all TPM transactions.
> The clk_toggle handles only the case while a TPM command is being
> sent and received.  We have to take into consideration other places
> too where TPM access is happening outside the TPM command flow. For
> eg: request_locality, check_locality, release_locality, wait_startup
> which might be called outside the flow of a TPM command.

Okay, this makes sense, and would be good to touch on in the commit
description if it stays this way, IMHO.

However, why not have check_locality, release_locality, wait_startup
use clk_toggle instead? That seems better to me??

> Will change it to clk_enable.  Should I then upload the next patch
> for review and remove the "RFC" tag now? And if so, should I retain
> the change history of the patch versions?

Yes for both.

I think we are well past the RFC stage now :)

Jason

RE: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Shaikh, Azhar]

>-----Original Message-----
>From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
>owner@vger.kernel.org] On Behalf Of Jason Gunthorpe
>Sent: Monday, November 20, 2017 11:21 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>
>Cc: jarkko.sakkinen@linux.intel.com; peterhuewe@gmx.de; linux-
>integrity@vger.kernel.org
>Subject: Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the
>duration of transmit_cmd()
>
>On Mon, Nov 20, 2017 at 06:52:13PM +0000, Shaikh, Azhar wrote:
>
>> We want to have the CLKRUN disabled for any/all TPM transactions.
>> The clk_toggle handles only the case while a TPM command is being sent
>> and received.  We have to take into consideration other places too
>> where TPM access is happening outside the TPM command flow. For
>> eg: request_locality, check_locality, release_locality, wait_startup
>> which might be called outside the flow of a TPM command.
>
>Okay, this makes sense, and would be good to touch on in the commit
>description if it stays this way, IMHO.

Sure, I will add this in the commit message.

>
>However, why not have check_locality, release_locality, wait_startup use
>clk_toggle instead? That seems better to me??
>

I think, better to have the clk disable/enable at one place instead of adding it all  locations where TPM access is done. The clk_toggle is kind of a special case where, for the complete duration of the TPM command processing, the CLKRUN protocol was supposed to be disabled. For rest of the places we can disable and re-enable the clkrun as soon as possible.

Also since it is part of the read/write APIs we won't miss any other location in the code where TPM access is done.

>> Will change it to clk_enable.  Should I then upload the next patch for
>> review and remove the "RFC" tag now? And if so, should I retain the
>> change history of the patch versions?
>
>Yes for both.
>
>I think we are well past the RFC stage now :)
>

Thank you :)
I will post the next patchset and remove the RFC tag.

>Jason

Regards,
Azhar Shaikh

Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Jason Gunthorpe]

On Mon, Nov 20, 2017 at 07:34:07PM +0000, Shaikh, Azhar wrote:

> >However, why not have check_locality, release_locality, wait_startup use
> >clk_toggle instead? That seems better to me??
> >
> 
> I think, better to have the clk disable/enable at one place instead
> of adding it all locations where TPM access is done. The clk_toggle
> is kind of a special case where, for the complete duration of the
> TPM command processing, the CLKRUN protocol was supposed to be
> disabled. For rest of the places we can disable and re-enable the
> clkrun as soon as possible.

> Also since it is part of the read/write APIs we won't miss any other
> location in the code where TPM access is done.

But I wonder if you will hit the same bug that motivated this change
in the other places?

It seems clearer to me to have a strong guard of the clock across the
entire high level action, since this hardware is so broken.

You could add a debug to ensure that read/write is never without
clk_enable being held.

Jason

RE: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Shaikh, Azhar]

>-----Original Message-----
>From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>Sent: Monday, November 20, 2017 11:38 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>
>Cc: jarkko.sakkinen@linux.intel.com; peterhuewe@gmx.de; linux-
>integrity@vger.kernel.org
>Subject: Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the
>duration of transmit_cmd()
>
>On Mon, Nov 20, 2017 at 07:34:07PM +0000, Shaikh, Azhar wrote:
>
>> >However, why not have check_locality, release_locality, wait_startup
>> >use clk_toggle instead? That seems better to me??
>> >
>>
>> I think, better to have the clk disable/enable at one place instead of
>> adding it all locations where TPM access is done. The clk_toggle is
>> kind of a special case where, for the complete duration of the TPM
>> command processing, the CLKRUN protocol was supposed to be disabled.
>> For rest of the places we can disable and re-enable the clkrun as soon
>> as possible.
>
>> Also since it is part of the read/write APIs we won't miss any other
>> location in the code where TPM access is done.
>
>But I wonder if you will hit the same bug that motivated this change in the
>other places?
>

I guess not, since, I have tested this patch on 6 devices which completed 10000 suspend/resume cycles successfully.
This is also one of the reason to keep tpm_platform_begin_xfer and tpm_platform_end_xfer functions.

>It seems clearer to me to have a strong guard of the clock across the entire
>high level action, since this hardware is so broken.
>

So, I should remove the tpm_platform_begin_xfer and tpm_platform_end_xfer functions and have that code in clk_toggle(clk_enable).
And then for having  high level action, I will have to add 

+if (chip->ops->clk_toggle != NULL)
+               chip->ops->clk_toggle(chip, true);
..
..
<snip>
+       if (chip->ops->clk_toggle != NULL)
+               chip->ops->clk_toggle(chip, false);


In 
-> tpm_tis_core_init()
-> tpm_tis_plat_remove()
-> tpm_tis_reenable_interrupts()
-> tpm_tis_update_timeouts()
-> tpm_transmit_cmd() [ Already implemented in this patch ]

>You could add a debug to ensure that read/write is never without clk_enable
>being held.
>

Sorry, but I didn't get this, adding a debug part.

>Jason

Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Jarkko Sakkinen]

On Wed, Nov 15, 2017 at 02:07:11PM -0800, Azhar Shaikh wrote:
> 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.
> 
> 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.om>

/Jarkko

Re: [PATCH RFC v3 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Jarkko Sakkinen]

On Wed, Nov 15, 2017 at 02:07:12PM -0800, Azhar Shaikh wrote:
> Move the static variable ilb_base_addr to tpm_tis_tcg_phy.
> 
> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis.c | 67 ++++++++++++++++++++++++++--------------------
>  1 file changed, 38 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 76a7b64195c8..d87b37c5404b 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -46,6 +46,7 @@ struct tpm_info {
>  struct tpm_tis_tcg_phy {
>  	struct tpm_tis_data priv;
>  	void __iomem *iobase;
> +	void __iomem *ilb_base_addr;
>  	bool begin_xfer_done;
>  };
>  
> @@ -134,19 +135,22 @@ static int check_acpi_tpm2(struct device *dev)
>  }
>  #endif
>  
> +static inline bool is_bsw(void)
> +{
>  #ifdef CONFIG_X86
> +	return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1 : 0);
> +#else
> +	return false;
> +#endif
> +}
> +
>  #define INTEL_LEGACY_BLK_BASE_ADDR      0xFED08000
>  #define ILB_REMAP_SIZE			0x100
> +
> +#ifdef CONFIG_X86
>  #define LPC_CNTRL_REG_OFFSET            0x84
>  #define LPC_CLKRUN_EN                   (1 << 2)
>  
> -static void __iomem *ilb_base_addr;
> -
> -static inline bool is_bsw(void)
> -{
> -	return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1 : 0);
> -}
> -
>  /**
>   * tpm_platform_begin_xfer() - clear LPC CLKRUN_EN i.e. clocks will be running
>   * @data:	struct tpm_tis_data instance
> @@ -160,11 +164,11 @@ static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
>  					phy->begin_xfer_done))
>  		return;
>  
> -	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
> +	clkrun_val = ioread32(phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
>  
>  	/* Disable LPC CLKRUN# */
>  	clkrun_val &= ~LPC_CLKRUN_EN;
> -	iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET);
> +	iowrite32(clkrun_val, phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
>  
>  	/*
>  	 * Write any random value on port 0x80 which is on LPC, to make
> @@ -185,15 +189,16 @@ static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
>  static void tpm_platform_end_xfer(struct tpm_tis_data *data)
>  {
>  	u32 clkrun_val;
> +	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>  
>  	if (!is_bsw() || (data->flags & TPM_TIS_CLK_ENABLE))
>  		return;
>  
> -	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
> +	clkrun_val = ioread32(phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
>  
>  	/* Enable LPC CLKRUN# */
>  	clkrun_val |= LPC_CLKRUN_EN;
> -	iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET);
> +	iowrite32(clkrun_val, phy->ilb_base_addr + LPC_CNTRL_REG_OFFSET);
>  
>  	/*
>  	 * Write any random value on port 0x80 which is on LPC, to make
> @@ -204,10 +209,6 @@ static void tpm_platform_end_xfer(struct tpm_tis_data *data)
>  }
>  
>  #else
> -static inline bool is_bsw(void)
> -{
> -	return false;
> -}
>  static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
>  {
>  }
> @@ -311,14 +312,25 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
>  	if (IS_ERR(phy->iobase))
>  		return PTR_ERR(phy->iobase);
>  
> +	if (is_bsw()) {
> +		phy->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
> +					ILB_REMAP_SIZE);
> +		if (!phy->ilb_base_addr)
> +			return -ENOMEM;
> +	}
> +
>  	if (interrupts)
>  		irq = tpm_info->irq;
>  
>  	if (itpm || is_itpm(ACPI_COMPANION(dev)))
>  		phy->priv.flags |= TPM_TIS_ITPM_WORKAROUND;
>  
> -	return tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
> +	rc = tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
>  				 ACPI_HANDLE(dev));
> +	if (rc && is_bsw())
> +		iounmap(phy->ilb_base_addr);
> +
> +	return rc;
>  }
>  
>  static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
> @@ -359,9 +371,14 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  static void tpm_tis_pnp_remove(struct pnp_dev *dev)
>  {
>  	struct tpm_chip *chip = pnp_get_drvdata(dev);
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(priv);
>  
>  	tpm_chip_unregister(chip);
>  	tpm_tis_remove(chip);
> +
> +	if (is_bsw())
> +		iounmap(phy->ilb_base_addr);
>  }
>  
>  static struct pnp_driver tis_pnp_driver = {
> @@ -408,10 +425,15 @@ static int tpm_tis_plat_probe(struct platform_device *pdev)
>  static int tpm_tis_plat_remove(struct platform_device *pdev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(priv);
>  
>  	tpm_chip_unregister(chip);
>  	tpm_tis_remove(chip);
>  
> +	if (is_bsw())
> +		iounmap(phy->ilb_base_addr);
> +
>  	return 0;
>  }
>  
> @@ -469,11 +491,6 @@ static int __init init_tis(void)
>  	if (rc)
>  		goto err_force;
>  
> -#ifdef CONFIG_X86
> -	if (is_bsw())
> -		ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
> -					ILB_REMAP_SIZE);
> -#endif
>  	rc = platform_driver_register(&tis_drv);
>  	if (rc)
>  		goto err_platform;
> @@ -492,10 +509,6 @@ static int __init init_tis(void)
>  err_platform:
>  	if (force_pdev)
>  		platform_device_unregister(force_pdev);
> -#ifdef CONFIG_X86
> -	if (is_bsw())
> -		iounmap(ilb_base_addr);
> -#endif
>  err_force:
>  	return rc;
>  }
> @@ -505,10 +518,6 @@ static void __exit cleanup_tis(void)
>  	pnp_unregister_driver(&tis_pnp_driver);
>  	platform_driver_unregister(&tis_drv);
>  
> -#ifdef CONFIG_X86
> -	if (is_bsw())
> -		iounmap(ilb_base_addr);
> -#endif
>  	if (force_pdev)
>  		platform_device_unregister(force_pdev);
>  }
> -- 
> 1.9.1
> 

Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Jarkko Sakkinen]

On Mon, Nov 20, 2017 at 06:52:13PM +0000, Shaikh, Azhar wrote:
> 
> 
> >-----Original Message-----
> >From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
> >Sent: Monday, November 20, 2017 10:35 AM
> >To: Shaikh, Azhar <azhar.shaikh@intel.com>
> >Cc: jarkko.sakkinen@linux.intel.com; peterhuewe@gmx.de; linux-
> >integrity@vger.kernel.org
> >Subject: Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the
> >duration of transmit_cmd()
> >
> >On Wed, Nov 15, 2017 at 02:07:11PM -0800, Azhar Shaikh wrote:
> >
> >> -static void tpm_platform_begin_xfer(void)
> >> +static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
> >>  {
> >>  	u32 clkrun_val;
> >> +	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
> >>
> >> -	if (!is_bsw())
> >> +	if (!is_bsw() || ((data->flags & TPM_TIS_CLK_ENABLE) &&
> >> +					phy->begin_xfer_done))
> >>  		return;
> >
> >I think everything looks OK now, but I was reading over the series again, and I
> >admit I don't quite get it..
> >
> >Why do we continue to have tpm_platform_begin_xfer after you added
> >clk_toggle?
> >
> 
> We want to have the CLKRUN disabled for any/all TPM transactions.
> The clk_toggle handles only the case while a TPM command is being sent and received.
> We have to take into consideration other places too where TPM access is happening outside the TPM command flow. For eg: request_locality, check_locality, release_locality, wait_startup which might be called outside the flow of a TPM command.
> 
> >Why not just directly enable CLK_RUN in clk_toggle and get rid of
> >tpm_platform_begin_xfer/etc ??
> >
> >Is there some reason we still need to to per transfer stuff???
> >
> >clk_enable or device_enable would also be a better name than clock_toggle.
> >
> 
> Will change it to clk_enable.
> Should I then upload the next patch for review and remove the "RFC" tag now? And if so, should I retain the change history of the patch versions?

Please do and keep my reviewed-by.

/Jarkko

Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Jason Gunthorpe]

On Mon, Nov 20, 2017 at 09:19:37PM +0000, Shaikh, Azhar wrote:

> -> tpm_tis_core_init()

Yes, and IIRC, this covers tpm_tis_update_timeouts() too?

> -> tpm_tis_plat_remove()

This should be in tpm_tis_remove and a little rework would be needed
here

> -> tpm_tis_reenable_interrupts()
> -> tpm_transmit_cmd() [ Already implemented in this patch ]

Yes

> >You could add a debug to ensure that read/write is never without clk_enable
> >being held.
> 
> Sorry, but I didn't get this, adding a debug part.

Just check a flag in the read/write functions to see if clkrun is on
and WARN if not

Not sure if this is going to be overall better than what you have or
not.. But considering the nature of the bug it seems safer to have a
wider range when CLKRUN is working?

Jason

RE: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Shaikh, Azhar]

>-----Original Message-----
>From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>Sent: Monday, November 20, 2017 3:26 PM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>
>Cc: jarkko.sakkinen@linux.intel.com; peterhuewe@gmx.de; linux-
>integrity@vger.kernel.org
>Subject: Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the
>duration of transmit_cmd()
>
>On Mon, Nov 20, 2017 at 09:19:37PM +0000, Shaikh, Azhar wrote:
>
>> -> tpm_tis_core_init()
>
>Yes, and IIRC, this covers tpm_tis_update_timeouts() too?
>

tpm_tis_update_timeouts() doesn't seem to be covered by tpm_tis_core_init(). I cannot find tpm_tis_update_timeouts() called from any function.

>> -> tpm_tis_plat_remove()
>
>This should be in tpm_tis_remove and a little rework would be needed here
>
>> -> tpm_tis_reenable_interrupts()
>> -> tpm_transmit_cmd() [ Already implemented in this patch ]
>
>Yes
>
>> >You could add a debug to ensure that read/write is never without
>> >clk_enable being held.
>>
>> Sorry, but I didn't get this, adding a debug part.
>
>Just check a flag in the read/write functions to see if clkrun is on and WARN if
>not
>
>Not sure if this is going to be overall better than what you have or not.. But
>considering the nature of the bug it seems safer to have a wider range when
>CLKRUN is working?
>

Having a wider range would mean clocks running for a bit longer duration than current implementation, which might have power implications, minimal though, would be good if we could avoid it from here :-)
Also since this patch has already passed rigorous testing as mentioned earlier, hence would be good if we have the current implementation as it is.

>Jason

Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()

[Jason Gunthorpe]

On Tue, Nov 21, 2017 at 07:18:25PM +0000, Shaikh, Azhar wrote:

> Having a wider range would mean clocks running for a bit longer
> duration than current implementation, which might have power
> implications, minimal though, would be good if we could avoid it
> from here :-) Also since this patch has already passed rigorous
> testing as mentioned earlier, hence would be good if we have the
> current implementation as it is.

I'll leave it to you then!

Jason