[v8,1/2] tpm_tis: Move ilb_base_addr to tpm_tis_data

Commit Message

Azhar Shaikh Jan. 2, 2018, 4:05 a.m. UTC

Move static variable ilb_base_addr to tpm_tis_data.

Signed-off-by: Azhar Shaikh <[e-mail]>
Reviewed-by: Jarkko Sakkinen  <[e-mail]>
Tested-by: Jarkko Sakkinen  <[e-mail]>
Signed-off-by: Jarkko Sakkinen  <[e-mail]>
---
 drivers/char/tpm/tpm_tis.c      | 75 +++++++++++++++--------------------------
 drivers/char/tpm/tpm_tis_core.c | 16 ++++++++-
 drivers/char/tpm/tpm_tis_core.h | 13 +++++++
 3 files changed, 56 insertions(+), 48 deletions(-)

Comments

Jason Gunthorpe Jan. 2, 2018, 4:40 a.m. UTC | #1

On Mon, Jan 01, 2018 at 08:05:43PM -0800, Azhar Shaikh wrote:

> -	return tpm_chip_register(chip);
> +	rc = tpm_chip_register(chip);
> +	if (rc && is_bsw())
> +		iounmap(priv->ilb_base_addr);
> +
> +	return rc;
>  out_err:
>  	tpm_tis_remove(chip);
> +	if (is_bsw())
> +		iounmap(priv->ilb_base_addr);
> +
>  	return rc;
>  }

I thought you were resending this to fix the above, why is is it still
wonky?

Jason

Azhar Shaikh Jan. 2, 2018, 5:01 a.m. UTC | #2

>-----Original Message-----
>From: Jason Gunthorpe [mailto:[e-mail]]
>Sent: Monday, January 1, 2018 8:41 PM
>To: Shaikh, Azhar <[e-mail]>
>Cc: [e-mail]; [e-mail]; linux-security-
>[e-mail]; [e-mail]; linux-
>[e-mail]
>Subject: Re: [PATCH v8 1/2] tpm_tis: Move ilb_base_addr to tpm_tis_data
>
>On Mon, Jan 01, 2018 at 08:05:43PM -0800, Azhar Shaikh wrote:
>
>> -	return tpm_chip_register(chip);
>> +	rc = tpm_chip_register(chip);
>> +	if (rc && is_bsw())
>> +		iounmap(priv->ilb_base_addr);
>> +
>> +	return rc;
>>  out_err:
>>  	tpm_tis_remove(chip);
>> +	if (is_bsw())
>> +		iounmap(priv->ilb_base_addr);
>> +
>>  	return rc;
>>  }
>
>I thought you were resending this to fix the above, why is is it still wonky?
>

Oh! I fixed it in the second patch ("tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()"). Should have fixed in this patch instead. My bad, didn't see it was introduced in this patch. I thought it was part of second patch. Will fix this.

>Jason

Azhar Shaikh Jan. 2, 2018, 5:31 a.m. UTC | #3

>-----Original Message-----
>From: [e-mail] [mailto:linux-integrity-
>[e-mail]] On Behalf Of Shaikh, Azhar
>Sent: Monday, January 1, 2018 9:02 PM
>To: Jason Gunthorpe <[e-mail]>
>Cc: [e-mail]; [e-mail]; linux-security-
>[e-mail]; [e-mail]; linux-
>[e-mail]
>Subject: RE: [PATCH v8 1/2] tpm_tis: Move ilb_base_addr to tpm_tis_data
>
>
>
>>-----Original Message-----
>>From: Jason Gunthorpe [mailto:[e-mail]]
>>Sent: Monday, January 1, 2018 8:41 PM
>>To: Shaikh, Azhar <[e-mail]>
>>Cc: [e-mail]; [e-mail]; linux-security-
>>[e-mail]; [e-mail]; linux-
>>[e-mail]
>>Subject: Re: [PATCH v8 1/2] tpm_tis: Move ilb_base_addr to tpm_tis_data
>>
>>On Mon, Jan 01, 2018 at 08:05:43PM -0800, Azhar Shaikh wrote:
>>
>>> -	return tpm_chip_register(chip);
>>> +	rc = tpm_chip_register(chip);
>>> +	if (rc && is_bsw())
>>> +		iounmap(priv->ilb_base_addr);
>>> +
>>> +	return rc;
>>>  out_err:
>>>  	tpm_tis_remove(chip);
>>> +	if (is_bsw())
>>> +		iounmap(priv->ilb_base_addr);
>>> +
>>>  	return rc;
>>>  }
>>
>>I thought you were resending this to fix the above, why is is it still wonky?
>>
>
>Oh! I fixed it in the second patch ("tpm: Keep CLKRUN enabled throughout
>the duration of transmit_cmd()"). Should have fixed in this patch instead. My
>bad, didn't see it was introduced in this patch. I thought it was part of second
>patch. Will fix this.
>

On checking, the ops->clk_enable is not still introduced yet. It is introduced in the second patch. The cleanup is done in the next patch.
So do you want me to still fix this here?

>>Jason

Regards,
Azhar Shaikh

Jason Gunthorpe Jan. 2, 2018, 2:57 p.m. UTC | #4

On Tue, Jan 02, 2018 at 05:31:30AM +0000, Shaikh, Azhar wrote:
> >>On Mon, Jan 01, 2018 at 08:05:43PM -0800, Azhar Shaikh wrote:
> >>
> >>> -	return tpm_chip_register(chip);
> >>> +	rc = tpm_chip_register(chip);
> >>> +	if (rc && is_bsw())
> >>> +		iounmap(priv->ilb_base_addr);
> >>> +
> >>> +	return rc;
> >>>  out_err:
> >>>  	tpm_tis_remove(chip);
> >>> +	if (is_bsw())
> >>> +		iounmap(priv->ilb_base_addr);
> >>> +
> >>>  	return rc;
> >>>  }
> >>
> >>I thought you were resending this to fix the above, why is is it still wonky?
> >>
> >
> >Oh! I fixed it in the second patch ("tpm: Keep CLKRUN enabled throughout
> >the duration of transmit_cmd()"). Should have fixed in this patch instead. My
> >bad, didn't see it was introduced in this patch. I thought it was part of second
> >patch. Will fix this.
> >
> 
> On checking, the ops->clk_enable is not still introduced yet. It is introduced in the second patch. The cleanup is done in the next patch.
> So do you want me to still fix this here?

Try hard to not edit your own patches in your series. No reason why
iounmap cannot be put into tpm_tis_remove in the first place

Jason

Patch

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index e2d1055fb814..923f8f2cbaca 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -134,33 +134,24 @@  static int check_acpi_tpm2(struct device *dev)
 #endif
 
 #ifdef CONFIG_X86
-#define INTEL_LEGACY_BLK_BASE_ADDR      0xFED08000
-#define ILB_REMAP_SIZE			0x100
-#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);
-}
+#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(void)
+static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
 {
 	u32 clkrun_val;
 
 	if (!is_bsw())
 		return;
 
-	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	clkrun_val = ioread32(data->ilb_base_addr + LPC_CNTRL_OFFSET);
 
 	/* Disable LPC CLKRUN# */
 	clkrun_val &= ~LPC_CLKRUN_EN;
-	iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	iowrite32(clkrun_val, data->ilb_base_addr + LPC_CNTRL_OFFSET);
 
 	/*
 	 * Write any random value on port 0x80 which is on LPC, to make
@@ -173,18 +164,18 @@  static void tpm_platform_begin_xfer(void)
 /**
  * tpm_platform_end_xfer() - set LPC CLKRUN_EN i.e. clocks can be turned off
  */
-static void tpm_platform_end_xfer(void)
+static void tpm_platform_end_xfer(struct tpm_tis_data *data)
 {
 	u32 clkrun_val;
 
 	if (!is_bsw())
 		return;
 
-	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	clkrun_val = ioread32(data->ilb_base_addr + LPC_CNTRL_OFFSET);
 
 	/* Enable LPC CLKRUN# */
 	clkrun_val |= LPC_CLKRUN_EN;
-	iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET);
+	iowrite32(clkrun_val, data->ilb_base_addr + LPC_CNTRL_OFFSET);
 
 	/*
 	 * Write any random value on port 0x80 which is on LPC, to make
@@ -194,16 +185,11 @@  static void tpm_platform_end_xfer(void)
 
 }
 #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 +199,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 +214,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 +228,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 +241,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 +254,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;
 }
@@ -351,9 +337,13 @@  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);
 
 	tpm_chip_unregister(chip);
 	tpm_tis_remove(chip);
+	if (is_bsw())
+		iounmap(priv->ilb_base_addr);
+
 }
 
 static struct pnp_driver tis_pnp_driver = {
@@ -400,10 +390,14 @@  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);
 
 	tpm_chip_unregister(chip);
 	tpm_tis_remove(chip);
 
+	if (is_bsw())
+		iounmap(priv->ilb_base_addr);
+
 	return 0;
 }
 
@@ -461,11 +455,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;
@@ -484,10 +473,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;
 }
@@ -497,10 +482,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);
 }
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index ae905d17c248..bea151844692 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -754,6 +754,13 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	priv->phy_ops = phy_ops;
 	dev_set_drvdata(&chip->dev, priv);
 
+	if (is_bsw()) {
+		priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
+					ILB_REMAP_SIZE);
+		if (!priv->ilb_base_addr)
+			return -ENOMEM;
+	}
+
 	if (wait_startup(chip, 0) != 0) {
 		rc = -ENODEV;
 		goto out_err;
@@ -844,9 +851,16 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		}
 	}
 
-	return tpm_chip_register(chip);
+	rc = tpm_chip_register(chip);
+	if (rc && is_bsw())
+		iounmap(priv->ilb_base_addr);
+
+	return rc;
 out_err:
 	tpm_tis_remove(chip);
+	if (is_bsw())
+		iounmap(priv->ilb_base_addr);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_tis_core_init);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 6bbac319ff3b..458847f72758 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -79,6 +79,9 @@  enum tis_defaults {
 #define	TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
 #define	TPM_RID(l)			(0x0F04 | ((l) << 12))
 
+#define INTEL_LEGACY_BLK_BASE_ADDR	0xFED08000
+#define ILB_REMAP_SIZE			0x100
+
 enum tpm_tis_flags {
 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
 };
@@ -89,6 +92,7 @@  struct tpm_tis_data {
 	int irq;
 	bool irq_tested;
 	unsigned int flags;
+	void __iomem *ilb_base_addr;
 	wait_queue_head_t int_queue;
 	wait_queue_head_t read_queue;
 	const struct tpm_tis_phy_ops *phy_ops;
@@ -144,6 +148,15 @@  static inline int tpm_tis_write32(struct tpm_tis_data *data, u32 addr,
 	return data->phy_ops->write32(data, addr, value);
 }
 
+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
+}
+
 void tpm_tis_remove(struct tpm_chip *chip);
 int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		      const struct tpm_tis_phy_ops *phy_ops,