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
