[PATCH] tpm_tis: Verify ACPI-specified interrupt

[Scot Doyle]

Some machines, such as the Acer C720 and Toshiba CB35, have TPMs
that do not use interrupts while also having an ACPI TPM entry
indicating a specific interrupt to be used. Since this interrupt
is invalid, these machines freeze on resume until the interrupt
times out.

Generate the ACPI-specified interrupt. If none is received, then
fall back to polling mode.

Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
Tested-by: James Duley <jagd...@gmail.com>
Tested-by: Michael Mullin <masm...@gmail.com>
---
drivers/char/tpm/tpm_tis.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..736ed4a 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -633,12 +633,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
iowrite32(intmask,
chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.locality));
- if (interrupts)
- chip->vendor.irq = irq;
- if (interrupts && !chip->vendor.irq) {
- irq_s =
- ioread8(chip->vendor.iobase +
- TPM_INT_VECTOR(chip->vendor.locality));
+ chip->vendor.irq = 0;
+ if (interrupts) {
+ if (irq)
+ irq_s = irq;
+ else
+ irq_s =
+ ioread8(chip->vendor.iobase +
+ TPM_INT_VECTOR(chip->vendor.locality));
if (irq_s) {
irq_e = irq_s;
} else {
--
2.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Jason Gunthorpe]

On Fri, Aug 22, 2014 at 12:58:41AM +0000, Scot Doyle wrote:
> Some machines, such as the Acer C720 and Toshiba CB35, have TPMs
> that do not use interrupts while also having an ACPI TPM entry

How do these machines work in Windows?

Why only resume? Shouldn't every TPM command (such as the 3 or 4 the
driver issues at startup) timeout too?

> indicating a specific interrupt to be used. Since this interrupt
> is invalid, these machines freeze on resume until the interrupt
> times out.

> Generate the ACPI-specified interrupt. If none is received, then
> fall back to polling mode.

So, this makes the IRQ detection code run unconditionally, but that
code was only ever really used in certain old non-probable case..

I wonder if it works reliably?

In any event, I think a FIRMWARE_BUG message should be printed if this
case is detected.

I'd be more comfortable with some kind of ACPI black list or patch or
something? What is normal for handling broken ACPI?

Jason

[Scot Doyle]

On Fri, 22 Aug 2014, Jason Gunthorpe wrote:
> On Fri, Aug 22, 2014 at 12:58:41AM +0000, Scot Doyle wrote:
>> Some machines, such as the Acer C720 and Toshiba CB35, have TPMs
>> that do not use interrupts while also having an ACPI TPM entry
>
> How do these machines work in Windows?

I don't know. Since they're Chromebooks (booted in legacy mode running
SeaBIOS instead of depthcharge or whatever ChromeOS uses), I think they're
mostly used to run Linux.

> Why only resume? Shouldn't every TPM command (such as the 3 or 4 the
> driver issues at startup) timeout too?

I noticed that startup(save_state) on suspend did take longer, but only
four or five seconds instead of the 160 seconds during selftest on resume.

>> indicating a specific interrupt to be used. Since this interrupt
>> is invalid, these machines freeze on resume until the interrupt
>> times out.
>
>> Generate the ACPI-specified interrupt. If none is received, then
>> fall back to polling mode.
>
> So, this makes the IRQ detection code run unconditionally, but that
> code was only ever really used in certain old non-probable case..
>
> I wonder if it works reliably?

That is good to know. I share your concerns about reliability, not having
the ability to test on other machines.

> In any event, I think a FIRMWARE_BUG message should be printed if this
> case is detected.

I agree.

> I'd be more comfortable with some kind of ACPI black list or patch or
> something? What is normal for handling broken ACPI?

I would be more comfortable with this general approach as well. However,
I've had to submit several patches for individual Chromebooks related to
backlight control since the VBT also is misconfigured. Would it be
possible to find a blacklist mechanism that didn't require identifying
each Chromebook separately, since they seem to have this issue on an
ongoing basis?

dmidecode outputs:

Handle 0x0000, DMI type 0, 24 bytes
BIOS Information
Vendor: coreboot
Version:
Release Date: 12/04/2013
ROM Size: 8192 kB
Characteristics:
PCI is supported
PC Card (PCMCIA) is supported
BIOS is upgradeable
Selectable boot is supported
ACPI is supported
Targeted content distribution is supported
BIOS Revision: 4.0
Firmware Revision: 0.0

Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Toshiba
Product Name: Leon
Version: 1.0
Serial Number: 123456789
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Not Specified
Family: Not Specified

All Chromebooks that I've seen list the BIOS vendor as 'coreboot'.

We also have access to TPM chip vendor and revision. All chromebooks that
I've seen so far have the same vendor (11) and revision (16).

So we have five pieces of identifying information...
1. TPM chip vendor
2. TPM chip revision
3. BIOS vendor
4. System manufacturer
5. System product name

and at least two possible actions...
1. ignore the acpi interrupt
2. verify the acpi interrupt

The safest approach would be to ignore the ACPI information for systems
matching all five identifiers.

A more general approach might be to verify the ACPI interrupt for
systems matching the first three identifiers.

Thoughts?

[Jason Gunthorpe]

On Fri, Aug 22, 2014 at 08:17:27PM +0000, Scot Doyle wrote:
> On Fri, 22 Aug 2014, Jason Gunthorpe wrote:
> > On Fri, Aug 22, 2014 at 12:58:41AM +0000, Scot Doyle wrote:
> >> Some machines, such as the Acer C720 and Toshiba CB35, have TPMs
> >> that do not use interrupts while also having an ACPI TPM entry
> >
> > How do these machines work in Windows?
>
> I don't know. Since they're Chromebooks (booted in legacy mode running
> SeaBIOS instead of depthcharge or whatever ChromeOS uses), I think they're
> mostly used to run Linux.

I remain somewhat confused - there have already been TPM patches for
Chromebooks from Google - presumably the TPM actually does work
fine. Make sure you are using a Linux with the ATMEL timeout fix, that
is particularly applicable to Chromebooks IIRC.

And again, the driver uses interrupts when booting, so I'm somewhat
confused what the problem is. I wouldn't think the driver would
successfully attach if interrupts were enabled but the interrupt
didn't work? Can you elaborate on what is going on during boot with
the interrupt, and the boot time GET_DURATIONS and TPM_STARTUP
sequences?

Perhaps the driver is timing out all commands and going ahead and
attaching anyhow? If this is the case I think we'd get a good result
if we just fixed that and had the driver simply not attach. Then your
resume will not be broken.

> > I'd be more comfortable with some kind of ACPI black list or patch or
> > something? What is normal for handling broken ACPI?
>
> I would be more comfortable with this general approach as well. However,
> I've had to submit several patches for individual Chromebooks related to
> backlight control since the VBT also is misconfigured. Would it be
> possible to find a blacklist mechanism that didn't require identifying
> each Chromebook separately, since they seem to have this issue on an
> ongoing basis?

So, if you are booting the Chromebook in some weird way, is this a
problem that can be addressed by patching SeaBIOS instead of the
kernel? The internet says the SeaBIOS payload is replaceable on the
Chromebook.

Can it fix the ACPI tables to be correct before lauching Linux?

> A more general approach might be to verify the ACPI interrupt for
> systems matching the first three identifiers.

Testing the interrupt and failing driver attach if it doesn't work
seems very reasonable to me, I would view that as a bug fix in the driver.

[Peter Hüwe]

CC: Luigi, he works at Google and is responsible for the TPMs in
Chromebooks ;)

Thanks,
Peter

[Scot Doyle]

Hopefully this explanation will provide any missing bits of information.
Please correct me where needed.


The Chromebooks use coreboot + payload as firmware. The three payloads of
interest are:

1. Depthcharge. Used when a ChromeOS user boots the machine. Since the
kernel must be signed, presumably the TPM chip is used extensively.
Signing prevents usage with other distributions.

2. The stock version of SeaBIOS. Used in "developer" mode, and provides
typical BIOS functionality. Kernel signature is not checked, so those
parts of the TPM are not needed. However, the machine will reboot on
resume unless the tpm_tis module is loaded and working, as documented
in commit f7595464.

3. A custom version of SeaBIOS. While it could be bespoke, usually it is
built by someone else for all users of a given Chromebook model (see
https://johnlewis.ie/custom-chromebook-firmware/rom-download/ ). It
functions like stock SeaBIOS, except that the TPM usually isn't started
by this payload. However, the ACPI DSDT still has a TPM entry listing
the IFX0102 chip, which causes the tpm_tis module to load. The
tpm_tis_init function calls tpm_get_timeouts, which in turn sees that
the chip has not been started and therefore issues a startup(clear)
command. Now the chip is running. It will not be reboot on resume
(unlike stock SeaBIOS), presumably since the firmware is not
interacting with the TPM. But because an error 38 is returned during
the selftest issued on resume, its functioning will degrade over
suspend/resume cycles until the machine refuses to enter suspend,


Manual workarounds for these three payloads:

1. Depthcharge. I don't know how ChromeOS interacts with the TPM chip.
From what I've seen, it may boot with kernel parameters tpm_tis.force=1
and tpm_tis.interrupts=0 (see #2 following). The kernel is older.

2. Stock SeaBIOS. Boot the kernel with tpm_tis.force=1 or
tpm_tis.interrupts=0. 'force=1' causes the module to ignore the ACPI
TPM interrupt entry and instead probe for interrupts. Since no
interrupts are found to be valid, the driver falls back to polling
mode. And 'interrupts=0' causes the interrupt probing to be skipped,
defaulting to polling mode. So, even though most users set BOTH of
these parameters, they have the same effect on these machines and only
one or the other are necessary.

3. Custom SeaBIOS. Blacklist the tpm_tis module so that it doesn't load
and therefore doesn't issue startup(clear) to the TPM chip.


While there are manual workarounds, a number of people would like the
tpm_tis module to automatically function correctly for stock and custom
SeaBIOS, as we use these machines with those payloads.

Because there are so many users, and flashing the payload is considered
risky, and those who do flash are not usually the ones who build their
firmware, fixing the ACPI table in firmware (which requires flashing)
would not be a general solution. Hopefully Google (or their manufacturing
partners) will correct this in future Chromebooks.


Details about tpm_tis module init using stock SeaBIOS:

The firmware sends a startup(clear). Then the tpm_tis module is loaded
because there is a matching ACPI entry for "IFX0102". By default, module
parameters are interrupts=1 and force=0. I will talk about these default
values, since that is the case that I'd like to fix. force=0 causes the
module to look for an ACPI entry. It finds an entry indicating IRQ 6 and
continues with the tpm_tis_init function.

tpm_tis_init starts in polling mode. It registers the hardware device,
performs some initial setup, queries the chip manufacturer and vendor id,
deterines the chip capabilities (I think they are all enabled), gets
timeouts, does a selftest (which passes), sets up the interrupt and read
queues, and enables chip interrupts. It does all of this in polling mode.
Then, because interrupts=1 and the interrupt was determined from the ACPI
table entry, it skips the code section which probes for available
interrupts. Next, it switches to interrupt mode. It tells the TPM to use
IRQ 6, requests IRQ 6 for use with the driver, does some clean up, and
exits, returning 0.

I've verified that no blocking commands are send to the TPM during the
tpm_tis_init function. Since none are sent, no interrupts fail, and the
attach procedes as normal.

Side note 1: my understanding is that the module exchanges data with the
TPM in the same way whether in polling mode or interrupt mode. These two
modes only indicate how the module determines that it may send additional
commands and/or receive expected data from the TPM. In other words, the
interrupt or polling tell the module when it can continue.

Side note 2: There seems to be a bug that would have prevented detection
of interrupt timeout anyway. I'll follow up with a patch.


Details about tpm_tis module init using custom SeaBIOS:

The same as stock SeaBIOS, except that no startup(clear) has been sent by
the firmware, so tpm_get_timeouts sees that the chip has not been started
and therefore issues a startup(clear) command itself.


Some possible solutions for discussion...

To make stock SeaBIOS work automatically:
1. For all systems in interrupt mode because of an ACPI entry: Use a
non-fragile method to detect correct functioning of the TPM and the
failure of interrupts. If detected, then fall back to polling mode.
Worst case is (slightly?) degraded performance.
2. Same as 1, but only for quirked machines

To make custom SeaBIOS work automatically:
3. For all systems: If error 38 received from selftest at resume, do not
issue subsequent save_state commands before suspend. (This approach has
been tested.)
4. For all systems: Do not send startup(clear) in tpm_get_timeouts unless
a module parameter is set.
5. Same as 4, but only for quirked machines

As suggested by Jason:
6. For all machines in interrupt mode. Use a non-fragile method to detect
the failure of interrupts. Don't attach if detected.


Thanks!

[Jason Gunthorpe]

On Mon, Aug 25, 2014 at 06:38:33AM +0000, Scot Doyle wrote:

> command. Now the chip is running. It will not be reboot on resume
> (unlike stock SeaBIOS), presumably since the firmware is not
> interacting with the TPM. But because an error 38 is returned during
> the selftest issued on resume, its functioning will degrade over
> suspend/resume cycles until the machine refuses to enter suspend,

So, be aware, on x86 the TPM specs require some BIOS support for
suspend/resume to work properly.

> Manual workarounds for these three payloads:
>
> 1. Depthcharge. I don't know how ChromeOS interacts with the TPM chip.
> From what I've seen, it may boot with kernel parameters tpm_tis.force=1
> and tpm_tis.interrupts=0 (see #2 following). The kernel is older.

The HW or interrupt routing is fundamentally broken somehow?

> 2. Stock SeaBIOS. Boot the kernel with tpm_tis.force=1 or
> tpm_tis.interrupts=0. 'force=1' causes the module to ignore the ACPI
> TPM interrupt entry and instead probe for interrupts. Since no
> interrupts are found to be valid, the driver falls back to polling
> mode. And 'interrupts=0' causes the interrupt probing to be skipped,
> defaulting to polling mode. So, even though most users set BOTH of
> these parameters, they have the same effect on these machines and only
> one or the other are necessary.
>
> 3. Custom SeaBIOS. Blacklist the tpm_tis module so that it doesn't load
> and therefore doesn't issue startup(clear) to the TPM chip.

It seems to me at least in this case you should be able to get rid of
the IRQ entry, people are going to be flashing the custom SeaBIOS
anyhow.

Further, in x86 land you really should have BIOS support for operating
a TPM if the OS is going to touch it. I don't really understand why
you want to flash a custom BIOS that doesn't work and then hack the
kernel to fix the custom BIOS's deficiencies? Fix the BIOS, you are
flashing it already anyhow.

> The firmware sends a startup(clear). Then the tpm_tis module is loaded
> because there is a matching ACPI entry for "IFX0102". By default, module
> parameters are interrupts=1 and force=0. I will talk about these default
> values, since that is the case that I'd like to fix. force=0 causes the
> module to look for an ACPI entry. It finds an entry indicating IRQ 6 and
> continues with the tpm_tis_init function.

So, sounds like IRQ=6 is a BIOS bug.

> tpm_tis_init starts in polling mode. It registers the hardware device,
> performs some initial setup, queries the chip manufacturer and vendor id,
> deterines the chip capabilities (I think they are all enabled), gets
> timeouts, does a selftest (which passes), sets up the interrupt and read
> queues, and enables chip interrupts. It does all of this in polling
> mode.

This is the wrong order, I think, the interrupts should be on before
issuing commands, other tpm drivers are working this way already.

> I've verified that no blocking commands are send to the TPM during the
> tpm_tis_init function. Since none are sent, no interrupts fail, and the
> attach procedes as normal.

tpm_get_timeouts and tpm_do_selftest should both be commands that
potentially wait for interrupt.

> Side note 1: my understanding is that the module exchanges data with the
> TPM in the same way whether in polling mode or interrupt mode.

Right, the interrupt simply indicates when the TPM has completed
executing the comamnd, the driver can either poll loop or wait on IRQ
to read back the status register bit indicating the command is done.

> To make stock SeaBIOS work automatically:
> 1. For all systems in interrupt mode because of an ACPI entry: Use a
> non-fragile method to detect correct functioning of the TPM and the
> failure of interrupts. If detected, then fall back to polling mode.
> Worst case is (slightly?) degraded performance.
> 2. Same as 1, but only for quirked machines
>
> To make custom SeaBIOS work automatically:
> 3. For all systems: If error 38 received from selftest at resume, do not
> issue subsequent save_state commands before suspend. (This approach has
> been tested.)

This isn't right, if tpm core is going to handle resume without BIOS
support then it must do the protocol properly. IIRC this is issuing a
startup and then restore state command at resume (the BIOS is expected
to do this on x86, and it is expected to not jump back to the OS if
something goes wrong with the TPM, or the resume image has been
tampered with)

This might be interesting to people doing embedded (and I can't recall
exactly, but I want to say that one of the drivers implemented this).

Clearly we'd need a way to turn on 'embedded' mode for the TPM, but
that could potentially be a module parameter on x86...

> 4. For all systems: Do not send startup(clear) in tpm_get_timeouts unless
> a module parameter is set.

No, all embedded systems require this functionality, why would you
want to take this out?

Jason

[Scot Doyle]

On Mon, 25 Aug 2014, Jason Gunthorpe wrote:

> On Mon, Aug 25, 2014, Scot Doyle wrote:
>> 3. Custom SeaBIOS. Blacklist the tpm_tis module so that it doesn't load
>> and therefore doesn't issue startup(clear) to the TPM chip.
>
> It seems to me at least in this case you should be able to get rid of
> the IRQ entry, people are going to be flashing the custom SeaBIOS
> anyhow.

The person building many of these custom SeaBIOS packages has removed the
TPM section from the DSDT, so this may be addressed.


On Mon, 25 Aug 2014, Jason Gunthorpe wrote:
> I think you'll have to directly test in the tis driver if the
> interrupt is working.
>
> The ordering in the TIS driver is wrong, interrupts should be turned
> on before any TPM commands are issued. This is what other drivers are
> doing.
>
> If you fix this, tis can then just count interrupts recieved and check
> if that is 0 to detect failure and then turn them off.

How about something like this?

It doesn't enable stock SeaBIOS machines to suspend/resume before the 30
second interrupt timeout, unless using interrupts=0 or force=1.

---

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..ae701d8 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -493,6 +493,8 @@ static irqreturn_t tis_int_probe(int irq, void *dev_id)
return IRQ_HANDLED;
}

+static bool interrupted = false;
+
static irqreturn_t tis_int_handler(int dummy, void *dev_id)
{
struct tpm_chip *chip = dev_id;
@@ -511,6 +513,8 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
for (i = 0; i < 5; i++)
if (check_locality(chip, i) >= 0)
break;
+ if (interrupt & TPM_INTF_CMD_READY_INT)
+ interrupted = true;
if (interrupt &
(TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
TPM_INTF_CMD_READY_INT))
@@ -612,12 +616,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
goto out_err;
}

- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }
-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);
@@ -693,7 +691,7 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
free_irq(i, chip);
}
}
- if (chip->vendor.irq) {
+ if (interrupts && chip->vendor.irq) {
iowrite8(chip->vendor.irq,
chip->vendor.iobase +
TPM_INT_VECTOR(chip->vendor.locality));
@@ -719,6 +717,32 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
}
}

+ /* Test interrupt and/or prepare for later save state */
+ interrupted = false;
+ if (tpm_do_selftest(chip)) {
+ if (!interrupts || interrupted) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ } else {
+ /* Turn off interrupt */
+ iowrite32(intmask,
+ chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ free_irq(chip->vendor.irq, chip);
+
+ /* Retry in polling mode */

+ chip->vendor.irq = 0;

+ if (tpm_do_selftest(chip)) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ } else {
+ dev_err(dev, "ACPI DSDT entry incorrect, polling instead\n");
+ }
+ }
+ }
+
INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);

[Jason Gunthorpe]

On Wed, Aug 27, 2014 at 04:31:56AM +0000, Scot Doyle wrote:
> > I think you'll have to directly test in the tis driver if the
> > interrupt is working.
> >
> > The ordering in the TIS driver is wrong, interrupts should be turned
> > on before any TPM commands are issued. This is what other drivers are
> > doing.
> >
> > If you fix this, tis can then just count interrupts recieved and check
> > if that is 0 to detect failure and then turn them off.
>
> How about something like this?
>
> It doesn't enable stock SeaBIOS machines to suspend/resume before the 30
> second interrupt timeout, unless using interrupts=0 or force=1.

? Can you explain that a bit more? interrupts should be detected off
by suspend/resume time, surely?

> +static bool interrupted = false;
> +

This needs to be stored in the private data.

> static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> {
> struct tpm_chip *chip = dev_id;
> @@ -511,6 +513,8 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> for (i = 0; i < 5; i++)
> if (check_locality(chip, i) >= 0)
> break;
> + if (interrupt & TPM_INTF_CMD_READY_INT)
> + interrupted = true;

Hmm, I'd think any interrupt will do for this purpose, drop the if?

> - if (tpm_do_selftest(chip)) {
> - dev_err(dev, "TPM self test failed\n");
> - rc = -ENODEV;
> - goto out_err;
> - }

Move gettimeout too

> - if (chip->vendor.irq) {
> + if (interrupts && chip->vendor.irq) {

Unrelated? Looks unnecessary:

if (!interrupts) {
irq = 0;

chip->vendor.irq = irq;

if (chip->vendor.irq) {

> + /* Test interrupt and/or prepare for later save state */
> + interrupted = false;
> + if (tpm_do_selftest(chip)) {

As you pointed out before, the commands don't actually fail if
interrupts are not enabled, they just take a longer time to complete.

So this should just be:

if (!tpm_get_timeouts(chip))
goto ..failed..;

if (!dev->interrupts) {

/* Turn off interrupt */

iowrite32(intmask,
chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.locality));

free_irq(chip->vendor.irq, chip);
chip->vendor.irq = 0;
dev_err(dev, FIRMWARE_BUG "TPM interrupt is not working, polling instead\n");

// No retry needed, the command completed already.
}

Jason

[Scot Doyle]

On Wed, 27 Aug 2014, Jason Gunthorpe wrote:

> On Wed, Aug 27, 2014 at 04:31:56AM +0000, Scot Doyle wrote:
>> It doesn't enable stock SeaBIOS machines to suspend/resume before the 30
>> second interrupt timeout, unless using interrupts=0 or force=1.
>
> ? Can you explain that a bit more? interrupts should be detected off
> by suspend/resume time, surely?

Yes, here's dmesg:

[ 1.491629] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
[ 33.247720] tpm_tis 00:08: tpm_transmit: tpm_send: error -62
[ 33.247731] tpm_tis 00:08: [Hardware Error]: TPM command timed out during continue self test
[ 33.349888] tpm_tis 00:08: tpm_transmit: tpm_send: error -5
[ 33.459911] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead

At module load, the misconfigured DSDT is causing the interrupt to be used
during selftest. The interrupt wait times out after 30 seconds, and the
irq is freed, with the module falling back to polling mode.

If suspend/resume occur before falling back to polling mode (within 30
seconds after module load), then the machine freezes on resume because
the module is waiting on the interrupts.

So, this should only affect machines with incorrect ACPI, that are not
using a module parameter, and that are suspended within 30 seconds after
module load. Considering that we are enabling such machines to
automatically work otherwise, I think this is fair.

>> - if (tpm_do_selftest(chip)) {
>> - dev_err(dev, "TPM self test failed\n");
>> - rc = -ENODEV;
>> - goto out_err;
>> - }
>
> Move gettimeout too

Can it be moved? It sends startup(clear) if the TPM isn't yet operational.

>> - if (chip->vendor.irq) {
>> + if (interrupts && chip->vendor.irq) {
>
> Unrelated? Looks unnecessary:
>
> if (!interrupts) {
> irq = 0;
>
> chip->vendor.irq = irq;
>
> if (chip->vendor.irq) {

Setting chip->vendor.irq would erase any we just found in probing?

>> + /* Test interrupt and/or prepare for later save state */
>> + interrupted = false;
>> + if (tpm_do_selftest(chip)) {
>
> As you pointed out before, the commands don't actually fail if
> interrupts are not enabled, they just take a longer time to complete.

Right, the TPM commands don't fail, but tpm_get_timeouts does. I've
simplified the section in this version.

And I've incorporated the other suggestions, thanks!

---
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e4d0888..6747a47 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -69,6 +69,7 @@ struct tpm_vendor_specific {

int irq;
int probed_irq;
+ bool int_received;

int region_size;
int have_region;
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..ad63027 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -505,6 +505,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
if (interrupt == 0)
return IRQ_NONE;

+ chip->vendor.int_received = true;
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -612,12 +613,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
goto out_err;
}

- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }

-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -693,7 +688,7 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
free_irq(i, chip);
}
}

- if (chip->vendor.irq) {
+ if (interrupts && chip->vendor.irq) {

iowrite8(chip->vendor.irq,
chip->vendor.iobase +
TPM_INT_VECTOR(chip->vendor.locality));

@@ -719,6 +714,29 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

}
}

+ /* Test interrupt and/or prepare for later save state */

+ chip->vendor.int_received = false;
+ if (tpm_do_selftest(chip)) {
+ if (interrupts && !chip->vendor.int_received) {

+ /* Turn off interrupt */
+ iowrite32(intmask,
+ chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ free_irq(chip->vendor.irq, chip);
+
+ /* Retry in polling mode */
+ chip->vendor.irq = 0;

+ if (!tpm_do_selftest(chip)) {
+ dev_err(dev, FW_BUG "TPM interrupt not working, polling instead\n");
+ goto cont;
+ }
+ }

+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }

+
+cont:

INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);

[Jason Gunthorpe]

On Wed, Aug 27, 2014 at 09:32:10PM +0000, Scot Doyle wrote:

> If suspend/resume occur before falling back to polling mode (within 30
> seconds after module load), then the machine freezes on resume because
> the module is waiting on the interrupts.

Okay, this is just a specific case of the broader TPM bug: the tpm
driver is registered with the system before it is actually ready to
drive the TPM.

> >> - if (tpm_do_selftest(chip)) {
> >> - dev_err(dev, "TPM self test failed\n");
> >> - rc = -ENODEV;
> >> - goto out_err;
> >> - }
> >
> > Move gettimeout too
>
> Can it be moved? It sends startup(clear) if the TPM isn't yet operational.

To move it means we have to understand why you are getting timeouts:

[ 33.247720] tpm_tis 00:08: tpm_transmit: tpm_send: error -62
[ 33.247731] tpm_tis 00:08: [Hardware Error]: TPM command timed out during continue self test

I had thought based on your other patch that these should not happen
since the raw register is polled after the timer expires?

What is going on here? Do you still have that other patch applied?

>
> >> - if (chip->vendor.irq) {
> >> + if (interrupts && chip->vendor.irq) {
> >
> > Unrelated? Looks unnecessary:
> >
> > if (!interrupts) {
> > irq = 0;
> >
> > chip->vendor.irq = irq;
> >
> > if (chip->vendor.irq) {
>
> Setting chip->vendor.irq would erase any we just found in probing?

Sorry, I ment the code I clipped is already present in the driver, in
that order, so the change is a NOP.

> Right, the TPM commands don't fail, but tpm_get_timeouts does. I've
> simplified the section in this version.

I'm not quite sure what that means, but..

The reason you want to use tpm_get_timeouts to test the IRQ is because
it has a very short timeout. self test has one of the longest
timeouts, so it is not the best choice.

What I was hoping to see, is that get_timeouts would hit the timer, do
the final read of the completion register, complete normally, then the
tis driver would see successful completion with no IRQ and turn them
off.

Is that doable?

> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index e4d0888..6747a47 100644

> +++ b/drivers/char/tpm/tpm.h
> @@ -69,6 +69,7 @@ struct tpm_vendor_specific {
>
> int irq;
> int probed_irq;
> + bool int_received;

Ugh, yes, right, tis doesn't have its own driver private structure
yet.

Please add a comment 'FIXME: belongs in tpm_tis driver private data'

Or, better, add a driver private structure to hold this data.

Jason

[Scot Doyle]

On Wed, 27 Aug 2014, Jason Gunthorpe wrote:

> On Wed, Aug 27, 2014 at 09:32:10PM +0000, Scot Doyle wrote:
>>>> - if (tpm_do_selftest(chip)) {
>>>> - dev_err(dev, "TPM self test failed\n");
>>>> - rc = -ENODEV;
>>>> - goto out_err;
>>>> - }
>>>
>>> Move gettimeout too
>>
>> Can it be moved? It sends startup(clear) if the TPM isn't yet operational.
>
> To move it means we have to understand why you are getting timeouts:
>
> [ 33.247720] tpm_tis 00:08: tpm_transmit: tpm_send: error -62
> [ 33.247731] tpm_tis 00:08: [Hardware Error]: TPM command timed out during continue self test
>
> I had thought based on your other patch that these should not happen
> since the raw register is polled after the timer expires?

It is polled after the timer expires in tpm_tis_send_data, but not in
tpm_tis_send, and the return value is used in tpm_tis_send...

> What is going on here? Do you still have that other patch applied?

..so with the other patch applied too, the output is:
[ 1.464217] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
[ 3.570836] tpm_tis 00:08: tpm_transmit: tpm_send: error -62
[ 3.660885] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead

Much better! Any thoughts before I proceed?

[Jason Gunthorpe]

On Thu, Aug 28, 2014 at 12:35:16AM +0000, Scot Doyle wrote:

> > To move it means we have to understand why you are getting timeouts:
> >
> > [ 33.247720] tpm_tis 00:08: tpm_transmit: tpm_send: error -62
> > [ 33.247731] tpm_tis 00:08: [Hardware Error]: TPM command timed out during continue self test
> >
> > I had thought based on your other patch that these should not happen
> > since the raw register is polled after the timer expires?
>
> It is polled after the timer expires in tpm_tis_send_data, but not in
> tpm_tis_send, and the return value is used in tpm_tis_send...

tpm_tis_send does:

wait_for_tpm_stat
(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
tpm_calc_ordinal_duration(chip, ordinal),
&chip->vendor.read_queue, false)

Which is:
rc = wait_event_interruptible_timeout(*queue,
wait_for_tpm_stat_cond(chip, mask, check_cancel,
&canceled),

And we know that wait_event_interruptible_timeout returns 1 if
the condition is true when the timeout expires.

So, all calls to wait_for_tpm_stat should succeed if the register has
the requested bits set at the end of the timer - thus if interrupts
are broken we should never see ETIME from wait_for_tpm_stat as the
chip does actually complete its work. (and this is the correct,
desired, behavior)

Is this analysis wrong somehow?

Lets simplify, instead of wrapping the whole command let us target
only the first part, tpm_tis_send, and let us do that for
get_timeouts.

--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -367,6 +367,12 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
rc = -ETIME;
goto out_err;
}
+
+ if (!chip->vendor.int_received) {
+ msleep(1);
+ if (!chip->vendor.int_received)
+ disable_interrupts();
+ }
}
return len;
out_err:

Jason

[Scot Doyle]

Something (systemd-udevd?) is interrupting the selftest with a signal
(current->pending.signal.sig[0] == 0x00000100 == SIGKILL?) about 30
seconds after module load begins. wait_for_tpm_stat sees that the return
value from wait_event_interruptible_timeout is positive and returns 0.
tpm_tis_send thinks everything is fine and continues. However, since a
signal was received, but not cleared, then the next time
wait_event_interruptible_timeout is used within wait_for_tpm_stat it
returns with -ERESTARTSYS, and continues doing so until tpm_send_data
returns -ETIME.

So I think that mystery is finally solved :-)

The attached patch results in the following output:
[ 1.536850] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
[ 7.650172] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead

I tried calling tpm_get_timeouts only during the interrupt test, but again
was timed out after 30 seconds. The interrupt wait in tis_send calls
tpm_calc_ordinal_duration, which uses a default timeout of two minutes
when chip->vendor.duration[duration_idx] hasn't been set. Thus the second
call to tpm_get_timeouts in tpm_tis_init.

What do you think about the guard logic? My intent is to prevent a signal
received after the test period from causing a fallback to polling mode.
Plus, it seems good to preserve the current logic where practical.

Thanks so much for the help!

---
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..92f4ab5 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -75,6 +75,11 @@ enum tis_defaults {
#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
#define TPM_RID(l) (0x0F04 | ((l) << 12))

+struct priv_data {
+ bool testing_int;
+ bool int_received;
+};
+
static LIST_HEAD(tis_chips);
static DEFINE_MUTEX(tis_lock);

@@ -338,6 +343,21 @@ out_err:
return rc;
}

+static void disable_interrupts(struct tpm_chip *chip)
+{
+ u32 intmask;
+ intmask =
+ ioread32(chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
+ TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;

+ iowrite32(intmask,
+ chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ free_irq(chip->vendor.irq, chip);

+ chip->vendor.irq = 0;
+}

+
/*
* If interrupts are used (signaled by an irq set in the vendor structure)
* tpm.c can skip polling for the data to be available as the interrupt is
@@ -347,6 +367,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
{
int rc;
u32 ordinal;
+ struct priv_data *priv = chip->vendor.priv;

rc = tpm_tis_send_data(chip, buf, len);
if (rc < 0)
@@ -366,6 +387,15 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
goto out_err;
}
}
+ if (priv->testing_int) {
+ priv->testing_int = false;
+ msleep(1);
+ if (!priv->int_received) {
+ disable_interrupts(chip);
+ dev_err(chip->dev,
+ FW_BUG "TPM interrupt not working, polling instead\n");
+ }
+ }
return len;
out_err:
tpm_tis_ready(chip);
@@ -496,6 +526,7 @@ static irqreturn_t tis_int_probe(int irq, void *dev_id)

static irqreturn_t tis_int_handler(int dummy, void *dev_id)

{
struct tpm_chip *chip = dev_id;

+ struct priv_data *priv = chip->vendor.priv;
u32 interrupt;
int i;

@@ -505,6 +536,8 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)

if (interrupt == 0)
return IRQ_NONE;

+ if (priv->testing_int)
+ priv->int_received = true;

if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)

@@ -534,10 +567,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
+ struct priv_data *priv;

if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
return -ENODEV;

+ priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+ chip->vendor.priv = priv;
+
chip->vendor.iobase = ioremap(start, len);
if (!chip->vendor.iobase) {
rc = -EIO;
@@ -612,12 +649,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
goto out_err;
}

- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }

-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -719,6 +750,22 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
}
}

+ /* Test interrupt */
+ if (chip->vendor.irq) {
+ priv->testing_int = true;
+ if (tpm_get_timeouts(chip)) {
+ dev_err(dev, "Could not get TPM timeouts\n");

+ rc = -ENODEV;
+ goto out_err;
+ }
+ }
+

+ if (tpm_do_selftest(chip)) {

+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+

INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);

[Jason Gunthorpe]

On Fri, Aug 29, 2014 at 11:59:32PM +0000, Scot Doyle wrote:

> (current->pending.signal.sig[0] == 0x00000100 == SIGKILL?) about 30
> seconds after module load begins. wait_for_tpm_stat sees that the return
> value from wait_event_interruptible_timeout is positive and returns 0.
> tpm_tis_send thinks everything is fine and continues. However, since a
> signal was received, but not cleared, then the next time
> wait_event_interruptible_timeout is used within wait_for_tpm_stat it
> returns with -ERESTARTSYS, and continues doing so until tpm_send_data
> returns -ETIME.

Oh, I see. That is another bug you've found - ERESTARTSYS should not
be translated into ETIME!

It is also not exciting that udev is overriding the driver timeouts. :(

> [ 1.536850] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
> [ 7.650172] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead
>
> I tried calling tpm_get_timeouts only during the interrupt test, but again
> was timed out after 30 seconds. The interrupt wait in tis_send calls
> tpm_calc_ordinal_duration, which uses a default timeout of two minutes
> when chip->vendor.duration[duration_idx] hasn't been set. Thus the second
> call to tpm_get_timeouts in tpm_tis_init.

So the strategy is to read the timeouts and hope that the chip reports
something small and reasonable, then do a second read?

Seems reasonable, but with this new arrangement we could also use an
alternate polling logic for 'testing_int' that did the normal polling
loop unconditionally and then checked if the interrupt was
delivered. This would give a minimal dealy.

> What do you think about the guard logic? My intent is to prevent a signal
> received after the test period from causing a fallback to polling mode.
> Plus, it seems good to preserve the current logic where practical.

I think this is looking very reasonable now, I'll have to read it
closer next week!

> + if (priv->testing_int)
> + priv->int_received = true;

This could just be a simple counter, if the counter is 0 then test
the interrupt otherwise proceed normally.

Jason

[Scot Doyle]

On Sat, 30 Aug 2014, Jason Gunthorpe wrote:

> On Fri, Aug 29, 2014 at 11:59:32PM +0000, Scot Doyle wrote:
>
>> I tried calling tpm_get_timeouts only during the interrupt test, but again
>> was timed out after 30 seconds. The interrupt wait in tis_send calls
>> tpm_calc_ordinal_duration, which uses a default timeout of two minutes
>> when chip->vendor.duration[duration_idx] hasn't been set. Thus the second
>> call to tpm_get_timeouts in tpm_tis_init.
>
> So the strategy is to read the timeouts and hope that the chip reports
> something small and reasonable, then do a second read?
>
> Seems reasonable, but with this new arrangement we could also use an
> alternate polling logic for 'testing_int' that did the normal polling
> loop unconditionally and then checked if the interrupt was
> delivered. This would give a minimal dealy.

I like the idea. And then tpm_do_selftest could be used for the interrupt
verification instead of a second tpm_get_timeouts?

The output is now
[ 1.526798] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
[ 5.914732] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead

Enjoy the weekend!

---
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c

index 2c46734..7a5f5b2 100644

--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -75,6 +75,11 @@ enum tis_defaults {
#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
#define TPM_RID(l) (0x0F04 | ((l) << 12))

+struct priv_data {

+ int test_irq;
+ int int_count;

@@ -358,13 +379,27 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)

if (chip->vendor.irq) {
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- if (wait_for_tpm_stat
- (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
- tpm_calc_ordinal_duration(chip, ordinal),
- &chip->vendor.read_queue, false) < 0) {
+ if (priv->test_irq)

+ chip->vendor.irq = 0;

+ rc = wait_for_tpm_stat
+ (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ tpm_calc_ordinal_duration(chip, ordinal),
+ &chip->vendor.read_queue, false);
+ if (priv->test_irq)
+ chip->vendor.irq = priv->test_irq;
+ if (rc < 0) {
rc = -ETIME;
goto out_err;
}
+ if (priv->test_irq) {
+ priv->test_irq = 0;
+ msleep(1);
+ if (!priv->int_count) {

+ disable_interrupts(chip);
+ dev_err(chip->dev,
+ FW_BUG "TPM interrupt not working, polling instead\n");
+ }
+ }
}
return len;
out_err:

@@ -505,6 +540,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)

if (interrupt == 0)
return IRQ_NONE;

+ ((struct priv_data*)chip->vendor.priv)->int_count++;

if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)

@@ -534,10 +570,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
+ struct priv_data *priv;

if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
return -ENODEV;

+ priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+ chip->vendor.priv = priv;
+
chip->vendor.iobase = ioremap(start, len);
if (!chip->vendor.iobase) {
rc = -EIO;

@@ -612,12 +652,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

goto out_err;
}

- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }
-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -719,6 +753,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
}
}

+ /* Test interrupt and prepare for later save state */
+ priv->test_irq = chip->vendor.irq;

+ if (tpm_do_selftest(chip)) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);

[Jason Gunthorpe]

On Sat, Aug 30, 2014 at 11:23:56PM +0000, Scot Doyle wrote:
> On Sat, 30 Aug 2014, Jason Gunthorpe wrote:
>
> > On Fri, Aug 29, 2014 at 11:59:32PM +0000, Scot Doyle wrote:
> >
> >> I tried calling tpm_get_timeouts only during the interrupt test, but again
> >> was timed out after 30 seconds. The interrupt wait in tis_send calls
> >> tpm_calc_ordinal_duration, which uses a default timeout of two minutes
> >> when chip->vendor.duration[duration_idx] hasn't been set. Thus the second
> >> call to tpm_get_timeouts in tpm_tis_init.
> >
> > So the strategy is to read the timeouts and hope that the chip reports
> > something small and reasonable, then do a second read?
> >
> > Seems reasonable, but with this new arrangement we could also use an
> > alternate polling logic for 'testing_int' that did the normal polling
> > loop unconditionally and then checked if the interrupt was
> > delivered. This would give a minimal dealy.
>
> I like the idea. And then tpm_do_selftest could be used for the interrupt
> verification instead of a second tpm_get_timeouts?

Yes, or the first tpm_get_timeouts can be used - Long term I would
like to see the entire tpm_get_timeouts,self_test,startup, etc
sequence moved into core code, so I don't really want to see drivers
splitting the sequence up.

Ideally the driver will just automatically test the IRQ on the very
first command it executes. That is now a very small easy step, so lets
just do that..

> The output is now
> [ 1.526798] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
> [ 5.914732] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead

Cool, why did it take 4 seconds though?

> +struct priv_data {
> + int test_irq;

Probably don't need this...

> @@ -358,13 +379,27 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>

And this can probably just become:

bool test_irq = priv->int_count == 0;
int oldirq = chip->vendor.irq;

> + ((struct priv_data*)chip->vendor.priv)->int_count++;

. Seems like there was no need for it to count, this can just be =
true?

> - if (tpm_do_selftest(chip)) {
> - dev_err(dev, "TPM self test failed\n");
> - rc = -ENODEV;
> - goto out_err;
> - }

And move tpm_get_timeouts down too.. Keep the sequence together.

Looks really good to me, I can try and test the next version here this
week.

Jason

[Scot Doyle]

On Tue, 2 Sep 2014, Jason Gunthorpe wrote:
> On Sat, Aug 30, 2014 at 11:23:56PM +0000, Scot Doyle wrote:
>> The output is now
>> [ 1.526798] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
>> [ 5.914732] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead
>
> Cool, why did it take 4 seconds though?

It's spending that time (now 3 seconds) in tpm_tis_send_data.

Output is
[ 2.928481] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
[ 5.943468] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead

> Looks really good to me, I can try and test the next version here this
> week.

Thanks!

---
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c

index 2c46734..6e42d60 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -75,6 +75,10 @@ enum tis_defaults {

#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
#define TPM_RID(l) (0x0F04 | ((l) << 12))

+struct priv_data {

+ bool irq_tested;

+};
+
static LIST_HEAD(tis_chips);
static DEFINE_MUTEX(tis_lock);

@@ -338,6 +342,21 @@ out_err:

return rc;
}

+static void disable_interrupts(struct tpm_chip *chip)
+{
+ u32 intmask;
+ intmask =
+ ioread32(chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
+ TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+ iowrite32(intmask,
+ chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ free_irq(chip->vendor.irq, chip);
+ chip->vendor.irq = 0;
+}
+
/*
* If interrupts are used (signaled by an irq set in the vendor structure)
* tpm.c can skip polling for the data to be available as the interrupt is

@@ -345,8 +364,10 @@ out_err:
*/

static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)

{
- int rc;
+ int rc, irq;
u32 ordinal;
+ bool test_irq;

+ struct priv_data *priv = chip->vendor.priv;

rc = tpm_tis_send_data(chip, buf, len);
if (rc < 0)

@@ -358,13 +379,30 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)

if (chip->vendor.irq) {
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- if (wait_for_tpm_stat
- (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
- tpm_calc_ordinal_duration(chip, ordinal),
- &chip->vendor.read_queue, false) < 0) {

+ test_irq = !priv->irq_tested;
+ if (test_irq) {
+ irq = chip->vendor.irq;

+ chip->vendor.irq = 0;
+ }

+ rc = wait_for_tpm_stat
+ (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ tpm_calc_ordinal_duration(chip, ordinal),
+ &chip->vendor.read_queue, false);

+ if (test_irq)
+ chip->vendor.irq = irq;

+ if (rc < 0) {
rc = -ETIME;
goto out_err;
}

+ if (test_irq) {
+ msleep(1);
+ if (!priv->irq_tested) {

+ disable_interrupts(chip);
+ dev_err(chip->dev,

+ FW_BUG "TPM interrupt not working, polling instead\n");
+ }
+ priv->irq_tested = true;
+ }
}
return len;
out_err:
@@ -505,6 +543,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)

if (interrupt == 0)
return IRQ_NONE;

+ ((struct priv_data*)chip->vendor.priv)->irq_tested = true;

if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)

@@ -534,10 +573,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
+ struct priv_data *priv;

if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
return -ENODEV;

+ priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+ chip->vendor.priv = priv;
+
chip->vendor.iobase = ioremap(start, len);
if (!chip->vendor.iobase) {
rc = -EIO;

@@ -605,19 +648,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
dev_dbg(dev, "\tData Avail Int Support\n");

- /* get the timeouts before testing for irqs */
- if (tpm_get_timeouts(chip)) {
- dev_err(dev, "Could not get TPM timeouts and durations\n");

- rc = -ENODEV;
- goto out_err;
- }

-

- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }

-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -719,6 +749,18 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
}
}

+ if (tpm_get_timeouts(chip)) {
+ dev_err(dev, "Could not get TPM timeouts and durations\n");

+ rc = -ENODEV;
+ goto out_err;
+ }
+
+ if (tpm_do_selftest(chip)) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);

[Jason Gunthorpe]

On Tue, Sep 02, 2014 at 08:22:58PM +0000, Scot Doyle wrote:

> It's spending that time (now 3 seconds) in tpm_tis_send_data.

Due to request_locality?

> > Looks really good to me, I can try and test the next version here this
> > week.
>
> Thanks!

So, I forgot my TIS systems have no IRQ, can't really test it
properly, but it compiles and doesn't muck up the no irq specified
case at least.

> + if (test_irq) {

Should be

if (test_irq && !priv->irq_tested)

We don't need to msleep if we got an irq already.

Reviewed-By: Jason Gunthorpe <jgunt...@obsidianresearch.com>

You should post a final version and try and get it tested on a normal
x86 system.

Jason

[Scot Doyle]

Some machines, such as the Acer C720 and Toshiba CB35, have TPMs that do

not send IRQs while also having an ACPI TPM entry indicating that they
will be sent. These machines freeze on resume while the tpm_tis module
waits for an IRQ, eventually timing out.

When in interrupt mode, the tpm_tis module should receive an IRQ during
module init. Fall back to polling mode if none is received when expected.

Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
Tested-by: Michael Mullin <masm...@gmail.com>
Reviewed-By: Jason Gunthorpe <jgunt...@obsidianresearch.com>
---
drivers/char/tpm/tpm_tis.c | 75 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 57 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..43aeb6961 100644

@@ -358,13 +379,27 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)

if (chip->vendor.irq) {
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- if (wait_for_tpm_stat
- (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
- tpm_calc_ordinal_duration(chip, ordinal),
- &chip->vendor.read_queue, false) < 0) {
+ test_irq = !priv->irq_tested;
+ if (test_irq) {
+ irq = chip->vendor.irq;
+ chip->vendor.irq = 0;
+ }
+ rc = wait_for_tpm_stat
+ (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ tpm_calc_ordinal_duration(chip, ordinal),
+ &chip->vendor.read_queue, false);
+ if (test_irq)
+ chip->vendor.irq = irq;
+ if (rc < 0) {
rc = -ETIME;
goto out_err;
}

+ if (test_irq && !priv->irq_tested) {

+ priv->irq_tested = true;
+ disable_interrupts(chip);
+ dev_err(chip->dev,
+ FW_BUG "TPM interrupt not working, polling instead\n");
+ }
}
return len;
out_err:

@@ -505,6 +540,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)

if (interrupt == 0)
return IRQ_NONE;

+ ((struct priv_data*)chip->vendor.priv)->irq_tested = true;
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)

@@ -534,10 +570,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
+ struct priv_data *priv;

if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
return -ENODEV;

+ priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+ chip->vendor.priv = priv;
+
chip->vendor.iobase = ioremap(start, len);
if (!chip->vendor.iobase) {
rc = -EIO;

@@ -605,19 +645,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
dev_dbg(dev, "\tData Avail Int Support\n");

- /* get the timeouts before testing for irqs */
- if (tpm_get_timeouts(chip)) {
- dev_err(dev, "Could not get TPM timeouts and durations\n");
- rc = -ENODEV;
- goto out_err;
- }
-
- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }
-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -719,6 +746,18 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

}
}

+ if (tpm_get_timeouts(chip)) {
+ dev_err(dev, "Could not get TPM timeouts and durations\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
+ if (tpm_do_selftest(chip)) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);
--

2.0.4

[Scot Doyle]

On Tue, 9 Sep 2014, Scot Doyle wrote:

> Some machines, such as the Acer C720 and Toshiba CB35, have TPMs that do
> not send IRQs while also having an ACPI TPM entry indicating that they
> will be sent. These machines freeze on resume while the tpm_tis module
> waits for an IRQ, eventually timing out.
>
> When in interrupt mode, the tpm_tis module should receive an IRQ during
> module init. Fall back to polling mode if none is received when expected.
>
> Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
> Tested-by: Michael Mullin <masm...@gmail.com>
> Reviewed-By: Jason Gunthorpe <jgunt...@obsidianresearch.com>

Would anyone be willing to test this patch on a system that uses the
tpm_tis module with interrupts?

[Scot Doyle]

On Mon, 8 Sep 2014, Jason Gunthorpe wrote:
> On Tue, Sep 02, 2014 at 08:22:58PM +0000, Scot Doyle wrote:
>
>> It's spending that time (now 3 seconds) in tpm_tis_send_data.
>
> Due to request_locality?

The first command transmitted (TPM_CAP_PROP) in tpm_get_timeouts goes
through tpm_tis_send which calls tpm_tis_send_data before setting up
polling mode for the interrupt test. In tpm_tis_send_data, the last call
to wait_for_tpm_stat is still timing out.

One solution would be to move the test from tpm_tis_send to
tpm_tis_send_data. Another would be to expand the test in tpm_tis_send to
include the call to tpm_tis_send_data.

The latter seems safer, since it provides more opportunity for an IRQ to
be generated. E.g. I'm not sure if TPM_CAP_PROP always generates an IRQ.
But the problem with this approach is that tpm_tis_send becomes a bit
messy. So this patch wraps tpm_tis_send in an attempt to keep the code
clean. (Is there a better name for the wrapped function than
tpm_tis_send_main?)

Thoughts?

With this patch, the output becomes:
[ 4.264619] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
[ 4.311628] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead

P.S. My apologies for revisiting this issue after it seemed to be
finalized.

---
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c

index 2c46734..2dbd652 100644

--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -75,6 +75,10 @@ enum tis_defaults {
#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
#define TPM_RID(l) (0x0F04 | ((l) << 12))

+struct priv_data {
+ bool irq_tested;
+};
+
static LIST_HEAD(tis_chips);
static DEFINE_MUTEX(tis_lock);

@@ -338,12 +342,27 @@ out_err:

return rc;
}

+static void disable_interrupts(struct tpm_chip *chip)
+{
+ u32 intmask;
+ intmask =
+ ioread32(chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
+ TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+ iowrite32(intmask,
+ chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ free_irq(chip->vendor.irq, chip);
+ chip->vendor.irq = 0;
+}
+
/*
* If interrupts are used (signaled by an irq set in the vendor structure)
* tpm.c can skip polling for the data to be available as the interrupt is

* waited for here
*/
-static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
+static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
{
int rc;
u32 ordinal;
@@ -373,6 +392,28 @@ out_err:
return rc;
}

+static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+ int rc, irq;

+ struct priv_data *priv = chip->vendor.priv;

+
+ if (!chip->vendor.irq || priv->irq_tested)
+ return tpm_tis_send_main(chip, buf, len);
+
+ /* Verify receipt of the expected IRQ */

+ irq = chip->vendor.irq;
+ chip->vendor.irq = 0;

+ rc = tpm_tis_send_main(chip, buf, len);

+ chip->vendor.irq = irq;

+ if (!priv->irq_tested) {
+ disable_interrupts(chip);
+ dev_err(chip->dev,
+ FW_BUG "TPM interrupt not working, polling instead\n");
+ }
+ priv->irq_tested = true;

+ return rc;
+}
+
struct tis_vendor_timeout_override {
u32 did_vid;
unsigned long timeout_us[4];
@@ -505,6 +546,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)

if (interrupt == 0)
return IRQ_NONE;

+ ((struct priv_data*)chip->vendor.priv)->irq_tested = true;
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)

@@ -534,10 +576,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
+ struct priv_data *priv;

if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
return -ENODEV;

+ priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+ chip->vendor.priv = priv;
+
chip->vendor.iobase = ioremap(start, len);
if (!chip->vendor.iobase) {
rc = -EIO;

@@ -605,19 +651,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
dev_dbg(dev, "\tData Avail Int Support\n");

- /* get the timeouts before testing for irqs */
- if (tpm_get_timeouts(chip)) {
- dev_err(dev, "Could not get TPM timeouts and durations\n");
- rc = -ENODEV;
- goto out_err;
- }
-
- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }
-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -719,6 +752,18 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

}
}

+ if (tpm_get_timeouts(chip)) {
+ dev_err(dev, "Could not get TPM timeouts and durations\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
+ if (tpm_do_selftest(chip)) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);

[Scot Doyle]

On Thu, 11 Sep 2014, Scot Doyle wrote:
>
> On Mon, 8 Sep 2014, Jason Gunthorpe wrote:
>> On Tue, Sep 02, 2014 at 08:22:58PM +0000, Scot Doyle wrote:
>>
>>> It's spending that time (now 3 seconds) in tpm_tis_send_data.
>>
>> Due to request_locality?
>
> The first command transmitted (TPM_CAP_PROP) in tpm_get_timeouts goes
> through tpm_tis_send which calls tpm_tis_send_data before setting up
> polling mode for the interrupt test. In tpm_tis_send_data, the last call
> to wait_for_tpm_stat is still timing out.
>
> One solution would be to move the test from tpm_tis_send to
> tpm_tis_send_data. Another would be to expand the test in tpm_tis_send to
> include the call to tpm_tis_send_data.
>
> The latter seems safer, since it provides more opportunity for an IRQ to
> be generated. E.g. I'm not sure if TPM_CAP_PROP always generates an IRQ.
> But the problem with this approach is that tpm_tis_send becomes a bit
> messy. So this patch wraps tpm_tis_send in an attempt to keep the code
> clean. (Is there a better name for the wrapped function than
> tpm_tis_send_main?)
>
> Thoughts?
>
> With this patch, the output becomes:
> [ 4.264619] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
> [ 4.311628] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead
>
> P.S. My apologies for revisiting this issue after it seemed to be
> finalized.

Hi Peter,

Would you prefer this revision on top of or in place of the previous patch?
https://github.com/PeterHuewe/linux-tpmdd/commit/1bf961689d9b826aa6a27b6a6c5c56d977d5fe2b

Thanks,
Scot

[Jason Gunthorpe]

On Thu, Sep 11, 2014 at 12:50:00AM +0000, Scot Doyle wrote:
>
> On Mon, 8 Sep 2014, Jason Gunthorpe wrote:
> > On Tue, Sep 02, 2014 at 08:22:58PM +0000, Scot Doyle wrote:
> >
> >> It's spending that time (now 3 seconds) in tpm_tis_send_data.
> >
> > Due to request_locality?
>
> The first command transmitted (TPM_CAP_PROP) in tpm_get_timeouts goes
> through tpm_tis_send which calls tpm_tis_send_data before setting up
> polling mode for the interrupt test. In tpm_tis_send_data, the last call
> to wait_for_tpm_stat is still timing out.
>
> One solution would be to move the test from tpm_tis_send to
> tpm_tis_send_data. Another would be to expand the test in tpm_tis_send to
> include the call to tpm_tis_send_data.
>
> The latter seems safer, since it provides more opportunity for an IRQ to
> be generated. E.g. I'm not sure if TPM_CAP_PROP always generates an IRQ.
> But the problem with this approach is that tpm_tis_send becomes a bit
> messy. So this patch wraps tpm_tis_send in an attempt to keep the code
> clean. (Is there a better name for the wrapped function than
> tpm_tis_send_main?)

This does look much nicer, lets use this version.

I think Peter were prefer a new clean patch that superceeds the
original.

> + if (!priv->irq_tested) {

I think the sleep and check is still needed here, the IRQ delivery
could race relative to the MMIO read of completion, a sleep is the
only way we could attempt to synchronize them..

> + disable_interrupts(chip);
> + dev_err(chip->dev,
> + FW_BUG "TPM interrupt not working, polling instead\n");
> + }
> + priv->irq_tested = true;
> + return rc;
> +}

Thanks,
Jason

[Peter Hüwe]

Am Montag, 22. September 2014, 19:13:38 schrieb Jason Gunthorpe:

>
> This does look much nicer, lets use this version.
>
> I think Peter were prefer a new clean patch that superceeds the
> original.
>
> > + if (!priv->irq_tested) {
>
> I think the sleep and check is still needed here, the IRQ delivery
> could race relative to the MMIO read of completion, a sleep is the
> only way we could attempt to synchronize them..
>

So will there be a v9?

Merge Window is coming up shortly, so I would like to have this sorted out
soon ;)

Thanks,
Peter

[Scot Doyle]

On Mon, 22 Sep 2014, Jason Gunthorpe wrote:

> On Thu, Sep 11, 2014 at 12:50:00AM +0000, Scot Doyle wrote:
>> + if (!priv->irq_tested) {
>
> I think the sleep and check is still needed here, the IRQ delivery
> could race relative to the MMIO read of completion, a sleep is the
> only way we could attempt to synchronize them..
>
>> + disable_interrupts(chip);
>> + dev_err(chip->dev,
>> + FW_BUG "TPM interrupt not working, polling instead\n");
>> + }
>> + priv->irq_tested = true;
>> + return rc;
>> +}

We re-tested this v9 patch. It has the msleep(1) added just above this
section for better formatting, as well as a devm_kzalloc check in
tpm_tis_init. Thanks!

[Scot Doyle]

Some machines, such as the Acer C720 and Toshiba CB35, have TPMs that do
not send IRQs while also having an ACPI TPM entry indicating that they
will be sent. These machines freeze on resume while the tpm_tis module
waits for an IRQ, eventually timing out.

When in interrupt mode, the tpm_tis module should receive an IRQ during
module init. Fall back to polling mode if none is received when expected.

Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
Tested-by: Michael Mullin <masm...@gmail.com>
Reviewed-By: Jason Gunthorpe <jgunt...@obsidianresearch.com>

---
drivers/char/tpm/tpm_tis.c | 76 +++++++++++++++++++++++++++++++++++++---------
1 file changed, 62 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..bfe1f1a 100644

@@ -373,6 +392,30 @@ out_err:

return rc;
}

+static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+ int rc, irq;
+ struct priv_data *priv = chip->vendor.priv;
+
+ if (!chip->vendor.irq || priv->irq_tested)
+ return tpm_tis_send_main(chip, buf, len);
+
+ /* Verify receipt of the expected IRQ */
+ irq = chip->vendor.irq;
+ chip->vendor.irq = 0;
+ rc = tpm_tis_send_main(chip, buf, len);
+ chip->vendor.irq = irq;
+ if (!priv->irq_tested)

+ msleep(1);
+ if (!priv->irq_tested) {

+ disable_interrupts(chip);
+ dev_err(chip->dev,
+ FW_BUG "TPM interrupt not working, polling instead\n");
+ }
+ priv->irq_tested = true;
+ return rc;
+}

+
struct tis_vendor_timeout_override {
u32 did_vid;
unsigned long timeout_us[4];

@@ -505,6 +548,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)

if (interrupt == 0)
return IRQ_NONE;

+ ((struct priv_data*)chip->vendor.priv)->irq_tested = true;
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)

@@ -534,9 +578,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
+ struct priv_data *priv;

+ priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);

+ if (priv == NULL)
+ return -ENOMEM;

if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
return -ENODEV;

+ chip->vendor.priv = priv;

chip->vendor.iobase = ioremap(start, len);
if (!chip->vendor.iobase) {

@@ -605,19 +654,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
dev_dbg(dev, "\tData Avail Int Support\n");

- /* get the timeouts before testing for irqs */
- if (tpm_get_timeouts(chip)) {
- dev_err(dev, "Could not get TPM timeouts and durations\n");
- rc = -ENODEV;
- goto out_err;
- }
-
- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }
-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -719,6 +755,18 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

}
}

+ if (tpm_get_timeouts(chip)) {
+ dev_err(dev, "Could not get TPM timeouts and durations\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
+ if (tpm_do_selftest(chip)) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);
--

2.1.0

[Scot Doyle]

On Tue, 23 Sep 2014, Scot Doyle wrote:

> Some machines, such as the Acer C720 and Toshiba CB35, have TPMs that do
> not send IRQs while also having an ACPI TPM entry indicating that they
> will be sent. These machines freeze on resume while the tpm_tis module
> waits for an IRQ, eventually timing out.
>
> When in interrupt mode, the tpm_tis module should receive an IRQ during
> module init. Fall back to polling mode if none is received when expected.
>
> Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
> Tested-by: Michael Mullin <masm...@gmail.com>
> Reviewed-By: Jason Gunthorpe <jgunt...@obsidianresearch.com>

Did I prematurely add your Reviewed-by? If so, I apologize.

[Stefan Berger]

You want to disable interrupts but you set all the flags? Maybe you meant:

intmask &= ~(FOO|BAR)

?

Stefan

[Scot Doyle]

On Tue, 23 Sep 2014, Stefan Berger wrote:
> On 09/23/2014 07:55 AM, Scot Doyle wrote:
>> On Tue, 23 Sep 2014, Scot Doyle wrote:
>> +static void disable_interrupts(struct tpm_chip *chip)
>> +{
>> + u32 intmask;
>> + intmask =
>> + ioread32(chip->vendor.iobase +
>> + TPM_INT_ENABLE(chip->vendor.locality));
>> + intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
>> + TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
>
> You want to disable interrupts but you set all the flags? Maybe you meant:
>
> intmask &= ~(FOO|BAR)
>
> ?

Thanks, would this work? I think it's how tpm_tis_init masks during a probe.

static void disable_interrupts(struct tpm_chip *chip)
{
u32 intmask;
intmask =
ioread32(chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.locality));
intmask &= ~TPM_GLOBAL_INT_ENABLE;

iowrite32(intmask,
chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.locality));
free_irq(chip->vendor.irq, chip);
chip->vendor.irq = 0;
}

[Stefan Berger]

Yes, this should be sufficient.

Stefan

[Scot Doyle]

Some machines, such as the Acer C720 and Toshiba CB35, have TPMs that do
not send IRQs while also having an ACPI TPM entry indicating that they
will be sent. These machines freeze on resume while the tpm_tis module
waits for an IRQ, eventually timing out.

When in interrupt mode, the tpm_tis module should receive an IRQ during
module init. Fall back to polling mode if none is received when expected.

Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
Tested-by: Michael Mullin <masm...@gmail.com>

---
drivers/char/tpm/tpm_tis.c | 75 +++++++++++++++++++++++++++++++++++++---------
1 file changed, 61 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..cbef80e 100644

--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -75,6 +75,10 @@ enum tis_defaults {
#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
#define TPM_RID(l) (0x0F04 | ((l) << 12))

+struct priv_data {
+ bool irq_tested;
+};
+
static LIST_HEAD(tis_chips);
static DEFINE_MUTEX(tis_lock);

@@ -338,12 +342,26 @@ out_err:
return rc;

}

+static void disable_interrupts(struct tpm_chip *chip)
+{
+ u32 intmask;
+ intmask =
+ ioread32(chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));

+ intmask &= ~TPM_GLOBAL_INT_ENABLE;
+ iowrite32(intmask,
+ chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));

+ free_irq(chip->vendor.irq, chip);
+ chip->vendor.irq = 0;
+}
+
/*
* If interrupts are used (signaled by an irq set in the vendor structure)
* tpm.c can skip polling for the data to be available as the interrupt is
* waited for here
*/
-static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
+static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
{
int rc;
u32 ordinal;

@@ -373,6 +391,30 @@ out_err:
return rc;
}

@@ -505,6 +547,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)

if (interrupt == 0)
return IRQ_NONE;

+ ((struct priv_data*)chip->vendor.priv)->irq_tested = true;
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)

@@ -534,9 +577,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
+ struct priv_data *priv;

+ priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+ if (priv == NULL)
+ return -ENOMEM;
if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
return -ENODEV;
+ chip->vendor.priv = priv;

chip->vendor.iobase = ioremap(start, len);
if (!chip->vendor.iobase) {

@@ -605,19 +653,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
dev_dbg(dev, "\tData Avail Int Support\n");

- /* get the timeouts before testing for irqs */
- if (tpm_get_timeouts(chip)) {
- dev_err(dev, "Could not get TPM timeouts and durations\n");
- rc = -ENODEV;
- goto out_err;
- }
-
- if (tpm_do_selftest(chip)) {
- dev_err(dev, "TPM self test failed\n");
- rc = -ENODEV;
- goto out_err;
- }
-
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);

@@ -719,6 +754,18 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,

}
}

+ if (tpm_get_timeouts(chip)) {
+ dev_err(dev, "Could not get TPM timeouts and durations\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
+ if (tpm_do_selftest(chip)) {
+ dev_err(dev, "TPM self test failed\n");
+ rc = -ENODEV;
+ goto out_err;
+ }
+
INIT_LIST_HEAD(&chip->vendor.list);
mutex_lock(&tis_lock);
list_add(&chip->vendor.list, &tis_chips);
--
2.1.0

[Jason Gunthorpe]

On Wed, Sep 24, 2014 at 10:41:10PM +0000, Scot Doyle wrote:
> Some machines, such as the Acer C720 and Toshiba CB35, have TPMs that do
> not send IRQs while also having an ACPI TPM entry indicating that they
> will be sent. These machines freeze on resume while the tpm_tis module
> waits for an IRQ, eventually timing out.
>
> When in interrupt mode, the tpm_tis module should receive an IRQ during
> module init. Fall back to polling mode if none is received when expected.
>
> Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
> Tested-by: Michael Mullin <masm...@gmail.com>

Looks good with enable fixed

Reviewed-By: Jason Gunthorpe <jgunt...@obsidianresearch.com>

> drivers/char/tpm/tpm_tis.c | 75 +++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 61 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 2c46734..cbef80e 100644

[Scot Doyle]

On Mon, 22 Sep 2014, Peter Hüwe wrote:

> Am Montag, 22. September 2014, 19:13:38 schrieb Jason Gunthorpe:
>
> >
> > This does look much nicer, lets use this version.
> >
> > I think Peter were prefer a new clean patch that superceeds the
> > original.
> >
> > > + if (!priv->irq_tested) {
> >
> > I think the sleep and check is still needed here, the IRQ delivery
> > could race relative to the MMIO read of completion, a sleep is the
> > only way we could attempt to synchronize them..
> >
>
> So will there be a v9?
>
> Merge Window is coming up shortly, so I would like to have this sorted out
> soon ;)
>
> Thanks,
> Peter

Hi Peter,

The v10 patch was reviewed by Jason and is ready to go. Will you be asking
James to pull this week?

Thanks,
Scot

[Peter Hüwe]

Hi Scot,

Am Montag, 29. September 2014, 19:24:57 schrieb Jason Gunthorpe:
> On Wed, Sep 24, 2014 at 10:41:10PM +0000, Scot Doyle wrote:
> > Some machines, such as the Acer C720 and Toshiba CB35, have TPMs that do
> > not send IRQs while also having an ACPI TPM entry indicating that they
> > will be sent. These machines freeze on resume while the tpm_tis module
> > waits for an IRQ, eventually timing out.
> >
> > When in interrupt mode, the tpm_tis module should receive an IRQ during
> > module init. Fall back to polling mode if none is received when expected.
> >
> > Signed-off-by: Scot Doyle <lkm...@scotdoyle.com>
> > Tested-by: Michael Mullin <masm...@gmail.com>
>
> Looks good with enable fixed
>
> Reviewed-By: Jason Gunthorpe <jgunt...@obsidianresearch.com>

Applied to my tree:
https://github.com/PeterHuewe/linux-tpmdd for-james

Will be included in the next pull-request.

Thanks,
Peter