[PATCH] tpm: Remove ineffective wmb() from tpm_pm_resume()

[PATCH] tpm: Remove ineffective wmb() from tpm_pm_resume()

[Richard Lyu]

The comment above the wmb() in tpm_pm_resume() states that the barrier
guarantees TPM_CHIP_FLAG_SUSPENDED is written last, so that hwrng does
not activate before the chip has been fully resumed.  It cannot do so:
it is placed after the store that clears the flag, and therefore does
not order the preceding resume work before that store.  It also has
nothing to pair with, as tpm_try_get_ops() contains no matching read
barrier.

Drop the barrier along with the comment rather than leave a no-op
behind.  Should such ordering turn out to be needed, it would require
paired barriers or locking.

Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Link: https://lore.kernel.org/all/a62fc816-259a-4d28-a265-e425bc17ed50@gmail.com/
Signed-off-by: Richard Lyu <richard.lyu@suse.com>
---
 drivers/char/tpm/tpm-interface.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b4e749e70b02..0bab78c8767c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -497,12 +497,6 @@ int tpm_pm_resume(struct device *dev)
 
 	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();
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
-- 
2.51.0

Re: [PATCH] tpm: Remove ineffective wmb() from tpm_pm_resume()

[Jarkko Sakkinen]

On Tue, Sep 08, 2026 at 05:58:02PM +0800, Richard Lyu wrote:
> The comment above the wmb() in tpm_pm_resume() states that the barrier
> guarantees TPM_CHIP_FLAG_SUSPENDED is written last, so that hwrng does
> not activate before the chip has been fully resumed.  It cannot do so:
> it is placed after the store that clears the flag, and therefore does
> not order the preceding resume work before that store.  It also has
> nothing to pair with, as tpm_try_get_ops() contains no matching read
> barrier.
> 
> Drop the barrier along with the comment rather than leave a no-op
> behind.  Should such ordering turn out to be needed, it would require
> paired barriers or locking.
> 
> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> Link: https://lore.kernel.org/all/a62fc816-259a-4d28-a265-e425bc17ed50@gmail.com/
> Signed-off-by: Richard Lyu <richard.lyu@suse.com>

I agree that wmb() is necessary but what is the bug here?

Agree with the change but I'm not convinced about fixes tag here.

> ---
>  drivers/char/tpm/tpm-interface.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b4e749e70b02..0bab78c8767c 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -497,12 +497,6 @@ int tpm_pm_resume(struct device *dev)
>  
>  	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();
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_pm_resume);
> -- 
> 2.51.0
> 

BR, Jarkko

Re: [PATCH] tpm: Remove ineffective wmb() from tpm_pm_resume()

[Richard Lyu]

On 2026/09/10 01:51, Jarkko Sakkinen wrote:
>On Tue, Sep 08, 2026 at 05:58:02PM +0800, Richard Lyu wrote:
>> The comment above the wmb() in tpm_pm_resume() states that the barrier
>> guarantees TPM_CHIP_FLAG_SUSPENDED is written last, so that hwrng does
>> not activate before the chip has been fully resumed.  It cannot do so:
>> it is placed after the store that clears the flag, and therefore does
>> not order the preceding resume work before that store.  It also has
>> nothing to pair with, as tpm_try_get_ops() contains no matching read
>> barrier.
>>
>> Drop the barrier along with the comment rather than leave a no-op
>> behind.  Should such ordering turn out to be needed, it would require
>> paired barriers or locking.
>>
>> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
>> Link: https://lore.kernel.org/all/a62fc816-259a-4d28-a265-e425bc17ed50@gmail.com/
>> Signed-off-by: Richard Lyu <richard.lyu@suse.com>
>
>I agree that wmb() is necessary but what is the bug here?
>
>Agree with the change but I'm not convinced about fixes tag here.

You're right. There is no actual bug being fixed here, so a Fixes tag is not
needed. This patch only removes redundant code. I will send a v2 without the
tag.

Best Regards,
Richard Lyu

[PATCH v2] tpm: Remove ineffective wmb() from tpm_pm_resume()

[Richard Lyu]

The comment above the wmb() in tpm_pm_resume() states that the barrier
guarantees TPM_CHIP_FLAG_SUSPENDED is written last, so that hwrng does
not activate before the chip has been fully resumed.  It cannot do so:
it is placed after the store that clears the flag, and therefore does
not order the preceding resume work before that store.  It also has
nothing to pair with, as tpm_try_get_ops() contains no matching read
barrier.

Drop the barrier along with the comment rather than leave a no-op
behind.  Should such ordering turn out to be needed, it would require
paired barriers or locking.

Link: https://lore.kernel.org/all/a62fc816-259a-4d28-a265-e425bc17ed50@gmail.com/
Signed-off-by: Richard Lyu <richard.lyu@suse.com>

---
v2:
- Drop the Fixes tag: this is a cleanup, not a bug fix.
- Link to v1: https://lore.kernel.org/all/aqIVn0cDBvIYKggE@r1chard/
---
 drivers/char/tpm/tpm-interface.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b4e749e70b02..0bab78c8767c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -497,12 +497,6 @@ int tpm_pm_resume(struct device *dev)
 
 	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();
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
-- 
2.51.0