* [PATCH] tpm_tis: Verify ACPI-specified interrupt @ 2014-08-22 0:58 Scot Doyle 2014-08-22 16:06 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-22 0:58 UTC (permalink / raw) To: Peter Huewe, Ashley Lai, Marcel Selhorst Cc: Jason Gunthorpe, Stefan Berger, tpmdd-devel, linux-kernel 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 <lkml14@scotdoyle.com> Tested-by: James Duley <jagduley@gmail.com> Tested-by: Michael Mullin <masmullin@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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] tpm_tis: Verify ACPI-specified interrupt 2014-08-22 0:58 [PATCH] tpm_tis: Verify ACPI-specified interrupt Scot Doyle @ 2014-08-22 16:06 ` Jason Gunthorpe 2014-08-22 20:17 ` Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-08-22 16:06 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tpm_tis: Verify ACPI-specified interrupt 2014-08-22 16:06 ` Jason Gunthorpe @ 2014-08-22 20:17 ` Scot Doyle 2014-08-22 20:32 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-22 20:17 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, tpmdd-devel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tpm_tis: Verify ACPI-specified interrupt 2014-08-22 20:17 ` Scot Doyle @ 2014-08-22 20:32 ` Jason Gunthorpe 2014-08-22 22:48 ` Peter Hüwe 2014-08-25 6:38 ` Scot Doyle 0 siblings, 2 replies; 35+ messages in thread From: Jason Gunthorpe @ 2014-08-22 20:32 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, tpmdd-devel, linux-kernel 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. Jason ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tpm_tis: Verify ACPI-specified interrupt 2014-08-22 20:32 ` Jason Gunthorpe @ 2014-08-22 22:48 ` Peter Hüwe 2014-08-25 6:38 ` Scot Doyle 1 sibling, 0 replies; 35+ messages in thread From: Peter Hüwe @ 2014-08-22 22:48 UTC (permalink / raw) To: Jason Gunthorpe Cc: Scot Doyle, Ashley Lai, Marcel Selhorst, Stefan Berger, tpmdd-devel, linux-kernel, semenzato CC: Luigi, he works at Google and is responsible for the TPMs in Chromebooks ;) Thanks, Peter Am Freitag, 22. August 2014, 22:32:41 schrieb 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. > > Jason ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tpm_tis: Verify ACPI-specified interrupt 2014-08-22 20:32 ` Jason Gunthorpe 2014-08-22 22:48 ` Peter Hüwe @ 2014-08-25 6:38 ` Scot Doyle 2014-08-25 18:24 ` Jason Gunthorpe 1 sibling, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-25 6:38 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel On Fri, 22 Aug 2014, Jason Gunthorpe wrote: > 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. > > Jason 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! ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tpm_tis: Verify ACPI-specified interrupt 2014-08-25 6:38 ` Scot Doyle @ 2014-08-25 18:24 ` Jason Gunthorpe 2014-08-27 4:31 ` [RFC PATCH v2] tpm_tis: verify interrupt during init Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-08-25 18:24 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [RFC PATCH v2] tpm_tis: verify interrupt during init 2014-08-25 18:24 ` Jason Gunthorpe @ 2014-08-27 4:31 ` Scot Doyle 2014-08-27 17:31 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-27 4:31 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v2] tpm_tis: verify interrupt during init 2014-08-27 4:31 ` [RFC PATCH v2] tpm_tis: verify interrupt during init Scot Doyle @ 2014-08-27 17:31 ` Jason Gunthorpe 2014-08-27 21:32 ` [RFC PATCH v3] " Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-08-27 17:31 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [RFC PATCH v3] tpm_tis: verify interrupt during init 2014-08-27 17:31 ` Jason Gunthorpe @ 2014-08-27 21:32 ` Scot Doyle 2014-08-27 21:47 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-27 21:32 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v3] tpm_tis: verify interrupt during init 2014-08-27 21:32 ` [RFC PATCH v3] " Scot Doyle @ 2014-08-27 21:47 ` Jason Gunthorpe 2014-08-28 0:35 ` Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-08-27 21:47 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v3] tpm_tis: verify interrupt during init 2014-08-27 21:47 ` Jason Gunthorpe @ 2014-08-28 0:35 ` Scot Doyle 2014-08-28 16:53 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-28 0:35 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v3] tpm_tis: verify interrupt during init 2014-08-28 0:35 ` Scot Doyle @ 2014-08-28 16:53 ` Jason Gunthorpe 2014-08-29 23:59 ` [RFC PATCH v4] " Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-08-28 16:53 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [RFC PATCH v4] tpm_tis: verify interrupt during init 2014-08-28 16:53 ` Jason Gunthorpe @ 2014-08-29 23:59 ` Scot Doyle 2014-08-30 17:49 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-29 23:59 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel On Thu, 28 Aug 2014, Jason Gunthorpe wrote: > 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? 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); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v4] tpm_tis: verify interrupt during init 2014-08-29 23:59 ` [RFC PATCH v4] " Scot Doyle @ 2014-08-30 17:49 ` Jason Gunthorpe 2014-08-30 23:23 ` [RFC PATCH v5] " Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-08-30 17:49 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [RFC PATCH v5] tpm_tis: verify interrupt during init 2014-08-30 17:49 ` Jason Gunthorpe @ 2014-08-30 23:23 ` Scot Doyle 2014-09-02 17:20 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-08-30 23:23 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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; +}; + 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) @@ -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); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v5] tpm_tis: verify interrupt during init 2014-08-30 23:23 ` [RFC PATCH v5] " Scot Doyle @ 2014-09-02 17:20 ` Jason Gunthorpe 2014-09-02 20:22 ` [RFC PATCH v6] " Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-09-02 17:20 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [RFC PATCH v6] tpm_tis: verify interrupt during init 2014-09-02 17:20 ` Jason Gunthorpe @ 2014-09-02 20:22 ` Scot Doyle 2014-09-08 22:02 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-09-02 20:22 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v6] tpm_tis: verify interrupt during init 2014-09-02 20:22 ` [RFC PATCH v6] " Scot Doyle @ 2014-09-08 22:02 ` Jason Gunthorpe 2014-09-09 2:13 ` [PATCH v7] " Scot Doyle 2014-09-11 0:50 ` [RFC PATCH v8] " Scot Doyle 0 siblings, 2 replies; 35+ messages in thread From: Jason Gunthorpe @ 2014-09-08 22:02 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 <jgunthorpe@obsidianresearch.com> You should post a final version and try and get it tested on a normal x86 system. Jason ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7] tpm_tis: verify interrupt during init 2014-09-08 22:02 ` Jason Gunthorpe @ 2014-09-09 2:13 ` Scot Doyle 2014-09-09 3:12 ` Scot Doyle 2014-09-11 0:50 ` [RFC PATCH v8] " Scot Doyle 1 sibling, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-09-09 2:13 UTC (permalink / raw) To: Peter Huewe, Ashley Lai, Marcel Selhorst Cc: Jason Gunthorpe, Michael Mullin, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 <lkml14@scotdoyle.com> Tested-by: Michael Mullin <masmullin@gmail.com> Reviewed-By: Jason Gunthorpe <jgunthorpe@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 --- 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,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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v7] tpm_tis: verify interrupt during init 2014-09-09 2:13 ` [PATCH v7] " Scot Doyle @ 2014-09-09 3:12 ` Scot Doyle 0 siblings, 0 replies; 35+ messages in thread From: Scot Doyle @ 2014-09-09 3:12 UTC (permalink / raw) To: tpmdd-devel Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Jason Gunthorpe, Michael Mullin, Stefan Berger, Luigi Semenzato, linux-kernel 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 <lkml14@scotdoyle.com> > Tested-by: Michael Mullin <masmullin@gmail.com> > Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Would anyone be willing to test this patch on a system that uses the tpm_tis module with interrupts? > --- > 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 > --- 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,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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 35+ messages in thread
* [RFC PATCH v8] tpm_tis: verify interrupt during init 2014-09-08 22:02 ` Jason Gunthorpe 2014-09-09 2:13 ` [PATCH v7] " Scot Doyle @ 2014-09-11 0:50 ` Scot Doyle 2014-09-16 23:36 ` Scot Doyle 2014-09-22 17:13 ` Jason Gunthorpe 1 sibling, 2 replies; 35+ messages in thread From: Scot Doyle @ 2014-09-11 0:50 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v8] tpm_tis: verify interrupt during init 2014-09-11 0:50 ` [RFC PATCH v8] " Scot Doyle @ 2014-09-16 23:36 ` Scot Doyle 2014-09-22 17:13 ` Jason Gunthorpe 1 sibling, 0 replies; 35+ messages in thread From: Scot Doyle @ 2014-09-16 23:36 UTC (permalink / raw) To: Peter Huewe Cc: Jason Gunthorpe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 > --- > 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); > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v8] tpm_tis: verify interrupt during init 2014-09-11 0:50 ` [RFC PATCH v8] " Scot Doyle 2014-09-16 23:36 ` Scot Doyle @ 2014-09-22 17:13 ` Jason Gunthorpe 2014-09-22 19:01 ` Peter Hüwe 2014-09-23 2:44 ` Scot Doyle 1 sibling, 2 replies; 35+ messages in thread From: Jason Gunthorpe @ 2014-09-22 17:13 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v8] tpm_tis: verify interrupt during init 2014-09-22 17:13 ` Jason Gunthorpe @ 2014-09-22 19:01 ` Peter Hüwe 2014-10-19 20:08 ` Scot Doyle 2014-09-23 2:44 ` Scot Doyle 1 sibling, 1 reply; 35+ messages in thread From: Peter Hüwe @ 2014-09-22 19:01 UTC (permalink / raw) To: Jason Gunthorpe Cc: Scot Doyle, Ashley Lai, Marcel Selhorst, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v8] tpm_tis: verify interrupt during init 2014-09-22 19:01 ` Peter Hüwe @ 2014-10-19 20:08 ` Scot Doyle 0 siblings, 0 replies; 35+ messages in thread From: Scot Doyle @ 2014-10-19 20:08 UTC (permalink / raw) To: Peter Hüwe Cc: Ashley Lai, Marcel Selhorst, Jason Gunthorpe, Stefan Berger, Luigi Semenzato, tpmdd-devel, linux-kernel [-- Attachment #1: Type: TEXT/PLAIN, Size: 764 bytes --] 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [RFC PATCH v8] tpm_tis: verify interrupt during init 2014-09-22 17:13 ` Jason Gunthorpe 2014-09-22 19:01 ` Peter Hüwe @ 2014-09-23 2:44 ` Scot Doyle 2014-09-23 2:51 ` [PATCH v9] " Scot Doyle 1 sibling, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-09-23 2:44 UTC (permalink / raw) To: Jason Gunthorpe Cc: Michael Mullin, Peter Huewe, Ashley Lai, Marcel Selhorst, Luigi Semenzato, tpmdd-devel, linux-kernel 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! ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v9] tpm_tis: verify interrupt during init 2014-09-23 2:44 ` Scot Doyle @ 2014-09-23 2:51 ` Scot Doyle 2014-09-23 11:55 ` Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-09-23 2:51 UTC (permalink / raw) To: Peter Huewe, Ashley Lai, Marcel Selhorst Cc: Michael Mullin, Jason Gunthorpe, Luigi Semenzato, tpmdd-devel, linux-kernel 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 <lkml14@scotdoyle.com> Tested-by: Michael Mullin <masmullin@gmail.com> Reviewed-By: Jason Gunthorpe <jgunthorpe@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 --- 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,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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v9] tpm_tis: verify interrupt during init 2014-09-23 2:51 ` [PATCH v9] " Scot Doyle @ 2014-09-23 11:55 ` Scot Doyle 2014-09-23 17:12 ` [tpmdd-devel] " Stefan Berger 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-09-23 11:55 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Michael Mullin, Luigi Semenzato, tpmdd-devel, linux-kernel 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 <lkml14@scotdoyle.com> > Tested-by: Michael Mullin <masmullin@gmail.com> > Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Did I prematurely add your Reviewed-by? If so, I apologize. > --- > 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 > --- 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,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 > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [tpmdd-devel] [PATCH v9] tpm_tis: verify interrupt during init 2014-09-23 11:55 ` Scot Doyle @ 2014-09-23 17:12 ` Stefan Berger 2014-09-24 19:38 ` Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Stefan Berger @ 2014-09-23 17:12 UTC (permalink / raw) To: Scot Doyle, Jason Gunthorpe Cc: Michael Mullin, Ashley Lai, linux-kernel, tpmdd-devel On 09/23/2014 07:55 AM, Scot Doyle wrote: > On Tue, 23 Sep 2014, Scot Doyle wrote: > --- > 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 > --- 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; You want to disable interrupts but you set all the flags? Maybe you meant: intmask &= ~(FOO|BAR) ? Stefan ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v9] tpm_tis: verify interrupt during init 2014-09-23 17:12 ` [tpmdd-devel] " Stefan Berger @ 2014-09-24 19:38 ` Scot Doyle 2014-09-24 19:41 ` Stefan Berger 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-09-24 19:38 UTC (permalink / raw) To: Stefan Berger Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Jason Gunthorpe, Luigi Semenzato, Michael Mullin, linux-kernel, tpmdd-devel 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; } ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v9] tpm_tis: verify interrupt during init 2014-09-24 19:38 ` Scot Doyle @ 2014-09-24 19:41 ` Stefan Berger 2014-09-24 22:41 ` [PATCH v10] " Scot Doyle 0 siblings, 1 reply; 35+ messages in thread From: Stefan Berger @ 2014-09-24 19:41 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Jason Gunthorpe, Luigi Semenzato, Michael Mullin, linux-kernel, tpmdd-devel On 09/24/2014 03:38 PM, Scot Doyle wrote: > 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; > } > Yes, this should be sufficient. Stefan ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v10] tpm_tis: verify interrupt during init 2014-09-24 19:41 ` Stefan Berger @ 2014-09-24 22:41 ` Scot Doyle 2014-09-29 17:24 ` Jason Gunthorpe 0 siblings, 1 reply; 35+ messages in thread From: Scot Doyle @ 2014-09-24 22:41 UTC (permalink / raw) To: Peter Huewe, Ashley Lai, Marcel Selhorst Cc: Michael Mullin, Jason Gunthorpe, Stefan Berger, Luigi Semenzato, linux-kernel, tpmdd-devel 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 <lkml14@scotdoyle.com> Tested-by: Michael Mullin <masmullin@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; } +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 +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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v10] tpm_tis: verify interrupt during init 2014-09-24 22:41 ` [PATCH v10] " Scot Doyle @ 2014-09-29 17:24 ` Jason Gunthorpe 2014-11-30 14:24 ` Peter Hüwe 0 siblings, 1 reply; 35+ messages in thread From: Jason Gunthorpe @ 2014-09-29 17:24 UTC (permalink / raw) To: Scot Doyle Cc: Peter Huewe, Ashley Lai, Marcel Selhorst, Michael Mullin, Stefan Berger, Luigi Semenzato, linux-kernel, tpmdd-devel 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 <lkml14@scotdoyle.com> > Tested-by: Michael Mullin <masmullin@gmail.com> Looks good with enable fixed Reviewed-By: Jason Gunthorpe <jgunthorpe@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 > +++ 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; > } > > +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 +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); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v10] tpm_tis: verify interrupt during init 2014-09-29 17:24 ` Jason Gunthorpe @ 2014-11-30 14:24 ` Peter Hüwe 0 siblings, 0 replies; 35+ messages in thread From: Peter Hüwe @ 2014-11-30 14:24 UTC (permalink / raw) To: Scot Doyle Cc: Jason Gunthorpe, Ashley Lai, Marcel Selhorst, Michael Mullin, Stefan Berger, Luigi Semenzato, linux-kernel, tpmdd-devel 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 <lkml14@scotdoyle.com> > > Tested-by: Michael Mullin <masmullin@gmail.com> > > Looks good with enable fixed > > Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Applied to my tree: https://github.com/PeterHuewe/linux-tpmdd for-james Will be included in the next pull-request. Thanks, Peter ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2014-11-30 14:19 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-22 0:58 [PATCH] tpm_tis: Verify ACPI-specified interrupt Scot Doyle 2014-08-22 16:06 ` Jason Gunthorpe 2014-08-22 20:17 ` Scot Doyle 2014-08-22 20:32 ` Jason Gunthorpe 2014-08-22 22:48 ` Peter Hüwe 2014-08-25 6:38 ` Scot Doyle 2014-08-25 18:24 ` Jason Gunthorpe 2014-08-27 4:31 ` [RFC PATCH v2] tpm_tis: verify interrupt during init Scot Doyle 2014-08-27 17:31 ` Jason Gunthorpe 2014-08-27 21:32 ` [RFC PATCH v3] " Scot Doyle 2014-08-27 21:47 ` Jason Gunthorpe 2014-08-28 0:35 ` Scot Doyle 2014-08-28 16:53 ` Jason Gunthorpe 2014-08-29 23:59 ` [RFC PATCH v4] " Scot Doyle 2014-08-30 17:49 ` Jason Gunthorpe 2014-08-30 23:23 ` [RFC PATCH v5] " Scot Doyle 2014-09-02 17:20 ` Jason Gunthorpe 2014-09-02 20:22 ` [RFC PATCH v6] " Scot Doyle 2014-09-08 22:02 ` Jason Gunthorpe 2014-09-09 2:13 ` [PATCH v7] " Scot Doyle 2014-09-09 3:12 ` Scot Doyle 2014-09-11 0:50 ` [RFC PATCH v8] " Scot Doyle 2014-09-16 23:36 ` Scot Doyle 2014-09-22 17:13 ` Jason Gunthorpe 2014-09-22 19:01 ` Peter Hüwe 2014-10-19 20:08 ` Scot Doyle 2014-09-23 2:44 ` Scot Doyle 2014-09-23 2:51 ` [PATCH v9] " Scot Doyle 2014-09-23 11:55 ` Scot Doyle 2014-09-23 17:12 ` [tpmdd-devel] " Stefan Berger 2014-09-24 19:38 ` Scot Doyle 2014-09-24 19:41 ` Stefan Berger 2014-09-24 22:41 ` [PATCH v10] " Scot Doyle 2014-09-29 17:24 ` Jason Gunthorpe 2014-11-30 14:24 ` Peter Hüwe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).