| Message ID | 20200622161248.51099-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Make pcie_find_root_port() work for PCIe root ports as well | expand |
On Mon, Jun 22, 2020 at 07:12:48PM +0300, Mika Westerberg wrote: > Commit 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and > pci_find_pcie_root_port()") unified the root port finding functionality > into a single function but missed the fact that the passed in device may > already be a root port. This causes the kernel to block power management > of PCIe hierarchies in recent systems because ->bridge_d3 started to > return false for such ports after the commit in question. > > Fixes: 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: stable@vger.kernel.org > --- > include/linux/pci.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index c79d83304e52..c17c24f5eeed 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -2169,8 +2169,13 @@ static inline int pci_pcie_type(const struct pci_dev *dev) > */ > static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) > { > - struct pci_dev *bridge = pci_upstream_bridge(dev); > + struct pci_dev *bridge; > > + /* If dev is already root port */ > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) > + return dev; > + > + bridge = pci_upstream_bridge(dev); > while (bridge) { > if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) > return bridge; I applied the patch below, which is slightly simplified but I think still equivalent, to for-linus for v5.8. Let me know if it's not. I dropped the stable tag because 6ae72bfa656e was merged for v5.8-rc1, and I assume v5.7 works correctly so it doesn't need any change. commit 5396956cc7c6 ("PCI: Make pcie_find_root_port() work for Root Ports") Author: Mika Westerberg <mika.westerberg@linux.intel.com> Date: Mon Jun 22 19:12:48 2020 +0300 PCI: Make pcie_find_root_port() work for Root Ports Commit 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") broke acpi_pci_bridge_d3() because calling pcie_find_root_port() on a Root Port returned NULL when it should return the Root Port, which in turn broke power management of PCIe hierarchies. Rework pcie_find_root_port() so it returns its argument when it is already a Root Port. [bhelgaas: test device only once, test for PCIe] Fixes: 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") Link: https://lore.kernel.org/r/20200622161248.51099-1-mika.westerberg@linux.intel.com Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/include/linux/pci.h b/include/linux/pci.h index c79d83304e52..34c1c4f45288 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2169,12 +2169,11 @@ static inline int pci_pcie_type(const struct pci_dev *dev) */ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) { - struct pci_dev *bridge = pci_upstream_bridge(dev); - - while (bridge) { - if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) - return bridge; - bridge = pci_upstream_bridge(bridge); + while (dev) { + if (pci_is_pcie(dev) && + pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) + return dev; + dev = pci_upstream_bridge(dev); } return NULL;
Hi Bjorn, On 2020/7/1 6:01, Bjorn Helgaas wrote: > On Mon, Jun 22, 2020 at 07:12:48PM +0300, Mika Westerberg wrote: >> Commit 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and >> pci_find_pcie_root_port()") unified the root port finding functionality >> into a single function but missed the fact that the passed in device may >> already be a root port. This causes the kernel to block power management >> of PCIe hierarchies in recent systems because ->bridge_d3 started to >> return false for such ports after the commit in question. >> >> Fixes: 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") >> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> Cc: stable@vger.kernel.org >> --- >> include/linux/pci.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index c79d83304e52..c17c24f5eeed 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -2169,8 +2169,13 @@ static inline int pci_pcie_type(const struct pci_dev *dev) >> */ >> static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) >> { >> - struct pci_dev *bridge = pci_upstream_bridge(dev); >> + struct pci_dev *bridge; >> >> + /* If dev is already root port */ >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) >> + return dev; >> + >> + bridge = pci_upstream_bridge(dev); >> while (bridge) { >> if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) >> return bridge; > I applied the patch below, which is slightly simplified but I think > still equivalent, to for-linus for v5.8. Let me know if it's not. > > I dropped the stable tag because 6ae72bfa656e was merged for v5.8-rc1, > and I assume v5.7 works correctly so it doesn't need any change. > > > commit 5396956cc7c6 ("PCI: Make pcie_find_root_port() work for Root Ports") > Author: Mika Westerberg <mika.westerberg@linux.intel.com> > Date: Mon Jun 22 19:12:48 2020 +0300 > > PCI: Make pcie_find_root_port() work for Root Ports > > Commit 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and > pci_find_pcie_root_port()") broke acpi_pci_bridge_d3() because calling > pcie_find_root_port() on a Root Port returned NULL when it should return > the Root Port, which in turn broke power management of PCIe hierarchies. > > Rework pcie_find_root_port() so it returns its argument when it is already > a Root Port. > > [bhelgaas: test device only once, test for PCIe] > Fixes: 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") > Link: https://lore.kernel.org/r/20200622161248.51099-1-mika.westerberg@linux.intel.com > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index c79d83304e52..34c1c4f45288 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -2169,12 +2169,11 @@ static inline int pci_pcie_type(const struct pci_dev *dev) > */ > static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) > { > - struct pci_dev *bridge = pci_upstream_bridge(dev); > - > - while (bridge) { > - if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) > - return bridge; > - bridge = pci_upstream_bridge(bridge); > + while (dev) { > + if (pci_is_pcie(dev) && > + pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) > + return dev; > + dev = pci_upstream_bridge(dev); > } > We may have some problems here, as after pcie_find_root_port() called, *dev will be either root port or NULL but users may want it unchanged. One such usage is in acpi_pci_bridge_d3(), drivers/pci/pci-acpi.c, *dev is used as origin after called this. So we should use a temporary point to *dev rather than directly modify it. Thanks, Yicong > return NULL; > . >
On Tue, Jun 30, 2020 at 05:01:07PM -0500, Bjorn Helgaas wrote: > On Mon, Jun 22, 2020 at 07:12:48PM +0300, Mika Westerberg wrote: > > Commit 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and > > pci_find_pcie_root_port()") unified the root port finding functionality > > into a single function but missed the fact that the passed in device may > > already be a root port. This causes the kernel to block power management > > of PCIe hierarchies in recent systems because ->bridge_d3 started to > > return false for such ports after the commit in question. > > > > Fixes: 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Cc: stable@vger.kernel.org > > --- > > include/linux/pci.h | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index c79d83304e52..c17c24f5eeed 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -2169,8 +2169,13 @@ static inline int pci_pcie_type(const struct pci_dev *dev) > > */ > > static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) > > { > > - struct pci_dev *bridge = pci_upstream_bridge(dev); > > + struct pci_dev *bridge; > > > > + /* If dev is already root port */ > > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) > > + return dev; > > + > > + bridge = pci_upstream_bridge(dev); > > while (bridge) { > > if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) > > return bridge; > > I applied the patch below, which is slightly simplified but I think > still equivalent, to for-linus for v5.8. Let me know if it's not. Thanks I just tested and it also works fine. > I dropped the stable tag because 6ae72bfa656e was merged for v5.8-rc1, > and I assume v5.7 works correctly so it doesn't need any change. Right, makes sense.
On Wed, Jul 01, 2020 at 09:53:51AM +0800, Yicong Yang wrote: > > static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) > > { > > - struct pci_dev *bridge = pci_upstream_bridge(dev); > > - > > - while (bridge) { > > - if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) > > - return bridge; > > - bridge = pci_upstream_bridge(bridge); > > + while (dev) { > > + if (pci_is_pcie(dev) && > > + pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) > > + return dev; > > + dev = pci_upstream_bridge(dev); > > } > > > > We may have some problems here, as after pcie_find_root_port() called, *dev will > be either root port or NULL but users may want it unchanged. One such usage is > in acpi_pci_bridge_d3(), drivers/pci/pci-acpi.c, *dev is used as origin > after called this. > > So we should use a temporary point to *dev rather than directly modify it. dev is already a copy of what is passed by the caller so it does not matter if it gets changed here. You would need to pass it through struct pci_dev **dev in order to modify the passed pointer.
On 2020/7/1 18:15, Mika Westerberg wrote: > On Wed, Jul 01, 2020 at 09:53:51AM +0800, Yicong Yang wrote: >>> static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) >>> { >>> - struct pci_dev *bridge = pci_upstream_bridge(dev); >>> - >>> - while (bridge) { >>> - if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) >>> - return bridge; >>> - bridge = pci_upstream_bridge(bridge); >>> + while (dev) { >>> + if (pci_is_pcie(dev) && >>> + pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) >>> + return dev; >>> + dev = pci_upstream_bridge(dev); >>> } >>> >> We may have some problems here, as after pcie_find_root_port() called, *dev will >> be either root port or NULL but users may want it unchanged. One such usage is >> in acpi_pci_bridge_d3(), drivers/pci/pci-acpi.c, *dev is used as origin >> after called this. >> >> So we should use a temporary point to *dev rather than directly modify it. > dev is already a copy of what is passed by the caller so it does not > matter if it gets changed here. You would need to pass it through struct > pci_dev **dev in order to modify the passed pointer. Ah...I must be fuzzy in mind then. Thanks. > . >
diff --git a/include/linux/pci.h b/include/linux/pci.h index c79d83304e52..c17c24f5eeed 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2169,8 +2169,13 @@ static inline int pci_pcie_type(const struct pci_dev *dev) */ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) { - struct pci_dev *bridge = pci_upstream_bridge(dev); + struct pci_dev *bridge; + /* If dev is already root port */ + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) + return dev; + + bridge = pci_upstream_bridge(dev); while (bridge) { if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) return bridge;
Commit 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") unified the root port finding functionality into a single function but missed the fact that the passed in device may already be a root port. This causes the kernel to block power management of PCIe hierarchies in recent systems because ->bridge_d3 started to return false for such ports after the commit in question. Fixes: 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()") Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: stable@vger.kernel.org --- include/linux/pci.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)