| Message ID | 20200929223216.22584-3-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm_tis: fix interrupts (again) | expand |
On Tue, Sep 29, 2020 at 03:32:14PM -0700, James Bottomley wrote: > If a TIS TPM doesn't have legacy cycles, any write to the interrupt > registers is ignored unless a locality is active. This means even to > set up the interrupt vectors a locality must first be activated. Fix > this by activating the 0 locality in the interrupt probe setup. > > Since the TPM_EOI signalling also requires an active locality, the > interrupt routine cannot end an interrupt if the locality is released. > This can lead to a situation where the TPM goes command ready after > locality release and since the interrupt cannot be ended it refires > continuously. Fix this by disabling all interrupts except locality > change when a locality is released (this only fires when a locality > becomes active, meaning the TPM_EOI should work). > > Finally, since we now disable all status based interrupts in the > locality release, they must be re-enabled before waiting to check the > condition, so add interrupt enabling to the status wait. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > --- > drivers/char/tpm/tpm_tis_core.c | 125 ++++++++++++++++++++++++++------ > 1 file changed, 101 insertions(+), 24 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index a9fa40714c64..02cc384fdaea 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -29,6 +29,46 @@ > > static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); > > +static void enable_interrupt(struct tpm_chip *chip, u8 mask) Even if this has not been followed before I'd prefeer that for new functions or when modifying the signature of function they'd be prefixed with tpm_tis_. It is is just more practical e.g. for grepping stuff. I'm fine with full 'interrupt' would but I also think that just 'int' would be perfectly fine. Not something I'm too opionated about. > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 intmask; > + > + /* Take control of the TPM's interrupt hardware and shut it off */ > + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > + > + intmask |= mask | TPM_GLOBAL_INT_ENABLE; > + > + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > +} > + > +static void disable_interrupt(struct tpm_chip *chip, u8 mask) > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 intmask; > + > + /* Take control of the TPM's interrupt hardware and shut it off */ > + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > + > + intmask &= ~mask; > + > + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > +} > + > +static void enable_stat_interrupt(struct tpm_chip *chip, u8 stat) I'd use plural (interrupts or ints). > +{ > + u32 mask = 0; > + > + if (stat & TPM_STS_COMMAND_READY) > + mask |= TPM_INTF_CMD_READY_INT; > + if (stat & TPM_STS_VALID) > + mask |= TPM_INTF_STS_VALID_INT; > + if (stat & TPM_STS_DATA_AVAIL) > + mask |= TPM_INTF_DATA_AVAIL_INT; > + > + enable_interrupt(chip, mask); > +} > + > static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, > bool check_cancel, bool *canceled) > { > @@ -65,11 +105,14 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > timeout = stop - jiffies; > if ((long)timeout <= 0) > return -ETIME; > + enable_stat_interrupt(chip, mask); > rc = wait_event_interruptible_timeout(*queue, > wait_for_tpm_stat_cond(chip, mask, check_cancel, > &canceled), > timeout); > if (rc > 0) { > + if (rc == 1) > + dev_err(&chip->dev, "Lost Interrupt waiting for TPM stat\n"); > if (canceled) > return -ECANCELED; > return 0; > @@ -137,6 +180,28 @@ static bool check_locality(struct tpm_chip *chip, int l) > static int release_locality(struct tpm_chip *chip, int l) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 int_status; > + int rc; > + > + /* > + * Note that once we relinquish the locality, all writes to > + * the interrupt registers become ineffective meaning we can't > + * do a TPM_EOI. This means we must disable every interrupt > + * except the locality change one to avoid interrupt > + * storms. > + */ > + disable_interrupt(chip, TPM_INTF_CMD_READY_INT > + | TPM_INTF_STS_VALID_INT > + | TPM_INTF_DATA_AVAIL_INT); > + > + rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); > + if (rc < 0) > + return rc; > + > + /* Clear all pending */ > + rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); > + if (rc < 0) > + return rc; > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > > @@ -163,12 +228,17 @@ static int request_locality(struct tpm_chip *chip, int l) > timeout = stop - jiffies; > if ((long)timeout <= 0) > return -1; > + > rc = wait_event_interruptible_timeout(priv->int_queue, > (check_locality > (chip, l)), > timeout); > - if (rc > 0) > + if (rc > 1) > + return l; > + if (rc == 1) { > + dev_info(&chip->dev, "Lost Interrupt waiting for locality\n"); > return l; > + } > if (rc == -ERESTARTSYS && freezing(current)) { > clear_thread_flag(TIF_SIGPENDING); > goto again; > @@ -464,6 +534,10 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > irq = priv->irq; > priv->irq = 0; > chip->flags &= ~TPM_CHIP_FLAG_IRQ; > + enable_interrupt(chip, TPM_INTF_CMD_READY_INT > + | TPM_INTF_LOCALITY_CHANGE_INT > + | TPM_INTF_DATA_AVAIL_INT > + | TPM_INTF_STS_VALID_INT); > rc = tpm_tis_send_main(chip, buf, len); > priv->irq = irq; > chip->flags |= TPM_CHIP_FLAG_IRQ; > @@ -718,7 +792,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > * irq is seen then leave the chip setup for IRQ operation, otherwise reverse > * everything and leave in polling mode. Returns 0 on success. > */ > -static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > +static int tpm_tis_probe_irq_single(struct tpm_chip *chip, > int flags, int irq) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > @@ -752,9 +826,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > if (rc < 0) > return rc; > > - /* Turn on */ > - rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > - intmask | TPM_GLOBAL_INT_ENABLE); > + /* > + * Turn on. The locality change interrupt is the only one > + * always enabled > + */ > + enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT); > if (rc < 0) > return rc; > > @@ -786,7 +862,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > * do not have ACPI/etc. We typically expect the interrupt to be declared if > * present. > */ > -static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask) > +static void tpm_tis_probe_irq(struct tpm_chip *chip) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > u8 original_int_vec; > @@ -800,11 +876,9 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask) > if (!original_int_vec) { > if (IS_ENABLED(CONFIG_X86)) > for (i = 3; i <= 15; i++) > - if (!tpm_tis_probe_irq_single(chip, intmask, 0, > - i)) > + if (!tpm_tis_probe_irq_single(chip, 0, i)) > return; > - } else if (!tpm_tis_probe_irq_single(chip, intmask, 0, > - original_int_vec)) > + } else if (!tpm_tis_probe_irq_single(chip, 0, original_int_vec)) > return; > } > > @@ -1029,8 +1103,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > } > > if (irq) { > - tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, > - irq); > + tpm_tis_probe_irq_single(chip, IRQF_SHARED, irq); > if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > dev_err(&chip->dev, FW_BUG > "TPM interrupt not working, polling instead\n"); > @@ -1038,7 +1111,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > disable_interrupts(chip); > } > } else { > - tpm_tis_probe_irq(chip, intmask); > + tpm_tis_probe_irq(chip); > } > } > > @@ -1064,12 +1137,23 @@ EXPORT_SYMBOL_GPL(tpm_tis_core_init); > static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > - u32 intmask; > int rc; > > if (chip->ops->clk_enable != NULL) > chip->ops->clk_enable(chip, true); > > + /* > + * must have the locality before we can enable interrupts, so > + * poll for the locality being ready > + */ > + chip->flags &= ~TPM_CHIP_FLAG_IRQ; > + if (request_locality(chip, 0) != 0) { > + dev_err(&chip->dev, "Failed to enable interrupts after suspend\n"); > + goto out; > + } > + chip->flags |= TPM_CHIP_FLAG_IRQ; > + > + > /* reenable interrupts that device may have lost or > * BIOS/firmware may have disabled > */ > @@ -1077,17 +1161,10 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) > if (rc < 0) > goto out; > > - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > - if (rc < 0) > - goto out; > - > - intmask |= TPM_INTF_CMD_READY_INT > - | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT > - | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE; > + enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT); > > - tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > - > -out: > + out: > + release_locality(chip, 0); > if (chip->ops->clk_enable != NULL) > chip->ops->clk_enable(chip, false); > > -- > 2.28.0 > Agree with the change minus the cosmetic stuff that I mentioned. /Jarko
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index a9fa40714c64..02cc384fdaea 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -29,6 +29,46 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); +static void enable_interrupt(struct tpm_chip *chip, u8 mask) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + u32 intmask; + + /* Take control of the TPM's interrupt hardware and shut it off */ + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); + + intmask |= mask | TPM_GLOBAL_INT_ENABLE; + + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); +} + +static void disable_interrupt(struct tpm_chip *chip, u8 mask) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + u32 intmask; + + /* Take control of the TPM's interrupt hardware and shut it off */ + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); + + intmask &= ~mask; + + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); +} + +static void enable_stat_interrupt(struct tpm_chip *chip, u8 stat) +{ + u32 mask = 0; + + if (stat & TPM_STS_COMMAND_READY) + mask |= TPM_INTF_CMD_READY_INT; + if (stat & TPM_STS_VALID) + mask |= TPM_INTF_STS_VALID_INT; + if (stat & TPM_STS_DATA_AVAIL) + mask |= TPM_INTF_DATA_AVAIL_INT; + + enable_interrupt(chip, mask); +} + static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, bool check_cancel, bool *canceled) { @@ -65,11 +105,14 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, timeout = stop - jiffies; if ((long)timeout <= 0) return -ETIME; + enable_stat_interrupt(chip, mask); rc = wait_event_interruptible_timeout(*queue, wait_for_tpm_stat_cond(chip, mask, check_cancel, &canceled), timeout); if (rc > 0) { + if (rc == 1) + dev_err(&chip->dev, "Lost Interrupt waiting for TPM stat\n"); if (canceled) return -ECANCELED; return 0; @@ -137,6 +180,28 @@ static bool check_locality(struct tpm_chip *chip, int l) static int release_locality(struct tpm_chip *chip, int l) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + u32 int_status; + int rc; + + /* + * Note that once we relinquish the locality, all writes to + * the interrupt registers become ineffective meaning we can't + * do a TPM_EOI. This means we must disable every interrupt + * except the locality change one to avoid interrupt + * storms. + */ + disable_interrupt(chip, TPM_INTF_CMD_READY_INT + | TPM_INTF_STS_VALID_INT + | TPM_INTF_DATA_AVAIL_INT); + + rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); + if (rc < 0) + return rc; + + /* Clear all pending */ + rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); + if (rc < 0) + return rc; tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); @@ -163,12 +228,17 @@ static int request_locality(struct tpm_chip *chip, int l) timeout = stop - jiffies; if ((long)timeout <= 0) return -1; + rc = wait_event_interruptible_timeout(priv->int_queue, (check_locality (chip, l)), timeout); - if (rc > 0) + if (rc > 1) + return l; + if (rc == 1) { + dev_info(&chip->dev, "Lost Interrupt waiting for locality\n"); return l; + } if (rc == -ERESTARTSYS && freezing(current)) { clear_thread_flag(TIF_SIGPENDING); goto again; @@ -464,6 +534,10 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) irq = priv->irq; priv->irq = 0; chip->flags &= ~TPM_CHIP_FLAG_IRQ; + enable_interrupt(chip, TPM_INTF_CMD_READY_INT + | TPM_INTF_LOCALITY_CHANGE_INT + | TPM_INTF_DATA_AVAIL_INT + | TPM_INTF_STS_VALID_INT); rc = tpm_tis_send_main(chip, buf, len); priv->irq = irq; chip->flags |= TPM_CHIP_FLAG_IRQ; @@ -718,7 +792,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) * irq is seen then leave the chip setup for IRQ operation, otherwise reverse * everything and leave in polling mode. Returns 0 on success. */ -static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, +static int tpm_tis_probe_irq_single(struct tpm_chip *chip, int flags, int irq) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); @@ -752,9 +826,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, if (rc < 0) return rc; - /* Turn on */ - rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), - intmask | TPM_GLOBAL_INT_ENABLE); + /* + * Turn on. The locality change interrupt is the only one + * always enabled + */ + enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT); if (rc < 0) return rc; @@ -786,7 +862,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, * do not have ACPI/etc. We typically expect the interrupt to be declared if * present. */ -static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask) +static void tpm_tis_probe_irq(struct tpm_chip *chip) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); u8 original_int_vec; @@ -800,11 +876,9 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask) if (!original_int_vec) { if (IS_ENABLED(CONFIG_X86)) for (i = 3; i <= 15; i++) - if (!tpm_tis_probe_irq_single(chip, intmask, 0, - i)) + if (!tpm_tis_probe_irq_single(chip, 0, i)) return; - } else if (!tpm_tis_probe_irq_single(chip, intmask, 0, - original_int_vec)) + } else if (!tpm_tis_probe_irq_single(chip, 0, original_int_vec)) return; } @@ -1029,8 +1103,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, } if (irq) { - tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, - irq); + tpm_tis_probe_irq_single(chip, IRQF_SHARED, irq); if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { dev_err(&chip->dev, FW_BUG "TPM interrupt not working, polling instead\n"); @@ -1038,7 +1111,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, disable_interrupts(chip); } } else { - tpm_tis_probe_irq(chip, intmask); + tpm_tis_probe_irq(chip); } } @@ -1064,12 +1137,23 @@ EXPORT_SYMBOL_GPL(tpm_tis_core_init); static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); - u32 intmask; int rc; if (chip->ops->clk_enable != NULL) chip->ops->clk_enable(chip, true); + /* + * must have the locality before we can enable interrupts, so + * poll for the locality being ready + */ + chip->flags &= ~TPM_CHIP_FLAG_IRQ; + if (request_locality(chip, 0) != 0) { + dev_err(&chip->dev, "Failed to enable interrupts after suspend\n"); + goto out; + } + chip->flags |= TPM_CHIP_FLAG_IRQ; + + /* reenable interrupts that device may have lost or * BIOS/firmware may have disabled */ @@ -1077,17 +1161,10 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) if (rc < 0) goto out; - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); - if (rc < 0) - goto out; - - intmask |= TPM_INTF_CMD_READY_INT - | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT - | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE; + enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT); - tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); - -out: + out: + release_locality(chip, 0); if (chip->ops->clk_enable != NULL) chip->ops->clk_enable(chip, false);
If a TIS TPM doesn't have legacy cycles, any write to the interrupt registers is ignored unless a locality is active. This means even to set up the interrupt vectors a locality must first be activated. Fix this by activating the 0 locality in the interrupt probe setup. Since the TPM_EOI signalling also requires an active locality, the interrupt routine cannot end an interrupt if the locality is released. This can lead to a situation where the TPM goes command ready after locality release and since the interrupt cannot be ended it refires continuously. Fix this by disabling all interrupts except locality change when a locality is released (this only fires when a locality becomes active, meaning the TPM_EOI should work). Finally, since we now disable all status based interrupts in the locality release, they must be re-enabled before waiting to check the condition, so add interrupt enabling to the status wait. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- drivers/char/tpm/tpm_tis_core.c | 125 ++++++++++++++++++++++++++------ 1 file changed, 101 insertions(+), 24 deletions(-)