RE: [BUG] bus: mhi: host: pci_generic: runtime PM usage count leaked on

[Alexandra Lu (Nokia)]

Resend in plain text as previous email in HTML format was rejected by the list servers.


Hi Mani,

The following was discovered by AI while I was working PCIe device managed by Linux kernel. There appears to be an unbalanced runtime PM reference in pci_generic.c
affecting devices that set .no_m3. It checked against v7.1 and it is still present there.

mhi_pci_probe() only releases the reference taken by local_pci_probe() when both PME from D3hot and M3 are supported:

               /* Allow runtime suspend only if both PME from D3Hot and M3 are supported */
               if (pci_pme_capable(pdev, PCI_D3hot) && !(info->no_m3)) {
                              pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
                              pm_runtime_use_autosuspend(&pdev->dev);
                              pm_runtime_mark_last_busy(&pdev->dev);
                              pm_runtime_put_noidle(&pdev->dev);
               }

but mhi_pci_remove() re-takes that reference based on PME capability alone:

               /* balancing probe put_noidle */
               if (pci_pme_capable(pdev, PCI_D3hot))
                              pm_runtime_get_noresume(&pdev->dev);

Commit 0494cf9793b7 ("bus: mhi: host: pci_generic: Disable runtime PM for QDU100") added the !(info->no_m3) term to the probe-side condition but left the removal-side condition unchanged, so the two no longer mirror each other. For a device that is PME capable and sets .no_m3, probe never performs the put that this get is documented to balance, and one usage count is leaked per bind/unbind cycle. Tracing it through, with the baseline of count=1 / runtime_auto=false that pci_pm_init() establishes via pm_runtime_forbid():

                                                                           M3 supported               no_m3
  bind:                 core get_sync                              2                           2
                     probe put_noidle                       1                           (skipped) 2
                              mission-mode allow                0 auto=true     1 auto=true
  unbind:            core get_sync                              1                           2
                               remove forbid                             2 auto=false   3 auto=false
                              remove get_noresume             3                          4
                               core put_noidle                          2                          3
                              core put_sync                               1                          2
  final                                                                               1 (baseline)    2 (baseline + 1)

It derived this after f227b246307e ("bus: mhi: host: pci_generic: Add pm_runtime_forbid() in remove callback"), since that commit also touches runtime PM balance in mhi_pci_remove(). It correctly balances the pm_runtime_allow() performed on the Mission Mode transition, but that is a separate pair from the put_noidle/get_noresume pair above, so the leak survives it unchanged. The only in-tree device setting .no_m3 today is qcom-qdu100, so whether this is currently observable in the field depends on that part advertising PME from D3hot. If it does not, the leak is latent rather than active. Either way the two conditions are intended to mirror each other and no longer do, and the practical impact has likely gone unnoticed because the PCI core forces unbound devices to D0 regardless of their runtime PM state. Reproducing on a .no_m3 device is just repeated bind/unbind:

  for i in $(seq 5); do
      echo $BDF > /sys/bus/pci/drivers/mhi-pci-generic/unbind
      echo $BDF > /sys/bus/pci/drivers/mhi-pci-generic/bind
  done


Fixes: 0494cf9793b7 ("bus: mhi: host: pci_generic: Disable runtime PM for QDU100")
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -1080,6 +1080,7 @@ struct mhi_pci_device {
              struct work_struct recovery_work;
              struct timer_list health_check_timer;
              unsigned long status;
+            bool no_m3;
              bool reset_on_remove;
};

@@ -1390,6 +1391,8 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
              mhi_cntrl->mru = info->mru_default;
              mhi_cntrl->name = info->name;

+            mhi_pdev->no_m3 = info->no_m3;
+
              if (pdev->is_physfn)
                             mhi_pdev->reset_on_remove = info->reset_on_remove;

@@ -1478,7 +1481,7 @@ static void mhi_pci_remove(struct pci_dev *pdev)
              }

               /* balancing probe put_noidle */
-             if (pci_pme_capable(pdev, PCI_D3hot))
+            if (pci_pme_capable(pdev, PCI_D3hot) && !mhi_pdev->no_m3)
                             pm_runtime_get_noresume(&pdev->dev);

               if (mhi_pdev->reset_on_remove)

Note that no_m3 is cached unconditionally rather than under "if (pdev->is_physfn)" as reset_on_remove is, because the probe-side condition applies to virtual functions too; gating the cache on is_physfn would leave the same asymmetry in place for VFs.

Thanks,
Alexandra