| Message ID | 20160524125323.GE1789@lahna.fi.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote: > > On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote: > > > [+cc Valdis, Dave] > > > > > > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote: > > > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote: > > > > > When a PCI device is removed through sysfs interface the upstream bridge > > > > > (PCIe port) can be runtime suspended if it was the last device on that bus. > > > > > Now, if the bridge is in D3 we cannot find devices below the bridge > > > > > anymore. For example following fails to find the removed device again: > > > > > > > > > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove > > > > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan > > > > > > > > > > Where 0000:00:01.0 is the bridge device. > > > > > > > > > > In order to be able to rescan devices below the bridge add > > > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This > > > > > should keep bridges powered on while their children devices are being > > > > > scanned. > > > > > > > > > > Reported-by: Peter Wu <peter@lekensteyn.nl> > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > > > > > This looks like basically the same idea as "ACPI / hotplug / PCI: > > > > Runtime resume bridge before rescan". > > > > > > > > The hotplug_event() path modified by that patch eventually calls > > > > pci_scan_bridge(): > > > > > > > > hotplug_event > > > > enable_slot > > > > pci_scan_bridge > > > > > > > > so this patch looks a little more general. Does it make "ACPI / > > > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary? > > > > Can I just replace that patch with this one? > > > > > > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge > > > before rescan" with this one and pushed the result to > > > > > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm > > > > > > Please take a look, test it, and let me know if I need to add the ACPI > > > patch back. > > > > > > This branch also includes the fix for the lockdep splat reported by > > > Valdis. This is what I hope to get into v4.7-rc1. > > > > Ping? I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1. > > It currently has these changes: > > > > 8b71f5652eea PCI: Add runtime PM support for PCIe ports > > af81f0fa638b PCI: Power on bridges before scanning new devices > > 9741a01c9f55 PCI: Put PCIe ports into D3 during suspend > > b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports > > Looks good to me. I've also tested those here and seems to work fine. > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > on the assumption that "PCI: Power on bridges before scanning new > > devices" is sufficient to cover both the ACPI and the generic PCi > > rescan cases, but I'd like some reassurance about that. > > I agree with your reasoning that the patch should not be needed anymore. > However, I have the machine which needed that patch at home so I'm not > able to test it now. I'll do that later today when I get back home. > > One thing I noticed, though. When a bridge is transitioned to D0 we only > wait for 10ms which is requirement for PCI functions. However, PCI PM > specification 1.2 (chapter 4.2) requires that for buses to transition > from B2 to B0 we need to wait minimum of 50ms before accessing a > function on that bus. > > We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is > not used anywhere. Maybe it was not needed originally because we never > powered down bridges anyway but now when we do, I think it is good idea > to do what the spec requires. > > What do you think? We could add a separate patch doing something like > below to make sure the spec is followed. > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e785dc260e72..b3b794caa380 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev) > } > > dev->pm_cap = pm; > - dev->d3_delay = PCI_PM_D3_WAIT; > + /* > + * PCI PM 1.2 specification requires minimum of 50ms before any > + * function on the bus is accessed after the bus is transitioned > + * from B2 to B0. > + */ > + dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT; Seems plausible to me, and seems safe enough to include for v4.7 if you post this with a changelog and signed-off-by. > dev->d3cold_delay = PCI_PM_D3COLD_WAIT; > dev->bridge_d3 = pci_bridge_d3_possible(dev); > dev->d3cold_allowed = true; -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > on the assumption that "PCI: Power on bridges before scanning new > > devices" is sufficient to cover both the ACPI and the generic PCi > > rescan cases, but I'd like some reassurance about that. > > I agree with your reasoning that the patch should not be needed anymore. > However, I have the machine which needed that patch at home so I'm not > able to test it now. I'll do that later today when I get back home. I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power on bridges before scanning new devices" seems not to be enough. This machine has SD-card reader connected to one PCIe port and once I unload the sdhci-pci driver and enable runtime PM for the device, next system suspend/resume cycle loses the SD-card reader PCI device. I will investigate more tomorrow -- it is getting late here. Anyway, maybe it is safer to postpone this series for v4.8 to give it more testing time in -next. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, May 25, 2016 12:13:09 AM Mika Westerberg wrote: > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > > on the assumption that "PCI: Power on bridges before scanning new > > > devices" is sufficient to cover both the ACPI and the generic PCi > > > rescan cases, but I'd like some reassurance about that. > > > > I agree with your reasoning that the patch should not be needed anymore. > > However, I have the machine which needed that patch at home so I'm not > > able to test it now. I'll do that later today when I get back home. > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power > on bridges before scanning new devices" seems not to be enough. This > machine has SD-card reader connected to one PCIe port and once I unload > the sdhci-pci driver and enable runtime PM for the device, next system > suspend/resume cycle loses the SD-card reader PCI device. > > I will investigate more tomorrow -- it is getting late here. > > Anyway, maybe it is safer to postpone this series for v4.8 to give it > more testing time in -next. Agreed. I don't see a compelling enough reason to rush these changes into 4.7 and it looks like there are gaps in our understanding of the issues involved. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mika, On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > One thing I noticed, though. When a bridge is transitioned to D0 we only > wait for 10ms which is requirement for PCI functions. However, PCI PM > specification 1.2 (chapter 4.2) requires that for buses to transition > from B2 to B0 we need to wait minimum of 50ms before accessing a > function on that bus. > > We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is > not used anywhere. Maybe it was not needed originally because we never > powered down bridges anyway but now when we do, I think it is good idea > to do what the spec requires. The macro was introduced with commit aa8c6c93747f7b55fa11e1624fec8ca33763a805 Author: Rafael J. Wysocki <rjw@sisk.pl> Date: Fri Jan 16 21:54:43 2009 +0100 PCI PM: Restore standard config registers of all devices early but the only usage of it was removed with commit 476e7faefc43f106a90b5c96166c59b75de19d30 Author: Rafael J. Wysocki <rjw@sisk.pl> Date: Thu Jan 22 23:39:57 2009 +0100 PCI PM: Do not wait for buses in B2 or B3 during resume Best regards, Lukas > > What do you think? We could add a separate patch doing something like > below to make sure the spec is followed. > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e785dc260e72..b3b794caa380 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev) > } > > dev->pm_cap = pm; > - dev->d3_delay = PCI_PM_D3_WAIT; > + /* > + * PCI PM 1.2 specification requires minimum of 50ms before any > + * function on the bus is accessed after the bus is transitioned > + * from B2 to B0. > + */ > + dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT; > dev->d3cold_delay = PCI_PM_D3COLD_WAIT; > dev->bridge_d3 = pci_bridge_d3_possible(dev); > dev->d3cold_allowed = true; -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote: > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > > on the assumption that "PCI: Power on bridges before scanning new > > > devices" is sufficient to cover both the ACPI and the generic PCi > > > rescan cases, but I'd like some reassurance about that. > > > > I agree with your reasoning that the patch should not be needed anymore. > > However, I have the machine which needed that patch at home so I'm not > > able to test it now. I'll do that later today when I get back home. > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power > on bridges before scanning new devices" seems not to be enough. This > machine has SD-card reader connected to one PCIe port and once I unload > the sdhci-pci driver and enable runtime PM for the device, next system > suspend/resume cycle loses the SD-card reader PCI device. > > I will investigate more tomorrow -- it is getting late here. I think I found reason for the issue. When the laptop resumes it will send ACPI BUS_CHECK event for the two PCIe root ports. This ends up in acpiphp_check_bridge() where it goes through all slots in that bridge checking if the devices are still present. This happens before we call pci_scan_bridge() for the bridge itself. Since the bridge is in D3 config space of the device behind it is not available and we determine that the device is not there anymore. It looks like we either need that ACPI hotplug patch or alternatively we could add pm_runtime_get/put() in acpiphp_check_bridge(). -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2016 at 02:16:26PM +0200, Lukas Wunner wrote: > Hi Mika, > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > One thing I noticed, though. When a bridge is transitioned to D0 we only > > wait for 10ms which is requirement for PCI functions. However, PCI PM > > specification 1.2 (chapter 4.2) requires that for buses to transition > > from B2 to B0 we need to wait minimum of 50ms before accessing a > > function on that bus. > > > > We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is > > not used anywhere. Maybe it was not needed originally because we never > > powered down bridges anyway but now when we do, I think it is good idea > > to do what the spec requires. > > The macro was introduced with > > commit aa8c6c93747f7b55fa11e1624fec8ca33763a805 > Author: Rafael J. Wysocki <rjw@sisk.pl> > Date: Fri Jan 16 21:54:43 2009 +0100 > PCI PM: Restore standard config registers of all devices early > > but the only usage of it was removed with > > commit 476e7faefc43f106a90b5c96166c59b75de19d30 > Author: Rafael J. Wysocki <rjw@sisk.pl> > Date: Thu Jan 22 23:39:57 2009 +0100 > PCI PM: Do not wait for buses in B2 or B3 during resume > Thanks for looking that up. If I understand correctly it was removed because we do not expect BIOS to put buses to B2/B3 so we do not need to wait the extra 50ms before accessing the device. However, now that we do that in the OS (and also during runtime), I think we need to take that into account again. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote: > On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote: > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > > > on the assumption that "PCI: Power on bridges before scanning new > > > > devices" is sufficient to cover both the ACPI and the generic PCi > > > > rescan cases, but I'd like some reassurance about that. > > > > > > I agree with your reasoning that the patch should not be needed anymore. > > > However, I have the machine which needed that patch at home so I'm not > > > able to test it now. I'll do that later today when I get back home. > > > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power > > on bridges before scanning new devices" seems not to be enough. This > > machine has SD-card reader connected to one PCIe port and once I unload > > the sdhci-pci driver and enable runtime PM for the device, next system > > suspend/resume cycle loses the SD-card reader PCI device. > > > > I will investigate more tomorrow -- it is getting late here. > > I think I found reason for the issue. > > When the laptop resumes it will send ACPI BUS_CHECK event for the two > PCIe root ports. This ends up in acpiphp_check_bridge() where it goes > through all slots in that bridge checking if the devices are still > present. This happens before we call pci_scan_bridge() for the bridge > itself. > > Since the bridge is in D3 config space of the device behind it is not > available and we determine that the device is not there anymore. > > It looks like we either need that ACPI hotplug patch or alternatively we > could add pm_runtime_get/put() in acpiphp_check_bridge(). Have you tried the latter? -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2016 at 10:45:48PM +0200, Rafael J. Wysocki wrote: > On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote: > > On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote: > > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > > > > on the assumption that "PCI: Power on bridges before scanning new > > > > > devices" is sufficient to cover both the ACPI and the generic PCi > > > > > rescan cases, but I'd like some reassurance about that. > > > > > > > > I agree with your reasoning that the patch should not be needed anymore. > > > > However, I have the machine which needed that patch at home so I'm not > > > > able to test it now. I'll do that later today when I get back home. > > > > > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power > > > on bridges before scanning new devices" seems not to be enough. This > > > machine has SD-card reader connected to one PCIe port and once I unload > > > the sdhci-pci driver and enable runtime PM for the device, next system > > > suspend/resume cycle loses the SD-card reader PCI device. > > > > > > I will investigate more tomorrow -- it is getting late here. > > > > I think I found reason for the issue. > > > > When the laptop resumes it will send ACPI BUS_CHECK event for the two > > PCIe root ports. This ends up in acpiphp_check_bridge() where it goes > > through all slots in that bridge checking if the devices are still > > present. This happens before we call pci_scan_bridge() for the bridge > > itself. > > > > Since the bridge is in D3 config space of the device behind it is not > > available and we determine that the device is not there anymore. > > > > It looks like we either need that ACPI hotplug patch or alternatively we > > could add pm_runtime_get/put() in acpiphp_check_bridge(). > > Have you tried the latter? Indeed I tried and it worked fine. I can make formal patch doing that which then replaces the current ACPI hotplug patch, if that is the preferred way. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, May 26, 2016 11:16:43 AM Mika Westerberg wrote: > On Wed, May 25, 2016 at 10:45:48PM +0200, Rafael J. Wysocki wrote: > > On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote: > > > On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote: > > > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > > > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > > > > > on the assumption that "PCI: Power on bridges before scanning new > > > > > > devices" is sufficient to cover both the ACPI and the generic PCi > > > > > > rescan cases, but I'd like some reassurance about that. > > > > > > > > > > I agree with your reasoning that the patch should not be needed anymore. > > > > > However, I have the machine which needed that patch at home so I'm not > > > > > able to test it now. I'll do that later today when I get back home. > > > > > > > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power > > > > on bridges before scanning new devices" seems not to be enough. This > > > > machine has SD-card reader connected to one PCIe port and once I unload > > > > the sdhci-pci driver and enable runtime PM for the device, next system > > > > suspend/resume cycle loses the SD-card reader PCI device. > > > > > > > > I will investigate more tomorrow -- it is getting late here. > > > > > > I think I found reason for the issue. > > > > > > When the laptop resumes it will send ACPI BUS_CHECK event for the two > > > PCIe root ports. This ends up in acpiphp_check_bridge() where it goes > > > through all slots in that bridge checking if the devices are still > > > present. This happens before we call pci_scan_bridge() for the bridge > > > itself. > > > > > > Since the bridge is in D3 config space of the device behind it is not > > > available and we determine that the device is not there anymore. > > > > > > It looks like we either need that ACPI hotplug patch or alternatively we > > > could add pm_runtime_get/put() in acpiphp_check_bridge(). > > > > Have you tried the latter? > > Indeed I tried and it worked fine. I can make formal patch doing that > which then replaces the current ACPI hotplug patch, if that is the > preferred way. It is somewhat cleaner, so I'd prefer it. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 28, 2016 at 02:21:11PM +0200, Rafael J. Wysocki wrote: > On Thursday, May 26, 2016 11:16:43 AM Mika Westerberg wrote: > > On Wed, May 25, 2016 at 10:45:48PM +0200, Rafael J. Wysocki wrote: > > > On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote: > > > > On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote: > > > > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote: > > > > > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > > > > > > > on the assumption that "PCI: Power on bridges before scanning new > > > > > > > devices" is sufficient to cover both the ACPI and the generic PCi > > > > > > > rescan cases, but I'd like some reassurance about that. > > > > > > > > > > > > I agree with your reasoning that the patch should not be needed anymore. > > > > > > However, I have the machine which needed that patch at home so I'm not > > > > > > able to test it now. I'll do that later today when I get back home. > > > > > > > > > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power > > > > > on bridges before scanning new devices" seems not to be enough. This > > > > > machine has SD-card reader connected to one PCIe port and once I unload > > > > > the sdhci-pci driver and enable runtime PM for the device, next system > > > > > suspend/resume cycle loses the SD-card reader PCI device. > > > > > > > > > > I will investigate more tomorrow -- it is getting late here. > > > > > > > > I think I found reason for the issue. > > > > > > > > When the laptop resumes it will send ACPI BUS_CHECK event for the two > > > > PCIe root ports. This ends up in acpiphp_check_bridge() where it goes > > > > through all slots in that bridge checking if the devices are still > > > > present. This happens before we call pci_scan_bridge() for the bridge > > > > itself. > > > > > > > > Since the bridge is in D3 config space of the device behind it is not > > > > available and we determine that the device is not there anymore. > > > > > > > > It looks like we either need that ACPI hotplug patch or alternatively we > > > > could add pm_runtime_get/put() in acpiphp_check_bridge(). > > > > > > Have you tried the latter? > > > > Indeed I tried and it worked fine. I can make formal patch doing that > > which then replaces the current ACPI hotplug patch, if that is the > > preferred way. > > It is somewhat cleaner, so I'd prefer it. Okay, I'm going to submit an updated version of that patch then. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e785dc260e72..b3b794caa380 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev) } dev->pm_cap = pm; - dev->d3_delay = PCI_PM_D3_WAIT; + /* + * PCI PM 1.2 specification requires minimum of 50ms before any + * function on the bus is accessed after the bus is transitioned + * from B2 to B0. + */ + dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT; dev->d3cold_delay = PCI_PM_D3COLD_WAIT; dev->bridge_d3 = pci_bridge_d3_possible(dev); dev->d3cold_allowed = true;