Re: [PATCH v5 01/10] tpm, tpm_tis: Avoid cache incoherency in test for interrupts

[Lino Sanfilippo]

Hi,

On 15.06.22 at 16:32, Jarkko Sakkinen wrote:
> On Fri, Jun 10, 2022 at 01:08:37PM +0200, LinoSanfilippo@xxxxxx wrote:
>> From: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
>>
>> The interrupt handler that sets the boolean variable irq_tested may run on
>> another CPU as the thread that checks irq_tested as part of the irq test in
>> tmp_tis_send().
>>
>> Since nothing guarantees cache coherency between CPUs for unsynchronized
>> accesses to boolean variables the testing thread might not perceive the
>> value change done in the interrupt handler.
>>
>> Avoid this issue by using a bitfield instead of a boolean variable and by
>> accessing this field with the bit manipulating functions that provide cache
>> coherency.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++------
>>  drivers/char/tpm/tpm_tis_core.h |  6 +++++-
>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index dc56b976d816..6f2cf75add8b 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>  	int rc, irq;
>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>
>> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
>> +	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
>> +	     test_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags))
>>  		return tpm_tis_send_main(chip, buf, len);
>>
>>  	/* Verify receipt of the expected IRQ */
>> @@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>  	rc = tpm_tis_send_main(chip, buf, len);
>>  	priv->irq = irq;
>>  	chip->flags |= TPM_CHIP_FLAG_IRQ;
>> -	if (!priv->irq_tested)
>> +	if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags))
>>  		tpm_msleep(1);
>> -	if (!priv->irq_tested)
>> +	if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags))
>>  		disable_interrupts(chip);
>> -	priv->irq_tested = true;
>> +	set_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags);
>>  	return rc;
>>  }
>>
>> @@ -693,7 +694,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>>  	if (interrupt == 0)
>>  		return IRQ_NONE;
>>
>> -	priv->irq_tested = true;
>> +	set_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags);
>>  	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
>>  		wake_up_interruptible(&priv->read_queue);
>>  	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
>> @@ -779,7 +780,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>  	if (rc < 0)
>>  		return rc;
>>
>> -	priv->irq_tested = false;
>> +	clear_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags);
>>
>>  	/* Generate an interrupt by having the core call through to
>>  	 * tpm_tis_send
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index 3be24f221e32..0f29d0b68c3e 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -88,11 +88,15 @@ enum tpm_tis_flags {
>>  	TPM_TIS_INVALID_STATUS		= BIT(1),
>>  };
>>
>> +enum tpm_tis_irqtest_flags {
>> +	TPM_TIS_IRQ_TESTED		= BIT(0),
>> +};
>> +
>>  struct tpm_tis_data {
>>  	u16 manufacturer_id;
>>  	int locality;
>>  	int irq;
>> -	bool irq_tested;
>> +	unsigned long irqtest_flags;
>>  	unsigned long flags;
>>  	void __iomem *ilb_base_addr;
>>  	u16 clkrun_enabled;
>> --
>> 2.36.1
>>
>
> Otherwise looks fine, but please add TPM_TIS_IRQ_TESTED to 'flags', and
> convert existing sites to use set_bit() and and test_bit().
>
> BR, Jarkko
>

Ok, will do.

Regards,
Lino