| Message ID | 1446740353-15235-6-git-send-email-martin.wilck@ts.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 05, 2015 at 05:19:12PM +0100, martin.wilck@ts.fujitsu.com wrote: > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > When probing IRQs, IRQF_SHARED may cause IRQs to be falsely > detected if other devices generate IRQ events while tpm_tis is > probing or testing. I have seen this on my test machine repeatedly. > > Therefore, refrain from probing IRQs that are already used by > other devices by default. Use "interrupts=2" module parameter > to obtain the previous behavior. > > Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com> The status register is cleared before generating interrupt in the probing code. And it's checked right in the beginning tis_int_probe(). If you had a bug like that then you would fix the race condition, not move away from IRQF_SHARED. /Jarkko ------------------------------------------------------------------------------ Presto, an open source distributed SQL query engine for big data, initially developed by Facebook, enables you to easily query your data on Hadoop in a more interactive manner. Teradata is also now providing full enterprise support for Presto. Download a free open source copy now. http://pubads.g.doubleclick.net/gampad/clk?id=250295911&iu=/4140
> > When probing IRQs, IRQF_SHARED may cause IRQs to be falsely > > detected if other devices generate IRQ events while tpm_tis is > > probing or testing. I have seen this on my test machine repeatedly. > > > > Therefore, refrain from probing IRQs that are already used by > > other devices by default. Use "interrupts=2" module parameter > > to obtain the previous behavior. > > > > Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > The status register is cleared before generating interrupt in the > probing code. And it's checked right in the beginning tis_int_probe(). The sequence of events is as follows: 1 TPM IRQ generation is enabled for IRQ X and test command sent 2 TPM finishes command and sets data ready / IRQ flags, but the IRQ doesn't arrive because it's not configured in the system. Normally this would cause a timeout and the IRQ would be found not to work, but... 3 The other device triggers an IRQ X, causing tis_int_probe() to get called and find the IRQ flags set. Now we conclude that the IRQ seems to work, which is wrong. (Btw, the IRQ check in tpm_tis_send can erroneously succeed for the same reason). > If you had a bug like that then you would fix the race condition, not > move away from IRQF_SHARED. I have no idea how this can be fixed. It isn't really a race condition. The device has generated an interrupt, and the IRQ handler has been called, but with IRQF_SHARED we can't conclude that the former was the reason for the latter. Probing would only work reliably by temporarily disabling the other device's IRQ, and that doesn't feel right at all. Martin ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index f5d7d52..21bd00a 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -622,9 +622,9 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id) return IRQ_HANDLED; } -static bool interrupts = true; -module_param(interrupts, bool, 0444); -MODULE_PARM_DESC(interrupts, "Enable interrupts"); +static int interrupts = 1; +module_param(interrupts, int, 0444); +MODULE_PARM_DESC(interrupts, "1 - enable interrupts (default), 0 - disable interrupts, 2 - use shared interrupts when probing"); static void tpm_tis_remove(struct tpm_chip *chip) { @@ -788,7 +788,8 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, iowrite8(i, chip->vendor.iobase + TPM_INT_VECTOR(chip->vendor.locality)); if (devm_request_irq - (dev, i, tis_int_probe, IRQF_SHARED, + (dev, i, tis_int_probe, + interrupts == 2 ? IRQF_SHARED : 0, chip->devname, chip) != 0) { dev_info(chip->pdev, "Unable to request irq: %d for probe\n",