| Message ID | 20240802-pci-bridge-d3-v5-1-2426dd9e8e27@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Allow D3Hot for PCI bridges in Devicetree based platforms | expand |
On Fri, Aug 02, 2024 at 01:19:24PM +0200, Rafael J. Wysocki wrote: > On Fri, Aug 2, 2024 at 11:49AM Lukas Wunner <lukas@wunner.de> wrote: > > > > On Fri, Aug 02, 2024 at 11:25:00AM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > > PCI core is already caching the value of pci_bridge_d3_possible() in > > > pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to > > > change, > > Is that really the case? > > Have you seen pci_bridge_d3_update()? Okay the value may change at runtime, e.g. due to user space manipulating d3cold_allowed in sysfs. > > I don't know if there was a reason to call pci_bridge_d3_possible() > > (instead of using the cached value) on probe, remove and shutdown. > > > > The change is probably safe but it would still be good to get some > > positive test results with Thunderbolt laptops etc to raise the > > confidence. > > If I'm not mistaken, the change is not correct. You're right. Because the value may change, different code paths may be chosen on probe, remove and shutdown. Sorry for missing that. Thanks, Lukas
On Fri, Aug 02, 2024 at 10:07:39PM +0200, Lukas Wunner wrote: > On Fri, Aug 02, 2024 at 01:19:24PM +0200, Rafael J. Wysocki wrote: > > On Fri, Aug 2, 2024 at 11:49AM Lukas Wunner <lukas@wunner.de> wrote: > > > > > > On Fri, Aug 02, 2024 at 11:25:00AM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > > > PCI core is already caching the value of pci_bridge_d3_possible() in > > > > pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to > > > > change, > > > > Is that really the case? > > > > Have you seen pci_bridge_d3_update()? > > Okay the value may change at runtime, e.g. due to user space > manipulating d3cold_allowed in sysfs. > The last part of the commit message is wrong, but pci_bridge_d3_update() is already updating pci_dev::bridge_d3. And pci_bridge_d3_possible() is not making use of any checks that could change dynamically IIUC. So what is wrong in using pci_dev::bridge_d3? Even more, if the client drivers have changed the state of pci_dev::bridge_d3 during runtime, then pci_bridge_d3_possible() won't catch that. Or is there a reason to not do so purposefully? > > > I don't know if there was a reason to call pci_bridge_d3_possible() > > > (instead of using the cached value) on probe, remove and shutdown. > > > > > > The change is probably safe but it would still be good to get some > > > positive test results with Thunderbolt laptops etc to raise the > > > confidence. > > > > If I'm not mistaken, the change is not correct. > > You're right. Because the value may change, different code paths > may be chosen on probe, remove and shutdown. Sorry for missing that. > Again, pci_bridge_d3_possible() is not making use of values that could change dynamically. - Mani
On Tue, Aug 06, 2024 at 08:46:48AM +0200, Lukas Wunner wrote: > On Mon, Aug 05, 2024 at 06:54:42PM +0530, Manivannan Sadhasivam wrote: > > So what is wrong in using pci_dev::bridge_d3? > > The bridge_d3 flag may change at runtime, e.g. when writing to the > d3cold_allowed attribute in sysfs. > > If e.g. bridge_d3 is set when pcie_portdrv_probe() runs but no longer > set when pcie_portdrv_remove() runs, there would be a runtime PM ref > imbalance. (Ref would be dropped on probe, but not reacquired on remove.) > Ah, so it is the other way around. Thanks for clearing it up! - Mani
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c index 14a4b89a3b83..1f02e5d7b2e9 100644 --- a/drivers/pci/pcie/portdrv.c +++ b/drivers/pci/pcie/portdrv.c @@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE | DPM_FLAG_SMART_SUSPEND); - if (pci_bridge_d3_possible(dev)) { + if (dev->bridge_d3) { /* * Keep the port resumed 100ms to make sure things like * config space accesses from userspace (lspci) will not @@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev, static void pcie_portdrv_remove(struct pci_dev *dev) { - if (pci_bridge_d3_possible(dev)) { + if (dev->bridge_d3) { pm_runtime_forbid(&dev->dev); pm_runtime_get_noresume(&dev->dev); pm_runtime_dont_use_autosuspend(&dev->dev); @@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev) static void pcie_portdrv_shutdown(struct pci_dev *dev) { - if (pci_bridge_d3_possible(dev)) { + if (dev->bridge_d3) { pm_runtime_forbid(&dev->dev); pm_runtime_get_noresume(&dev->dev); pm_runtime_dont_use_autosuspend(&dev->dev);