>-----Original Message----- >From: linux-integrity-owner@xxxxxxxxxxxxxxx [mailto:linux-integrity- >owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander.Steffen@xxxxxxxxxxxx >Sent: Wednesday, December 20, 2017 6:07 AM >To: javierm@xxxxxxxxxx; hdegoede@xxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx >Cc: james@xxxxxxxxxxxx; Shaikh, Azhar <azhar.shaikh@xxxxxxxxx>; >arnd@xxxxxxxx; jarkko.sakkinen@xxxxxxxxxxxxxxx; peterhuewe@xxxxxx; >jgg@xxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-integrity@xxxxxxxxxxxxxxx >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
