[PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

Hello,

Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.

Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.

The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
This issue and the propossed solution were discussed in this [2] thread,
and the reporter (Jame Ettle) confirmed that his system works again after
the fix in this series.

The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.

James,

Even when there shouldn't be any functional changes, I included some other
fixes / cleanups in this series so it would be great if you can test them
again. I can't because I don't have access to a machine affected by this.

[0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
[2]: https://patchwork.kernel.org/patch/10119417/
[3]: git.infradead.org/users/jjs/linux-tpmdd.git

Best regards,
Javier


Javier Martinez Canillas (4):
  tpm: fix access attempt to an already unmapped I/O memory region
  tpm: delete the TPM_TIS_CLK_ENABLE flag
  tpm: follow coding style for variable declaration in
    tpm_tis_core_init()
  tpm: only attempt to disable the LPC CLKRUN if is already enabled

 drivers/char/tpm/tpm_tis.c      | 17 +----------------
 drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++-------
 drivers/char/tpm/tpm_tis_core.h |  1 -
 3 files changed, 18 insertions(+), 24 deletions(-)

-- 
2.14.3

[PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Javier Martinez Canillas]

The driver maps the I/O memory address to control the LPC bus CLKRUN_EN,
but on the error path the memory is accessed by the .clk_enable handler
after this was already unmapped. So only unmap the I/O memory region if
it will not be used anymore.

Also, the correct thing to do is to cleanup the resources in the inverse
order that were acquired to prevent issues like these.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/char/tpm/tpm_tis_core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c2227983ed88..3455abbb2035 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -923,7 +923,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 
 	rc = tpm_chip_register(chip);
 	if (rc && is_bsw())
-		iounmap(priv->ilb_base_addr);
+		goto out_err;
 
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
@@ -931,12 +931,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	return rc;
 out_err:
 	tpm_tis_remove(chip);
-	if (is_bsw())
-		iounmap(priv->ilb_base_addr);
 
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
 
+	if (is_bsw())
+		iounmap(priv->ilb_base_addr);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_tis_core_init);
-- 
2.14.3

[PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag

[Javier Martinez Canillas]

This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
systems, but the only way this can happen is if the code is not correct.

So it's an unnecessary check that just makes the code harder to read.

Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/char/tpm/tpm_tis.c      | 15 ---------------
 drivers/char/tpm/tpm_tis_core.c |  2 --
 drivers/char/tpm/tpm_tis_core.h |  1 -
 3 files changed, 18 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index d29add49b033..d0ad9a89b02b 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -138,9 +138,6 @@ 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);
 
-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
-		WARN(1, "CLKRUN not enabled!\n");
-
 	while (len--)
 		*result++ = ioread8(phy->iobase + addr);
 
@@ -152,9 +149,6 @@ 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);
 
-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
-		WARN(1, "CLKRUN not enabled!\n");
-
 	while (len--)
 		iowrite8(*value++, phy->iobase + addr);
 
@@ -165,9 +159,6 @@ 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);
 
-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
-		WARN(1, "CLKRUN not enabled!\n");
-
 	*result = ioread16(phy->iobase + addr);
 
 	return 0;
@@ -177,9 +168,6 @@ 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);
 
-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
-		WARN(1, "CLKRUN not enabled!\n");
-
 	*result = ioread32(phy->iobase + addr);
 
 	return 0;
@@ -189,9 +177,6 @@ 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);
 
-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
-		WARN(1, "CLKRUN not enabled!\n");
-
 	iowrite32(value, phy->iobase + addr);
 
 	return 0;
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 3455abbb2035..09da1e1adc40 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -746,7 +746,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value)
 		return;
 
 	if (value) {
-		data->flags |= TPM_TIS_CLK_ENABLE;
 		data->clkrun_enabled++;
 		if (data->clkrun_enabled > 1)
 			return;
@@ -777,7 +776,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value)
 		 * sure LPC clock is running before sending any TPM command.
 		 */
 		outb(0xCC, 0x80);
-		data->flags &= ~TPM_TIS_CLK_ENABLE;
 	}
 }
 
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index afc50cde1ba6..d5c6a2e952b3 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -86,7 +86,6 @@ enum tis_defaults {
 
 enum tpm_tis_flags {
 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
-	TPM_TIS_CLK_ENABLE		= BIT(1),
 };
 
 struct tpm_tis_data {
-- 
2.14.3

[PATCH 3/4] tpm: follow coding style for variable declaration in tpm_tis_core_init()

[Javier Martinez Canillas]

The coding style says "use just one data declaration per line (no commas
for multiple data declarations)" so follow this convention.

Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/char/tpm/tpm_tis_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 09da1e1adc40..534cd46fdfae 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -798,7 +798,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		      const struct tpm_tis_phy_ops *phy_ops,
 		      acpi_handle acpi_dev_handle)
 {
-	u32 vendor, intfcaps, intmask;
+	u32 vendor;
+	u32 intfcaps;
+	u32 intmask;
 	u8 rid;
 	int rc, probe;
 	struct tpm_chip *chip;
-- 
2.14.3

[PATCH 4/4] tpm: only attempt to disable the LPC CLKRUN if is already enabled

[Javier Martinez Canillas]

Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.

Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.

One flaw with the logic is that it assumes that the CLKRUN is always
enabled, and so it unconditionally enables it after a TPM transaction.

But it could be that the CLKRUN# signal was already disabled in the LPC
bus and so after the driver probes, CLKRUN_EN will remain enabled which
may break other devices that are attached to the LPC bus but don't have
support for the CLKRUN protocol.

Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: James Ettle <james@ettle.org.uk>

---

This patch fixes the bug reported for the Fedora kernel [0] and the kernel
bugzilla [1]. The issue and the propossed solution were discussed in this
[2] thread.

[0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
[2]: https://patchwork.kernel.org/patch/10119417/


 drivers/char/tpm/tpm_tis.c      |  2 +-
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index d0ad9a89b02b..c79193f6d092 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -314,7 +314,7 @@ static int tpm_tis_plat_remove(struct platform_device *pdev)
 	tpm_chip_unregister(chip);
 	tpm_tis_remove(chip);
 
-	if (is_bsw())
+	if (is_bsw() && priv->ilb_base_addr)
 		iounmap(priv->ilb_base_addr);
 
 	return 0;
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 534cd46fdfae..17fb9c600d0e 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -742,7 +742,8 @@ 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())
+	if (!IS_ENABLED(CONFIG_X86) || !is_bsw() ||
+	    !data->ilb_base_addr)
 		return;
 
 	if (value) {
@@ -801,6 +802,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	u32 vendor;
 	u32 intfcaps;
 	u32 intmask;
+	u32 clkrun_val;
 	u8 rid;
 	int rc, probe;
 	struct tpm_chip *chip;
@@ -826,6 +828,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 					ILB_REMAP_SIZE);
 		if (!priv->ilb_base_addr)
 			return -ENOMEM;
+
+		clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
+		/* Check if CLKRUN# is already not enabled in the LPC bus */
+		if (!(clkrun_val & LPC_CLKRUN_EN)) {
+			iounmap(priv->ilb_base_addr);
+			priv->ilb_base_addr = NULL;
+		}
 	}
 
 	if (chip->ops->clk_enable != NULL)
@@ -935,7 +944,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
 
-	if (is_bsw())
+	if (is_bsw() && priv->ilb_base_addr)
 		iounmap(priv->ilb_base_addr);
 
 	return rc;
-- 
2.14.3

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Hans de Goede]

Hi,

On 20-12-17 12:35, Javier Martinez Canillas wrote:
> Hello,
> 
> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
> signal during TPM transactions.
> 
> Unfortunately this breaks other devices that are attached to the LPC bus
> like for example PS/2 mouse and keyboards.
> 
> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
> This issue and the propossed solution were discussed in this [2] thread,
> and the reporter (Jame Ettle) confirmed that his system works again after
> the fix in this series.
> 
> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
> 
> James,
> 
> Even when there shouldn't be any functional changes, I included some other
> fixes / cleanups in this series so it would be great if you can test them
> again. I can't because I don't have access to a machine affected by this.
> 
> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
> [2]: https://patchwork.kernel.org/patch/10119417/
> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
> 
> Best regards,
> Javier
> 
> 
> Javier Martinez Canillas (4):
>    tpm: fix access attempt to an already unmapped I/O memory region
>    tpm: delete the TPM_TIS_CLK_ENABLE flag
>    tpm: follow coding style for variable declaration in
>      tpm_tis_core_init()
>    tpm: only attempt to disable the LPC CLKRUN if is already enabled
> 
>   drivers/char/tpm/tpm_tis.c      | 17 +----------------
>   drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++-------
>   drivers/char/tpm/tpm_tis_core.h |  1 -
>   3 files changed, 18 insertions(+), 24 deletions(-)

Note I'm just reading along here, but I'm wondering if both the TPM
and now also some PS/2 controllers need CLK_RUN to be disabled,
why don't we just disable it once permanently and be done with it?

It seems that on machines with a PS/2 controller connected to
the LPC bus the BIOS is already doing this, so I've a feeling that
it not being done on devices with a TPM is a bug in the firmware
there and we should just disable it everywhere (and probably
find a better place then the TPM driver to do the disabling).

Note this is just an observation, I could be completely wrong here,
but I've a feeling that just disabling CLKRUN all together is the
right thing to do and that seems like an easier fix to me.

Specifically what worries me is: what if another driver also takes
the temporarily disable CLK_RUN approach because of similar issues?
Now we've 2 drivers playing with the CLKRUN state and racing with
each others.

Regards,

Hans

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

Hi Hans,

Thanks a lot for your feedback.

On 12/20/2017 12:43 PM, Hans de Goede wrote:
> Hi,
> 
> On 20-12-17 12:35, Javier Martinez Canillas wrote:
>> Hello,
>>
>> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
>> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
>> signal during TPM transactions.
>>
>> Unfortunately this breaks other devices that are attached to the LPC bus
>> like for example PS/2 mouse and keyboards.
>>
>> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
>> This issue and the propossed solution were discussed in this [2] thread,
>> and the reporter (Jame Ettle) confirmed that his system works again after
>> the fix in this series.
>>
>> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
>>
>> James,
>>
>> Even when there shouldn't be any functional changes, I included some other
>> fixes / cleanups in this series so it would be great if you can test them
>> again. I can't because I don't have access to a machine affected by this.
>>
>> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
>> [2]: https://patchwork.kernel.org/patch/10119417/
>> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
>>
>> Best regards,
>> Javier
>>
>>
>> Javier Martinez Canillas (4):
>>    tpm: fix access attempt to an already unmapped I/O memory region
>>    tpm: delete the TPM_TIS_CLK_ENABLE flag
>>    tpm: follow coding style for variable declaration in
>>      tpm_tis_core_init()
>>    tpm: only attempt to disable the LPC CLKRUN if is already enabled
>>
>>   drivers/char/tpm/tpm_tis.c      | 17 +----------------
>>   drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++-------
>>   drivers/char/tpm/tpm_tis_core.h |  1 -
>>   3 files changed, 18 insertions(+), 24 deletions(-)
> 
> Note I'm just reading along here, but I'm wondering if both the TPM
> and now also some PS/2 controllers need CLK_RUN to be disabled,
> why don't we just disable it once permanently and be done with it?
>

That's the same question I asked to Azhar who authored the patch that
added the CLKRUN toggle logic to the driver, but he didn't answer yet:

https://www.spinics.net/lists/linux-integrity/msg01107.html

My understanding is that by permanently disabling it, then other devices
that do support the CLKRUN protocol will continue to work correctly since
the CLKRUN# signal is optional and only used if the host LCLK is stopped.

The disadvantage though will be that the power management feature of stopping
the LPC host LCLK clock when entering into low-power states will not be used.
 
> It seems that on machines with a PS/2 controller connected to
> the LPC bus the BIOS is already doing this, so I've a feeling that
> it not being done on devices with a TPM is a bug in the firmware

Absolutely agree, system integratos should make sure that all the devices
connected to the LPC either have CLKRUN protocol support and is enabled
or disable the CLKRUN protocol permanently.

> there and we should just disable it everywhere (and probably
> find a better place then the TPM driver to do the disabling).
>

Indeed. Touching a global bus configuration in a driver for a single device
isn't safe to say the least. One problem is that we don't have a LPC bus
subsystem so that's why these sort of quirks are done at the driver level.

> Note this is just an observation, I could be completely wrong here,
> but I've a feeling that just disabling CLKRUN all together is the
> right thing to do and that seems like an easier fix to me.
>

I think your observation is correct. The only problem is the power management
feature being disabled as mentioned. Although as you said it seems that most
BIOS do that (as shown by the patch I posted that just avoids toggling the
CLKRUN state if it's already disabled).

> Specifically what worries me is: what if another driver also takes
> the temporarily disable CLK_RUN approach because of similar issues?
> Now we've 2 drivers playing with the CLKRUN state and racing with
> each others.
>

Agreed. You don't even need another driver, if a Braswell system has 2 TPMs
then you have a race condition and one driver could enable the CLKRUN state
while the other driver thinks that's already disabled, and TPM transactions
won't work in that case.

So yeah, it's not a great situation.

> Regards,
> 
> Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Alexander.Steffen]

> Hi Hans,
> 
> Thanks a lot for your feedback.
> 
> On 12/20/2017 12:43 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 20-12-17 12:35, Javier Martinez Canillas wrote:
> >> Hello,
> >>
> >> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell
> systems")
> >> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
> >> signal during TPM transactions.
> >>
> >> Unfortunately this breaks other devices that are attached to the LPC bus
> >> like for example PS/2 mouse and keyboards.
> >>
> >> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
> >> This issue and the propossed solution were discussed in this [2] thread,
> >> and the reporter (Jame Ettle) confirmed that his system works again after
> >> the fix in this series.
> >>
> >> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
> >>
> >> James,
> >>
> >> Even when there shouldn't be any functional changes, I included some
> other
> >> fixes / cleanups in this series so it would be great if you can test them
> >> again. I can't because I don't have access to a machine affected by this.
> >>
> >> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
> >> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
> >> [2]: https://patchwork.kernel.org/patch/10119417/
> >> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
> >>
> >> Best regards,
> >> Javier
> >>
> >>
> >> Javier Martinez Canillas (4):
> >>    tpm: fix access attempt to an already unmapped I/O memory region
> >>    tpm: delete the TPM_TIS_CLK_ENABLE flag
> >>    tpm: follow coding style for variable declaration in
> >>      tpm_tis_core_init()
> >>    tpm: only attempt to disable the LPC CLKRUN if is already enabled
> >>
> >>   drivers/char/tpm/tpm_tis.c      | 17 +----------------
> >>   drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++-------
> >>   drivers/char/tpm/tpm_tis_core.h |  1 -
> >>   3 files changed, 18 insertions(+), 24 deletions(-)
> >
> > Note I'm just reading along here, but I'm wondering if both the TPM
> > and now also some PS/2 controllers need CLK_RUN to be disabled,
> > why don't we just disable it once permanently and be done with it?
> >
> 
> That's the same question I asked to Azhar who authored the patch that
> added the CLKRUN toggle logic to the driver, but he didn't answer yet:
> 
> https://www.spinics.net/lists/linux-integrity/msg01107.html
> 
> My understanding is that by permanently disabling it, then other devices
> that do support the CLKRUN protocol will continue to work correctly since
> the CLKRUN# signal is optional and only used if the host LCLK is stopped.
> 
> The disadvantage though will be that the power management feature of
> stopping
> the LPC host LCLK clock when entering into low-power states will not be
> used.
> 
> > It seems that on machines with a PS/2 controller connected to
> > the LPC bus the BIOS is already doing this, so I've a feeling that
> > it not being done on devices with a TPM is a bug in the firmware
> 
> Absolutely agree, system integratos should make sure that all the devices
> connected to the LPC either have CLKRUN protocol support and is enabled
> or disable the CLKRUN protocol permanently.

As far as I understand it, this is exactly the issue here: They know that there are devices that do not support the CLKRUN protocol (the TPM in this case), but they still need to enable it to prevent other issues. So for the TPM to continue to work, CLKRUN needs to be disabled temporarily while the TPM is active.

> > there and we should just disable it everywhere (and probably
> > find a better place then the TPM driver to do the disabling).
> >
> 
> Indeed. Touching a global bus configuration in a driver for a single device
> isn't safe to say the least. One problem is that we don't have a LPC bus
> subsystem so that's why these sort of quirks are done at the driver level.
> 
> > Note this is just an observation, I could be completely wrong here,
> > but I've a feeling that just disabling CLKRUN all together is the
> > right thing to do and that seems like an easier fix to me.
> >
> 
> I think your observation is correct. The only problem is the power
> management
> feature being disabled as mentioned. Although as you said it seems that
> most
> BIOS do that (as shown by the patch I posted that just avoids toggling the
> CLKRUN state if it's already disabled).
> 
> > Specifically what worries me is: what if another driver also takes
> > the temporarily disable CLK_RUN approach because of similar issues?
> > Now we've 2 drivers playing with the CLKRUN state and racing with
> > each others.
> >
> 
> Agreed. You don't even need another driver, if a Braswell system has 2 TPMs
> then you have a race condition and one driver could enable the CLKRUN
> state
> while the other driver thinks that's already disabled, and TPM transactions
> won't work in that case.

Even though it is an unusual configuration to have multiple TPMs in a system, this is something that we could fix in the driver by putting locks around the state changes, right? And if other drivers also want to change the CLKRUN state, at the very least they'd need to use the same (global) lock.

> So yeah, it's not a great situation.
> 
> > Regards,
> >
> > Hans
> >
> 
> Best regards,
> --
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

Hello Alexander,

On 12/20/2017 03:07 PM, Alexander.Steffen@infineon.com wrote:
>> Hi Hans,
>>
>> Thanks a lot for your feedback.
>>
>> On 12/20/2017 12:43 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 20-12-17 12:35, Javier Martinez Canillas wrote:
>>>> Hello,
>>>>
>>>> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell
>> systems")
>>>> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
>>>> signal during TPM transactions.
>>>>
>>>> Unfortunately this breaks other devices that are attached to the LPC bus
>>>> like for example PS/2 mouse and keyboards.
>>>>
>>>> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
>>>> This issue and the propossed solution were discussed in this [2] thread,
>>>> and the reporter (Jame Ettle) confirmed that his system works again after
>>>> the fix in this series.
>>>>
>>>> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
>>>>
>>>> James,
>>>>
>>>> Even when there shouldn't be any functional changes, I included some
>> other
>>>> fixes / cleanups in this series so it would be great if you can test them
>>>> again. I can't because I don't have access to a machine affected by this.
>>>>
>>>> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
>>>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
>>>> [2]: https://patchwork.kernel.org/patch/10119417/
>>>> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
>>>>
>>>> Best regards,
>>>> Javier
>>>>
>>>>
>>>> Javier Martinez Canillas (4):
>>>>    tpm: fix access attempt to an already unmapped I/O memory region
>>>>    tpm: delete the TPM_TIS_CLK_ENABLE flag
>>>>    tpm: follow coding style for variable declaration in
>>>>      tpm_tis_core_init()
>>>>    tpm: only attempt to disable the LPC CLKRUN if is already enabled
>>>>
>>>>   drivers/char/tpm/tpm_tis.c      | 17 +----------------
>>>>   drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++-------
>>>>   drivers/char/tpm/tpm_tis_core.h |  1 -
>>>>   3 files changed, 18 insertions(+), 24 deletions(-)
>>>
>>> Note I'm just reading along here, but I'm wondering if both the TPM
>>> and now also some PS/2 controllers need CLK_RUN to be disabled,
>>> why don't we just disable it once permanently and be done with it?
>>>
>>
>> That's the same question I asked to Azhar who authored the patch that
>> added the CLKRUN toggle logic to the driver, but he didn't answer yet:
>>
>> https://www.spinics.net/lists/linux-integrity/msg01107.html
>>
>> My understanding is that by permanently disabling it, then other devices
>> that do support the CLKRUN protocol will continue to work correctly since
>> the CLKRUN# signal is optional and only used if the host LCLK is stopped.
>>
>> The disadvantage though will be that the power management feature of
>> stopping
>> the LPC host LCLK clock when entering into low-power states will not be
>> used.
>>
>>> It seems that on machines with a PS/2 controller connected to
>>> the LPC bus the BIOS is already doing this, so I've a feeling that
>>> it not being done on devices with a TPM is a bug in the firmware
>>
>> Absolutely agree, system integratos should make sure that all the devices
>> connected to the LPC either have CLKRUN protocol support and is enabled
>> or disable the CLKRUN protocol permanently.
> 
> As far as I understand it, this is exactly the issue here: They know that there are devices that do not support the CLKRUN protocol (the TPM in this case), but they still need to enable it to prevent other issues. So for the TPM to continue to work, CLKRUN needs to be disabled temporarily while the TPM is active.

I do understand that part. What's not clear to me is if those other issues
as you said exist in practice. My understanding from reading the Low Pin
Count Specification is that the CLKRUN# signal is optional and that the
peripherals that support the CLKRUN protocol should still work even when
the CLKRUN_EN is disabled.

With the disadvantage that the system will be less power efficient of
course.

But yes, the reason why I proposed this minimal change as a fix was to
preserve the behavior of the original patch that caused the regression,
while allowing systems that already have the CLKRUN protocol disabled to
also work (which was what caused the issue since the CLKRUN protocol was
enabled unconditionally after a TPM transaction).

> 
>>> there and we should just disable it everywhere (and probably
>>> find a better place then the TPM driver to do the disabling).
>>>
>>
>> Indeed. Touching a global bus configuration in a driver for a single device
>> isn't safe to say the least. One problem is that we don't have a LPC bus
>> subsystem so that's why these sort of quirks are done at the driver level.
>>
>>> Note this is just an observation, I could be completely wrong here,
>>> but I've a feeling that just disabling CLKRUN all together is the
>>> right thing to do and that seems like an easier fix to me.
>>>
>>
>> I think your observation is correct. The only problem is the power
>> management
>> feature being disabled as mentioned. Although as you said it seems that
>> most
>> BIOS do that (as shown by the patch I posted that just avoids toggling the
>> CLKRUN state if it's already disabled).
>>
>>> Specifically what worries me is: what if another driver also takes
>>> the temporarily disable CLK_RUN approach because of similar issues?
>>> Now we've 2 drivers playing with the CLKRUN state and racing with
>>> each others.
>>>
>>
>> Agreed. You don't even need another driver, if a Braswell system has 2 TPMs
>> then you have a race condition and one driver could enable the CLKRUN
>> state
>> while the other driver thinks that's already disabled, and TPM transactions
>> won't work in that case.
> 
> Even though it is an unusual configuration to have multiple TPMs in a system, this is something that we could fix in the driver by putting locks around the state changes, right? And if other drivers also want to change the CLKRUN state, at the very least they'd need to use the same (global) lock.
> 

Yes, I didn't say that this couldn't be solved with a synchronization mechanism
to prevent concurrent access to the LPC control registers. What I meant is that
there isn't such a mechanism in place now so the code is racy and that the lack
of a LPC subsystem forces us to have an (ugly) global lock if we want to fix it.

That's a separate issue though and should be addressed in a different patch-set.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Shaikh, Azhar]

>-----Original Message-----
>From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
>owner@vger.kernel.org] On Behalf Of Alexander.Steffen@infineon.com
>Sent: Wednesday, December 20, 2017 6:07 AM
>To: javierm@redhat.com; hdegoede@redhat.com; linux-
>kernel@vger.kernel.org
>Cc: james@ettle.org.uk; Shaikh, Azhar <azhar.shaikh@intel.com>;
>arnd@arndb.de; jarkko.sakkinen@linux.intel.com; peterhuewe@gmx.de;
>jgg@ziepe.ca; gregkh@linuxfoundation.org; linux-integrity@vger.kernel.org
>Subject: RE: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell
>systems due CLKRUN enabled
>
>> Hi Hans,
>>
>> Thanks a lot for your feedback.
>>
>> On 12/20/2017 12:43 PM, Hans de Goede wrote:
>> > Hi,
>> >
>> > On 20-12-17 12:35, Javier Martinez Canillas wrote:
>> >> Hello,
>> >>
>> >> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell
>> systems")
>> >> added logic in the TPM TIS driver to disable the Low Pin Count
>> >> CLKRUN signal during TPM transactions.
>> >>
>> >> Unfortunately this breaks other devices that are attached to the
>> >> LPC bus like for example PS/2 mouse and keyboards.
>> >>
>> >> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
>> >> This issue and the propossed solution were discussed in this [2]
>> >> thread, and the reporter (Jame Ettle) confirmed that his system
>> >> works again after the fix in this series.
>> >>
>> >> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
>> >>
>> >> James,
>> >>
>> >> Even when there shouldn't be any functional changes, I included
>> >> some
>> other
>> >> fixes / cleanups in this series so it would be great if you can
>> >> test them again. I can't because I don't have access to a machine affected
>by this.
>> >>
>> >> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
>> >> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
>> >> [2]: https://patchwork.kernel.org/patch/10119417/
>> >> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
>> >>
>> >> Best regards,
>> >> Javier
>> >>
>> >>
>> >> Javier Martinez Canillas (4):
>> >>    tpm: fix access attempt to an already unmapped I/O memory region
>> >>    tpm: delete the TPM_TIS_CLK_ENABLE flag
>> >>    tpm: follow coding style for variable declaration in
>> >>      tpm_tis_core_init()
>> >>    tpm: only attempt to disable the LPC CLKRUN if is already
>> >> enabled
>> >>
>> >>   drivers/char/tpm/tpm_tis.c      | 17 +----------------
>> >>   drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++-------
>> >>   drivers/char/tpm/tpm_tis_core.h |  1 -
>> >>   3 files changed, 18 insertions(+), 24 deletions(-)
>> >
>> > Note I'm just reading along here, but I'm wondering if both the TPM
>> > and now also some PS/2 controllers need CLK_RUN to be disabled, why
>> > don't we just disable it once permanently and be done with it?
>> >
>>
>> That's the same question I asked to Azhar who authored the patch that
>> added the CLKRUN toggle logic to the driver, but he didn't answer yet:
>>
>> https://www.spinics.net/lists/linux-integrity/msg01107.html
>>
>> My understanding is that by permanently disabling it, then other
>> devices that do support the CLKRUN protocol will continue to work
>> correctly since the CLKRUN# signal is optional and only used if the host LCLK
>is stopped.
>>
>> The disadvantage though will be that the power management feature of
>> stopping the LPC host LCLK clock when entering into low-power states
>> will not be used.
>>
>> > It seems that on machines with a PS/2 controller connected to the
>> > LPC bus the BIOS is already doing this, so I've a feeling that it
>> > not being done on devices with a TPM is a bug in the firmware
>>
>> Absolutely agree, system integratos should make sure that all the
>> devices connected to the LPC either have CLKRUN protocol support and
>> is enabled or disable the CLKRUN protocol permanently.
>
>As far as I understand it, this is exactly the issue here: They know that there
>are devices that do not support the CLKRUN protocol (the TPM in this case),
>but they still need to enable it to prevent other issues. So for the TPM to
>continue to work, CLKRUN needs to be disabled temporarily while the TPM is
>active.
>

Yes that was the reason to have this fix. We needed CLKRUN to be enabled for Braswell SOC . But the TPM in this case SLB9655 does not support CLKRUN (please check this public documentation https://www.infineon.com/dgdl/Infineon-TPM+SLB+9665-DS-v10_15-EN.pdf?fileId=5546d4625185e0e201518b83d9273d87 section 2.3 Power Management). So as Alexander mentioned CLKRUN is disabled while TPM transactions are in progress.

>> > there and we should just disable it everywhere (and probably find a
>> > better place then the TPM driver to do the disabling).
>> >
>>
>> Indeed. Touching a global bus configuration in a driver for a single
>> device isn't safe to say the least. One problem is that we don't have
>> a LPC bus subsystem so that's why these sort of quirks are done at the driver
>level.
>>
>> > Note this is just an observation, I could be completely wrong here,
>> > but I've a feeling that just disabling CLKRUN all together is the
>> > right thing to do and that seems like an easier fix to me.
>> >
>>
>> I think your observation is correct. The only problem is the power
>> management feature being disabled as mentioned. Although as you said
>> it seems that most BIOS do that (as shown by the patch I posted that
>> just avoids toggling the CLKRUN state if it's already disabled).
>>
>> > Specifically what worries me is: what if another driver also takes
>> > the temporarily disable CLK_RUN approach because of similar issues?
>> > Now we've 2 drivers playing with the CLKRUN state and racing with
>> > each others.
>> >
>>
>> Agreed. You don't even need another driver, if a Braswell system has 2
>> TPMs then you have a race condition and one driver could enable the
>> CLKRUN state while the other driver thinks that's already disabled,
>> and TPM transactions won't work in that case.
>
>Even though it is an unusual configuration to have multiple TPMs in a system,
>this is something that we could fix in the driver by putting locks around the
>state changes, right? And if other drivers also want to change the CLKRUN
>state, at the very least they'd need to use the same (global) lock.
>
>> So yeah, it's not a great situation.
>>
>> > Regards,
>> >
>> > Hans
>> >
>>
>> Best regards,
>> --
>> Javier Martinez Canillas
>> Software Engineer - Desktop Hardware Enablement Red Hat

RE: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag

[Shaikh, Azhar]

>-----Original Message-----
>From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>Sent: Wednesday, December 20, 2017 3:36 AM
>To: linux-kernel@vger.kernel.org
>Cc: James Ettle <james@ettle.org.uk>; Hans de Goede
><hdegoede@redhat.com>; Shaikh, Azhar <azhar.shaikh@intel.com>; Javier
>Martinez Canillas <javierm@redhat.com>; Arnd Bergmann <arnd@arndb.de>;
>Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
><peterhuewe@gmx.de>; Jason Gunthorpe <jgg@ziepe.ca>; Greg Kroah-
>Hartman <gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org
>Subject: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag
>
>This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
>systems, but the only way this can happen is if the code is not correct.
>
>So it's an unnecessary check that just makes the code harder to read.
>

Hi Javier,

This code was implemented as a suggestion from Jason on the previous patches. 
https://www.spinics.net/lists/linux-integrity/msg00827.html


>Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>---
>
> drivers/char/tpm/tpm_tis.c      | 15 ---------------
> drivers/char/tpm/tpm_tis_core.c |  2 --  drivers/char/tpm/tpm_tis_core.h |  1
>-
> 3 files changed, 18 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index
>d29add49b033..d0ad9a89b02b 100644
>--- a/drivers/char/tpm/tpm_tis.c
>+++ b/drivers/char/tpm/tpm_tis.c
>@@ -138,9 +138,6 @@ 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);
>
>-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>-		WARN(1, "CLKRUN not enabled!\n");
>-
> 	while (len--)
> 		*result++ = ioread8(phy->iobase + addr);
>
>@@ -152,9 +149,6 @@ 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);
>
>-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>-		WARN(1, "CLKRUN not enabled!\n");
>-
> 	while (len--)
> 		iowrite8(*value++, phy->iobase + addr);
>
>@@ -165,9 +159,6 @@ 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);
>
>-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>-		WARN(1, "CLKRUN not enabled!\n");
>-
> 	*result = ioread16(phy->iobase + addr);
>
> 	return 0;
>@@ -177,9 +168,6 @@ 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);
>
>-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>-		WARN(1, "CLKRUN not enabled!\n");
>-
> 	*result = ioread32(phy->iobase + addr);
>
> 	return 0;
>@@ -189,9 +177,6 @@ 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);
>
>-	if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
>-		WARN(1, "CLKRUN not enabled!\n");
>-
> 	iowrite32(value, phy->iobase + addr);
>
> 	return 0;
>diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c index 3455abbb2035..09da1e1adc40
>100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -746,7 +746,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip
>*chip, bool value)
> 		return;
>
> 	if (value) {
>-		data->flags |= TPM_TIS_CLK_ENABLE;
> 		data->clkrun_enabled++;
> 		if (data->clkrun_enabled > 1)
> 			return;
>@@ -777,7 +776,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip
>*chip, bool value)
> 		 * sure LPC clock is running before sending any TPM
>command.
> 		 */
> 		outb(0xCC, 0x80);
>-		data->flags &= ~TPM_TIS_CLK_ENABLE;
> 	}
> }
>
>diff --git a/drivers/char/tpm/tpm_tis_core.h
>b/drivers/char/tpm/tpm_tis_core.h index afc50cde1ba6..d5c6a2e952b3
>100644
>--- a/drivers/char/tpm/tpm_tis_core.h
>+++ b/drivers/char/tpm/tpm_tis_core.h
>@@ -86,7 +86,6 @@ enum tis_defaults {
>
> enum tpm_tis_flags {
> 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
>-	TPM_TIS_CLK_ENABLE		= BIT(1),
> };
>
> struct tpm_tis_data {
>--
>2.14.3

RE: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Shaikh, Azhar]

>-----Original Message-----
>From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>Sent: Wednesday, December 20, 2017 3:36 AM
>To: linux-kernel@vger.kernel.org
>Cc: James Ettle <james@ettle.org.uk>; Hans de Goede
><hdegoede@redhat.com>; Shaikh, Azhar <azhar.shaikh@intel.com>; Javier
>Martinez Canillas <javierm@redhat.com>; Arnd Bergmann <arnd@arndb.de>;
>Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
><peterhuewe@gmx.de>; Jason Gunthorpe <jgg@ziepe.ca>; Greg Kroah-
>Hartman <gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org
>Subject: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O
>memory region
>
>The driver maps the I/O memory address to control the LPC bus CLKRUN_EN,
>but on the error path the memory is accessed by the .clk_enable handler after
>this was already unmapped. So only unmap the I/O memory region if it will
>not be used anymore.
>
>Also, the correct thing to do is to cleanup the resources in the inverse order
>that were acquired to prevent issues like these.
>

Thanks for catching this!

>Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>---
>
> drivers/char/tpm/tpm_tis_core.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c index c2227983ed88..3455abbb2035
>100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -923,7 +923,7 @@ int tpm_tis_core_init(struct device *dev, struct
>tpm_tis_data *priv, int irq,
>
> 	rc = tpm_chip_register(chip);
> 	if (rc && is_bsw())
>-		iounmap(priv->ilb_base_addr);
>+		goto out_err;
>
> 	if (chip->ops->clk_enable != NULL)
> 		chip->ops->clk_enable(chip, false);
>@@ -931,12 +931,13 @@ int tpm_tis_core_init(struct device *dev, struct
>tpm_tis_data *priv, int irq,
> 	return rc;
> out_err:
> 	tpm_tis_remove(chip);
>-	if (is_bsw())
>-		iounmap(priv->ilb_base_addr);
>
> 	if (chip->ops->clk_enable != NULL)
> 		chip->ops->clk_enable(chip, false);
>
>+	if (is_bsw())
>+		iounmap(priv->ilb_base_addr);
>+
> 	return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_tis_core_init);
>--
>2.14.3

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

Hello Azhar,

On 12/20/2017 04:08 PM, Shaikh, Azhar wrote:

[snip]

>>>
>>>> It seems that on machines with a PS/2 controller connected to the
>>>> LPC bus the BIOS is already doing this, so I've a feeling that it
>>>> not being done on devices with a TPM is a bug in the firmware
>>>
>>> Absolutely agree, system integratos should make sure that all the
>>> devices connected to the LPC either have CLKRUN protocol support and
>>> is enabled or disable the CLKRUN protocol permanently.
>>
>> As far as I understand it, this is exactly the issue here: They know that there
>> are devices that do not support the CLKRUN protocol (the TPM in this case),
>> but they still need to enable it to prevent other issues. So for the TPM to
>> continue to work, CLKRUN needs to be disabled temporarily while the TPM is
>> active.
>>
> 
> Yes that was the reason to have this fix. We needed CLKRUN to be enabled for Braswell SOC . But the TPM in this case SLB9655 does not support CLKRUN (please check this public documentation https://www.infineon.com/dgdl/Infineon-TPM+SLB+9665-DS-v10_15-EN.pdf?fileId=5546d4625185e0e201518b83d9273d87 section 2.3 Power Management). So as Alexander mentioned CLKRUN is disabled while TPM transactions are in progress.
>

Yes I do understand that. Please read my answer to Alexander's email and also
my question (and Hans') about keeping the CLKRUN protocol permanently disabled.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Shaikh, Azhar]

>-----Original Message-----
>From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>Sent: Wednesday, December 20, 2017 7:31 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>;
>Alexander.Steffen@infineon.com; hdegoede@redhat.com; linux-
>kernel@vger.kernel.org
>Cc: james@ettle.org.uk; arnd@arndb.de; jarkko.sakkinen@linux.intel.com;
>peterhuewe@gmx.de; jgg@ziepe.ca; gregkh@linuxfoundation.org; linux-
>integrity@vger.kernel.org
>Subject: Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell
>systems due CLKRUN enabled
>
>Hello Azhar,
>
>On 12/20/2017 04:08 PM, Shaikh, Azhar wrote:
>
>[snip]
>
>>>>
>>>>> It seems that on machines with a PS/2 controller connected to the
>>>>> LPC bus the BIOS is already doing this, so I've a feeling that it
>>>>> not being done on devices with a TPM is a bug in the firmware
>>>>
>>>> Absolutely agree, system integratos should make sure that all the
>>>> devices connected to the LPC either have CLKRUN protocol support and
>>>> is enabled or disable the CLKRUN protocol permanently.
>>>
>>> As far as I understand it, this is exactly the issue here: They know
>>> that there are devices that do not support the CLKRUN protocol (the
>>> TPM in this case), but they still need to enable it to prevent other
>>> issues. So for the TPM to continue to work, CLKRUN needs to be
>>> disabled temporarily while the TPM is active.
>>>
>>
>> Yes that was the reason to have this fix. We needed CLKRUN to be enabled
>for Braswell SOC . But the TPM in this case SLB9655 does not support CLKRUN
>(please check this public documentation
>https://www.infineon.com/dgdl/Infineon-TPM+SLB+9665-DS-v10_15-
>EN.pdf?fileId=5546d4625185e0e201518b83d9273d87 section 2.3 Power
>Management). So as Alexander mentioned CLKRUN is disabled while TPM
>transactions are in progress.
>>
>
>Yes I do understand that. Please read my answer to Alexander's email and
>also my question (and Hans') about keeping the CLKRUN protocol
>permanently disabled.
>

We had to enable CLKRUN for BSW issues as mentioned here https://www.intel.com/content/www/us/en/processors/pentium/pentium-celeron-n-series-spec-update.html  on Page 24 CHP 49 and Page 25 CHP 51



>Best regards,
>--
>Javier Martinez Canillas
>Software Engineer - Desktop Hardware Enablement Red Hat

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

Hello Azhar,

On 12/20/2017 04:41 PM, Shaikh, Azhar wrote:
> 
> 
>> -----Original Message-----
>> From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>> Sent: Wednesday, December 20, 2017 7:31 AM
>> To: Shaikh, Azhar <azhar.shaikh@intel.com>;
>> Alexander.Steffen@infineon.com; hdegoede@redhat.com; linux-
>> kernel@vger.kernel.org
>> Cc: james@ettle.org.uk; arnd@arndb.de; jarkko.sakkinen@linux.intel.com;
>> peterhuewe@gmx.de; jgg@ziepe.ca; gregkh@linuxfoundation.org; linux-
>> integrity@vger.kernel.org
>> Subject: Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell
>> systems due CLKRUN enabled
>>
>> Hello Azhar,
>>
>> On 12/20/2017 04:08 PM, Shaikh, Azhar wrote:
>>
>> [snip]
>>
>>>>>
>>>>>> It seems that on machines with a PS/2 controller connected to the
>>>>>> LPC bus the BIOS is already doing this, so I've a feeling that it
>>>>>> not being done on devices with a TPM is a bug in the firmware
>>>>>
>>>>> Absolutely agree, system integratos should make sure that all the
>>>>> devices connected to the LPC either have CLKRUN protocol support and
>>>>> is enabled or disable the CLKRUN protocol permanently.
>>>>
>>>> As far as I understand it, this is exactly the issue here: They know
>>>> that there are devices that do not support the CLKRUN protocol (the
>>>> TPM in this case), but they still need to enable it to prevent other
>>>> issues. So for the TPM to continue to work, CLKRUN needs to be
>>>> disabled temporarily while the TPM is active.
>>>>
>>>
>>> Yes that was the reason to have this fix. We needed CLKRUN to be enabled
>> for Braswell SOC . But the TPM in this case SLB9655 does not support CLKRUN
>> (please check this public documentation
>> https://www.infineon.com/dgdl/Infineon-TPM+SLB+9665-DS-v10_15-
>> EN.pdf?fileId=5546d4625185e0e201518b83d9273d87 section 2.3 Power
>> Management). So as Alexander mentioned CLKRUN is disabled while TPM
>> transactions are in progress.
>>>
>>
>> Yes I do understand that. Please read my answer to Alexander's email and
>> also my question (and Hans') about keeping the CLKRUN protocol
>> permanently disabled.
>>
> 
> We had to enable CLKRUN for BSW issues as mentioned here https://www.intel.com/content/www/us/en/processors/pentium/pentium-celeron-n-series-spec-update.html  on Page 24 CHP 49 and Page 25 CHP 51
> 

Thanks for the pointer. But it's still not clear to me after reading the
mentioned erratas why the CLKRUN protocol must be enabled.

CHP49 says "System May Experience Inability to Boot or May Cease Operation"
and that it may be related to the LPC circuitry to stop functioning which
causes the inability to boot when activity is high for several years.

And that the workaround is:

"Firmware code changes for LPC and RTC circuitry and mitigations for SD Card
circuitry have been identified and may be implemented for this erratum."

Is this Firmware code changes to enable the CLKRUN protocol to minimize the
LPC bus activity and so prevent the LPC circuitry to stop functioning?

CHP51 says "LPC Clock Control Using the LPC_CLKRUN# May Not Behave As Expected"
and that the implication is that "The SoC may prevent a peripheral device from
successfully requesting the LPC clock".

So I would say that CLKRUN protocol should NOT be enabled instead since the
CLKRUN# signal is not reliable. Or what am I missing here?

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Jason Gunthorpe]

On Wed, Dec 20, 2017 at 05:45:16PM +0100, Javier Martinez Canillas wrote:

> CHP51 says "LPC Clock Control Using the LPC_CLKRUN# May Not Behave As Expected"
> and that the implication is that "The SoC may prevent a peripheral device from
> successfully requesting the LPC clock".

Now we are back to the beginning - the LPC_CLKRUN protocol is simply
broken in BSW chipsets, and it has nothing to do with the TPM?

Intel is trying to work around that broken-ness and still preserve
power management in the case where only the TPM is connected to the
LPC bus.. It is questionable to me if this is even a good idea, or if
Linux is the right place to implement this work around (eg something
in SMM mode may be more appropriate for a chipset bug)

I think your patch is still the right improvement, if the BIOS turned
the feature off, we should not turn it back on.

Jason

Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Jason Gunthorpe]

On Wed, Dec 20, 2017 at 12:35:35PM +0100, Javier Martinez Canillas wrote:
> The driver maps the I/O memory address to control the LPC bus CLKRUN_EN,
> but on the error path the memory is accessed by the .clk_enable handler
> after this was already unmapped. So only unmap the I/O memory region if
> it will not be used anymore.
> 
> Also, the correct thing to do is to cleanup the resources in the inverse
> order that were acquired to prevent issues like these.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
>  drivers/char/tpm/tpm_tis_core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index c2227983ed88..3455abbb2035 100644
> +++ b/drivers/char/tpm/tpm_tis_core.c

Yoiks. This patch is helping but the more I look at this the wronger
everything looks..

1) tpm_chip_unregister makes chip->ops == NULL, so this sequence:

static int tpm_tis_plat_remove(struct platform_device *pdev)
	tpm_chip_unregister(chip);
	tpm_tis_remove(chip);
void tpm_tis_remove(struct tpm_chip *chip)
	if (chip->ops->clk_enable != NULL)

Will oops

2) tpm_chip_register can also NULL ops in error cases, so this
   sequence can oops:

       rc = tpm_chip_register(chip);
       if (rc && is_bsw())
               iounmap(priv->ilb_base_addr);

        if (chip->ops->clk_enable != NULL)
                chip->ops->clk_enable(chip, false);

3) iounmap should not be split between tpm_tis and tpm_tis_core
   Put it at the end of tpm_tis_remove.

4) This sequence:

+       return tpm_chip_register(chip);
+out_err:
+       tpm_tis_remove(chip);
+       return rc;

   Doesn't look right. If tpm_chip_register fails then
   tpm_tis_remove will never be called. This was sort of OK when
   tpm_tis_remove didn't manage any resources, but now that it does
   the above needs fixing too.

The below draft fixes everything except #1. That needs a more thoughtful
idea..

Jason

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index d29add49b03388..09f18e2e644774 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -275,9 +275,6 @@ static void tpm_tis_pnp_remove(struct pnp_dev *dev)
 
 	tpm_chip_unregister(chip);
 	tpm_tis_remove(chip);
-	if (is_bsw())
-		iounmap(priv->ilb_base_addr);
-
 }
 
 static struct pnp_driver tis_pnp_driver = {
@@ -328,10 +325,6 @@ static int tpm_tis_plat_remove(struct platform_device *pdev)
 
 	tpm_chip_unregister(chip);
 	tpm_tis_remove(chip);
-
-	if (is_bsw())
-		iounmap(priv->ilb_base_addr);
-
 	return 0;
 }
 
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c2227983ed88d4..ffda1694a6aba3 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -727,6 +727,9 @@ void tpm_tis_remove(struct tpm_chip *chip)
 
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
+
+	if (priv->ilb_base_addr)
+		iounmap(priv->ilb_base_addr);
 }
 EXPORT_SYMBOL_GPL(tpm_tis_remove);
 
@@ -921,22 +924,15 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		}
 	}
 
-	rc = tpm_chip_register(chip);
-	if (rc && is_bsw())
-		iounmap(priv->ilb_base_addr);
-
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
 
-	return rc;
+	rc = tpm_chip_register(chip);
+	if (rc):
+		goto out_err;
+	return 0;
 out_err:
 	tpm_tis_remove(chip);
-	if (is_bsw())
-		iounmap(priv->ilb_base_addr);
-
-	if (chip->ops->clk_enable != NULL)
-		chip->ops->clk_enable(chip, false);
-
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_tis_core_init);

Re: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag

[Jason Gunthorpe]

On Wed, Dec 20, 2017 at 03:19:19PM +0000, Shaikh, Azhar wrote:
> >This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
> >systems, but the only way this can happen is if the code is not correct.
> >
> >So it's an unnecessary check that just makes the code harder to read.
> 
> This code was implemented as a suggestion from Jason on the previous patches. 
> https://www.spinics.net/lists/linux-integrity/msg00827.html

The concept was to be like ASSERT_RTNL, maybe it just needs a suitably
named static inline to addrress Javier's readability concerns?

Jason

Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Javier Martinez Canillas]

On 12/20/2017 07:08 PM, Jason Gunthorpe wrote:
> On Wed, Dec 20, 2017 at 12:35:35PM +0100, Javier Martinez Canillas wrote:
>> The driver maps the I/O memory address to control the LPC bus CLKRUN_EN,
>> but on the error path the memory is accessed by the .clk_enable handler
>> after this was already unmapped. So only unmap the I/O memory region if
>> it will not be used anymore.
>>
>> Also, the correct thing to do is to cleanup the resources in the inverse
>> order that were acquired to prevent issues like these.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>>  drivers/char/tpm/tpm_tis_core.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index c2227983ed88..3455abbb2035 100644
>> +++ b/drivers/char/tpm/tpm_tis_core.c
> 
> Yoiks. This patch is helping but the more I look at this the wronger
> everything looks..
>
> 1) tpm_chip_unregister makes chip->ops == NULL, so this sequence:
> 
> static int tpm_tis_plat_remove(struct platform_device *pdev)
> 	tpm_chip_unregister(chip);
> 	tpm_tis_remove(chip);
> void tpm_tis_remove(struct tpm_chip *chip)
> 	if (chip->ops->clk_enable != NULL)
> 
> Will oops
>
> 2) tpm_chip_register can also NULL ops in error cases, so this
>    sequence can oops:
> 
>        rc = tpm_chip_register(chip);
>        if (rc && is_bsw())
>                iounmap(priv->ilb_base_addr);
> 
>         if (chip->ops->clk_enable != NULL)
>                 chip->ops->clk_enable(chip, false);
> 
> 3) iounmap should not be split between tpm_tis and tpm_tis_core
>    Put it at the end of tpm_tis_remove.
> 
> 4) This sequence:
> 
> +       return tpm_chip_register(chip);
> +out_err:
> +       tpm_tis_remove(chip);
> +       return rc;
> 
>    Doesn't look right. If tpm_chip_register fails then
>    tpm_tis_remove will never be called. This was sort of OK when
>    tpm_tis_remove didn't manage any resources, but now that it does
>    the above needs fixing too.
>

Right, I only noticed the issue this patch fixes and (wrongly) assumed the
rest was correct.

> The below draft fixes everything except #1. That needs a more thoughtful
> idea..
>

I'll just drop this patch from the series and you can fix all the issues in
the error / driver removal paths. It's not a dependency anyways, I included
it just because noticed the issue while reading the code.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag

[Javier Martinez Canillas]

On 12/20/2017 07:10 PM, Jason Gunthorpe wrote:
> On Wed, Dec 20, 2017 at 03:19:19PM +0000, Shaikh, Azhar wrote:
>>> This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
>>> systems, but the only way this can happen is if the code is not correct.
>>>
>>> So it's an unnecessary check that just makes the code harder to read.
>>
>> This code was implemented as a suggestion from Jason on the previous patches. 
>> https://www.spinics.net/lists/linux-integrity/msg00827.html
> 
> The concept was to be like ASSERT_RTNL, maybe it just needs a suitably
> named static inline to addrress Javier's readability concerns?
>

I really think is not worth it and pollutes all the tpm_tcg_{read,write}
functions with those is_bsw() and flags checks. Your example is different
since is a core API used by in lot of places in the kernel, but it's not
the case here.

But I don't have a strong opinion either, it was Jarkko who questioned
the value of the flag so I can drop this patch too if you disagree with
the change. I'm mostly interested in PATCH 4/4 that's the actual fix.
 
> Jason
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

On 12/20/2017 06:44 PM, Jason Gunthorpe wrote:
> On Wed, Dec 20, 2017 at 05:45:16PM +0100, Javier Martinez Canillas wrote:
> 
>> CHP51 says "LPC Clock Control Using the LPC_CLKRUN# May Not Behave As Expected"
>> and that the implication is that "The SoC may prevent a peripheral device from
>> successfully requesting the LPC clock".
> 
> Now we are back to the beginning - the LPC_CLKRUN protocol is simply
> broken in BSW chipsets, and it has nothing to do with the TPM?
> 
> Intel is trying to work around that broken-ness and still preserve
> power management in the case where only the TPM is connected to the
> LPC bus.. It is questionable to me if this is even a good idea, or if
> Linux is the right place to implement this work around (eg something
> in SMM mode may be more appropriate for a chipset bug)
> 
> I think your patch is still the right improvement, if the BIOS turned
> the feature off, we should not turn it back on.
>

Yes, the patch has merits on its own since fixes a flaw in the logic of
the original CLKRUN patch. I think we should merge it and then the other
issues can be fixed (or rework how the CLKRUN is managed) as follow-ups.
 
> Jason
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Jason Gunthorpe]

On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote:

> > The below draft fixes everything except #1. That needs a more thoughtful
> > idea..
> >
> 
> I'll just drop this patch from the series and you can fix all the issues in
> the error / driver removal paths. It's not a dependency anyways, I included
> it just because noticed the issue while reading the code.

Azhar, this means it becomes your problem :)

Since the patches Jarkko has queued from you introduce oops's you need
to fix them..

Jason

RE: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Shaikh, Azhar]

>-----Original Message-----
>From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>Sent: Wednesday, December 20, 2017 10:55 AM
>To: Javier Martinez Canillas <javierm@redhat.com>
>Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de
>Goede <hdegoede@redhat.com>; Shaikh, Azhar <azhar.shaikh@intel.com>;
>Arnd Bergmann <arnd@arndb.de>; Jarkko Sakkinen
><jarkko.sakkinen@linux.intel.com>; Peter Huewe <peterhuewe@gmx.de>;
>Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
>integrity@vger.kernel.org
>Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O
>memory region
>
>On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote:
>
>> > The below draft fixes everything except #1. That needs a more
>> > thoughtful idea..
>> >
>>
>> I'll just drop this patch from the series and you can fix all the
>> issues in the error / driver removal paths. It's not a dependency
>> anyways, I included it just because noticed the issue while reading the code.
>
>Azhar, this means it becomes your problem :)
>

Sure, I will fix this.
Javier you can drop this patch.

>Since the patches Jarkko has queued from you introduce oops's you need to
>fix them..
>
>Jason

Regards,
Azhar Shaikh

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[James Ettle]

OK, I built a kernel based on:

- git clone of git.infradead.org/users/jjs/linux-tpmdd.git taken last night
- applying the patch in https://patchwork.kernel.org/patch/10119417/
- stock config-4.14.6-300.fc27.x86_64 + make oldconfig

The tpm modules are loaded and the keyboard is working.

Note: I won't have access to this machine over the next few weeks.

Thanks,
James.


On 20/12/17 11:35, Javier Martinez Canillas wrote:
> Hello,
> 
> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
> signal during TPM transactions.
> 
> Unfortunately this breaks other devices that are attached to the LPC bus
> like for example PS/2 mouse and keyboards.
> 
> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
> This issue and the propossed solution were discussed in this [2] thread,
> and the reporter (Jame Ettle) confirmed that his system works again after
> the fix in this series.
> 
> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
> 
> James,
> 
> Even when there shouldn't be any functional changes, I included some other
> fixes / cleanups in this series so it would be great if you can test them
> again. I can't because I don't have access to a machine affected by this.
> 
> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
> [2]: https://patchwork.kernel.org/patch/10119417/
> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
> 
> Best regards,
> Javier
> 
> 
> Javier Martinez Canillas (4):
>   tpm: fix access attempt to an already unmapped I/O memory region
>   tpm: delete the TPM_TIS_CLK_ENABLE flag
>   tpm: follow coding style for variable declaration in
>     tpm_tis_core_init()
>   tpm: only attempt to disable the LPC CLKRUN if is already enabled
> 
>  drivers/char/tpm/tpm_tis.c      | 17 +----------------
>  drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++-------
>  drivers/char/tpm/tpm_tis_core.h |  1 -
>  3 files changed, 18 insertions(+), 24 deletions(-)
> 

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

Hello James,

On 12/21/2017 11:51 AM, James Ettle wrote:
> OK, I built a kernel based on:
> 
> - git clone of git.infradead.org/users/jjs/linux-tpmdd.git taken last night
> - applying the patch in https://patchwork.kernel.org/patch/10119417/

I meant linux-tpmdd + the 4 patches in this series.

> - stock config-4.14.6-300.fc27.x86_64 + make oldconfig
> 
> The tpm modules are loaded and the keyboard is working.
> 
> Note: I won't have access to this machine over the next few weeks.
> 

No worries, there shouldn't be any function changes anyways with the previous
patch shared in the other thread and the changes are quite simple so I think
it's OK.

Again thanks a lot for your help and merry Christmas / happy new year.

> Thanks,
> James.
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Javier Martinez Canillas]

Hello Azhar,

On 12/20/2017 08:15 PM, Shaikh, Azhar wrote:
> 
> 
>> -----Original Message-----
>> From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>> Sent: Wednesday, December 20, 2017 10:55 AM
>> To: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de
>> Goede <hdegoede@redhat.com>; Shaikh, Azhar <azhar.shaikh@intel.com>;
>> Arnd Bergmann <arnd@arndb.de>; Jarkko Sakkinen
>> <jarkko.sakkinen@linux.intel.com>; Peter Huewe <peterhuewe@gmx.de>;
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
>> integrity@vger.kernel.org
>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O
>> memory region
>>
>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote:
>>
>>>> The below draft fixes everything except #1. That needs a more
>>>> thoughtful idea..
>>>>
>>>
>>> I'll just drop this patch from the series and you can fix all the
>>> issues in the error / driver removal paths. It's not a dependency
>>> anyways, I included it just because noticed the issue while reading the code.
>>
>> Azhar, this means it becomes your problem :)
>>
> 
> Sure, I will fix this.
> Javier you can drop this patch.
>

On a second though, we should just merge $SUBJECT since is an obvious fix for
one of the issues. Then you could fix the remaining bugs in error and driver
removal paths.
 
>> Since the patches Jarkko has queued from you introduce oops's you need to
>> fix them..
>>
>> Jason
> 
> Regards,
> Azhar Shaikh
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Shaikh, Azhar]

HI Javier,


>-----Original Message-----
>From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
>owner@vger.kernel.org] On Behalf Of Javier Martinez Canillas
>Sent: Thursday, December 21, 2017 4:49 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe
><jgg@ziepe.ca>
>Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de
>Goede <hdegoede@redhat.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko
>Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
><peterhuewe@gmx.de>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org
>Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O
>memory region
>
>Hello Azhar,
>
>On 12/20/2017 08:15 PM, Shaikh, Azhar wrote:
>>
>>
>>> -----Original Message-----
>>> From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>>> Sent: Wednesday, December 20, 2017 10:55 AM
>>> To: Javier Martinez Canillas <javierm@redhat.com>
>>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>;
>>> Hans de Goede <hdegoede@redhat.com>; Shaikh, Azhar
>>> <azhar.shaikh@intel.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko
>>> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
>>> <peterhuewe@gmx.de>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>;
>>> linux- integrity@vger.kernel.org
>>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already
>>> unmapped I/O memory region
>>>
>>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote:
>>>
>>>>> The below draft fixes everything except #1. That needs a more
>>>>> thoughtful idea..
>>>>>
>>>>
>>>> I'll just drop this patch from the series and you can fix all the
>>>> issues in the error / driver removal paths. It's not a dependency
>>>> anyways, I included it just because noticed the issue while reading the
>code.
>>>
>>> Azhar, this means it becomes your problem :)
>>>
>>
>> Sure, I will fix this.
>> Javier you can drop this patch.
>>
>
>On a second though, we should just merge $SUBJECT since is an obvious fix
>for one of the issues. Then you could fix the remaining bugs in error and
>driver removal paths.

I think you are asking to keep this patch as a part of this series, itself and not upload separately. Is this correct? Please correct me if I am wrong.

>
>>> Since the patches Jarkko has queued from you introduce oops's you
>>> need to fix them..
>>>
>>> Jason
>>
>> Regards,
>> Azhar Shaikh
>>
>
>Best regards,
>--
>Javier Martinez Canillas
>Software Engineer - Desktop Hardware Enablement Red Hat

Regards,
Azhar Shaikh

Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Javier Martinez Canillas]

On 12/21/2017 04:39 PM, Shaikh, Azhar wrote:
> HI Javier,
> 
> 
>> -----Original Message-----
>> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
>> owner@vger.kernel.org] On Behalf Of Javier Martinez Canillas
>> Sent: Thursday, December 21, 2017 4:49 AM
>> To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe
>> <jgg@ziepe.ca>
>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de
>> Goede <hdegoede@redhat.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko
>> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
>> <peterhuewe@gmx.de>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org
>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O
>> memory region
>>
>> Hello Azhar,
>>
>> On 12/20/2017 08:15 PM, Shaikh, Azhar wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>>>> Sent: Wednesday, December 20, 2017 10:55 AM
>>>> To: Javier Martinez Canillas <javierm@redhat.com>
>>>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>;
>>>> Hans de Goede <hdegoede@redhat.com>; Shaikh, Azhar
>>>> <azhar.shaikh@intel.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko
>>>> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
>>>> <peterhuewe@gmx.de>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>;
>>>> linux- integrity@vger.kernel.org
>>>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already
>>>> unmapped I/O memory region
>>>>
>>>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote:
>>>>
>>>>>> The below draft fixes everything except #1. That needs a more
>>>>>> thoughtful idea..
>>>>>>
>>>>>
>>>>> I'll just drop this patch from the series and you can fix all the
>>>>> issues in the error / driver removal paths. It's not a dependency
>>>>> anyways, I included it just because noticed the issue while reading the
>> code.
>>>>
>>>> Azhar, this means it becomes your problem :)
>>>>
>>>
>>> Sure, I will fix this.
>>> Javier you can drop this patch.
>>>
>>
>> On a second though, we should just merge $SUBJECT since is an obvious fix
>> for one of the issues. Then you could fix the remaining bugs in error and
>> driver removal paths.
> 
> I think you are asking to keep this patch as a part of this series, itself and not upload separately. Is this correct? Please correct me if I am wrong.
>

Yes, what I meant is that we should just keep this patch in the series since
it fixes one of the resource cleanup bugs. Then you can fix the other issues
as a follow-up.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Shaikh, Azhar]

>-----Original Message-----
>From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>Sent: Thursday, December 21, 2017 7:54 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe
><jgg@ziepe.ca>
>Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>; Hans de
>Goede <hdegoede@redhat.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko
>Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
><peterhuewe@gmx.de>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org
>Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O
>memory region
>
>On 12/21/2017 04:39 PM, Shaikh, Azhar wrote:
>> HI Javier,
>>
>>
>>> -----Original Message-----
>>> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
>>> owner@vger.kernel.org] On Behalf Of Javier Martinez Canillas
>>> Sent: Thursday, December 21, 2017 4:49 AM
>>> To: Shaikh, Azhar <azhar.shaikh@intel.com>; Jason Gunthorpe
>>> <jgg@ziepe.ca>
>>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>;
>>> Hans de Goede <hdegoede@redhat.com>; Arnd Bergmann
><arnd@arndb.de>;
>>> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
>>> <peterhuewe@gmx.de>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>;
>>> linux-integrity@vger.kernel.org
>>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already
>>> unmapped I/O memory region
>>>
>>> Hello Azhar,
>>>
>>> On 12/20/2017 08:15 PM, Shaikh, Azhar wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>>>>> Sent: Wednesday, December 20, 2017 10:55 AM
>>>>> To: Javier Martinez Canillas <javierm@redhat.com>
>>>>> Cc: linux-kernel@vger.kernel.org; James Ettle <james@ettle.org.uk>;
>>>>> Hans de Goede <hdegoede@redhat.com>; Shaikh, Azhar
>>>>> <azhar.shaikh@intel.com>; Arnd Bergmann <arnd@arndb.de>; Jarkko
>>>>> Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
>>>>> <peterhuewe@gmx.de>; Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org>;
>>>>> linux- integrity@vger.kernel.org
>>>>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already
>>>>> unmapped I/O memory region
>>>>>
>>>>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas
>wrote:
>>>>>
>>>>>>> The below draft fixes everything except #1. That needs a more
>>>>>>> thoughtful idea..
>>>>>>>
>>>>>>
>>>>>> I'll just drop this patch from the series and you can fix all the
>>>>>> issues in the error / driver removal paths. It's not a dependency
>>>>>> anyways, I included it just because noticed the issue while
>>>>>> reading the
>>> code.
>>>>>
>>>>> Azhar, this means it becomes your problem :)
>>>>>
>>>>
>>>> Sure, I will fix this.
>>>> Javier you can drop this patch.
>>>>
>>>
>>> On a second though, we should just merge $SUBJECT since is an obvious
>>> fix for one of the issues. Then you could fix the remaining bugs in
>>> error and driver removal paths.
>>
>> I think you are asking to keep this patch as a part of this series, itself and not
>upload separately. Is this correct? Please correct me if I am wrong.
>>
>
>Yes, what I meant is that we should just keep this patch in the series since it
>fixes one of the resource cleanup bugs. Then you can fix the other issues as a
>follow-up.
>

Ok, but even the patch which I will be submitting will be doing the cleanup. So it will just split the change.


>Best regards,
>--
>Javier Martinez Canillas
>Software Engineer - Desktop Hardware Enablement Red Hat

Regards,
Azhar Shaikh 

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Jeffery Miller]

On Thu, Dec 21, 2017 at 6:46 AM, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> I meant linux-tpmdd + the 4 patches in this series.

Javier,
I applied the four patches to the linux-tpmdd master 46dd3b29e328 this
morning and it
fixed the touchpad on my affected machine.

To be clear, the patchwork patches I applied were:
10125523
10125515
10125513
10125531

I have a slightly different machine (Lenovo Yoga 11e) but found the
same original CLKRUN commit doing a bisect.

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Javier Martinez Canillas]

Hello Jeffery,

On 12/21/2017 06:25 PM, Jeffery Miller wrote:
> On Thu, Dec 21, 2017 at 6:46 AM, Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>>
>> I meant linux-tpmdd + the 4 patches in this series.
> 
> Javier,
> I applied the four patches to the linux-tpmdd master 46dd3b29e328 this
> morning and it
> fixed the touchpad on my affected machine.
> 
> To be clear, the patchwork patches I applied were:
> 10125523
> 10125515
> 10125513
> 10125531
> 
> I have a slightly different machine (Lenovo Yoga 11e) but found the
> same original CLKRUN commit doing a bisect.
> 

Great, thanks for testing!

Can I add your Tested-by tag then?

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Jeffery Miller]

On Thu, Dec 21, 2017 at 12:44 PM, Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Hello Jeffery,
> Can I add your Tested-by tag then?

Yes you can.

tested-by: Jeffery Miller <jmiller@neverware.com>

Thanks,
Jeff

Re: [PATCH 3/4] tpm: follow coding style for variable declaration in tpm_tis_core_init()

[Jarkko Sakkinen]

On Wed, Dec 20, 2017 at 12:35:37PM +0100, Javier Martinez Canillas wrote:
> The coding style says "use just one data declaration per line (no commas
> for multiple data declarations)" so follow this convention.
> 
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  drivers/char/tpm/tpm_tis_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 09da1e1adc40..534cd46fdfae 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -798,7 +798,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		      const struct tpm_tis_phy_ops *phy_ops,
>  		      acpi_handle acpi_dev_handle)
>  {
> -	u32 vendor, intfcaps, intmask;
> +	u32 vendor;
> +	u32 intfcaps;
> +	u32 intmask;
>  	u8 rid;
>  	int rc, probe;
>  	struct tpm_chip *chip;
> -- 
> 2.14.3
> 

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

/Jarkko

Re: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag

[Jarkko Sakkinen]

On Wed, Dec 20, 2017 at 12:35:36PM +0100, Javier Martinez Canillas wrote:
> This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
> systems, but the only way this can happen is if the code is not correct.
> 
> So it's an unnecessary check that just makes the code harder to read.
> 
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

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

/Jarkko

Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Jarkko Sakkinen]

On Wed, Dec 20, 2017 at 12:35:35PM +0100, Javier Martinez Canillas wrote:
> The driver maps the I/O memory address to control the LPC bus CLKRUN_EN,
> but on the error path the memory is accessed by the .clk_enable handler
> after this was already unmapped. So only unmap the I/O memory region if
> it will not be used anymore.
> 
> Also, the correct thing to do is to cleanup the resources in the inverse
> order that were acquired to prevent issues like these.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  drivers/char/tpm/tpm_tis_core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index c2227983ed88..3455abbb2035 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -923,7 +923,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  
>  	rc = tpm_chip_register(chip);
>  	if (rc && is_bsw())
> -		iounmap(priv->ilb_base_addr);
> +		goto out_err;
>  
>  	if (chip->ops->clk_enable != NULL)
>  		chip->ops->clk_enable(chip, false);
> @@ -931,12 +931,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	return rc;
>  out_err:
>  	tpm_tis_remove(chip);
> -	if (is_bsw())
> -		iounmap(priv->ilb_base_addr);
>  
>  	if (chip->ops->clk_enable != NULL)
>  		chip->ops->clk_enable(chip, false);
>  
> +	if (is_bsw())
> +		iounmap(priv->ilb_base_addr);
> +
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(tpm_tis_core_init);
> -- 
> 2.14.3
> 

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

/Jarkko

Re: [PATCH 4/4] tpm: only attempt to disable the LPC CLKRUN if is already enabled

[Jarkko Sakkinen]

On Wed, Dec 20, 2017 at 12:35:38PM +0100, Javier Martinez Canillas wrote:
> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
> signal during TPM transactions.
> 
> Unfortunately this breaks other devices that are attached to the LPC bus
> like for example PS/2 mouse and keyboards.
> 
> One flaw with the logic is that it assumes that the CLKRUN is always
> enabled, and so it unconditionally enables it after a TPM transaction.
> 
> But it could be that the CLKRUN# signal was already disabled in the LPC
> bus and so after the driver probes, CLKRUN_EN will remain enabled which
> may break other devices that are attached to the LPC bus but don't have
> support for the CLKRUN protocol.
> 
> Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: James Ettle <james@ettle.org.uk>
> 
> ---
> 
> This patch fixes the bug reported for the Fedora kernel [0] and the kernel
> bugzilla [1]. The issue and the propossed solution were discussed in this
> [2] thread.
> 
> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
> [2]: https://patchwork.kernel.org/patch/10119417/
> 
> 
>  drivers/char/tpm/tpm_tis.c      |  2 +-
>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index d0ad9a89b02b..c79193f6d092 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -314,7 +314,7 @@ static int tpm_tis_plat_remove(struct platform_device *pdev)
>  	tpm_chip_unregister(chip);
>  	tpm_tis_remove(chip);
>  
> -	if (is_bsw())
> +	if (is_bsw() && priv->ilb_base_addr)
>  		iounmap(priv->ilb_base_addr);
>  
>  	return 0;
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 534cd46fdfae..17fb9c600d0e 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -742,7 +742,8 @@ 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())
> +	if (!IS_ENABLED(CONFIG_X86) || !is_bsw() ||
> +	    !data->ilb_base_addr)
>  		return;
>  
>  	if (value) {
> @@ -801,6 +802,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	u32 vendor;
>  	u32 intfcaps;
>  	u32 intmask;
> +	u32 clkrun_val;
>  	u8 rid;
>  	int rc, probe;
>  	struct tpm_chip *chip;
> @@ -826,6 +828,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  					ILB_REMAP_SIZE);
>  		if (!priv->ilb_base_addr)
>  			return -ENOMEM;
> +
> +		clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
> +		/* Check if CLKRUN# is already not enabled in the LPC bus */
> +		if (!(clkrun_val & LPC_CLKRUN_EN)) {
> +			iounmap(priv->ilb_base_addr);
> +			priv->ilb_base_addr = NULL;
> +		}
>  	}
>  
>  	if (chip->ops->clk_enable != NULL)
> @@ -935,7 +944,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	if (chip->ops->clk_enable != NULL)
>  		chip->ops->clk_enable(chip, false);
>  
> -	if (is_bsw())
> +	if (is_bsw() && priv->ilb_base_addr)
>  		iounmap(priv->ilb_base_addr);
>  
>  	return rc;
> -- 
> 2.14.3
> 

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

/Jarkko

Re: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag

[Jarkko Sakkinen]

On Wed, Dec 20, 2017 at 07:26:03PM +0100, Javier Martinez Canillas wrote:
> On 12/20/2017 07:10 PM, Jason Gunthorpe wrote:
> > On Wed, Dec 20, 2017 at 03:19:19PM +0000, Shaikh, Azhar wrote:
> >>> This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
> >>> systems, but the only way this can happen is if the code is not correct.
> >>>
> >>> So it's an unnecessary check that just makes the code harder to read.
> >>
> >> This code was implemented as a suggestion from Jason on the previous patches. 
> >> https://www.spinics.net/lists/linux-integrity/msg00827.html
> > 
> > The concept was to be like ASSERT_RTNL, maybe it just needs a suitably
> > named static inline to addrress Javier's readability concerns?
> >
> 
> I really think is not worth it and pollutes all the tpm_tcg_{read,write}
> functions with those is_bsw() and flags checks. Your example is different
> since is a core API used by in lot of places in the kernel, but it's not
> the case here.
> 
> But I don't have a strong opinion either, it was Jarkko who questioned
> the value of the flag so I can drop this patch too if you disagree with
> the change. I'm mostly interested in PATCH 4/4 that's the actual fix.

Not going to fight over this one. I would apply the patch but if there
is strong opposition I can reconsider.

/Jarkko

Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled

[Jarkko Sakkinen]

On Wed, Dec 20, 2017 at 12:43:39PM +0100, Hans de Goede wrote:
> Note I'm just reading along here, but I'm wondering if both the TPM
> and now also some PS/2 controllers need CLK_RUN to be disabled,
> why don't we just disable it once permanently and be done with it?
> 
> It seems that on machines with a PS/2 controller connected to
> the LPC bus the BIOS is already doing this, so I've a feeling that
> it not being done on devices with a TPM is a bug in the firmware
> there and we should just disable it everywhere (and probably
> find a better place then the TPM driver to do the disabling).
> 
> Note this is just an observation, I could be completely wrong here,
> but I've a feeling that just disabling CLKRUN all together is the
> right thing to do and that seems like an easier fix to me.

Agreed. Thanks for noting this. Right now, I would just apply TPM
driver specific fixes.

/Jarkko

Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region

[Jarkko Sakkinen]

On Wed, Dec 20, 2017 at 12:35:35PM +0100, Javier Martinez Canillas wrote:
> The driver maps the I/O memory address to control the LPC bus CLKRUN_EN,
> but on the error path the memory is accessed by the .clk_enable handler
> after this was already unmapped. So only unmap the I/O memory region if
> it will not be used anymore.
> 
> Also, the correct thing to do is to cleanup the resources in the inverse
> order that were acquired to prevent issues like these.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

I applied Azhar's updated patches to master and next. Shouldn't this be
dropped now?

/Jarkko