Re: [PATCH net 2/5] idpf: disable PTM on probe failure and on remove

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

- [Medium] Incomplete unwind: the new `pcie_ptm_enabled(pdev)` guard in
  `idpf_decfg_device()`…

Pre-existing issues:
- [Medium] Unserialized read-modify-write on shared upstream PTM state:
  `idpf_decfg_device()` (idpf_main.c:111-112) now calls…

--

idpf: disable PTM on probe failure and on remove

idpf_probe() enables PCIe PTM but never disables it, so the PTM enable
counts of the device and of its upstream path stay elevated after probe
failure or removal.  This adds a pci_disable_ptm() call to
idpf_decfg_device(), guarded by pcie_ptm_enabled() so that a failed
pci_enable_ptm() does not drive the count negative.

[Severity: Medium]
Does the pcie_ptm_enabled() guard skip the disable in exactly the case
the commit message describes as broken?

pcie_ptm_enabled() only looks at this device's own count:

    static inline bool pcie_ptm_enabled(struct pci_dev *dev)
    {
        return dev && atomic_read(&dev->ptm_enable_cnt) > 0;
    }

But pci_enable_ptm() in drivers/pci/pcie/ptm.c enables the upstream path
first, and only then tries the local device:

    parent = pci_upstream_ptm(dev);
    if (!parent)
            return -EINVAL;
    /* Enable PTM for the parent */
    rc = pci_enable_ptm(parent);
    if (rc)
            return rc;
    ...
    if (atomic_inc_return(&dev->ptm_enable_cnt) > 1)
            return 0;

    rc = __pci_enable_ptm(dev);
    if (rc) {
            atomic_dec(&dev->ptm_enable_cnt);
            return rc;
    }

So when __pci_enable_ptm(dev) fails, the local count returns to zero but
the parents that were just enabled and refcounted are never unwound.
__pci_enable_ptm() fails locally for a device with no PTM capability, and
for an endpoint that is not a PTM requester:

    if (!ptm)
            return -EINVAL;
    ...
    case PCI_EXP_TYPE_ENDPOINT:
    case PCI_EXP_TYPE_LEG_END:
            if (!dev->ptm_requester)
                    return -EINVAL;

idpf_cfg_device() discards that error and continues probing:

    err = pci_enable_ptm(pdev);
    if (err)
            pci_dbg(pdev, "PCIe PTM is not supported by PCIe bus/controller\n");

In that state pdev->ptm_enable_cnt is 0 while each upstream PTM device
sits at +1, pcie_ptm_enabled(pdev) is false, and idpf_decfg_device() does
nothing on probe failure or on remove.

Doesn't that leave the upstream enable counts and PTM Control bits
elevated with no bound driver, which is what the changelog says this fixes
("the PTM enable counts of this device and of its upstream path stay
elevated with no bound driver")?  Every bind/unbind cycle of such a
function, for example an idpf VF under a PTM-capable upstream port, would
add another leaked upstream reference, and a sibling endpoint's later
pci_disable_ptm() can then never bring the shared parent back to zero.

Calling pci_disable_ptm() unconditionally is not the answer either, for
the reason given in the changelog.  Would it make sense to record whether
pci_enable_ptm() succeeded in idpf_cfg_device() and key the unwind off
that instead, or to have the PCI core unwind the parent enables on local
failure?

[Severity: Medium]
This is a pre-existing issue in the PCI core and was not introduced by
this patch, but this is the first place idpf reaches it, so noting it
here.

pci_disable_ptm() recurses over bridges shared with sibling endpoints,
and the counter transition and the config-space write are separate steps
with no common lock:

    if (atomic_dec_and_test(&dev->ptm_enable_cnt))
            __pci_disable_ptm(dev);

    parent = pci_upstream_ptm(dev);
    if (parent)
            pci_disable_ptm(parent);

pci_enable_ptm() is symmetric:

    if (atomic_inc_return(&dev->ptm_enable_cnt) > 1)
            return 0;

    rc = __pci_enable_ptm(dev);

Can a remove running idpf_decfg_device() (shared parent 1 -> 0, then the
disable write) interleave with an asynchronous probe of a sibling under
the same upstream port (0 -> 1, then the enable write)?  If the sibling's
enable write lands before the remover's disable write, the shared parent
ends up PTM-disabled in hardware while ptm_enable_cnt is 1, and the
sibling would then gate on pcie_ptm_enabled() in idpf_ptp and offer
getcrosststamp over a PTM path that is off.

The window is narrow and confined to driver bind/unbind, and every
pci_disable_ptm() caller in the tree has the same exposure, so this looks
like something for the PCI core rather than this patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908214502.528440-1-anthony.l.nguyen%40intel.com