[BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[James Ettle]

Hello,

[First: Apologies if cross-posting from Kernel.org BZ is bad form; my distro BZ advised I post this to your mailing list as well.]

Situation: enabling TPM on a Clevo W510LU with an Intel N3160 CPU breaks PS/2 keyboard and mouse. They just don't respond until after a suspend/resume cycle, and after that they later stop after a while.

I have confirmed this by blacklisting tpm modules. I noticed this first with kernel 4.13, and have bisected it down to:

5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad is the first bad commit
commit 5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad
Author: Azhar Shaikh <azhar.shaikh@intel.com>
Date:   Sun Jun 18 19:17:59 2017 -0700

    tpm: Enable CLKRUN protocol for Braswell systems
    
    To overcome a hardware limitation on Intel Braswell systems,
    disable CLKRUN protocol during TPM transactions and re-enable
    once the transaction is completed.
    
    Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
    Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
    Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
    Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
    Signed-off-by: James Morris <james.l.morris@oracle.com>

:040000 040000 5437c91886cb62c497255f2c60dbedd7268ab50d 1863a1738ded35a817aad52f9f2b451bd43623d7 M	drivers

Currently in Kernel.org bugzilla 197287.

Please let me know if you need any further info.

Many thanks,
James.

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello James,

On 12/11/2017 08:37 PM, James Ettle wrote:
> Hello,
> 
> [First: Apologies if cross-posting from Kernel.org BZ is bad form; my distro BZ advised I post this to your mailing list as well.]
> 
> Situation: enabling TPM on a Clevo W510LU with an Intel N3160 CPU breaks PS/2 keyboard and mouse. They just don't respond until after a suspend/resume cycle, and after that they later stop after a while.
> 
> I have confirmed this by blacklisting tpm modules. I noticed this first with kernel 4.13, and have bisected it down to:
> 
> 5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad is the first bad commit
> commit 5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad
> Author: Azhar Shaikh <azhar.shaikh@intel.com>
> Date:   Sun Jun 18 19:17:59 2017 -0700
> 
>     tpm: Enable CLKRUN protocol for Braswell systems
>     
>     To overcome a hardware limitation on Intel Braswell systems,
>     disable CLKRUN protocol during TPM transactions and re-enable
>     once the transaction is completed.
>     
>     Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
>     Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>     Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>     Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>     Signed-off-by: James Morris <james.l.morris@oracle.com>
> 
> :040000 040000 5437c91886cb62c497255f2c60dbedd7268ab50d 1863a1738ded35a817aad52f9f2b451bd43623d7 M	drivers
> 
> Currently in Kernel.org bugzilla 197287.
> 
> Please let me know if you need any further info.
>

I don't have access to a Braswell machine to reproduce this. I tried to do it
on different Intel systems by modifying is_bsw() to always return true, but
that didn't work either. They work correctly even when CLKRUN_EN is toggled.

I'm not familiar with LPC so please let me know if my assumptions are wrong,
but I find suspicious that a driver for a single device attached to the bus
can control the CLKRUN# signal which AFAIU may be needed for other devices.

So that would explain why the mentioned commit causes issues for PS/2 mouse
and keyboards, since these are attached to the LPC bus and may rely on the
CLKRUN# signal for proper operation.

I see that the following [0,1] patches for the tpm_tis driver landed a few
days ago, they change how the CLKRUN protocol is enabled/disabled. Instead
of doing it per each TPM transaction, it does it once for the duration of
a TPM command.

Not sure if that will make things better or worse for you, but it would be
good to try in case it makes a difference.

[0]: http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/667dcc75be864ff4c17cf58891853b7393bba3e2
[1]: http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/db3248e8a036c39141c8f7e9f1cf5c5ae6815f76

> Many thanks,
> James.
> 

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Tue, Dec 12, 2017 at 12:38:00PM +0100, Javier Martinez Canillas wrote:

> I'm not familiar with LPC so please let me know if my assumptions are wrong,
> but I find suspicious that a driver for a single device attached to the bus
> can control the CLKRUN# signal which AFAIU may be needed for other devices.

My understanding was that the fix was to force CLKRUN on before
starting a LPC transaction for TPM.

I guess the issues is blindly assuming that CLKRUN started out as off.
It needs to remember the old setting and put it back, and hope against
all hope there isn't another thread mucking with it.

Guessing that on a system with a LPC connected superio controller for
PS/2 that CLKRUN will be always high???

Jason

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello Jason,

On 12/12/2017 07:57 PM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2017 at 12:38:00PM +0100, Javier Martinez Canillas wrote:
> 
>> I'm not familiar with LPC so please let me know if my assumptions are wrong,
>> but I find suspicious that a driver for a single device attached to the bus
>> can control the CLKRUN# signal which AFAIU may be needed for other devices.
> 
> My understanding was that the fix was to force CLKRUN on before
> starting a LPC transaction for TPM.
>

Yes, I got that part from the commit message and the thread when was posted.
 
> I guess the issues is blindly assuming that CLKRUN started out as off.
> It needs to remember the old setting and put it back, and hope against
> all hope there isn't another thread mucking with it.
>

This is the part that worries me, and it seems that assumption was wrong
due the reported issues with other PS/2 devices attached to the LPC bus.

> Guessing that on a system with a LPC connected superio controller for
> PS/2 that CLKRUN will be always high???
>

I'm not really familiar with the LPC bus trasmision protocol to comment
on this. But if different devices rely on the CLKRUN state, then as you
said it needs to remember the old setting and also some synchronization
mechanism should be used to serialize access to it and prevent a race?

> Jason
> 

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[James Ettle]

OK, I built a kernel 4.14.5 from vanilla kernel.org sources using the Fedora .config (couldn't get the Fedora package to build).

I see no difference with both [0, 1] patches applied and the tpm modules loaded -- no keyboard or touchpad.

[Note: I'm not *consciously* using the TPM for anything, but /dev/tpm0 is present and have never initialised it. I don't know if anything in Fedora uses it by default if present.]

Regards,
James.


On 12/12/17 11:38, Javier Martinez Canillas wrote:
> Hello James,
> 
> On 12/11/2017 08:37 PM, James Ettle wrote:
>> Hello,
>>
>> [First: Apologies if cross-posting from Kernel.org BZ is bad form; my distro BZ advised I post this to your mailing list as well.]
>>
>> Situation: enabling TPM on a Clevo W510LU with an Intel N3160 CPU breaks PS/2 keyboard and mouse. They just don't respond until after a suspend/resume cycle, and after that they later stop after a while.
>>
>> I have confirmed this by blacklisting tpm modules. I noticed this first with kernel 4.13, and have bisected it down to:
>>
>> 5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad is the first bad commit
>> commit 5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad
>> Author: Azhar Shaikh <azhar.shaikh@intel.com>
>> Date:   Sun Jun 18 19:17:59 2017 -0700
>>
>>     tpm: Enable CLKRUN protocol for Braswell systems
>>     
>>     To overcome a hardware limitation on Intel Braswell systems,
>>     disable CLKRUN protocol during TPM transactions and re-enable
>>     once the transaction is completed.
>>     
>>     Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
>>     Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>     Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>     Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>     Signed-off-by: James Morris <james.l.morris@oracle.com>
>>
>> :040000 040000 5437c91886cb62c497255f2c60dbedd7268ab50d 1863a1738ded35a817aad52f9f2b451bd43623d7 M	drivers
>>
>> Currently in Kernel.org bugzilla 197287.
>>
>> Please let me know if you need any further info.
>>
> 
> I don't have access to a Braswell machine to reproduce this. I tried to do it
> on different Intel systems by modifying is_bsw() to always return true, but
> that didn't work either. They work correctly even when CLKRUN_EN is toggled.
> 
> I'm not familiar with LPC so please let me know if my assumptions are wrong,
> but I find suspicious that a driver for a single device attached to the bus
> can control the CLKRUN# signal which AFAIU may be needed for other devices.
> 
> So that would explain why the mentioned commit causes issues for PS/2 mouse
> and keyboards, since these are attached to the LPC bus and may rely on the
> CLKRUN# signal for proper operation.
> 
> I see that the following [0,1] patches for the tpm_tis driver landed a few
> days ago, they change how the CLKRUN protocol is enabled/disabled. Instead
> of doing it per each TPM transaction, it does it once for the duration of
> a TPM command.
> 
> Not sure if that will make things better or worse for you, but it would be
> good to try in case it makes a difference.
> 
> [0]: http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/667dcc75be864ff4c17cf58891853b7393bba3e2
> [1]: http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/db3248e8a036c39141c8f7e9f1cf5c5ae6815f76
> 
>> Many thanks,
>> James.
>>
> 
> Best regards,
> 

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

On Mon, Dec 11, 2017 at 07:37:29PM +0000, James Ettle wrote:
> Hello,
> 
> [First: Apologies if cross-posting from Kernel.org BZ is bad form; my
> distro BZ advised I post this to your mailing list as well.]
> 
> Situation: enabling TPM on a Clevo W510LU with an Intel N3160 CPU
> breaks PS/2 keyboard and mouse. They just don't respond until after a
> suspend/resume cycle, and after that they later stop after a while.
> 
> I have confirmed this by blacklisting tpm modules. I noticed this
> first with kernel 4.13, and have bisected it down to:

In my GIT tree there is now:

commit db3248e8a036c39141c8f7e9f1cf5c5ae6815f76 (HEAD -> next, origin/next, origin/master, master)
Author: Azhar Shaikh <azhar.shaikh@intel.com>
Date:   Wed Dec 6 17:38:10 2017 -0800

    tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
    
    Commit 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
    systems") disabled CLKRUN protocol during TPM transactions and re-enabled
    once the transaction is completed. But there were still some corner cases
    observed where, reading of TPM header failed for savestate command
    while going to suspend, which resulted in suspend failure.
    To fix this issue keep the CLKRUN protocol disabled for the entire
    duration of a single TPM command and not disabling and re-enabling
    again for every TPM transaction. For the other TPM accesses outside
    TPM command flow, add a higher level of disabling and re-enabling
    the CLKRUN protocol, instead of doing for every TPM transaction.
    
    Fixes: 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell systems")
    
    Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
    Reviewed-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
    Tested-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
    Signed-off-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>

Can you try this?

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello Jarkko,

On 12/14/2017 12:21 PM, Jarkko Sakkinen wrote:
> On Mon, Dec 11, 2017 at 07:37:29PM +0000, James Ettle wrote:
>> Hello,
>>
>> [First: Apologies if cross-posting from Kernel.org BZ is bad form; my
>> distro BZ advised I post this to your mailing list as well.]
>>
>> Situation: enabling TPM on a Clevo W510LU with an Intel N3160 CPU
>> breaks PS/2 keyboard and mouse. They just don't respond until after a
>> suspend/resume cycle, and after that they later stop after a while.
>>
>> I have confirmed this by blacklisting tpm modules. I noticed this
>> first with kernel 4.13, and have bisected it down to:
> 
> In my GIT tree there is now:
> 
> commit db3248e8a036c39141c8f7e9f1cf5c5ae6815f76 (HEAD -> next, origin/next, origin/master, master)
> Author: Azhar Shaikh <azhar.shaikh@intel.com>
> Date:   Wed Dec 6 17:38:10 2017 -0800
> 
>     tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
>     
>     Commit 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
>     systems") disabled CLKRUN protocol during TPM transactions and re-enabled
>     once the transaction is completed. But there were still some corner cases
>     observed where, reading of TPM header failed for savestate command
>     while going to suspend, which resulted in suspend failure.
>     To fix this issue keep the CLKRUN protocol disabled for the entire
>     duration of a single TPM command and not disabling and re-enabling
>     again for every TPM transaction. For the other TPM accesses outside
>     TPM command flow, add a higher level of disabling and re-enabling
>     the CLKRUN protocol, instead of doing for every TPM transaction.
>     
>     Fixes: 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell systems")
>     
>     Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
>     Reviewed-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
>     Tested-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
>     Signed-off-by: Jarkko Sakkinen  <jarkko.sakkinen@linux.intel.com>
> 
> Can you try this?
>

I already suggested the same [0], but James said that made no difference [1].
 
> /Jarkko
>

[0]: https://lkml.org/lkml/2017/12/12/268
[1]: https://lkml.org/lkml/2017/12/12/1127

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello James,

On 12/12/2017 10:17 PM, James Ettle wrote:
> OK, I built a kernel 4.14.5 from vanilla kernel.org sources using the Fedora .config (couldn't get the Fedora package to build).
> 
> I see no difference with both [0, 1] patches applied and the tpm modules loaded -- no keyboard or touchpad.
> 
> [Note: I'm not *consciously* using the TPM for anything, but /dev/tpm0 is present and have never initialised it. I don't know if anything in Fedora uses it by default if present.]
>

On a default Fedora install the TPM isn't used, but the CLKRUN_EN toggling
happens for all I/O operations with the TPM and the driver does access the
TPM on its probe function.

So even if you don't use it, the driver will mess with the CLKRUN signal.

Jarkko and Azhar,

I wonder what's the solution for this. I understand the quirk is needed for
some systems, but on the other hand breaks functionality for other devices.

I think that at the very least this should be disabled by default and have a
Kconfig option or module parameter to enable if needed. Since as James said,
it causes regressions even for people that are not using the TPM device.

Although probably reverting the offending commits is the right thing to do
until a proper solution is proposed.

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:

> Although probably reverting the offending commits is the right thing to do
> until a proper solution is proposed.

Yes, if a fix is not forthcoming soon Jarkko should revert.

Jason

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
> 
> > Although probably reverting the offending commits is the right thing to do
> > until a proper solution is proposed.
> 
> Yes, if a fix is not forthcoming soon Jarkko should revert.
> 
> Jason

I think I should drop the current patch going to 4.16 and revert the old
patch. Do we agree on this?

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
> > On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
> > 
> > > Although probably reverting the offending commits is the right thing to do
> > > until a proper solution is proposed.
> > 
> > Yes, if a fix is not forthcoming soon Jarkko should revert.
> > 
> 
> I think I should drop the current patch going to 4.16 and revert the old
> patch. Do we agree on this?

I think the entire is_bsw thing needs to go, clearly manipulating
CLKRUN directly was not fully thought out?

Hopefully Intel will come up with a fix patch to preserve PS/2
functionality and it won't come to a revert..

Jason

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote:
> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
> > > On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
> > > 
> > > > Although probably reverting the offending commits is the right thing to do
> > > > until a proper solution is proposed.
> > > 
> > > Yes, if a fix is not forthcoming soon Jarkko should revert.
> > > 
> > 
> > I think I should drop the current patch going to 4.16 and revert the old
> > patch. Do we agree on this?
> 
> I think the entire is_bsw thing needs to go, clearly manipulating
> CLKRUN directly was not fully thought out?
> 
> Hopefully Intel will come up with a fix patch to preserve PS/2
> functionality and it won't come to a revert..

Agreed.

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

On 12/16/2017 06:01 PM, Jarkko Sakkinen wrote:
> On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote:
>> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
>>> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
>>>> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
>>>>
>>>>> Although probably reverting the offending commits is the right thing to do
>>>>> until a proper solution is proposed.
>>>>
>>>> Yes, if a fix is not forthcoming soon Jarkko should revert.
>>>>
>>>
>>> I think I should drop the current patch going to 4.16 and revert the old
>>> patch. Do we agree on this?
>>
>> I think the entire is_bsw thing needs to go, clearly manipulating
>> CLKRUN directly was not fully thought out?
>>
>> Hopefully Intel will come up with a fix patch to preserve PS/2
>> functionality and it won't come to a revert..
> 
> Agreed.
>

Agreed. Although it would be good to have a solution soon, because this has been
broken since v4.13 and v4.15-rc4 is likely to come this weekend. So we may end
with Braswell system being non-function for 3 kernel releases...

As mentioned I can write a patch to hide this behavior under a Kconfig option
that's disabled by default (and maybe depend on BROKEN) or a module parameter
until a proper solution is found.

> /Jarkko
> 

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

On 12/16/2017 09:59 PM, Javier Martinez Canillas wrote:
> On 12/16/2017 06:01 PM, Jarkko Sakkinen wrote:
>> On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote:
>>> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
>>>> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
>>>>> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
>>>>>
>>>>>> Although probably reverting the offending commits is the right thing to do
>>>>>> until a proper solution is proposed.
>>>>>
>>>>> Yes, if a fix is not forthcoming soon Jarkko should revert.
>>>>>
>>>>
>>>> I think I should drop the current patch going to 4.16 and revert the old
>>>> patch. Do we agree on this?
>>>
>>> I think the entire is_bsw thing needs to go, clearly manipulating
>>> CLKRUN directly was not fully thought out?
>>>
>>> Hopefully Intel will come up with a fix patch to preserve PS/2
>>> functionality and it won't come to a revert..
>>
>> Agreed.
>>
> 
> Agreed. Although it would be good to have a solution soon, because this has been
> broken since v4.13 and v4.15-rc4 is likely to come this weekend. So we may end
> with Braswell system being non-function for 3 kernel releases...
> 
> As mentioned I can write a patch to hide this behavior under a Kconfig option
> that's disabled by default (and maybe depend on BROKEN) or a module parameter
> until a proper solution is found.
> 

I meant something like the following [0]. That way, it'll be disabled by default
but still users that really need this workaround can do:

$ ./scripts/config --enable EXPERT
$ ./scripts/config --enable CONFIG_TCG_TIS_ENABLE_CLKRUN_QUIRK

What do you think?

[0]:
>From 2ac0d743392ab96d5bf804830e0f0763a253c6e8 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Mon, 18 Dec 2017 11:10:01 +0100
Subject: [PATCH 1/1] tpm: Add Kconfig option to avoid disabling LPC CLKRUN
 unconditionally

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 transations.

Unfortunately this breaks other devices that are attached into the LPC
bus (e.g: PS/2 mouse and keyboards), so the CLKRUN signal shouldn't be
disabled unconditionally.

Until a proper solution is found, add a Kconfig option to explicitly
enable that behavior if needed instead of doing it unconditionally on
all Braswell machines.

Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/char/tpm/Kconfig        | 10 ++++++++++
 drivers/char/tpm/tpm_tis_core.c |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index a30352202f1f..220bd24e399f 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -43,6 +43,16 @@ config TCG_TIS
 	  within Linux. To compile this driver as a module, choose  M here;
 	  the module will be called tpm_tis.
 
+config TCG_TIS_ENABLE_CLKRUN_QUIRK
+	bool "Enable LPC CLKRUN workaround for Braswell systems TPM" if EXPERT
+	depends on TCG_TIS_CORE && X86
+	default n
+	---help---
+	  On Intel Braswell systems, the Low Pin Count bus CLKRUN signal has to
+	  be disabled during TPM devices transactions to operate correctly. This
+	  could break other devices attached to the LPC bus (i.e: PS/2 mouse and
+	  keyboards) so only enable if the TPM is the only device in the LPC bus.
+
 config TCG_TIS_SPI
 	tristate "TPM Interface Specification 1.3 Interface / TPM 2.0 FIFO Interface - (SPI)"
 	depends on SPI
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e7bd2e750f69..0858c08b3b89 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -688,7 +688,7 @@ 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(TCG_TIS_ENABLE_CLKRUN_QUIRK) || !is_bsw())
 		return;
 
 	if (value) {
-- 
2.14.3

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

On 12/16/2017 06:01 PM, Jarkko Sakkinen wrote:
> On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote:
>> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
>>> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
>>>> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
>>>>
>>>>> Although probably reverting the offending commits is the right thing to do
>>>>> until a proper solution is proposed.
>>>>
>>>> Yes, if a fix is not forthcoming soon Jarkko should revert.
>>>>
>>>
>>> I think I should drop the current patch going to 4.16 and revert the old
>>> patch. Do we agree on this?
>>
>> I think the entire is_bsw thing needs to go, clearly manipulating
>> CLKRUN directly was not fully thought out?
>>
>> Hopefully Intel will come up with a fix patch to preserve PS/2
>> functionality and it won't come to a revert..
> 
> Agreed.

One flaw I found with the logic is that it assumes that the CLKRUN signal will
always be enabled. And so the driver enables it unconditionally after is probed
(I believe Jason mentioned that before?).

But I wonder if what's causing issues with the PS/2 devices is that the devices
assume the CLKRUN signal is disabled but this changes because of the tpm driver?

James,

Can you please test the following (untested) patch on top of the other two
mentioned patches to see if it makes a difference for you?

>From 72a57eaa425d639d2622339173ff6bf9d9c86ff3 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Mon, 18 Dec 2017 12:56:28 +0100
Subject: [PATCH 1/1] tpm: only attempt to disable the LPC CLKRUN if is already
 enabled

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, the signal will remain enabled which
may break other devices transactions since the clocks will be restarted
by the CLKRUN# signal.

Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e7bd2e750f69..42eb2c54494e 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -746,7 +746,7 @@ 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, intfcaps, intmask, clkrun_val;
 	u8 rid;
 	int rc, probe;
 	struct tpm_chip *chip;
@@ -772,6 +772,15 @@ 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)) {
+			priv->flags |= TPM_TIS_CLK_ENABLE;
+			iounmap(priv->ilb_base_addr);
+			priv->ilb_base_addr = NULL;
+			chip->ops->clk_enable = NULL;
+		}
 	}
 
 	if (chip->ops->clk_enable != NULL)
@@ -868,7 +877,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())
+	if (rc && is_bsw() && priv->ilb_base_addr)
 		iounmap(priv->ilb_base_addr);
 
 	if (chip->ops->clk_enable != NULL)
-- 
2.14.3

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

On 12/16/2017 06:01 PM, Jarkko Sakkinen wrote:
> On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote:
>> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
>>> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
>>>> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
>>>>
>>>>> Although probably reverting the offending commits is the right thing to do
>>>>> until a proper solution is proposed.
>>>>
>>>> Yes, if a fix is not forthcoming soon Jarkko should revert.
>>>>
>>>
>>> I think I should drop the current patch going to 4.16 and revert the old
>>> patch. Do we agree on this?
>>
>> I think the entire is_bsw thing needs to go, clearly manipulating
>> CLKRUN directly was not fully thought out?
>>
>> Hopefully Intel will come up with a fix patch to preserve PS/2
>> functionality and it won't come to a revert..
> 
> Agreed.

One flaw I found with the logic is that it assumes that the CLKRUN signal will
always be enabled. And so the driver enables it unconditionally after is probed
(I believe Jason mentioned that before?).

But I wonder if what's causing issues with the PS/2 devices is that the devices
assume the CLKRUN signal is disabled but this changes because of the tpm driver?

James,

Can you please test the following (untested) patch on top of the other two
mentioned patches to see if it makes a difference for you?

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:

[snip]

> 
> James,
> 
> Can you please test the following (untested) patch on top of the other two
> mentioned patches to see if it makes a difference for you?
> 

I should had tried to at least compile the patch :)

Updated patch below:

>From 370d45a34dc8914066a995a3a6d6df1953ea9f60 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Mon, 18 Dec 2017 12:56:28 +0100
Subject: [PATCH v2] tpm: only attempt to disable the LPC CLKRUN if is already
 enabled

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, the signal will remain enabled which
may break other devices transactions since the clocks will be restarted
by the CLKRUN# signal.

Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/char/tpm/tpm_tis_core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e7bd2e750f69..5f2b1fc2194f 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -688,7 +688,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) {
@@ -746,7 +747,7 @@ 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, intfcaps, intmask, clkrun_val;
 	u8 rid;
 	int rc, probe;
 	struct tpm_chip *chip;
@@ -772,6 +773,14 @@ 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)) {
+			priv->flags |= TPM_TIS_CLK_ENABLE;
+			iounmap(priv->ilb_base_addr);
+			priv->ilb_base_addr = NULL;
+		}
 	}
 
 	if (chip->ops->clk_enable != NULL)
@@ -868,7 +877,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())
+	if (rc && is_bsw() && priv->ilb_base_addr)
 		iounmap(priv->ilb_base_addr);
 
 	if (chip->ops->clk_enable != NULL)
-- 
2.14.3

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:

[snip]

> 
> James,
> 
> Can you please test the following (untested) patch on top of the other two
> mentioned patches to see if it makes a difference for you?
> 

I should had tried to at least compile the patch :)

Updated patch below:

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
> 
> [snip]
> 
> > 
> > James,
> > 
> > Can you please test the following (untested) patch on top of the other two
> > mentioned patches to see if it makes a difference for you?
> > 
> 
> I should had tried to at least compile the patch :)

I think this is backwards..

If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
TPM shouldn't do anything at all.

If CLKRUN_EN is off, then it should try to turn it on/off to save
power.

Perhaps the best work around is to just delete the turning off of
CLKRUN_EN ? Uses more power but keeps the clock running which should
keep both TPM and superio happy.

Jason

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[James Ettle]

The keyboard and touchpad work OK with the patch quoted below and the earlier two applied, i.e. the three patches with signatures:

667dcc75be864ff4c17cf58891853b7393bba3e2
db3248e8a036c39141c8f7e9f1cf5c5ae6815f76
370d45a34dc8914066a995a3a6d6df1953ea9f60

I applied these to a vanilla kernel.org 4.14.7 source using Fedora 4.14.5-300.fc27 .config. Confirmed the tpm modules are loaded.

Tests:
1. Keyboard and touchpad work OK on boot - PASS
2. Still work after suspend/resume - PASS

Let me know if you want any further tests.

Many thanks,
James.


On 18/12/17 12:29, Javier Martinez Canillas wrote:
> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
> 
> [snip]
> 
>>
>> James,
>>
>> Can you please test the following (untested) patch on top of the other two
>> mentioned patches to see if it makes a difference for you?
>>
> 
> I should had tried to at least compile the patch :)
> 
> Updated patch below:
> 
> From 370d45a34dc8914066a995a3a6d6df1953ea9f60 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> Date: Mon, 18 Dec 2017 12:56:28 +0100
> Subject: [PATCH v2] tpm: only attempt to disable the LPC CLKRUN if is already
>  enabled
> 
> 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, the signal will remain enabled which
> may break other devices transactions since the clocks will be restarted
> by the CLKRUN# signal.
> 
> Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index e7bd2e750f69..5f2b1fc2194f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -688,7 +688,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) {
> @@ -746,7 +747,7 @@ 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, intfcaps, intmask, clkrun_val;
>  	u8 rid;
>  	int rc, probe;
>  	struct tpm_chip *chip;
> @@ -772,6 +773,14 @@ 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)) {
> +			priv->flags |= TPM_TIS_CLK_ENABLE;
> +			iounmap(priv->ilb_base_addr);
> +			priv->ilb_base_addr = NULL;
> +		}
>  	}
>  
>  	if (chip->ops->clk_enable != NULL)
> @@ -868,7 +877,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())
> +	if (rc && is_bsw() && priv->ilb_base_addr)
>  		iounmap(priv->ilb_base_addr);
>  
>  	if (chip->ops->clk_enable != NULL)
> 

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello Jason,

Thanks a lot for your feedback.

On 12/18/2017 06:55 PM, Jason Gunthorpe wrote:
> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
>> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
>>
>> [snip]
>>
>>>
>>> James,
>>>
>>> Can you please test the following (untested) patch on top of the other two
>>> mentioned patches to see if it makes a difference for you?
>>>
>>
>> I should had tried to at least compile the patch :)
> 
> I think this is backwards..
> 
> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
> TPM shouldn't do anything at all.
> 
> If CLKRUN_EN is off, then it should try to turn it on/off to save
> power.
>

My knowledge of LPC is near to non-existent so I please forgive me if I'm wrong,
but my understanding is that it's the opposite of what you said.

When CLKRUN_EN is SET, power management is ENABLED on the LPC bus and the host
LCLK clock may be stopped when entering in some low-power states.

The CLKRUN# signal is then used by peripherals to restart the LCLK clock after
resuming from low-power states to be able to transmit again.

When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus and the
host LCLK clock is never stopped even when entering in some low-power states.

IIUC, if CLKRUN_EN is enabled, then all the devices attached to the LPC bus
have to support the CLKRUN protocol. My guess is that on some Braswell systems
LPC power management is enabled but the TPM device doesn't have CLKRUN support.

And that was the motivation of commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol
for Braswell systems") that introduced the regression.

> Perhaps the best work around is to just delete the turning off of
> CLKRUN_EN ? Uses more power but keeps the clock running which should
> keep both TPM and superio happy.
>

But that's exactly the goal of the commit 5e572cab92f0, to disable the CLKRUN
protocol (probably for what I mentioned before). So doing so will cause issues
for those systems again.

What the patch I shared with James does is to avoid disabling the CLKRUN_EN
if this is already disabled. Which should be a no-op anyways but the problem
is that latter it's enabled even when the driver found disabled to start with.

I still don't like the overall solution since it's too error prone. Touching a
global bus configuration in a driver for a single peripheral isn't safe to say
the least.

But regardless of that, the patch I shared has merits on its own since is wrong
to keep the bus configuration in a different state that was before the driver
was probed. And in fact, James reported that it makes his system to work again.

> Jason
>

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello James,

On 12/18/2017 07:26 PM, James Ettle wrote:
> The keyboard and touchpad work OK with the patch quoted below and the earlier two applied, i.e. the three patches with signatures:
> 
> 667dcc75be864ff4c17cf58891853b7393bba3e2
> db3248e8a036c39141c8f7e9f1cf5c5ae6815f76
> 370d45a34dc8914066a995a3a6d6df1953ea9f60
> 
> I applied these to a vanilla kernel.org 4.14.7 source using Fedora 4.14.5-300.fc27 .config. Confirmed the tpm modules are loaded.
> 
> Tests:
> 1. Keyboard and touchpad work OK on boot - PASS
> 2. Still work after suspend/resume - PASS
> 
> Let me know if you want any further tests.
>

Great, thanks a lot for testing!
 
> Many thanks,
> James.
>
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Shaikh, Azhar]

>-----Original Message-----
>From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>Sent: Monday, December 18, 2017 11:30 AM
>To: Jason Gunthorpe <jgg@ziepe.ca>
>Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; James Ettle
><james@ettle.org.uk>; linux-integrity@vger.kernel.org; Shaikh, Azhar
><azhar.shaikh@intel.com>; linux-kernel@vger.kernel.org;
>james.l.morris@oracle.com
>Subject: Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on
>Braswell system
>
>Hello Jason,
>
>Thanks a lot for your feedback.
>
>On 12/18/2017 06:55 PM, Jason Gunthorpe wrote:
>> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
>>> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
>>>
>>> [snip]
>>>
>>>>
>>>> James,
>>>>
>>>> Can you please test the following (untested) patch on top of the
>>>> other two mentioned patches to see if it makes a difference for you?
>>>>
>>>
>>> I should had tried to at least compile the patch :)
>>
>> I think this is backwards..
>>
>> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
>> TPM shouldn't do anything at all.
>>
>> If CLKRUN_EN is off, then it should try to turn it on/off to save
>> power.
>>
>
>My knowledge of LPC is near to non-existent so I please forgive me if I'm
>wrong, but my understanding is that it's the opposite of what you said.
>
>When CLKRUN_EN is SET, power management is ENABLED on the LPC bus and
>the host LCLK clock may be stopped when entering in some low-power states.
>
>The CLKRUN# signal is then used by peripherals to restart the LCLK clock after
>resuming from low-power states to be able to transmit again.
>
>When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus
>and the host LCLK clock is never stopped even when entering in some low-
>power states.
>

Hi Javier,

Yes you are correct with the above understanding of the CLKRUN.

>IIUC, if CLKRUN_EN is enabled, then all the devices attached to the LPC bus
>have to support the CLKRUN protocol. My guess is that on some Braswell
>systems LPC power management is enabled but the TPM device doesn't have
>CLKRUN support.
>

I think this is what might be happening here.

Since I do not have an end product to test this on, I managed to get one BSW Reference Verification Platform(RVP). I flashed the latest Ubuntu (17.10) which is on 4.13.13 kernel version and does have the patch.

I was able to use the PS/2 mouse and keyboard immediately after bootup and also after suspend/resume cycle. The system does have a TPM.(/dev/tpm0 and /dev/tpmrm0 exist)


>And that was the motivation of commit 5e572cab92f0 ("tpm: Enable CLKRUN
>protocol for Braswell systems") that introduced the regression.
>
>> Perhaps the best work around is to just delete the turning off of
>> CLKRUN_EN ? Uses more power but keeps the clock running which should
>> keep both TPM and superio happy.
>>
>
>But that's exactly the goal of the commit 5e572cab92f0, to disable the CLKRUN
>protocol (probably for what I mentioned before). So doing so will cause issues
>for those systems again.
>
>What the patch I shared with James does is to avoid disabling the CLKRUN_EN
>if this is already disabled. Which should be a no-op anyways but the problem
>is that latter it's enabled even when the driver found disabled to start with.
>
>I still don't like the overall solution since it's too error prone. Touching a global
>bus configuration in a driver for a single peripheral isn't safe to say the least.
>
>But regardless of that, the patch I shared has merits on its own since is wrong
>to keep the bus configuration in a different state that was before the driver
>was probed. And in fact, James reported that it makes his system to work
>again.
>
>> Jason
>>
>
>Best regards,
>--
>Javier Martinez Canillas
>Software Engineer - Desktop Hardware Enablement Red Hat

RE: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Shaikh, Azhar]

>-----Original Message-----
>From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
>owner@vger.kernel.org] On Behalf Of Shaikh, Azhar
>Sent: Monday, December 18, 2017 11:34 AM
>To: Javier Martinez Canillas <javierm@redhat.com>; Jason Gunthorpe
><jgg@ziepe.ca>
>Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; James Ettle
><james@ettle.org.uk>; linux-integrity@vger.kernel.org; linux-
>kernel@vger.kernel.org; james.l.morris@oracle.com
>Subject: RE: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on
>Braswell system
>
>
>
>>-----Original Message-----
>>From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>>Sent: Monday, December 18, 2017 11:30 AM
>>To: Jason Gunthorpe <jgg@ziepe.ca>
>>Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; James Ettle
>><james@ettle.org.uk>; linux-integrity@vger.kernel.org; Shaikh, Azhar
>><azhar.shaikh@intel.com>; linux-kernel@vger.kernel.org;
>>james.l.morris@oracle.com
>>Subject: Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on
>>Braswell system
>>
>>Hello Jason,
>>
>>Thanks a lot for your feedback.
>>
>>On 12/18/2017 06:55 PM, Jason Gunthorpe wrote:
>>> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
>>>> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
>>>>
>>>> [snip]
>>>>
>>>>>
>>>>> James,
>>>>>
>>>>> Can you please test the following (untested) patch on top of the
>>>>> other two mentioned patches to see if it makes a difference for you?
>>>>>
>>>>
>>>> I should had tried to at least compile the patch :)
>>>
>>> I think this is backwards..
>>>
>>> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
>>> TPM shouldn't do anything at all.
>>>
>>> If CLKRUN_EN is off, then it should try to turn it on/off to save
>>> power.
>>>
>>
>>My knowledge of LPC is near to non-existent so I please forgive me if
>>I'm wrong, but my understanding is that it's the opposite of what you said.
>>
>>When CLKRUN_EN is SET, power management is ENABLED on the LPC bus
>and
>>the host LCLK clock may be stopped when entering in some low-power
>states.
>>
>>The CLKRUN# signal is then used by peripherals to restart the LCLK
>>clock after resuming from low-power states to be able to transmit again.
>>
>>When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus
>>and the host LCLK clock is never stopped even when entering in some
>>low- power states.
>>
>
>Hi Javier,
>
>Yes you are correct with the above understanding of the CLKRUN.
>
>>IIUC, if CLKRUN_EN is enabled, then all the devices attached to the LPC
>>bus have to support the CLKRUN protocol. My guess is that on some
>>Braswell systems LPC power management is enabled but the TPM device
>>doesn't have CLKRUN support.
>>
>
>I think this is what might be happening here.
>
>Since I do not have an end product to test this on, I managed to get one BSW
>Reference Verification Platform(RVP). I flashed the latest Ubuntu (17.10)
>which is on 4.13.13 kernel version and does have the patch.
>

The correct kernel version is 4.13.0-19

>I was able to use the PS/2 mouse and keyboard immediately after bootup and
>also after suspend/resume cycle. The system does have a TPM.(/dev/tpm0
>and /dev/tpmrm0 exist)
>
>

The RVP has a Nuvoton TPM

>>And that was the motivation of commit 5e572cab92f0 ("tpm: Enable CLKRUN
>>protocol for Braswell systems") that introduced the regression.
>>
>>> Perhaps the best work around is to just delete the turning off of
>>> CLKRUN_EN ? Uses more power but keeps the clock running which should
>>> keep both TPM and superio happy.
>>>
>>
>>But that's exactly the goal of the commit 5e572cab92f0, to disable the
>>CLKRUN protocol (probably for what I mentioned before). So doing so
>>will cause issues for those systems again.
>>
>>What the patch I shared with James does is to avoid disabling the
>>CLKRUN_EN if this is already disabled. Which should be a no-op anyways
>>but the problem is that latter it's enabled even when the driver found
>disabled to start with.
>>
>>I still don't like the overall solution since it's too error prone.
>>Touching a global bus configuration in a driver for a single peripheral isn't
>safe to say the least.
>>
>>But regardless of that, the patch I shared has merits on its own since
>>is wrong to keep the bus configuration in a different state that was
>>before the driver was probed. And in fact, James reported that it makes
>>his system to work again.
>>
>>> Jason
>>>
>>
>>Best regards,
>>--
>>Javier Martinez Canillas
>>Software Engineer - Desktop Hardware Enablement Red Hat

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Mon, Dec 18, 2017 at 08:29:30PM +0100, Javier Martinez Canillas wrote:

> My knowledge of LPC is near to non-existent so I please forgive me if I'm wrong,
> but my understanding is that it's the opposite of what you said.
> 
> When CLKRUN_EN is SET, power management is ENABLED on the LPC bus and the host
> LCLK clock may be stopped when entering in some low-power states.
>
> The CLKRUN# signal is then used by peripherals to restart the LCLK clock after
> resuming from low-power states to be able to transmit again.
> 
> When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus and the
> host LCLK clock is never stopped even when entering in some low-power states.

Okay, that makes sense.

Jason

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Mon, Dec 18, 2017 at 07:34:29PM +0000, Shaikh, Azhar wrote:

> >IIUC, if CLKRUN_EN is enabled, then all the devices attached to the
> >LPC bus have to support the CLKRUN protocol. My guess is that on
> >some Braswell systems LPC power management is enabled but the TPM
> >device doesn't have CLKRUN support.
> 
> I think this is what might be happening here.

That makes it a BIOS bug, not a chipset bug, and we shouldn't be
trying to fix it like this in Linux.

Based on the original discussion I always thought this was an Intel
chipset bug and applies to all cases.

Jason

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello Azhar,

On 12/18/2017 08:34 PM, Shaikh, Azhar wrote:
> 
> 
>> -----Original Message-----
>> From: Javier Martinez Canillas [mailto:javierm@redhat.com]
>> Sent: Monday, December 18, 2017 11:30 AM
>> To: Jason Gunthorpe <jgg@ziepe.ca>
>> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; James Ettle
>> <james@ettle.org.uk>; linux-integrity@vger.kernel.org; Shaikh, Azhar
>> <azhar.shaikh@intel.com>; linux-kernel@vger.kernel.org;
>> james.l.morris@oracle.com
>> Subject: Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on
>> Braswell system
>>
>> Hello Jason,
>>
>> Thanks a lot for your feedback.
>>
>> On 12/18/2017 06:55 PM, Jason Gunthorpe wrote:
>>> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
>>>> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
>>>>
>>>> [snip]
>>>>
>>>>>
>>>>> James,
>>>>>
>>>>> Can you please test the following (untested) patch on top of the
>>>>> other two mentioned patches to see if it makes a difference for you?
>>>>>
>>>>
>>>> I should had tried to at least compile the patch :)
>>>
>>> I think this is backwards..
>>>
>>> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
>>> TPM shouldn't do anything at all.
>>>
>>> If CLKRUN_EN is off, then it should try to turn it on/off to save
>>> power.
>>>
>>
>> My knowledge of LPC is near to non-existent so I please forgive me if I'm
>> wrong, but my understanding is that it's the opposite of what you said.
>>
>> When CLKRUN_EN is SET, power management is ENABLED on the LPC bus and
>> the host LCLK clock may be stopped when entering in some low-power states.
>>
>> The CLKRUN# signal is then used by peripherals to restart the LCLK clock after
>> resuming from low-power states to be able to transmit again.
>>
>> When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus
>> and the host LCLK clock is never stopped even when entering in some low-
>> power states.
>>
> 
> Hi Javier,
> 
> Yes you are correct with the above understanding of the CLKRUN.
>

Thanks for the confirmation.
 
>> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the LPC bus
>> have to support the CLKRUN protocol. My guess is that on some Braswell
>> systems LPC power management is enabled but the TPM device doesn't have
>> CLKRUN support.
>>
> 
> I think this is what might be happening here.
>

One question, what happens if the CLKRUN protocol is disabled and never enabled
again? My understanding is that the CLKRUN# signal is not required if the host
never stops the LCLK clock, and peripherals that do support the CLKRUN protocol
should still work if the CLKRUN protocol is disabled and the LCLK isn't stopped.

So from what we discussed above, I think it may be correct to completely disable
CLKRUN protocol if there's a peripheral that doesn't support it.
 
> Since I do not have an end product to test this on, I managed to get one BSW Reference Verification Platform(RVP). I flashed the latest Ubuntu (17.10) which is on 4.13.13 kernel version and does have the patch.
> 
> I was able to use the PS/2 mouse and keyboard immediately after bootup and also after suspend/resume cycle. The system does have a TPM.(/dev/tpm0 and /dev/tpmrm0 exist)
>

It's not clear to me if the RVP system is the same one where you found the issue
and lead to the commit that caused the regression.

Could you please test what happens if you disable the CLKRUN_EN and never enable
it again?

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello Jason,

On 12/18/2017 09:19 PM, Jason Gunthorpe wrote:
> On Mon, Dec 18, 2017 at 07:34:29PM +0000, Shaikh, Azhar wrote:
> 
>>> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the
>>> LPC bus have to support the CLKRUN protocol. My guess is that on
>>> some Braswell systems LPC power management is enabled but the TPM
>>> device doesn't have CLKRUN support.
>>
>> I think this is what might be happening here.
> 
> That makes it a BIOS bug, not a chipset bug, and we shouldn't be
> trying to fix it like this in Linux.
>

Indeed, the system integrator should make sure that all peripherals that
are connected through the LPC bus either support the CLKRUN protocol and
CLKRUN_EN is enabled or CLKRUN_EN should be disabled.
 
> Based on the original discussion I always thought this was an Intel
> chipset bug and applies to all cases.
>

After thinking about this and with a better understanding of the issue,
I think we have 2 options (please let me know if I got something wrong):

1) Leave the code as is and apply the patch I shared with James. In that
   case the CLKRUN protocol will be disabled only during TPM transactions
   and not enabled again after transactions if it wasn't enabled.

   This shouldn't affect other peripherals since even if they have CLKRUN
   support, they should work correctly while CLKRUN protocol is disabled.

   The disadvantage is that TPM devices that have CLKRUN support (do they
   exist?) will not take the advantage of the power management feature of
   stopping the LPC host LCLK clock during low-power states.

2) Drop the pending CLKRUN patch in linux-tpmdd and revert CLKRUN commit
   in mainline. And instead of disabling the CLKRUN protocol during the
   TPM transactions, disable if the CLKRUN_EN is enabled and the system
   is in a list of systems that have a TPM that doesn't support CLKRUN.

   This list could be for example a struct dmi_system_id array and match
   using DMI data on module_init().

   The advantage is that TPM devices with CLKRUN protocol support could
   make use of the CLKRUN power management feature and only systems with
   a TPM that doesn't support the CLKRUN protocol will disable it.

   The disadvantage is that the struct dmi_system_id array to match will
   have to be maintained and every known-to-be-broken system added to it.

Thoughts?

> Jason
> 

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Tue, Dec 19, 2017 at 12:34:46AM +0100, Javier Martinez Canillas wrote:
> Hello Jason,
> 
> On 12/18/2017 09:19 PM, Jason Gunthorpe wrote:
> > On Mon, Dec 18, 2017 at 07:34:29PM +0000, Shaikh, Azhar wrote:
> > 
> >>> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the
> >>> LPC bus have to support the CLKRUN protocol. My guess is that on
> >>> some Braswell systems LPC power management is enabled but the TPM
> >>> device doesn't have CLKRUN support.
> >>
> >> I think this is what might be happening here.
> > 
> > That makes it a BIOS bug, not a chipset bug, and we shouldn't be
> > trying to fix it like this in Linux.
> >
> 
> Indeed, the system integrator should make sure that all peripherals that
> are connected through the LPC bus either support the CLKRUN protocol and
> CLKRUN_EN is enabled or CLKRUN_EN should be disabled.

I would like to hear from Intel why they made this patch in the first
place, since we are mostly guessing...

> 1) Leave the code as is and apply the patch I shared with James. In that
>    case the CLKRUN protocol will be disabled only during TPM transactions
>    and not enabled again after transactions if it wasn't enabled.

This seems necessary no matter what, especially if it works..

> 2) Drop the pending CLKRUN patch in linux-tpmdd and revert CLKRUN commit
>    in mainline. And instead of disabling the CLKRUN protocol during the
>    TPM transactions, disable if the CLKRUN_EN is enabled and the system
>    is in a list of systems that have a TPM that doesn't support CLKRUN.

This is also a good possible idea.

Again, depends why Intel did this. I assumed it was a chipset bug
since Intel was sending it not a system builder..

Jason

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

James, Javier, thank you for sorting this out. I'll just have couple of
minor comments on the patch.

On Mon, Dec 18, 2017 at 01:22:28PM +0100, Javier Martinez Canillas wrote:
> +	u32 vendor, intfcaps, intmask, clkrun_val;

Could these split into four lines (one declaration per line)?

>  	u8 rid;
>  	int rc, probe;
>  	struct tpm_chip *chip;
> @@ -772,6 +772,15 @@ 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)) {
> +			priv->flags |= TPM_TIS_CLK_ENABLE;

Is the flag added just so surpress those WARN()'s?

I've forgot why the WARN()'s even exist assuming that driver is
functioning correctly.

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

On Mon, Dec 18, 2017 at 06:26:35PM +0000, James Ettle wrote:
> The keyboard and touchpad work OK with the patch quoted below and the earlier two applied, i.e. the three patches with signatures:
> 
> 667dcc75be864ff4c17cf58891853b7393bba3e2
> db3248e8a036c39141c8f7e9f1cf5c5ae6815f76
> 370d45a34dc8914066a995a3a6d6df1953ea9f60
> 
> I applied these to a vanilla kernel.org 4.14.7 source using Fedora 4.14.5-300.fc27 .config. Confirmed the tpm modules are loaded.
> 
> Tests:
> 1. Keyboard and touchpad work OK on boot - PASS
> 2. Still work after suspend/resume - PASS
> 
> Let me know if you want any further tests.
> 
> Many thanks,
> James.

Thank you for your awesome help! I guess Javier can add your
Tested-by to the patch?

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

On Mon, Dec 18, 2017 at 10:55:02AM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
> > On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
> > 
> > [snip]
> > 
> > > 
> > > James,
> > > 
> > > Can you please test the following (untested) patch on top of the other two
> > > mentioned patches to see if it makes a difference for you?
> > > 
> > 
> > I should had tried to at least compile the patch :)
> 
> I think this is backwards..
> 
> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
> TPM shouldn't do anything at all.
> 
> If CLKRUN_EN is off, then it should try to turn it on/off to save
> power.
> 
> Perhaps the best work around is to just delete the turning off of
> CLKRUN_EN ? Uses more power but keeps the clock running which should
> keep both TPM and superio happy.
> 
> Jason

On some BSW systems keeping CLKRUN_EN on during TPM command caused
issues. That was the original reason for the fixes.

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

On Mon, Dec 18, 2017 at 01:19:02PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 18, 2017 at 07:34:29PM +0000, Shaikh, Azhar wrote:
> 
> > >IIUC, if CLKRUN_EN is enabled, then all the devices attached to the
> > >LPC bus have to support the CLKRUN protocol. My guess is that on
> > >some Braswell systems LPC power management is enabled but the TPM
> > >device doesn't have CLKRUN support.
> > 
> > I think this is what might be happening here.
> 
> That makes it a BIOS bug, not a chipset bug, and we shouldn't be
> trying to fix it like this in Linux.
> 
> Based on the original discussion I always thought this was an Intel
> chipset bug and applies to all cases.
> 
> Jason

It would not be first time when some BIOS issues are circumvented in the
kernel.

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

Hello Jarkko,

On 12/19/2017 01:59 PM, Jarkko Sakkinen wrote:
> James, Javier, thank you for sorting this out. I'll just have couple of
> minor comments on the patch.
> 
> On Mon, Dec 18, 2017 at 01:22:28PM +0100, Javier Martinez Canillas wrote:
>> +	u32 vendor, intfcaps, intmask, clkrun_val;
> 
> Could these split into four lines (one declaration per line)?
>

Yes, I didn't like that either but did this way for consistency with the driver.

>>  	u8 rid;
>>  	int rc, probe;
>>  	struct tpm_chip *chip;
>> @@ -772,6 +772,15 @@ 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)) {
>> +			priv->flags |= TPM_TIS_CLK_ENABLE;
> 
> Is the flag added just so surpress those WARN()'s?
>

I believe so. I just included here again for consistency, but I think the flag
and the warns can just go away.

> I've forgot why the WARN()'s even exist assuming that driver is
> functioning correctly.
> 
> /Jarkko
>

If you agree with the patch, I can post it as a formal patch on a series that
do these cleanups as preparatory work. I've also noticed a NULL pointer deref
bug in an error path so I'll also fix that too.

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

On Tue, Dec 19, 2017 at 12:34:46AM +0100, Javier Martinez Canillas wrote:
> Hello Jason,
> 
> On 12/18/2017 09:19 PM, Jason Gunthorpe wrote:
> > On Mon, Dec 18, 2017 at 07:34:29PM +0000, Shaikh, Azhar wrote:
> > 
> >>> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the
> >>> LPC bus have to support the CLKRUN protocol. My guess is that on
> >>> some Braswell systems LPC power management is enabled but the TPM
> >>> device doesn't have CLKRUN support.
> >>
> >> I think this is what might be happening here.
> > 
> > That makes it a BIOS bug, not a chipset bug, and we shouldn't be
> > trying to fix it like this in Linux.
> >
> 
> Indeed, the system integrator should make sure that all peripherals that
> are connected through the LPC bus either support the CLKRUN protocol and
> CLKRUN_EN is enabled or CLKRUN_EN should be disabled.
>  
> > Based on the original discussion I always thought this was an Intel
> > chipset bug and applies to all cases.
> >
> 
> After thinking about this and with a better understanding of the issue,
> I think we have 2 options (please let me know if I got something wrong):
> 
> 1) Leave the code as is and apply the patch I shared with James. In that
>    case the CLKRUN protocol will be disabled only during TPM transactions
>    and not enabled again after transactions if it wasn't enabled.
> 
>    This shouldn't affect other peripherals since even if they have CLKRUN
>    support, they should work correctly while CLKRUN protocol is disabled.
> 
>    The disadvantage is that TPM devices that have CLKRUN support (do they
>    exist?) will not take the advantage of the power management feature of
>    stopping the LPC host LCLK clock during low-power states.

CLKRUN is enabled after TPM has processed the command so how this could
make power mgmt worse?

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Javier Martinez Canillas]

On 12/19/2017 02:10 PM, Jarkko Sakkinen wrote:
> On Tue, Dec 19, 2017 at 12:34:46AM +0100, Javier Martinez Canillas wrote:
>> Hello Jason,
>>
>> On 12/18/2017 09:19 PM, Jason Gunthorpe wrote:
>>> On Mon, Dec 18, 2017 at 07:34:29PM +0000, Shaikh, Azhar wrote:
>>>
>>>>> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the
>>>>> LPC bus have to support the CLKRUN protocol. My guess is that on
>>>>> some Braswell systems LPC power management is enabled but the TPM
>>>>> device doesn't have CLKRUN support.
>>>>
>>>> I think this is what might be happening here.
>>>
>>> That makes it a BIOS bug, not a chipset bug, and we shouldn't be
>>> trying to fix it like this in Linux.
>>>
>>
>> Indeed, the system integrator should make sure that all peripherals that
>> are connected through the LPC bus either support the CLKRUN protocol and
>> CLKRUN_EN is enabled or CLKRUN_EN should be disabled.
>>  
>>> Based on the original discussion I always thought this was an Intel
>>> chipset bug and applies to all cases.
>>>
>>
>> After thinking about this and with a better understanding of the issue,
>> I think we have 2 options (please let me know if I got something wrong):
>>
>> 1) Leave the code as is and apply the patch I shared with James. In that
>>    case the CLKRUN protocol will be disabled only during TPM transactions
>>    and not enabled again after transactions if it wasn't enabled.
>>
>>    This shouldn't affect other peripherals since even if they have CLKRUN
>>    support, they should work correctly while CLKRUN protocol is disabled.
>>
>>    The disadvantage is that TPM devices that have CLKRUN support (do they
>>    exist?) will not take the advantage of the power management feature of
>>    stopping the LPC host LCLK clock during low-power states.
> 
> CLKRUN is enabled after TPM has processed the command so how this could
> make power mgmt worse?
>

I meant if there are Braswell systems with a TPM that _does_ support CLKRUN
protocol. Since the current code disables ClKRUN_EN for all Braswell boards.
 
> /Jarkko
> 

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

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[James Ettle]

I'm OK with that.

Do you need any more info on the specifics of the machine I'm using?

(Note: I'm not currently aware of any BIOS updates for it. If this is ultimately a BIOS problem I wouldn't know what sort of report to file, or how to file it anyway...)

Thanks,
James

On 19/12/17 13:00, Jarkko Sakkinen wrote:
> On Mon, Dec 18, 2017 at 06:26:35PM +0000, James Ettle wrote:
>> The keyboard and touchpad work OK with the patch quoted below and the earlier two applied, i.e. the three patches with signatures:
>>
>> 667dcc75be864ff4c17cf58891853b7393bba3e2
>> db3248e8a036c39141c8f7e9f1cf5c5ae6815f76
>> 370d45a34dc8914066a995a3a6d6df1953ea9f60
>>
>> I applied these to a vanilla kernel.org 4.14.7 source using Fedora 4.14.5-300.fc27 .config. Confirmed the tpm modules are loaded.
>>
>> Tests:
>> 1. Keyboard and touchpad work OK on boot - PASS
>> 2. Still work after suspend/resume - PASS
>>
>> Let me know if you want any further tests.
>>
>> Many thanks,
>> James.
> 
> Thank you for your awesome help! I guess Javier can add your
> Tested-by to the patch?
> 
> /Jarkko
> 

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jason Gunthorpe]

On Tue, Dec 19, 2017 at 03:04:32PM +0200, Jarkko Sakkinen wrote:

> > That makes it a BIOS bug, not a chipset bug, and we shouldn't be
> > trying to fix it like this in Linux.
> > 
> > Based on the original discussion I always thought this was an Intel
> > chipset bug and applies to all cases.
> 
> It would not be first time when some BIOS issues are circumvented in the
> kernel.

Yes, but we don't circumvent BIOS bugs by detecting the chipset type,
we look for the buggy BIOS.

Jason

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5963 bytes --]

Hi Javier,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20171214]
[cannot apply to char-misc/char-misc-testing v4.15-rc3 v4.15-rc2 v4.15-rc1 v4.15-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Javier-Martinez-Canillas/tpm-only-attempt-to-disable-the-LPC-CLKRUN-if-is-already/20171220-041605
config: x86_64-randconfig-x007-201751 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/char/tpm/tpm_tis_core.c: In function 'tpm_tis_core_init':
>> drivers/char/tpm/tpm_tis_core.c:836:26: error: assignment of member 'clk_enable' in read-only object
       chip->ops->clk_enable = NULL;
                             ^

vim +/clk_enable +836 drivers/char/tpm/tpm_tis_core.c

   815	
   816		/* Maximum timeouts */
   817		chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
   818		chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
   819		chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX);
   820		chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX);
   821		priv->phy_ops = phy_ops;
   822		dev_set_drvdata(&chip->dev, priv);
   823	
   824		if (is_bsw()) {
   825			priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
   826						ILB_REMAP_SIZE);
   827			if (!priv->ilb_base_addr)
   828				return -ENOMEM;
   829	
   830			clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
   831			/* Check if CLKRUN# is already not enabled in the LPC bus */
   832			if (!(clkrun_val & LPC_CLKRUN_EN)) {
   833				priv->flags |= TPM_TIS_CLK_ENABLE;
   834				iounmap(priv->ilb_base_addr);
   835				priv->ilb_base_addr = NULL;
 > 836				chip->ops->clk_enable = NULL;
   837			}
   838		}
   839	
   840		if (chip->ops->clk_enable != NULL)
   841			chip->ops->clk_enable(chip, true);
   842	
   843		if (wait_startup(chip, 0) != 0) {
   844			rc = -ENODEV;
   845			goto out_err;
   846		}
   847	
   848		/* Take control of the TPM's interrupt hardware and shut it off */
   849		rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
   850		if (rc < 0)
   851			goto out_err;
   852	
   853		intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
   854			   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
   855		intmask &= ~TPM_GLOBAL_INT_ENABLE;
   856		tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
   857	
   858		rc = tpm2_probe(chip);
   859		if (rc)
   860			goto out_err;
   861	
   862		rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor);
   863		if (rc < 0)
   864			goto out_err;
   865	
   866		priv->manufacturer_id = vendor;
   867	
   868		rc = tpm_tis_read8(priv, TPM_RID(0), &rid);
   869		if (rc < 0)
   870			goto out_err;
   871	
   872		dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
   873			 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
   874			 vendor >> 16, rid);
   875	
   876		probe = probe_itpm(chip);
   877		if (probe < 0) {
   878			rc = -ENODEV;
   879			goto out_err;
   880		}
   881	
   882		/* Figure out the capabilities */
   883		rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
   884		if (rc < 0)
   885			goto out_err;
   886	
   887		dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
   888			intfcaps);
   889		if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
   890			dev_dbg(dev, "\tBurst Count Static\n");
   891		if (intfcaps & TPM_INTF_CMD_READY_INT)
   892			dev_dbg(dev, "\tCommand Ready Int Support\n");
   893		if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
   894			dev_dbg(dev, "\tInterrupt Edge Falling\n");
   895		if (intfcaps & TPM_INTF_INT_EDGE_RISING)
   896			dev_dbg(dev, "\tInterrupt Edge Rising\n");
   897		if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
   898			dev_dbg(dev, "\tInterrupt Level Low\n");
   899		if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
   900			dev_dbg(dev, "\tInterrupt Level High\n");
   901		if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
   902			dev_dbg(dev, "\tLocality Change Int Support\n");
   903		if (intfcaps & TPM_INTF_STS_VALID_INT)
   904			dev_dbg(dev, "\tSts Valid Int Support\n");
   905		if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
   906			dev_dbg(dev, "\tData Avail Int Support\n");
   907	
   908		/* INTERRUPT Setup */
   909		init_waitqueue_head(&priv->read_queue);
   910		init_waitqueue_head(&priv->int_queue);
   911		if (irq != -1) {
   912			/* Before doing irq testing issue a command to the TPM in polling mode
   913			 * to make sure it works. May as well use that command to set the
   914			 * proper timeouts for the driver.
   915			 */
   916			if (tpm_get_timeouts(chip)) {
   917				dev_err(dev, "Could not get TPM timeouts and durations\n");
   918				rc = -ENODEV;
   919				goto out_err;
   920			}
   921	
   922			if (irq) {
   923				tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
   924							 irq);
   925				if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
   926					dev_err(&chip->dev, FW_BUG
   927						"TPM interrupt not working, polling instead\n");
   928			} else {
   929				tpm_tis_probe_irq(chip, intmask);
   930			}
   931		}
   932	
   933		rc = tpm_chip_register(chip);
   934		if (rc && is_bsw() && priv->ilb_base_addr)
   935			iounmap(priv->ilb_base_addr);
   936	
   937		if (chip->ops->clk_enable != NULL)
   938			chip->ops->clk_enable(chip, false);
   939	
   940		return rc;
   941	out_err:
   942		tpm_tis_remove(chip);
   943		if (is_bsw())
   944			iounmap(priv->ilb_base_addr);
   945	
   946		if (chip->ops->clk_enable != NULL)
   947			chip->ops->clk_enable(chip, false);
   948	
   949		return rc;
   950	}
   951	EXPORT_SYMBOL_GPL(tpm_tis_core_init);
   952	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29561 bytes --]

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

[kbuild test robot]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: multipart/mixed; boundary="LQksG6bCIzRHxTLp", Size: 6040 bytes --]

Hi Javier,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20171214]
[cannot apply to char-misc/char-misc-testing v4.15-rc3 v4.15-rc2 v4.15-rc1 v4.15-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Javier-Martinez-Canillas/tpm-only-attempt-to-disable-the-LPC-CLKRUN-if-is-already/20171220-041605
config: x86_64-randconfig-x007-201751 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/char/tpm/tpm_tis_core.c: In function 'tpm_tis_core_init':
>> drivers/char/tpm/tpm_tis_core.c:836:26: error: assignment of member 'clk_enable' in read-only object
       chip->ops->clk_enable = NULL;
                             ^

vim +/clk_enable +836 drivers/char/tpm/tpm_tis_core.c

   815	
   816		/* Maximum timeouts */
   817		chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
   818		chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
   819		chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX);
   820		chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX);
   821		priv->phy_ops = phy_ops;
   822		dev_set_drvdata(&chip->dev, priv);
   823	
   824		if (is_bsw()) {
   825			priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
   826						ILB_REMAP_SIZE);
   827			if (!priv->ilb_base_addr)
   828				return -ENOMEM;
   829	
   830			clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
   831			/* Check if CLKRUN# is already not enabled in the LPC bus */
   832			if (!(clkrun_val & LPC_CLKRUN_EN)) {
   833				priv->flags |= TPM_TIS_CLK_ENABLE;
   834				iounmap(priv->ilb_base_addr);
   835				priv->ilb_base_addr = NULL;
 > 836				chip->ops->clk_enable = NULL;
   837			}
   838		}
   839	
   840		if (chip->ops->clk_enable != NULL)
   841			chip->ops->clk_enable(chip, true);
   842	
   843		if (wait_startup(chip, 0) != 0) {
   844			rc = -ENODEV;
   845			goto out_err;
   846		}
   847	
   848		/* Take control of the TPM's interrupt hardware and shut it off */
   849		rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
   850		if (rc < 0)
   851			goto out_err;
   852	
   853		intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
   854			   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
   855		intmask &= ~TPM_GLOBAL_INT_ENABLE;
   856		tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
   857	
   858		rc = tpm2_probe(chip);
   859		if (rc)
   860			goto out_err;
   861	
   862		rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor);
   863		if (rc < 0)
   864			goto out_err;
   865	
   866		priv->manufacturer_id = vendor;
   867	
   868		rc = tpm_tis_read8(priv, TPM_RID(0), &rid);
   869		if (rc < 0)
   870			goto out_err;
   871	
   872		dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
   873			 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
   874			 vendor >> 16, rid);
   875	
   876		probe = probe_itpm(chip);
   877		if (probe < 0) {
   878			rc = -ENODEV;
   879			goto out_err;
   880		}
   881	
   882		/* Figure out the capabilities */
   883		rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
   884		if (rc < 0)
   885			goto out_err;
   886	
   887		dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
   888			intfcaps);
   889		if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
   890			dev_dbg(dev, "\tBurst Count Static\n");
   891		if (intfcaps & TPM_INTF_CMD_READY_INT)
   892			dev_dbg(dev, "\tCommand Ready Int Support\n");
   893		if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
   894			dev_dbg(dev, "\tInterrupt Edge Falling\n");
   895		if (intfcaps & TPM_INTF_INT_EDGE_RISING)
   896			dev_dbg(dev, "\tInterrupt Edge Rising\n");
   897		if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
   898			dev_dbg(dev, "\tInterrupt Level Low\n");
   899		if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
   900			dev_dbg(dev, "\tInterrupt Level High\n");
   901		if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
   902			dev_dbg(dev, "\tLocality Change Int Support\n");
   903		if (intfcaps & TPM_INTF_STS_VALID_INT)
   904			dev_dbg(dev, "\tSts Valid Int Support\n");
   905		if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
   906			dev_dbg(dev, "\tData Avail Int Support\n");
   907	
   908		/* INTERRUPT Setup */
   909		init_waitqueue_head(&priv->read_queue);
   910		init_waitqueue_head(&priv->int_queue);
   911		if (irq != -1) {
   912			/* Before doing irq testing issue a command to the TPM in polling mode
   913			 * to make sure it works. May as well use that command to set the
   914			 * proper timeouts for the driver.
   915			 */
   916			if (tpm_get_timeouts(chip)) {
   917				dev_err(dev, "Could not get TPM timeouts and durations\n");
   918				rc = -ENODEV;
   919				goto out_err;
   920			}
   921	
   922			if (irq) {
   923				tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
   924							 irq);
   925				if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
   926					dev_err(&chip->dev, FW_BUG
   927						"TPM interrupt not working, polling instead\n");
   928			} else {
   929				tpm_tis_probe_irq(chip, intmask);
   930			}
   931		}
   932	
   933		rc = tpm_chip_register(chip);
   934		if (rc && is_bsw() && priv->ilb_base_addr)
   935			iounmap(priv->ilb_base_addr);
   936	
   937		if (chip->ops->clk_enable != NULL)
   938			chip->ops->clk_enable(chip, false);
   939	
   940		return rc;
   941	out_err:
   942		tpm_tis_remove(chip);
   943		if (is_bsw())
   944			iounmap(priv->ilb_base_addr);
   945	
   946		if (chip->ops->clk_enable != NULL)
   947			chip->ops->clk_enable(chip, false);
   948	
   949		return rc;
   950	}
   951	EXPORT_SYMBOL_GPL(tpm_tis_core_init);
   952	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation


    [ Part 2, Application/GZIP 30 KB. ]
    [ Unable to print this part. ]

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Jarkko Sakkinen]

> I'm OK with that.
> 
> Do you need any more info on the specifics of the machine I'm using?
> 
> (Note: I'm not currently aware of any BIOS updates for it. If this is ultimately a BIOS problem I wouldn't know what sort of report to file, or how to file it anyway...)
> 
> Thanks,
> James

I just found out that the original bug was specific to BSW with
Infineon SLB9655 chip.

Unless you have this specific chip you can just give Tested-by
(and Reviewed-by).

/Jarkko

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]



> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index e7bd2e750f69..0858c08b3b89 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -688,7 +688,7 @@ 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(TCG_TIS_ENABLE_CLKRUN_QUIRK) || !is_bsw())

Missing CONFIG_?
								pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Pavel Machek]

[-- Attachment #1: Type: multipart/signed, Size: 801 bytes --]