[PATCH] PCI/PTM: Keep ptm_enable_cnt balanced when pci_enable_ptm() fails

[PATCH] PCI/PTM: Keep ptm_enable_cnt balanced when pci_enable_ptm() fails

[Li Yifan]

Since commit e1092d5e15e6 ("PCI/PTM: Do not enable PTM automatically
for Root and Switch Upstream Ports"), pci_enable_ptm() takes a
reference on the device and on each port of the upstream path, and
pci_disable_ptm() releases them. If pci_enable_ptm() fails, the counts
can become incorrect in two ways:

1. pci_disable_ptm() decrements dev->ptm_enable_cnt without a check.
   Many drivers ignore the return value of pci_enable_ptm() and call
   pci_disable_ptm() unconditionally, for example mlx5 and bnxt. If
   pci_enable_ptm() failed, for example because the upstream port does
   not support PTM, the count becomes -1 and stays negative after the
   driver unbinds. Then pcie_ptm_enabled() returns true, and
   pci_resume_ptm() can enable PTM on the device after a resume,
   although the upstream path does not support PTM. Before
   e1092d5e15e6, pci_disable_ptm() did nothing if PTM was not enabled.

2. If __pci_enable_ptm() fails for the device after the upstream path
   is enabled, pci_enable_ptm() drops the reference on the device, but
   it keeps the references on the upstream ports. For example, this
   occurs if the device has no PTM Capability and its upstream port
   has one. Callers that check pcie_ptm_enabled() before
   pci_disable_ptm() never release these references, so the upstream
   ports keep PTM enabled with no user. Callers that call
   pci_disable_ptm() unconditionally release them only through the
   underflow in 1.

Fix both problems in one change, because a fix for only one of them
makes the other one worse. Release the upstream references when
__pci_enable_ptm() fails. Make pci_disable_ptm() do nothing if PTM is
not enabled for the device, and use atomic_dec_if_positive() so that
the check and the decrement are one atomic operation.

The fix for 2 is based on a patch from Wentao Liang [1].

Fixes: e1092d5e15e6 ("PCI/PTM: Do not enable PTM automatically for Root and Switch Upstream Ports")
Cc: stable@vger.kernel.org
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/20261102222817.GA983941@bhelgaas
Link: https://lore.kernel.org/r/20260616141733.1688264-1-vulab@iscas.ac.cn # [1]
Signed-off-by: Li Yifan <yifan2.li@intel.com>
---
Wentao's patch [1] fixes only problem 2, and it is not applied. It
does not build on v7.4-rc1 ("parent" is declared inside the
"if (!dev->ptm_root)" block), and with only that fix, callers that
call pci_disable_ptm() unconditionally release the upstream
references twice. This patch includes his fix with the build fix.
Wentao, if you agree, please reply with your Signed-off-by, and I will
add you as Co-developed-by.

Tested on v7.2.7 with this patch (it applies without change), on a Nova
Lake-S system with an I226-LM (81:00.0, no driver bound) behind Root
Port 80:1d.0. A test module calls pci_enable_ptm() and
pci_disable_ptm() three times and reads ptm_enable_cnt of both devices.
It makes pci_enable_ptm() fail in two ways: it hides the PTM Capability
of the Root Port (no upstream PTM), or it clears ptm_requester of the
I226 (the device fails after the upstream path is enabled). "Guarded"
means that pci_disable_ptm() is called only if pcie_ptm_enabled().

                                  without patch        with patch
  enable succeeds                 dev 1/0, RP 1/0      same
  no upstream PTM, unconditional  dev -1, -2, -3       dev 0
  no upstream PTM, guarded        dev 0                dev 0
  device fails, unconditional     dev -1, -2, -3       dev 0, RP 0
  device fails, guarded           RP 1, 2, 3 (PTM on)  dev 0, RP 0

 drivers/pci/pcie/ptm.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index bd3bd39f6372..522975c87bd9 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -171,6 +171,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
  */
 int pci_enable_ptm(struct pci_dev *dev)
 {
+	struct pci_dev *parent = NULL;
 	int rc;
 	char clock_desc[8];
 
@@ -183,8 +184,6 @@ int pci_enable_ptm(struct pci_dev *dev)
 	 * we can enable PTM.
 	 */
 	if (!dev->ptm_root) {
-		struct pci_dev *parent;
-
 		parent = pci_upstream_ptm(dev);
 		if (!parent)
 			return -EINVAL;
@@ -201,6 +200,9 @@ int pci_enable_ptm(struct pci_dev *dev)
 	rc = __pci_enable_ptm(dev);
 	if (rc) {
 		atomic_dec(&dev->ptm_enable_cnt);
+		/* Release the references taken on the upstream path above */
+		if (parent)
+			pci_disable_ptm(parent);
 		return rc;
 	}
 
@@ -240,13 +242,18 @@ static void __pci_disable_ptm(struct pci_dev *dev)
  * pci_disable_ptm() - Disable Precision Time Measurement
  * @dev: PCI device
  *
- * Disable Precision Time Measurement for @dev.
+ * Disable Precision Time Measurement for @dev.  Do nothing if PTM is not
+ * enabled for @dev, for example because pci_enable_ptm() failed.
  */
 void pci_disable_ptm(struct pci_dev *dev)
 {
 	struct pci_dev *parent;
+	int cnt;
 
-	if (atomic_dec_and_test(&dev->ptm_enable_cnt))
+	cnt = atomic_dec_if_positive(&dev->ptm_enable_cnt);
+	if (cnt < 0)
+		return;
+	if (cnt == 0)
 		__pci_disable_ptm(dev);
 
 	parent = pci_upstream_ptm(dev);

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

Re: [PATCH] PCI/PTM: Keep ptm_enable_cnt balanced when pci_enable_ptm() fails

[sashiko-bot]

> Since commit e1092d5e15e6 ("PCI/PTM: Do not enable PTM automatically
> for Root and Switch Upstream Ports"), pci_enable_ptm() takes a
> reference on the device and on each port of the upstream path, and
> pci_disable_ptm() releases them. If pci_enable_ptm() fails, the counts
> can become incorrect in two ways:
> 
> 1. pci_disable_ptm() decrements dev->ptm_enable_cnt without a check.
>    Many drivers ignore the return value of pci_enable_ptm() and call
>    pci_disable_ptm() unconditionally, for example mlx5 and bnxt. If
>    pci_enable_ptm() failed, for example because the upstream port does
>    not support PTM, the count becomes -1 and stays negative after the
>    driver unbinds. Then pcie_ptm_enabled() returns true, and
>    pci_resume_ptm() can enable PTM on the device after a resume,
>    although the upstream path does not support PTM. Before
>    e1092d5e15e6, pci_disable_ptm() did nothing if PTM was not enabled.
> [ ... ]
> Cc: stable@vger.kernel.org
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Link: https://lore.kernel.org/r/20261102222817.GA983941@bhelgaas
> Link: https://lore.kernel.org/r/20260616141733.1688264-1-vulab@iscas.ac.cn # [1]
> Signed-off-by: Li Yifan <yifan2.li@intel.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104121457.2108515-1-yifan2.li@intel.com?part=1