| Message ID | 20180314114125.71132-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Mar 14, 2018 at 12:41 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > 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 <mika.westerberg@linux.intel.com> > --- > drivers/pci/pcie/pcie-dpc.c | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c > index 38e40c6c576f..14b96983dbbd 100644 > --- a/drivers/pci/pcie/pcie-dpc.c > +++ b/drivers/pci/pcie/pcie-dpc.c > @@ -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) > +{ > + dpc_interrupt_enable(get_service_data(dev), false); > + return 0; > +} > + > +static int dpc_resume(struct pcie_device *dev) > +{ > + dpc_interrupt_enable(get_service_data(dev), true); > + return 0; > +} 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. > + > 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) > --
On Wed, Mar 14, 2018 at 12:48:49PM +0100, Rafael J. Wysocki wrote: > 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.
On Wed, Mar 14, 2018 at 02:05:47PM +0200, Mika Westerberg wrote: > On Wed, Mar 14, 2018 at 12:48:49PM +0100, Rafael J. Wysocki wrote: > > 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. :) Thanks, Lukas
On Wed, Mar 14, 2018 at 01:33:32PM +0100, Lukas Wunner wrote: > On Wed, Mar 14, 2018 at 02:05:47PM +0200, Mika Westerberg wrote: > > On Wed, Mar 14, 2018 at 12:48:49PM +0100, Rafael J. Wysocki wrote: > > > 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. :) Thanks for the pointers. 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.
On Tue, Mar 20, 2018 at 12:45:08PM +0200, Mika Westerberg wrote: > > On Wed, Mar 14, 2018 at 02:05:47PM +0200, Mika Westerberg wrote: > > > On Wed, Mar 14, 2018 at 12:48:49PM +0100, Rafael J. Wysocki wrote: > > > > 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. > > 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, I must have been blind not to see it. Lukas
On Tue, Mar 20, 2018 at 12:35:56PM +0100, Lukas Wunner wrote: > On Tue, Mar 20, 2018 at 12:45:08PM +0200, Mika Westerberg wrote: > > > On Wed, Mar 14, 2018 at 02:05:47PM +0200, Mika Westerberg wrote: > > > > On Wed, Mar 14, 2018 at 12:48:49PM +0100, Rafael J. Wysocki wrote: > > > > > 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. > > > > 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. Thanks, Lukas
On Thu, Mar 22, 2018 at 11:45:17AM +0100, Lukas Wunner wrote: > 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.
On Thu, Mar 22, 2018 at 06:53:17PM +0200, Mika Westerberg wrote: > On Thu, Mar 22, 2018 at 11:45:17AM +0100, Lukas Wunner wrote: > > 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. 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. Thanks, Lukas
diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c index 38e40c6c576f..14b96983dbbd 100644 --- a/drivers/pci/pcie/pcie-dpc.c +++ b/drivers/pci/pcie/pcie-dpc.c @@ -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) +{ + dpc_interrupt_enable(get_service_data(dev), false); + return 0; +} + +static int dpc_resume(struct pcie_device *dev) +{ + dpc_interrupt_enable(get_service_data(dev), true); + return 0; +} + 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)
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 <mika.westerberg@linux.intel.com> --- drivers/pci/pcie/pcie-dpc.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)