diff mbox series

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

Message ID 20220610110846.8307-2-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>

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@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++------
 drivers/char/tpm/tpm_tis_core.h |  6 +++++-
 2 files changed, 12 insertions(+), 7 deletions(-)

Comments

Jarkko Sakkinen June 15, 2022, 2:32 p.m. UTC | #1 
On Fri, Jun 10, 2022 at 01:08:37PM +0200, LinoSanfilippo@gmx.de wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> 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@kunbus.com>
> ---
>  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
Lino Sanfilippo June 15, 2022, 10:06 p.m. UTC | #2 
Hi,

On 15.06.22 at 16:32, Jarkko Sakkinen wrote:
> On Fri, Jun 10, 2022 at 01:08:37PM +0200, LinoSanfilippo@gmx.de wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> 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@kunbus.com>
>> ---
>>  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
diff mbox series

Patch

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;