From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 52F213EC6B9 for ; Tue, 25 Aug 2026 09:42:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787650937; cv=none; b=rwQUU3ia5j1Jh7jxGCFr37jkwrGxi9Yc26GWKSZIpe+5GG4k7f4QS6/R1aXMOU6DBmodYArMoMn6qiHqWYiumDHG2A7AUhinK54+wZczWmEWJfw93deGU3COalxFYH8ZrGWr07dqAoErwOt5wNGNrxA/cvNWemllOuS7Lq9k1dc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787650937; c=relaxed/simple; bh=TRCjrVT6a0nm5nOjYaRUYlFXYFxAVmK1oCXXhDKXrQU=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=iGfD4XpblP0wvkSdNIrFEzLK+OG0dISOFW5pKZ4310c7hXuwGxwBvrsKsh4/CAbpBTz9sKx8oRnu6u6UWlpNbAcjIiNeHz03kFdGTQx6nA2LTnLJE2kOOdAg+w1l1sgawP7Gp45MVSy9QTmzpHQkZmORhpM01Nh7xyrTxz7gJ5k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=nJkQrKGH; arc=none smtp.client-ip=209.85.128.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="nJkQrKGH" Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-4957952e0f8so2825345e9.2 for ; Tue, 25 Aug 2026 02:42:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1787650933; x=1788255733; darn=vger.kernel.org; h=content-transfer-encoding:content-type:in-reply-to:content-language :references:cc:to:subject:from:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to:content-type; bh=qmTx7tD1rUFhIsjzkYVNdRt+/Y88by0P9eG+nC4RYUI=; b=nJkQrKGHMg+QqYc9gXw8NbGWR+jTs3nnBYuDucUOfqom4ztT9+UOAFrhGuL5NzbEXo 4xKAbV9XzDi2DUI1V/Ifzx+5u6nMDZ+7b1fjgF9yhtW8wj8d+dtKVJk73p6B26Ch9HCj fYSJ6WK8j03C2mhN/tlGx+QYP1T5ohuzx2xDjuXjEaI2uLt4mZDjHR0DD2LvKFiAkt0T rwAEIXveV0gvJf2YC/sVwFCfb+Py3swp7hmE94i9xnPPAmBvuucuT2PGrD4kmkA0X/9Z zpS16GY/F/j0q0W4Qck0hDvrfXO2pTOq9IQVklBjAlTMFJGzT2+SbH9vzddTcWuMZED2 id5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1787650933; x=1788255733; h=content-transfer-encoding:content-type:in-reply-to:content-language :references:cc:to:subject:from:user-agent:mime-version:date :message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=qmTx7tD1rUFhIsjzkYVNdRt+/Y88by0P9eG+nC4RYUI=; b=CZqC+q5q9o/dIxygu6htylGs2GJVLceFfdOMZWyXYHPNvT29lUd/ZQorXskrAEKg7n I2eSqVvduD2c+z3i9lZDYs29Yyi4zt2OAYFddR3YKKqRBJtH4L1X+BEDzz6Od9t0JQl9 vct013gHLmdJWiOgpImbl8V2XYjtoCTCN/Q69Wfx/fByNGi/zCn9nmZ1M+gdkjzOfYlv Koy+k+AMf9mIOFG/PDQkWHRElxZD42AtiZ/3bF2e56Li/yq4tK0PkyOl8AH2knyZDYB6 qYuOABsA8H99qcb5aCWCAaICagqHl/TyOjtwHEP8kXmz7584TuM8amK7DT8PeXF7CR7N /kNQ== X-Forwarded-Encrypted: i=1; AHgh+RrMLMQe7PX39ktNpUQK0Nd3hpc2w2nju/OpLeMW026lgGc1FQCjOhdPtsWRgj5jTNvCybKH01SJso89tX8=@vger.kernel.org X-Gm-Message-State: AFuF++mHofxXLhHa/dYTTAryaljYyGyt3T30LaQrR548elTSPCa6KEOh w2KbmtYFCWamxW+DSY0IXWD10JTkqoJFXJDEYxiz5pNiuLdVfG+t7PLb X-Gm-Gg: AR+sD11jdJFXbXhFa+qkf7/oa0Irvj3jbsoGyIWOxZv4w8/CNYMFWorXqnp+UpShxp8 i7SyHp1wCETg/neMmpfcULMLD4+pPcDOGXFHbYs+jriJgEPUnd6oh3yRs+X0ex71dKIqxtVtb2P e2q02D9wktMxSfAJCTjmciFFzgL9FJk6kx1qeaBNIpf4/lvWLTKCENKM10IH5xE3lUQkgp3EFH0 RQaVbE9N+vlWckC1tWXCrOv19nkQKIcEWzfS1cJVFthG65cGWIkcJALd1DCOn+88+aGYJpucBJS GNmfc4rAMExSlr0LE4EHScTvHGeiNqAX8GzLGWyZwRSdtbaJNRQ+eGl2mBp8GE+LqupyZNhytUc e/ArR3jk4V6Bsz8cI3zQ6b8PX2tiNGAOHqZ4dIzopniP8cgFCWz4iAQwBbyUsav2HO1Qm3o5qyY 1JNOg8dFnWSlc64yF8aXhS8Zk13llUUCyUSmxpIfGtLWMJqb6LxlQsqbGu3oE6/VkkQ6Yu6BGVl obNwfjHXJ7hmLnzamEuqDNahk/+V8I= X-Received: by 2002:a05:600c:1392:b0:499:a660:f4ae with SMTP id 5b1f17b1804b1-499b8353024mr204215045e9.1.1787650933330; Tue, 25 Aug 2026 02:42:13 -0700 (PDT) Received: from [128.93.83.149] (wifi-pro-83-149.paris.inria.fr. [128.93.83.149]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-499d63553d6sm23460235e9.8.2026.08.25.02.42.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 25 Aug 2026 02:42:13 -0700 (PDT) Message-ID: Date: Tue, 25 Aug 2026 11:42:12 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Thomas Fourier Subject: Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume To: Richard Lyu , Jarkko Sakkinen Cc: stable@vger.kernel.org, Peter Huewe , Jason Gunthorpe , Jerry Snitselaar , "open list:TPM DEVICE DRIVER" , open list References: <20260803092240.18348-2-fourier.thomas@gmail.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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.