Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume

>> memory reordering between the wake up and clearing the flag is allowed.

When exactly can this reordering happen?

>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 12b7394b34bd..7f500797b7a7 100644 --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c @@ -173,6 +173,9 @@ int
>> tpm_try_get_ops(struct tpm_chip *chip) if (chip->flags &
>> TPM_CHIP_FLAG_SUSPENDED) goto out_lock;
>>
>> +	/* Ensure that device is fully resumed */
>> +	rmb();
>> +
>>  	rc = tpm_chip_start(chip);
>>  	if (rc)
>>  		goto out_lock;

Where inside tpm_chip_start do we actually need to avoid loading a 
stale state or flag?

>> diff --git a/drivers/char/tpm/tpm-interface.c
>> b/drivers/char/tpm/tpm-interface.c index f745a098908b..2de12b02f62b 100644
>> --- a/drivers/char/tpm/tpm-interface.c +++
>> b/drivers/char/tpm/tpm-interface.c @@ -474,13 +474,12 @@ int
>> tpm_pm_resume(struct device *dev) if (chip == NULL) return -ENODEV;
>>
>> -	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>> -
>>  	/*
>>  	 * Guarantee that SUSPENDED is written last, so that hwrng does not
>>  	 * activate before the chip has been fully resumed.
>>  	 */
>>  	wmb();
>> +	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>
>Can you rationalize this change?

I agree the clearing of the flag should be moved after wmb() to guarantee 
that SUSPENDED is written last, the flag has to be cleared after the barrier.
That part makes sense.
  
My remaining question is whether we actually need the wmb() barrier here?