| Message ID | 20220509080559.4381-3-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TPM irq fixes | expand |
On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote: > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > It is not necessary to claim and release the default locality for each TPM > command. Instead claim the locality once at driver startup and release it > at driver shutdown. > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> We are doing what we're being because of Intel TXT: https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/ Unfortunately cannot accept this change. BR, Jarkko
On 11.05.22 at 13:27, Jarkko Sakkinen wrote: > On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote: >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> It is not necessary to claim and release the default locality for each TPM >> command. Instead claim the locality once at driver startup and release it >> at driver shutdown. >> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > We are doing what we're being because of Intel TXT: > > https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/ > > Unfortunately cannot accept this change. > I do not see how the patch affects the crb code since the only changes concern the tpm_class_ops of the tis core. AFAICS crb uses its own set of tpm_class_ops which are still used to claim and release the locality. Or do I miss something? Regards, Lino
On Wed, May 11, 2022 at 09:29:57PM +0200, Lino Sanfilippo wrote: > > > On 11.05.22 at 13:27, Jarkko Sakkinen wrote: > > On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote: > >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > >> > >> It is not necessary to claim and release the default locality for each TPM > >> command. Instead claim the locality once at driver startup and release it > >> at driver shutdown. > >> > >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > > > We are doing what we're being because of Intel TXT: > > > > https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/ > > > > Unfortunately cannot accept this change. > > > > I do not see how the patch affects the crb code since the only changes concern the > tpm_class_ops of the tis core. AFAICS crb uses its own set of tpm_class_ops > which are still used to claim and release the locality. > > Or do I miss something? Ugh, yes breaking everything when TXT is used with tpm_tis. > Regards, > Lino BR, Jarkko
On 13.05.22 at 19:59, Jarkko Sakkinen wrote: > On Wed, May 11, 2022 at 09:29:57PM +0200, Lino Sanfilippo wrote: >> >> >> On 11.05.22 at 13:27, Jarkko Sakkinen wrote: >>> On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote: >>>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >>>> >>>> It is not necessary to claim and release the default locality for each TPM >>>> command. Instead claim the locality once at driver startup and release it >>>> at driver shutdown. >>>> >>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >>> >>> We are doing what we're being because of Intel TXT: >>> >>> https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/ >>> >>> Unfortunately cannot accept this change. >>> >> >> I do not see how the patch affects the crb code since the only changes concern the >> tpm_class_ops of the tis core. AFAICS crb uses its own set of tpm_class_ops >> which are still used to claim and release the locality. >> >> Or do I miss something? > > Ugh, yes breaking everything when TXT is used with tpm_tis. > >> Regards, >> Lino > > BR, Jarkko > Ok, I got it now. The idea of this patch was to maintain the locality 0 for the whole driver lifetime, so that we can always be sure that the locality is already claimed when the interrupt status register is read or written in the interrupt handler. But of course this does not work if some other instance like TXT also wants to claim localities. I will try another approach to make sure that the locality is already taken when the irq handler is called and only released after the handler has finished. Regards, Lino
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 52369ef39b03..46f504fb5084 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -638,9 +638,6 @@ static int probe_itpm(struct tpm_chip *chip) if (vendor != TPM_VID_INTEL) return 0; - if (request_locality(chip, 0) != 0) - return -EBUSY; - rc = tpm_tis_send_data(chip, cmd_getticks, len); if (rc == 0) goto out; @@ -659,7 +656,6 @@ static int probe_itpm(struct tpm_chip *chip) out: tpm_tis_ready(chip); - release_locality(chip, priv->locality); return rc; } @@ -721,17 +717,11 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) cap_t cap; int ret; - ret = request_locality(chip, 0); - if (ret < 0) - return ret; - if (chip->flags & TPM_CHIP_FLAG_TPM2) ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); else ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); - release_locality(chip, 0); - return ret; } @@ -855,6 +845,8 @@ void tpm_tis_remove(struct tpm_chip *chip) tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt); + release_locality(chip, 0); + tpm_tis_clkrun_enable(chip, false); if (priv->ilb_base_addr) @@ -925,8 +917,6 @@ static const struct tpm_class_ops tpm_tis = { .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_canceled = tpm_tis_req_canceled, - .request_locality = request_locality, - .relinquish_locality = release_locality, .clk_enable = tpm_tis_clkrun_enable, }; @@ -963,9 +953,15 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, dev_set_drvdata(&chip->dev, priv); + rc = request_locality(chip, 0); + if (rc) + return rc; + rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor); - if (rc < 0) + if (rc < 0) { + release_locality(chip, 0); return rc; + } priv->manufacturer_id = vendor; @@ -978,8 +974,10 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, if (is_bsw()) { priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR, ILB_REMAP_SIZE); - if (!priv->ilb_base_addr) + if (!priv->ilb_base_addr) { + release_locality(chip, 0); return -ENOMEM; + } clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET); /* Check if CLKRUN# is already not enabled in the LPC bus */ @@ -1006,14 +1004,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT; intmask &= ~TPM_GLOBAL_INT_ENABLE; - rc = request_locality(chip, 0); - if (rc < 0) { - rc = -ENODEV; - goto out_err; - } - tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); - release_locality(chip, 0); rc = tpm_chip_start(chip); if (rc) @@ -1072,15 +1063,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, * to make sure it works. May as well use that command to set the * proper timeouts for the driver. */ - - rc = request_locality(chip, 0); - if (rc < 0) - goto out_err; - rc = tpm_get_timeouts(chip); - - release_locality(chip, 0); - if (rc) { dev_err(dev, "Could not get TPM timeouts and durations\n"); rc = -ENODEV; @@ -1169,16 +1152,9 @@ int tpm_tis_resume(struct device *dev) * TPM 1.2 requires self-test on resume. This function actually returns * an error code but for unknown reason it isn't handled. */ - if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) { - ret = request_locality(chip, 0); - if (ret < 0) - return ret; - + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) tpm1_do_selftest(chip); - release_locality(chip, 0); - } - return 0; } EXPORT_SYMBOL_GPL(tpm_tis_resume);