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