Re: [PATCH v4 6/6] tpm, tpm_tis: Only enable supported IRQs

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: peterhuewe@gmx.de, jgg@ziepe.ca, stefanb@linux.vnet.ibm.com,
	linux@mniewoehner.de, linux-integrity@vger.kernel.org,
	linux-kernel@vger.kernel.org, lukas@wunner.de,
	p.rosenberger@kunbus.com,
	Lino Sanfilippo <l.sanfilippo@kunbus.com>
Subject: Re: [PATCH v4 6/6] tpm, tpm_tis: Only enable supported IRQs
Date: Wed, 11 May 2022 21:58:00 +0200	[thread overview]
Message-ID: <e5081429-d313-ade2-0dda-b7ac88ebf4f9@gmx.de> (raw)
In-Reply-To: <YnvRXiMxMRF3mIb8@kernel.org>

On 11.05.22 at 17:08, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:59AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> Instead of blindly trying to enable all possible interrupts, use the result
>> from the capability query and request only the interrupts that are actually
>> supported.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 67 ++++++++++++++++++---------------
>>  drivers/char/tpm/tpm_tis_core.h |  1 +
>>  2 files changed, 37 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 4c65718feb7d..784e153e2895 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -976,13 +976,46 @@ 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) {
>> +		priv->supported_irqs |= 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) {
>> +		priv->supported_irqs |= TPM_INTF_LOCALITY_CHANGE_INT;
>> +		dev_dbg(dev, "\tLocality Change Int Support\n");
>> +	}
>> +	if (intfcaps & TPM_INTF_STS_VALID_INT) {
>> +		priv->supported_irqs |= TPM_INTF_STS_VALID_INT;
>> +		dev_dbg(dev, "\tSts Valid Int Support\n");
>> +	}
>> +	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
>> +		priv->supported_irqs |= TPM_INTF_DATA_AVAIL_INT;
>> +		dev_dbg(dev, "\tData Avail Int Support\n");
>> +	}
>> +
>>  	/* Take control of the TPM's interrupt hardware and shut it off */
>>  	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>>  	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;
>> +	intmask |= priv->supported_irqs;
>>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>>
>>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>> @@ -1009,32 +1042,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);
>> @@ -1101,9 +1108,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>>  	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->supported_irqs | 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 c8972ea8e13e..3d6b05c6fdba 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -97,6 +97,7 @@ struct tpm_tis_data {
>>  	u16 manufacturer_id;
>>  	int locality;
>>  	int irq;
>> +	unsigned int supported_irqs;
>>  	unsigned long irqtest_flags;
>>  	unsigned long flags;
>>  	void __iomem *ilb_base_addr;
>> --
>> 2.36.0
>>
>
> Does the existing code cause issues in a some specific environment?
>
> BR, Jarkko
>

Not that I know of. This patch is not supposed to be a bugfix but an improvement of
the existing code by using the information about supported interrupts which is collected during
the capability query. After the query we know exactly which irqs are supported, so why not use
this knowledge when setting the irq mask?

Regards,
Lino