diff mbox series

[v5,05/10] tpm, tpm_tis: Store result of interrupt capability query

Message ID 20220610110846.8307-6-LinoSanfilippo@gmx.de (mailing list archive)
State New, archived
Headers show
Series TPM IRQ fixes | expand

Commit Message

Lino Sanfilippo June 10, 2022, 11:08 a.m. UTC 
From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

According to the TPM Interface Specification (TIS) support for "stsValid"
and "commandReady" interrupts is only optional.
This has to be taken into account when handling the interrupts in functions
like wait_for_tpm_stat(). So query the set of supported interrupts and
store it in a global variable so that it can be accessed later.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 73 +++++++++++++++++----------------
 drivers/char/tpm/tpm_tis_core.h |  1 +
 2 files changed, 38 insertions(+), 36 deletions(-)

Comments

Jarkko Sakkinen June 15, 2022, 6:18 p.m. UTC | #1 
On Fri, Jun 10, 2022 at 01:08:41PM +0200, LinoSanfilippo@gmx.de wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> According to the TPM Interface Specification (TIS) support for "stsValid"
> and "commandReady" interrupts is only optional.
> This has to be taken into account when handling the interrupts in functions
> like wait_for_tpm_stat(). So query the set of supported interrupts and
> store it in a global variable so that it can be accessed later.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 73 +++++++++++++++++----------------
>  drivers/char/tpm/tpm_tis_core.h |  1 +
>  2 files changed, 38 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 718525fcadc0..2f03fefa1706 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1007,8 +1007,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	if (rc < 0)
>  		goto out_err;
>  
> -	intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
> -		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
> +	/* Figure out the capabilities */
> +	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
> +	if (rc < 0)
> +		goto out_err;
> +
> +	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
> +		intfcaps);
> +	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> +		dev_dbg(dev, "\tBurst Count Static\n");
> +	if (intfcaps & TPM_INTF_CMD_READY_INT) {
> +		intmask |= TPM_INTF_CMD_READY_INT;
> +		dev_dbg(dev, "\tCommand Ready Int Support\n");
> +	}
> +	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> +		dev_dbg(dev, "\tInterrupt Edge Falling\n");
> +	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> +		dev_dbg(dev, "\tInterrupt Edge Rising\n");
> +	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> +		dev_dbg(dev, "\tInterrupt Level Low\n");
> +	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> +		dev_dbg(dev, "\tInterrupt Level High\n");
> +	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> +		intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
> +		dev_dbg(dev, "\tLocality Change Int Support\n");
> +	if (intfcaps & TPM_INTF_STS_VALID_INT) {
> +		intmask |= TPM_INTF_STS_VALID_INT;
> +		dev_dbg(dev, "\tSts Valid Int Support\n");
> +	}
> +	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
> +		intmask |= TPM_INTF_DATA_AVAIL_INT;
> +		dev_dbg(dev, "\tData Avail Int Support\n");
> +	}
> +
>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>  
>  	rc = request_locality(chip, 0);
> @@ -1042,32 +1073,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		goto out_err;
>  	}
>  
> -	/* Figure out the capabilities */
> -	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
> -	if (rc < 0)
> -		goto out_err;
> -
> -	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
> -		intfcaps);
> -	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> -		dev_dbg(dev, "\tBurst Count Static\n");
> -	if (intfcaps & TPM_INTF_CMD_READY_INT)
> -		dev_dbg(dev, "\tCommand Ready Int Support\n");
> -	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> -		dev_dbg(dev, "\tInterrupt Edge Falling\n");
> -	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> -		dev_dbg(dev, "\tInterrupt Edge Rising\n");
> -	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> -		dev_dbg(dev, "\tInterrupt Level Low\n");
> -	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> -		dev_dbg(dev, "\tInterrupt Level High\n");
> -	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> -		dev_dbg(dev, "\tLocality Change Int Support\n");
> -	if (intfcaps & TPM_INTF_STS_VALID_INT)
> -		dev_dbg(dev, "\tSts Valid Int Support\n");
> -	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
> -		dev_dbg(dev, "\tData Avail Int Support\n");
> -
>  	/* INTERRUPT Setup */
>  	init_waitqueue_head(&priv->read_queue);
>  	init_waitqueue_head(&priv->int_queue);
> @@ -1098,7 +1103,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		else
>  			tpm_tis_probe_irq(chip, intmask);
>  
> -		if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> +		if (chip->flags & TPM_CHIP_FLAG_IRQ) {
> +			priv->irqs_in_use = intmask;
> +		} else {
>  			dev_err(&chip->dev, FW_BUG
>  					"TPM interrupt not working, polling instead\n");
>  
> @@ -1145,13 +1152,7 @@ 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;
> +	intmask = priv->irqs_in_use | TPM_GLOBAL_INT_ENABLE;
>  
>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>  
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 0f29d0b68c3e..8e02faa4079d 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -96,6 +96,7 @@ struct tpm_tis_data {
>  	u16 manufacturer_id;
>  	int locality;
>  	int irq;
> +	unsigned int irqs_in_use;
>  	unsigned long irqtest_flags;
>  	unsigned long flags;
>  	void __iomem *ilb_base_addr;
> -- 
> 2.36.1
> 

int_mask would be imho a better name.

Can you squash this and the following patch? It's good to slice in
small patches but here I think it would make sense to tie the field
to use case, in order for it to make sense.

BR, Jarkko
Lino Sanfilippo June 15, 2022, 10:07 p.m. UTC | #2 
On 15.06.22 at 20:18, Jarkko Sakkinen wrote:
> On Fri, Jun 10, 2022 at 01:08:41PM +0200, LinoSanfilippo@gmx.de wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> According to the TPM Interface Specification (TIS) support for "stsValid"
>> and "commandReady" interrupts is only optional.
>> This has to be taken into account when handling the interrupts in functions
>> like wait_for_tpm_stat(). So query the set of supported interrupts and
>> store it in a global variable so that it can be accessed later.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 73 +++++++++++++++++----------------
>>  drivers/char/tpm/tpm_tis_core.h |  1 +
>>  2 files changed, 38 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 718525fcadc0..2f03fefa1706 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -1007,8 +1007,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  	if (rc < 0)
>>  		goto out_err;
>>
>> -	intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
>> -		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
>> +	/* Figure out the capabilities */
>> +	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
>> +	if (rc < 0)
>> +		goto out_err;
>> +
>> +	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
>> +		intfcaps);
>> +	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
>> +		dev_dbg(dev, "\tBurst Count Static\n");
>> +	if (intfcaps & TPM_INTF_CMD_READY_INT) {
>> +		intmask |= TPM_INTF_CMD_READY_INT;
>> +		dev_dbg(dev, "\tCommand Ready Int Support\n");
>> +	}
>> +	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
>> +		dev_dbg(dev, "\tInterrupt Edge Falling\n");
>> +	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
>> +		dev_dbg(dev, "\tInterrupt Edge Rising\n");
>> +	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
>> +		dev_dbg(dev, "\tInterrupt Level Low\n");
>> +	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
>> +		dev_dbg(dev, "\tInterrupt Level High\n");
>> +	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
>> +		intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
>> +		dev_dbg(dev, "\tLocality Change Int Support\n");
>> +	if (intfcaps & TPM_INTF_STS_VALID_INT) {
>> +		intmask |= TPM_INTF_STS_VALID_INT;
>> +		dev_dbg(dev, "\tSts Valid Int Support\n");
>> +	}
>> +	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
>> +		intmask |= TPM_INTF_DATA_AVAIL_INT;
>> +		dev_dbg(dev, "\tData Avail Int Support\n");
>> +	}
>> +
>>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>>
>>  	rc = request_locality(chip, 0);
>> @@ -1042,32 +1073,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  		goto out_err;
>>  	}
>>
>> -	/* Figure out the capabilities */
>> -	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
>> -	if (rc < 0)
>> -		goto out_err;
>> -
>> -	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
>> -		intfcaps);
>> -	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
>> -		dev_dbg(dev, "\tBurst Count Static\n");
>> -	if (intfcaps & TPM_INTF_CMD_READY_INT)
>> -		dev_dbg(dev, "\tCommand Ready Int Support\n");
>> -	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
>> -		dev_dbg(dev, "\tInterrupt Edge Falling\n");
>> -	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
>> -		dev_dbg(dev, "\tInterrupt Edge Rising\n");
>> -	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
>> -		dev_dbg(dev, "\tInterrupt Level Low\n");
>> -	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
>> -		dev_dbg(dev, "\tInterrupt Level High\n");
>> -	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
>> -		dev_dbg(dev, "\tLocality Change Int Support\n");
>> -	if (intfcaps & TPM_INTF_STS_VALID_INT)
>> -		dev_dbg(dev, "\tSts Valid Int Support\n");
>> -	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
>> -		dev_dbg(dev, "\tData Avail Int Support\n");
>> -
>>  	/* INTERRUPT Setup */
>>  	init_waitqueue_head(&priv->read_queue);
>>  	init_waitqueue_head(&priv->int_queue);
>> @@ -1098,7 +1103,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  		else
>>  			tpm_tis_probe_irq(chip, intmask);
>>
>> -		if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>> +		if (chip->flags & TPM_CHIP_FLAG_IRQ) {
>> +			priv->irqs_in_use = intmask;
>> +		} else {
>>  			dev_err(&chip->dev, FW_BUG
>>  					"TPM interrupt not working, polling instead\n");
>>
>> @@ -1145,13 +1152,7 @@ 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;
>> +	intmask = priv->irqs_in_use | TPM_GLOBAL_INT_ENABLE;
>>
>>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index 0f29d0b68c3e..8e02faa4079d 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -96,6 +96,7 @@ struct tpm_tis_data {
>>  	u16 manufacturer_id;
>>  	int locality;
>>  	int irq;
>> +	unsigned int irqs_in_use;
>>  	unsigned long irqtest_flags;
>>  	unsigned long flags;
>>  	void __iomem *ilb_base_addr;
>> --
>> 2.36.1
>>
>
> int_mask would be imho a better name.
>
> Can you squash this and the following patch? It's good to slice in
> small patches but here I think it would make sense to tie the field
> to use case, in order for it to make sense.
>

Sure, will do so.

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 718525fcadc0..2f03fefa1706 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1007,8 +1007,39 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	if (rc < 0)
 		goto out_err;
 
-	intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
-		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+	/* Figure out the capabilities */
+	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
+	if (rc < 0)
+		goto out_err;
+
+	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
+		intfcaps);
+	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
+		dev_dbg(dev, "\tBurst Count Static\n");
+	if (intfcaps & TPM_INTF_CMD_READY_INT) {
+		intmask |= TPM_INTF_CMD_READY_INT;
+		dev_dbg(dev, "\tCommand Ready Int Support\n");
+	}
+	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
+		dev_dbg(dev, "\tInterrupt Edge Falling\n");
+	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
+		dev_dbg(dev, "\tInterrupt Edge Rising\n");
+	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
+		dev_dbg(dev, "\tInterrupt Level Low\n");
+	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
+		dev_dbg(dev, "\tInterrupt Level High\n");
+	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
+		intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
+		dev_dbg(dev, "\tLocality Change Int Support\n");
+	if (intfcaps & TPM_INTF_STS_VALID_INT) {
+		intmask |= TPM_INTF_STS_VALID_INT;
+		dev_dbg(dev, "\tSts Valid Int Support\n");
+	}
+	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
+		intmask |= TPM_INTF_DATA_AVAIL_INT;
+		dev_dbg(dev, "\tData Avail Int Support\n");
+	}
+
 	intmask &= ~TPM_GLOBAL_INT_ENABLE;
 
 	rc = request_locality(chip, 0);
@@ -1042,32 +1073,6 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		goto out_err;
 	}
 
-	/* Figure out the capabilities */
-	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
-	if (rc < 0)
-		goto out_err;
-
-	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
-		intfcaps);
-	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
-		dev_dbg(dev, "\tBurst Count Static\n");
-	if (intfcaps & TPM_INTF_CMD_READY_INT)
-		dev_dbg(dev, "\tCommand Ready Int Support\n");
-	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
-		dev_dbg(dev, "\tInterrupt Edge Falling\n");
-	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
-		dev_dbg(dev, "\tInterrupt Edge Rising\n");
-	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
-		dev_dbg(dev, "\tInterrupt Level Low\n");
-	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
-		dev_dbg(dev, "\tInterrupt Level High\n");
-	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
-		dev_dbg(dev, "\tLocality Change Int Support\n");
-	if (intfcaps & TPM_INTF_STS_VALID_INT)
-		dev_dbg(dev, "\tSts Valid Int Support\n");
-	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
-		dev_dbg(dev, "\tData Avail Int Support\n");
-
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&priv->read_queue);
 	init_waitqueue_head(&priv->int_queue);
@@ -1098,7 +1103,9 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		else
 			tpm_tis_probe_irq(chip, intmask);
 
-		if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+		if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+			priv->irqs_in_use = intmask;
+		} else {
 			dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
 
@@ -1145,13 +1152,7 @@  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;
+	intmask = priv->irqs_in_use | TPM_GLOBAL_INT_ENABLE;
 
 	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
 
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 0f29d0b68c3e..8e02faa4079d 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -96,6 +96,7 @@  struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
+	unsigned int irqs_in_use;
 	unsigned long irqtest_flags;
 	unsigned long flags;
 	void __iomem *ilb_base_addr;