diff mbox series

[v4,2/6] tpm, tpm_tis: Claim and release locality only once

Message ID 20220509080559.4381-3-LinoSanfilippo@gmx.de (mailing list archive)
State New
Headers show
Series TPM irq fixes | expand

Commit Message

Lino Sanfilippo May 9, 2022, 8:05 a.m. UTC
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>
---
 drivers/char/tpm/tpm_tis_core.c | 50 +++++++++------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

Comments

Jarkko Sakkinen May 11, 2022, 11:27 a.m. UTC | #1
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
Lino Sanfilippo May 11, 2022, 7:29 p.m. UTC | #2
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
Jarkko Sakkinen May 13, 2022, 5:59 p.m. UTC | #3
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
Lino Sanfilippo May 16, 2022, 8:23 p.m. UTC | #4
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 mbox series

Patch

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);