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

[Thomas Fourier <fourier.thomas@gmail.com>]

Thank you all for your time and comments. Revisiting my patch after my 
vacation, I now think none of the barriers are actually necessary.

On 21/08/2026 11:40, Richard Lyu wrote:
>>> 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?
tpm_chip_start() reads chip->locality for example.  I'm not sure it can 
be written to concurrently but some drivers write to it. It might not be 
a problem as it would just trigger a call to tpm_request_locality().

Since speculative writes are not possible, so rmb() is sufficient; in 
any case, no need for a read-to-write memory barrier.
> 
>>> 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?
This change is mostly based on your comment: tpm_pm_resume() is called 
after the hardware-specific operations have been performed, and the flag 
must be set after these operations are complete.

I assumed hwrng corresponds to tpm_hwrng_read() which calls 
tpm_get_random() which itself calls tpm_try_get_ops(). After 
tpm_chip_start() is called, tpm_try_get_ops() can return a valid 
pointer, and tpm_hwrng_read() can work.

Individual drivers implement the .resume() method by doing 
hardware-specific operations then calling tpm_pm_resume(), so ordering 
needs to happen between those hardware-specific operations and 
tpm_pm_resume().  This ensures that the order of operations is:
   - hardware-specific resume operations,
   - tpm_pm_resume(), including clearing the TPM_CHIP_FLAG_SUSPENDED flag,
   - check that TPM_CHIP_FLAG_SUSPENDED is clear,
   - run of tpm_chip_start().

If tpm_try_get_ops() succeeds, then tpm_chip_start() is run so the 
hardware-specific resume operations are complete.

> 
> 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?
Revisiting the patch, I agree that this point is not fully clear. 
Looking at the implementations of tpm drivers, most use the 
tpm_pm_resume() function directly as the .resume() method.

Three drivers implement specific .resume() methods:
   - tpm_infineon.c: which uses writeb/outb that adds the necessary 
barriers,
   - st33zp24/st33zp24.c: which calls tpm_pm_resume() without 
driver-specific operation depending on the config,
   - tpm_tis_core.c: which makes a self-test before calling tpm_pm_resume().

None of those cases seem to require a barrier at all.