PCI/DPC: Disable interrupt generation during suspend

Commit eed85ff4c0da ("PCI/DPC: Enable DPC only if AER is available") made DPC control dependent whether AER is enabled in the OS. However, it does not take into account situations where BIOS has not given OS control of AER:

acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI] acpi PNP0A08:00: _OSC: platform does not support [AER]
acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME PCIeCapability]

I think here it is better not to enable DPC even if the capability is available because then it would be against what "Determination of DPC Control" note in PCIe 4.0 sec 6.1.10 recommends.

Signed-off-by Mika Westerberg <[email protected]>

Quoted 9 lines · Mika Westerberg

Commit eed85ff4c0da ("PCI/DPC: Enable DPC only if AER is available") made DPC control dependent whether AER is enabled in the OS. However, it does not take into account situations where BIOS has not given OS control of AER:

acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI] acpi PNP0A08:00: _OSC: platform does not support [AER]
acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME PCIeCapability]

Quoted 5 lines

I think here it is better not to enable DPC even if the capability is available because then it would be against what "Determination of DPC Control" note in PCIe 4.0 sec 6.1.10 recommends.

Signed-off-by Mika Westerberg <[email protected]>

This may clash with the recent PCIe ports initialization rework from Bjorn, please double check.

When system is resumed if the interrupt generation is enabled it may trigger immediately when interrupts are enabled depending on what is in the status register. This may generate spurious DPC conditions and unnecessary removal of devices.

Make this work better with system suspend and disable interrupt generation when the system is suspended.

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

drivers/pci/pcie/pcie-dpc.c +12 -4

index 38e40c6c576f..14b96983dbbd 100644
@@ -280,23 +280,44 @@ static int dpc_probe(struct pcie_device *dev)
        return status;
 }
 
-static void dpc_remove(struct pcie_device *dev)
+static void dpc_interrupt_enable(struct dpc_dev *dpc, bool enable)
 {
-       struct dpc_dev *dpc = get_service_data(dev);
-       struct pci_dev *pdev = dev->port;
+       struct pci_dev *pdev = dpc->dev->port;
        u16 ctl;
 
        pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
-       ctl &= ~(PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN);
+       if (enable)
+               ctl |= PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN;
+       else
+               ctl &= ~(PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN);
        pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
 }
 
+static void dpc_remove(struct pcie_device *dev)
+{
+       dpc_interrupt_enable(get_service_data(dev), false);
+}
+
+static int dpc_suspend(struct pcie_device *dev)

drivers/pci/pcie/pcie-dpc.c +3 -1

+
 static struct pcie_port_service_driver dpcdriver = {
        .name           = "dpc",
        .port_type      = PCIE_ANY_PORT,
        .service        = PCIE_PORT_SERVICE_DPC,
        .probe          = dpc_probe,
        .remove         = dpc_remove,
+       .suspend        = dpc_suspend,
+       .resume         = dpc_resume,
 };
 
 static int __init dpc_service_init(void)
--

Have you considered putting these things into the ->suspend_late and ->resume_early callbacks, respectively?

That might be slightly better as runtime resume is still enabled when the ->suspend and ->resume callbacks run.

Quoted 5 lines · Rafael J. Wysocki

Have you considered putting these things into the ->suspend_late and ->resume_early callbacks, respectively?

That might be slightly better as runtime resume is still enabled when the ->suspend and ->resume callbacks run.

There is no ->suspend_late or ->resume_early callbacks in struct pcie_port_service_driver so I followed what drivers/pci/pcie/pme.c is doing. I guess we could add those callbacks as well if you think they are better suited here.

Quoted 11 lines · Mika Westerberg

Quoted 5 lines · Rafael J. Wysocki

Have you considered putting these things into the ->suspend_late and ->resume_early callbacks, respectively?

That might be slightly better as runtime resume is still enabled when the ->suspend and ->resume callbacks run.

There is no ->suspend_late or ->resume_early callbacks in struct pcie_port_service_driver so I followed what drivers/pci/pcie/pme.c is doing. I guess we could add those callbacks as well if you think they are better suited here.

Maybe these two commits I did 2 years ago but never upstreamed are of help:

https://github.com/l1k/linux/commit/4f59372f830899e89fea9ba7c090680900d5998e
https://github.com/l1k/linux/commit/ce63b6ed01602e77c56043ee6dfa52ca9d257f88

I'm not sure if they still compile & work, sorry. :)

Quoted 5 lines · Mika Westerberg

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines

Have you considered putting these things into the ->suspend_late and ->resume_early callbacks, respectively?

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

That might be slightly better as runtime resume is still enabled when the ->suspend and ->resume callbacks run.

There is no ->suspend_late or ->resume_early callbacks in struct pcie_port_service_driver so I followed what drivers/pci/pcie/pme.c is doing. I guess we could add those callbacks as well if you think they are better suited here.

I took a look and then realized that we most probably do not need the custom portdrv specific hooks at all. They are basically doing the same than what PM core is (iterate over children and call service driver PM hook). I don't see any reason why we could not rely on the PM core ops instead of these custom ones but maybe I'm missing something.

Quoted 13 lines · Mika Westerberg

Quoted 12 lines · Mika Westerberg

Quoted 11 lines · Rafael J. Wysocki

Quoted 5 lines

Have you considered putting these things into the ->suspend_late and ->resume_early callbacks, respectively?

That might be slightly better as runtime resume is still enabled when the ->suspend and ->resume callbacks run.

There is no ->suspend_late or ->resume_early callbacks in struct pcie_port_service_driver so I followed what drivers/pci/pcie/pme.c is doing. I guess we could add those callbacks as well if you think they are better suited here.

Quoted 5 lines

I took a look and then realized that we most probably do not need the custom portdrv specific hooks at all. They are basically doing the same than what PM core is (iterate over children and call service driver PM hook). I don't see any reason why we could not rely on the PM core ops instead of these custom ones but maybe I'm missing something.

Can't think of a reason why this solution shouldn't work

Now I've thought of one.

The port may have more children besides the port service devices, namely all the PCI devices below the port. The PM core doesn't impose a specific ordering on suspend/resume but will try to parallelize among all the children.

Usually that's not what you want. On resume, you want to resume the port itself (including its port services) *before* resuming the PCI child devices. And the other way round on suspend.

Quoted 10 lines · Lukas Wunner

Now I've thought of one.

The port may have more children besides the port service devices, namely all the PCI devices below the port. The PM core doesn't impose a specific ordering on suspend/resume but will try to parallelize among all the children.

Usually that's not what you want. On resume, you want to resume the port itself (including its port services) *before* resuming the PCI child devices. And the other way round on suspend.

That's a good point.

So I guess there is no way avoiding adding suspend_late/resume_early callbacks to the pcie port service structure. I'll do that in the next revision.

Quoted 12 lines · Mika Westerberg

Quoted 10 lines · Lukas Wunner

Now I've thought of one.

The port may have more children besides the port service devices, namely all the PCI devices below the port. The PM core doesn't impose a specific ordering on suspend/resume but will try to parallelize among all the children.

Usually that's not what you want. On resume, you want to resume the port itself (including its port services) *before* resuming the PCI child devices. And the other way round on suspend.

Quoted 5 lines

That's a good point.

So I guess there is no way avoiding adding suspend_late/resume_early callbacks to the pcie port service structure. I'll do that in the next revision.

Well, there *are* ways to avoid it but they might not be better.

Iterating over the port services' callbacks is equivalent to ordering the port service devices after the port's PCI device but before its PCI child devices in devices_kset.

That can also be achieved by adding a device link from every PCI child device (consumer) to every port service device (provider). The result however is a combinatorial explosion. Say you've got 64 down stream bridges in a PCIe switch and the upstream bridge has 3 port services, that's 3 x 64 = 192 device links. That's probably clumsier than iterating over the port services.

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver, and I also hope we can avoid adding device links. Those both sound pretty complicated.

Can you do something like the patch below, which does something similar for PME?

Quoted 4 lines · Bjorn Helgaas

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver, and I also hope we can avoid adding device links. Those both sound pretty complicated.

Quoted 2 lines

Can you do something like the patch below, which does something similar for PME?

AFAICT the core PCI PM code follows the same ordering than what PM core does so it may be possible that not all service drivers get resumed/suspended before other children (PCI buses). Basically this would be the same than just using core PM ops in DPC driver (in which case DPC specific things are still kept in DPC driver not in PCI core).

Quoted 5 lines · Mika Westerberg

Quoted 4 lines · Bjorn Helgaas

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver, and I also hope we can avoid adding device links. Those both sound pretty complicated.

Quoted 8 lines

Quoted 2 lines

Can you do something like the patch below, which does something similar for PME?

AFAICT the core PCI PM code follows the same ordering than what PM core does so it may be possible that not all service drivers get resumed/suspended before other children (PCI buses). Basically this would be the same than just using core PM ops in DPC driver (in which case DPC specific things are still kept in DPC driver not in PCI core).

I'm not sure I follow this. I assume the core PCI PM code guarantees that a bridge is suspended after its children and resumed before them. Are you saying that's not the case?

Quoted 15 lines · Bjorn Helgaas

Quoted 5 lines · Mika Westerberg

Quoted 4 lines · Bjorn Helgaas

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver, and I also hope we can avoid adding device links. Those both sound pretty complicated.

Quoted 8 lines

Quoted 2 lines

Can you do something like the patch below, which does something similar for PME?

AFAICT the core PCI PM code follows the same ordering than what PM core does so it may be possible that not all service drivers get resumed/suspended before other children (PCI buses). Basically this would be the same than just using core PM ops in DPC driver (in which case DPC specific things are still kept in DPC driver not in PCI core).

Quoted 3 lines

I'm not sure I follow this. I assume the core PCI PM code guarantees that a bridge is suspended after its children and resumed before them. Are you saying that's not the case?

if this is a PCIe port, then there are two categories of childres:
port services and the PCI devices below it.

There are no ordering constraints between the former and the latter, which appears to be a problem here.

Quoted 20 lines · Rafael J. Wysocki

Quoted 15 lines · Bjorn Helgaas

Quoted 5 lines · Mika Westerberg

Quoted 4 lines · Bjorn Helgaas

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver, and I also hope we can avoid adding device links. Those both sound pretty complicated.

Quoted 8 lines

Quoted 2 lines

Can you do something like the patch below, which does something similar for PME?

AFAICT the core PCI PM code follows the same ordering than what PM core does so it may be possible that not all service drivers get resumed/suspended before other children (PCI buses). Basically this would be the same than just using core PM ops in DPC driver (in which case DPC specific things are still kept in DPC driver not in PCI core).

Quoted 3 lines

I'm not sure I follow this. I assume the core PCI PM code guarantees that a bridge is suspended after its children and resumed before them. Are you saying that's not the case?

Quoted 5 lines

if this is a PCIe port, then there are two categories of childres:
port services and the PCI devices below it.

There are no ordering constraints between the former and the latter, which appears to be a problem here.

It seems like a pretty fundamental problem if port services can be suspended before PCI devices below the port. I assume that would have to be fixed somehow in the PCI core and the port driver, but this patch only touches the DPC service driver.

I'd really like to get rid of the port services driver altogether, and this ordering issue seems like a good reason to do that.

Quoted 20 lines · Rafael J. Wysocki

Quoted 15 lines · Bjorn Helgaas

Quoted 5 lines · Mika Westerberg

Quoted 4 lines · Bjorn Helgaas

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver, and I also hope we can avoid adding device links. Those both sound pretty complicated.

Quoted 8 lines

Quoted 2 lines

Can you do something like the patch below, which does something similar for PME?

AFAICT the core PCI PM code follows the same ordering than what PM core does so it may be possible that not all service drivers get resumed/suspended before other children (PCI buses). Basically this would be the same than just using core PM ops in DPC driver (in which case DPC specific things are still kept in DPC driver not in PCI core).

Quoted 3 lines

I'm not sure I follow this. I assume the core PCI PM code guarantees that a bridge is suspended after its children and resumed before them. Are you saying that's not the case?

Quoted 5 lines

if this is a PCIe port, then there are two categories of childres:
port services and the PCI devices below it.

There are no ordering constraints between the former and the latter, which appears to be a problem here.

It seems like a pretty fundamental problem if port services can be suspended before PCI devices below the port. I assume that would have to be fixed somehow in the PCI core and the port driver, but this patch only touches the DPC service driver.

I'd really like to get rid of the port services driver altogether, and this ordering issue seems like a good reason to do that.

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver,

Quoted 2 lines · Bjorn Helgaas

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver,

I'm fairly certain that we cannot avoid adding at least a ->resume_noirq callback to struct pcie_port_service_driver to fix a pciehp use case:

On ->resume_noirq the PCI core walks down the hierarchy to put every device in D0 and restore its state (with a few exceptions such as direct complete). However with hotplug ports, it's possible that the user has unplugged devices while the system was asleep, or replaced them with other devices. That's a very real use case with Thunderbolt and we're handling it poorly or not at all currently. We need to check if the devices below a hotplug port are still there or have been replaced (can probably be recognized by looking at vendor/device IDs across the entire sub-hierarchy) during the ->resume_noirq phase. We could mark them with pci_dev_set_disconnected(), then skip putting them into D0 if that flag has been set.

Quoted 7 lines · Lukas Wunner

Quoted 2 lines · Bjorn Helgaas

I hope we can avoid adding suspend_late/resume_early callbacks in struct pcie_port_service_driver,

I'm fairly certain that we cannot avoid adding at least a ->resume_noirq callback to struct pcie_port_service_driver to fix a pciehp use case:

Quoted 11 lines

On ->resume_noirq the PCI core walks down the hierarchy to put every device in D0 and restore its state (with a few exceptions such as direct complete). However with hotplug ports, it's possible that the user has unplugged devices while the system was asleep, or replaced them with other devices. That's a very real use case with Thunderbolt and we're handling it poorly or not at all currently. We need to check if the devices below a hotplug port are still there or have been replaced (can probably be recognized by looking at vendor/device IDs across the entire sub-hierarchy) during the ->resume_noirq phase. We could mark them with pci_dev_set_disconnected(), then skip putting them into D0 if that flag has been set.

This is definitely a real issue, and I think it affects more than just Thunderbolt. We've never handled undocks very well either.

I certainly agree we need a way to handle this; I would just rather do it by integrating pciehp and the other services more directly into the PCI core instead of using the port driver. Maybe we'll need a short-term ->resume_noirq, but I don't think the port driver is a good long-term solution.