On Mon, Aug 9, 2021 at 5:47 PM Lukas Wunner [off-list ref] wrote:

[cc += Mika]

On Mon, Aug 09, 2021 at 12:24:12PM +0800, Kai-Heng Feng wrote:

quoted

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213873

The last comment on this bugzilla says "BIOS will fix this."
and the status is RESOLVED WILL_NOT_FIX.

Why is the patch still necessary?

Well, let me update the bug report.

quoted

Some platforms cannot detect ethernet hotplug once its upstream port is
runtime suspended because PME isn't enabled in _OSC.

If PME is not handled natively, why does the NIC runtime suspend?
Shouldn't this be fixed in the NIC driver by keeping the device
runtime active if PME cannot be used?

That means we need to fix every user of pci_dev_run_wake(), or fix the
issue in pci_dev_run_wake() helper itself.
However, I am afraid that implementing the fix in pci_dev_run_wake()
may break the while loop check:
bool pci_dev_run_wake(struct pci_dev *dev)
{
 ...
        while (bus->parent) {
                struct pci_dev *bridge = bus->self;

                if (device_can_wakeup(&bridge->dev))
                        return true;

                bus = bus->parent;
...
}

So I took the current approach.

quoted

Disallow port runtime suspend when any child device requires wakeup, so
pci_pme_list_scan() can still read the PME status from the devices
behind the port.

pci_pme_list_scan() is for broken devices which fail to signal PME.
Is this NIC really among them or does PME fail merely because it's not
granted to OSPM?

The latter, PME IRQ isn't enabled because it's not granted by BIOS _OSC.

quoted

--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c

@@ -59,14 +59,30 @@ static int pcie_port_runtime_suspend(struct device *dev)
      return pcie_port_device_runtime_suspend(dev);
 }

+static int pcie_port_wakeup_check(struct device *dev, void *data)
+{
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     if (!pdev)
+             return 0;
+
+     return pdev->wakeup_prepared;
+}
+
 static int pcie_port_runtime_idle(struct device *dev)
 {
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     if (!pcie_port_find_device(pdev, PCIE_PORT_SERVICE_PME) &&
+         device_for_each_child(dev, NULL, pcie_port_wakeup_check))
+             return -EBUSY;
+
      /*
       * Assume the PCI core has set bridge_d3 whenever it thinks the port
       * should be good to go to D3.  Everything else, including moving
       * the port to D3, is handled by the PCI core.
       */
-     return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
+     return pdev->bridge_d3 ? 0 : -EBUSY;

If an additional check is necessary for this issue, it should be
integrated into pci_dev_check_d3cold() instead of pcie_port_runtime_idle().

I think PME IRQ and D3cold are different things here.
The root port of the affected NIC doesn't support D3cold because
there's no power resource.

Kai-Heng

Thanks,

Lukas