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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
