[tpmdd-devel,5/6] tpm_tis: don't use IRQF_SHARED by default when probing IRQ
diff mbox

Message ID 1446740353-15235-6-git-send-email-martin.wilck@ts.fujitsu.com
State New
Headers show

Commit Message

Martin Wilck Nov. 5, 2015, 4:19 p.m. UTC
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>
---
 drivers/char/tpm/tpm_tis.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Jarkko Sakkinen Nov. 9, 2015, 2:12 p.m. UTC | #1
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
Martin Wilck Nov. 10, 2015, 8:04 a.m. UTC | #2
> > 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

------------------------------------------------------------------------------

Patch
diff mbox

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",