| Message ID | 20210805162917.3989-8-ameynarkhede03@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Expose and manage PCI device reset | expand |
[+cc Ben, Mika] On Thu, Aug 05, 2021 at 09:59:15PM +0530, Amey Narkhede wrote: > From: Shanker Donthineni <sdonthineni@nvidia.com> > > The pci_dev objects are created through two mechanisms 1) during PCI > bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object > is being set at different places depends on the type of firmware used, > device creation mechanism, and acpi_pci_bridge_d3(). > > The software features which have a dependency on ACPI fwnode properties > and need to be handled before device_add() will not work. One use case, > the software has to check the existence of _RST method to support ACPI > based reset method. > > This patch does the two changes in order to provide fwnode consistently. > - Set ACPI and OF fwnodes from pci_setup_device(). > - Remove pci_set_acpi_fwnode() in acpi_pci_bridge_d3(). > > After this patch, ACPI/OF firmware properties are visible at the same > time during the early stage of pci_dev setup. And also call sites should > be able to use firmware agnostic functions device_property_xxx() for the > early PCI quirks in the future. > > Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/pci/pci-acpi.c | 1 - > drivers/pci/probe.c | 7 ++++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index eaddbf701759..dae021322b3f 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) > return false; > > /* Assume D3 support if the bridge is power-manageable by ACPI. */ > - pci_set_acpi_fwnode(dev); > adev = ACPI_COMPANION(&dev->dev); I *think* the Root Port code farther down in this function is also now unnecessary: acpi_pci_bridge_d3(...) { ... root = pcie_find_root_port(dev); adev = ACPI_COMPANION(&root->dev); if (root == dev) { /* * It is possible that the ACPI companion is not yet bound * for the root port so look it up manually here. */ if (!adev && !pci_dev_is_added(root)) adev = acpi_pci_find_companion(&root->dev); } Since we're now setting the ACPI_COMPANION for every pci_dev long before we get here, I think this could now be simplified to something like this: acpi_pci_bridge_d3(...) { if (!dev->is_hotplug_bridge) return false; adev = ACPI_COMPANION(&dev->dev); if (adev && acpi_device_power_manageable(adev)) return true; root = pcie_find_root_port(dev); if (!root) return false; adev = ACPI_COMPANION(&root->dev); if (!adev) return false; rc = acpi_dev_get_property(dev, "HotPlugSupportInD3", ACPI_TYPE_INTEGER, &val); if (rc < 0) return false; return val == 1; } acpi_pci_bridge_d3() was added by 26ad34d510a8 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports") [1], so I cc'd Mika in case he has any comment. > if (adev && acpi_device_power_manageable(adev)) > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 379e85037d9b..15a6975d3757 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1789,6 +1789,9 @@ int pci_setup_device(struct pci_dev *dev) > dev->error_state = pci_channel_io_normal; > set_pcie_port_type(dev); > > + pci_set_of_node(dev); > + pci_set_acpi_fwnode(dev); Is there a reason why you moved pci_set_of_node() from pci_scan_device() to here? I think it's a good change; I'm just curious if you tripped over something that required it. The pci_set_of_node() was added to pci_scan_device() by 98d9f30c820d ("pci/of: Match PCI devices to OF nodes dynamically") [2], so I cc'd Ben just in case there's some reason he didn't put it in pci_setup_device() in the first place. > pci_dev_assign_slot(dev); > > /* > @@ -1924,6 +1927,7 @@ int pci_setup_device(struct pci_dev *dev) > default: /* unknown header */ > pci_err(dev, "unknown header type %02x, ignoring device\n", > dev->hdr_type); > + pci_release_of_node(dev); > return -EIO; > > bad: > @@ -2351,10 +2355,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn) > dev->vendor = l & 0xffff; > dev->device = (l >> 16) & 0xffff; > > - pci_set_of_node(dev); > - > if (pci_setup_device(dev)) { > - pci_release_of_node(dev); > pci_bus_put(dev->bus); > kfree(dev); > return NULL; [1] https://git.kernel.org/linus/26ad34d510a8 [2] https://git.kernel.org/linus/98d9f30c820d
Hi Bjorn, On 8/13/21 6:04 PM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > [+cc Ben, Mika] > > On Thu, Aug 05, 2021 at 09:59:15PM +0530, Amey Narkhede wrote: >> From: Shanker Donthineni <sdonthineni@nvidia.com> >> >> The pci_dev objects are created through two mechanisms 1) during PCI >> bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object >> is being set at different places depends on the type of firmware used, >> device creation mechanism, and acpi_pci_bridge_d3(). >> >> The software features which have a dependency on ACPI fwnode properties >> and need to be handled before device_add() will not work. One use case, >> the software has to check the existence of _RST method to support ACPI >> based reset method. >> >> This patch does the two changes in order to provide fwnode consistently. >> - Set ACPI and OF fwnodes from pci_setup_device(). >> - Remove pci_set_acpi_fwnode() in acpi_pci_bridge_d3(). >> >> After this patch, ACPI/OF firmware properties are visible at the same >> time during the early stage of pci_dev setup. And also call sites should >> be able to use firmware agnostic functions device_property_xxx() for the >> early PCI quirks in the future. >> >> Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com> >> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> >> --- >> drivers/pci/pci-acpi.c | 1 - >> drivers/pci/probe.c | 7 ++++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >> index eaddbf701759..dae021322b3f 100644 >> --- a/drivers/pci/pci-acpi.c >> +++ b/drivers/pci/pci-acpi.c >> @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) >> return false; >> >> /* Assume D3 support if the bridge is power-manageable by ACPI. */ >> - pci_set_acpi_fwnode(dev); >> adev = ACPI_COMPANION(&dev->dev); > I *think* the Root Port code farther down in this function is also now > unnecessary: > > acpi_pci_bridge_d3(...) > { > ... > root = pcie_find_root_port(dev); > adev = ACPI_COMPANION(&root->dev); > if (root == dev) { > /* > * It is possible that the ACPI companion is not yet bound > * for the root port so look it up manually here. > */ > if (!adev && !pci_dev_is_added(root)) > adev = acpi_pci_find_companion(&root->dev); > } > > Since we're now setting the ACPI_COMPANION for every pci_dev long > before we get here, I think this could now be simplified to something > like this: > > acpi_pci_bridge_d3(...) > { > if (!dev->is_hotplug_bridge) > return false; > > adev = ACPI_COMPANION(&dev->dev); > if (adev && acpi_device_power_manageable(adev)) > return true; > > root = pcie_find_root_port(dev); > if (!root) > return false; > > adev = ACPI_COMPANION(&root->dev); > if (!adev) > return false; > > rc = acpi_dev_get_property(dev, "HotPlugSupportInD3", > ACPI_TYPE_INTEGER, &val); > if (rc < 0) > return false; > > return val == 1; > } Agree, thanks for your suggestion. Yes, it can be simplified too. Can I do something like this using the unified device property API? static bool acpi_pci_bridge_d3(struct pci_dev *dev) { struct acpi_device *adev; struct pci_dev *root; u8 val; if (!dev->is_hotplug_bridge) return false; adev = ACPI_COMPANION(&dev->dev); if (adev && acpi_device_power_manageable(adev)) return true; root = pcie_find_root_port(dev); if (!root) return false; if (device_property_read_u8(&root->dev, "HotPlugSupportInD3", &val)) return false; return val == 1; } > acpi_pci_bridge_d3() was added by 26ad34d510a8 ("PCI / ACPI: Whitelist > D3 for more PCIe hotplug ports") [1], so I cc'd Mika in case he has > any comment. > >> if (adev && acpi_device_power_manageable(adev)) >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 379e85037d9b..15a6975d3757 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1789,6 +1789,9 @@ int pci_setup_device(struct pci_dev *dev) >> dev->error_state = pci_channel_io_normal; >> set_pcie_port_type(dev); >> >> + pci_set_of_node(dev); >> + pci_set_acpi_fwnode(dev); > Is there a reason why you moved pci_set_of_node() from > pci_scan_device() to here? I think it's a good change; I'm just > curious if you tripped over something that required it. There is no specific reason and not required but setting both the fwnodes at the same time improves the code readability and provides consistent device properties for callers. > The pci_set_of_node() was added to pci_scan_device() by 98d9f30c820d > ("pci/of: Match PCI devices to OF nodes dynamically") [2], so I cc'd > Ben just in case there's some reason he didn't put it in > pci_setup_device() in the first place. > Thanks, Shanker Donthineni
On Fri, Aug 13, 2021 at 10:35:46PM -0500, Shanker R Donthineni wrote: > Hi Bjorn, > > On 8/13/21 6:04 PM, Bjorn Helgaas wrote: > > External email: Use caution opening links or attachments > > > > > > [+cc Ben, Mika] > > > > On Thu, Aug 05, 2021 at 09:59:15PM +0530, Amey Narkhede wrote: > >> From: Shanker Donthineni <sdonthineni@nvidia.com> > >> > >> The pci_dev objects are created through two mechanisms 1) during PCI > >> bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object > >> is being set at different places depends on the type of firmware used, > >> device creation mechanism, and acpi_pci_bridge_d3(). > >> > >> The software features which have a dependency on ACPI fwnode properties > >> and need to be handled before device_add() will not work. One use case, > >> the software has to check the existence of _RST method to support ACPI > >> based reset method. > >> > >> This patch does the two changes in order to provide fwnode consistently. > >> - Set ACPI and OF fwnodes from pci_setup_device(). > >> - Remove pci_set_acpi_fwnode() in acpi_pci_bridge_d3(). > >> > >> After this patch, ACPI/OF firmware properties are visible at the same > >> time during the early stage of pci_dev setup. And also call sites should > >> be able to use firmware agnostic functions device_property_xxx() for the > >> early PCI quirks in the future. > >> > >> Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com> > >> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > >> --- > >> drivers/pci/pci-acpi.c | 1 - > >> drivers/pci/probe.c | 7 ++++--- > >> 2 files changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > >> index eaddbf701759..dae021322b3f 100644 > >> --- a/drivers/pci/pci-acpi.c > >> +++ b/drivers/pci/pci-acpi.c > >> @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) > >> return false; > >> > >> /* Assume D3 support if the bridge is power-manageable by ACPI. */ > >> - pci_set_acpi_fwnode(dev); > >> adev = ACPI_COMPANION(&dev->dev); > > I *think* the Root Port code farther down in this function is also now > > unnecessary: > > > > acpi_pci_bridge_d3(...) > > { > > ... > > root = pcie_find_root_port(dev); > > adev = ACPI_COMPANION(&root->dev); > > if (root == dev) { > > /* > > * It is possible that the ACPI companion is not yet bound > > * for the root port so look it up manually here. > > */ > > if (!adev && !pci_dev_is_added(root)) > > adev = acpi_pci_find_companion(&root->dev); > > } > > > > Since we're now setting the ACPI_COMPANION for every pci_dev long > > before we get here, I think this could now be simplified to something > > like this: > > > > acpi_pci_bridge_d3(...) > > { > > if (!dev->is_hotplug_bridge) > > return false; > > > > adev = ACPI_COMPANION(&dev->dev); > > if (adev && acpi_device_power_manageable(adev)) > > return true; > > > > root = pcie_find_root_port(dev); > > if (!root) > > return false; > > > > adev = ACPI_COMPANION(&root->dev); > > if (!adev) > > return false; > > > > rc = acpi_dev_get_property(dev, "HotPlugSupportInD3", > > ACPI_TYPE_INTEGER, &val); > > if (rc < 0) > > return false; > > > > return val == 1; > > } > > Agree, thanks for your suggestion. Yes, it can be simplified too. > Can I do something like this using the unified device property API? > > static bool acpi_pci_bridge_d3(struct pci_dev *dev) > { > struct acpi_device *adev; > struct pci_dev *root; > u8 val; > > if (!dev->is_hotplug_bridge) > return false; > > adev = ACPI_COMPANION(&dev->dev); > if (adev && acpi_device_power_manageable(adev)) > return true; > > root = pcie_find_root_port(dev); > if (!root) > return false; > > if (device_property_read_u8(&root->dev, "HotPlugSupportInD3", &val)) > return false; I guess that might be OK. TBH I don't really like the device_property_read_u8() thing because (1) we know this is an ACPI property and I don't see a reason to use an "generic" interface that doesn't buy us anything, and (2) the connection to the source of the data (a _DSD method) is really, really hard to find. Admittedly, it's still pretty hard to connect acpi_dev_get_property() with "_DSD". The only real clue is the comment about "Look for a special _DSD property ..." > return val == 1; > } > > > acpi_pci_bridge_d3() was added by 26ad34d510a8 ("PCI / ACPI: Whitelist > > D3 for more PCIe hotplug ports") [1], so I cc'd Mika in case he has > > any comment. > > > >> if (adev && acpi_device_power_manageable(adev)) > >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > >> index 379e85037d9b..15a6975d3757 100644 > >> --- a/drivers/pci/probe.c > >> +++ b/drivers/pci/probe.c > >> @@ -1789,6 +1789,9 @@ int pci_setup_device(struct pci_dev *dev) > >> dev->error_state = pci_channel_io_normal; > >> set_pcie_port_type(dev); > >> > >> + pci_set_of_node(dev); > >> + pci_set_acpi_fwnode(dev); > > Is there a reason why you moved pci_set_of_node() from > > pci_scan_device() to here? I think it's a good change; I'm just > > curious if you tripped over something that required it. > > There is no specific reason and not required but setting both the fwnodes > at the same time improves the code readability and provides consistent > device properties for callers. Sounds good. Bjorn
Hi Bjorn, On 8/13/21 11:10 PM, Bjorn Helgaas wrote: >>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >>>> index eaddbf701759..dae021322b3f 100644 >>>> --- a/drivers/pci/pci-acpi.c >>>> +++ b/drivers/pci/pci-acpi.c >>>> @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) >>>> return false; >>>> >>>> /* Assume D3 support if the bridge is power-manageable by ACPI. */ >>>> - pci_set_acpi_fwnode(dev); >>>> adev = ACPI_COMPANION(&dev->dev); >>> I *think* the Root Port code farther down in this function is also now >>> unnecessary: >>> >>> acpi_pci_bridge_d3(...) >>> { >>> ... >>> root = pcie_find_root_port(dev); >>> adev = ACPI_COMPANION(&root->dev); >>> if (root == dev) { >>> /* >>> * It is possible that the ACPI companion is not yet bound >>> * for the root port so look it up manually here. >>> */ >>> if (!adev && !pci_dev_is_added(root)) >>> adev = acpi_pci_find_companion(&root->dev); >>> } >>> >>> Since we're now setting the ACPI_COMPANION for every pci_dev long >>> before we get here, I think this could now be simplified to something >>> like this: >>> >>> acpi_pci_bridge_d3(...) >>> { >>> if (!dev->is_hotplug_bridge) >>> return false; >>> >>> adev = ACPI_COMPANION(&dev->dev); >>> if (adev && acpi_device_power_manageable(adev)) >>> return true; >>> >>> root = pcie_find_root_port(dev); >>> if (!root) >>> return false; >>> >>> adev = ACPI_COMPANION(&root->dev); >>> if (!adev) >>> return false; >>> >>> rc = acpi_dev_get_property(dev, "HotPlugSupportInD3", >>> ACPI_TYPE_INTEGER, &val); >>> if (rc < 0) >>> return false; >>> >>> return val == 1; >>> } >> Agree, thanks for your suggestion. Yes, it can be simplified too. >> Can I do something like this using the unified device property API? >> >> static bool acpi_pci_bridge_d3(struct pci_dev *dev) >> { >> struct acpi_device *adev; >> struct pci_dev *root; >> u8 val; >> >> if (!dev->is_hotplug_bridge) >> return false; >> >> adev = ACPI_COMPANION(&dev->dev); >> if (adev && acpi_device_power_manageable(adev)) >> return true; >> >> root = pcie_find_root_port(dev); >> if (!root) >> return false; >> >> if (device_property_read_u8(&root->dev, "HotPlugSupportInD3", &val)) >> return false; > I guess that might be OK. > > TBH I don't really like the device_property_read_u8() thing because > (1) we know this is an ACPI property and I don't see a reason to use > an "generic" interface that doesn't buy us anything, and (2) the > connection to the source of the data (a _DSD method) is really, really > hard to find. > > Admittedly, it's still pretty hard to connect acpi_dev_get_property() > with "_DSD". The only real clue is the comment about "Look for a > special _DSD property ..." > Does it satisfy you if I change the comment and still use device_property API? static bool acpi_pci_bridge_d3(struct pci_dev *dev) { struct pci_dev *rpdev; u8 val; if (!dev->is_hotplug_bridge) return false; /* Assume D3 support if the bridge is power-manageable by ACPI. */ if (acpi_pci_power_manageable(dev)) return true; /* * Look for 'HotPlugSupportInD3' property for the root port and if * it is set we know the hierarchy behind it supports D3 just fine. */ rpdev = pcie_find_root_port(dev); if (!rpdev) return false; if (device_property_read_u8(&rpdev->dev, "HotPlugSupportInD3", &val)) return false; return val == 1; } If not, I'll do changes like this. static bool acpi_pci_bridge_d3(struct pci_dev *dev) { const union acpi_object *obj; struct acpi_device *adev; struct pci_dev *rpdev; if (!dev->is_hotplug_bridge) return false; /* Assume D3 support if the bridge is power-manageable by ACPI. */ if (acpi_pci_power_manageable(dev)) return true; /* * Look for 'HotPlugSupportInD3' property for the root port and if * it is set we know the hierarchy behind it supports D3 just fine. */ rpdev = pcie_find_root_port(dev); if (!rpdev) return false; adev = ACPI_COMPANION(&rpdev->dev); if (!adev) return false; if (acpi_dev_get_property(adev, "HotPlugSupportInD3", ACPI_TYPE_INTEGER, &obj) < 0) return false; return obj->integer.value == 1; }
On Sat, Aug 14, 2021 at 11:16:11AM -0500, Shanker R Donthineni wrote: > Hi Bjorn, > > On 8/13/21 11:10 PM, Bjorn Helgaas wrote: > >>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > >>>> index eaddbf701759..dae021322b3f 100644 > >>>> --- a/drivers/pci/pci-acpi.c > >>>> +++ b/drivers/pci/pci-acpi.c > >>>> @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) > >>>> return false; > >>>> > >>>> /* Assume D3 support if the bridge is power-manageable by ACPI. */ > >>>> - pci_set_acpi_fwnode(dev); > >>>> adev = ACPI_COMPANION(&dev->dev); > >>> I *think* the Root Port code farther down in this function is also now > >>> unnecessary: > >>> > >>> acpi_pci_bridge_d3(...) > >>> { > >>> ... > >>> root = pcie_find_root_port(dev); > >>> adev = ACPI_COMPANION(&root->dev); > >>> if (root == dev) { > >>> /* > >>> * It is possible that the ACPI companion is not yet bound > >>> * for the root port so look it up manually here. > >>> */ > >>> if (!adev && !pci_dev_is_added(root)) > >>> adev = acpi_pci_find_companion(&root->dev); > >>> } > >>> > >>> Since we're now setting the ACPI_COMPANION for every pci_dev long > >>> before we get here, I think this could now be simplified to something > >>> like this: > >>> > >>> acpi_pci_bridge_d3(...) > >>> { > >>> if (!dev->is_hotplug_bridge) > >>> return false; > >>> > >>> adev = ACPI_COMPANION(&dev->dev); > >>> if (adev && acpi_device_power_manageable(adev)) > >>> return true; > >>> > >>> root = pcie_find_root_port(dev); > >>> if (!root) > >>> return false; > >>> > >>> adev = ACPI_COMPANION(&root->dev); > >>> if (!adev) > >>> return false; > >>> > >>> rc = acpi_dev_get_property(dev, "HotPlugSupportInD3", > >>> ACPI_TYPE_INTEGER, &val); > >>> if (rc < 0) > >>> return false; > >>> > >>> return val == 1; > >>> } > >> Agree, thanks for your suggestion. Yes, it can be simplified too. > >> Can I do something like this using the unified device property API? > >> > >> static bool acpi_pci_bridge_d3(struct pci_dev *dev) > >> { > >> struct acpi_device *adev; > >> struct pci_dev *root; > >> u8 val; > >> > >> if (!dev->is_hotplug_bridge) > >> return false; > >> > >> adev = ACPI_COMPANION(&dev->dev); > >> if (adev && acpi_device_power_manageable(adev)) > >> return true; > >> > >> root = pcie_find_root_port(dev); > >> if (!root) > >> return false; > >> > >> if (device_property_read_u8(&root->dev, "HotPlugSupportInD3", &val)) > >> return false; > > I guess that might be OK. > > > > TBH I don't really like the device_property_read_u8() thing because > > (1) we know this is an ACPI property and I don't see a reason to use > > an "generic" interface that doesn't buy us anything, and (2) the > > connection to the source of the data (a _DSD method) is really, really > > hard to find. > > > > Admittedly, it's still pretty hard to connect acpi_dev_get_property() > > with "_DSD". The only real clue is the comment about "Look for a > > special _DSD property ..." > > > Does it satisfy you if I change the comment and still use device_property API? > > static bool acpi_pci_bridge_d3(struct pci_dev *dev) > { > struct pci_dev *rpdev; > u8 val; > > if (!dev->is_hotplug_bridge) > return false; > > /* Assume D3 support if the bridge is power-manageable by ACPI. */ > if (acpi_pci_power_manageable(dev)) > return true; > > /* > * Look for 'HotPlugSupportInD3' property for the root port and if > * it is set we know the hierarchy behind it supports D3 just fine. > */ > rpdev = pcie_find_root_port(dev); > if (!rpdev) > return false; > > if (device_property_read_u8(&rpdev->dev, "HotPlugSupportInD3", &val)) > return false; > > return val == 1; > } > > If not, I'll do changes like this. I guess either one is fine. But I think we should extend the comment and commit log to make it clear that device_property_read_u8() and acpi_dev_get_property() are ultimately looking for a _DSD. I should have asked for this when we merged 26ad34d510a8 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports") in the first place. If we expect that power management *should* be enabled for a bridge, and we observe that it *isn't* enabled, it is unreasonably difficult to figure out from the code what is missing in the firmware, namely, the _DSD laid out in the commit log for 26ad34d510a8. > static bool acpi_pci_bridge_d3(struct pci_dev *dev) > { > const union acpi_object *obj; > struct acpi_device *adev; > struct pci_dev *rpdev; > > > if (!dev->is_hotplug_bridge) > return false; > > /* Assume D3 support if the bridge is power-manageable by ACPI. */ > if (acpi_pci_power_manageable(dev)) > return true; > > /* > * Look for 'HotPlugSupportInD3' property for the root port and if > * it is set we know the hierarchy behind it supports D3 just fine. > */ > rpdev = pcie_find_root_port(dev); > if (!rpdev) > return false; > > adev = ACPI_COMPANION(&rpdev->dev); > if (!adev) > return false; > > if (acpi_dev_get_property(adev, "HotPlugSupportInD3", > ACPI_TYPE_INTEGER, &obj) < 0) > return false; > > return obj->integer.value == 1; > } > >
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index eaddbf701759..dae021322b3f 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) return false; /* Assume D3 support if the bridge is power-manageable by ACPI. */ - pci_set_acpi_fwnode(dev); adev = ACPI_COMPANION(&dev->dev); if (adev && acpi_device_power_manageable(adev)) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 379e85037d9b..15a6975d3757 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1789,6 +1789,9 @@ int pci_setup_device(struct pci_dev *dev) dev->error_state = pci_channel_io_normal; set_pcie_port_type(dev); + pci_set_of_node(dev); + pci_set_acpi_fwnode(dev); + pci_dev_assign_slot(dev); /* @@ -1924,6 +1927,7 @@ int pci_setup_device(struct pci_dev *dev) default: /* unknown header */ pci_err(dev, "unknown header type %02x, ignoring device\n", dev->hdr_type); + pci_release_of_node(dev); return -EIO; bad: @@ -2351,10 +2355,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn) dev->vendor = l & 0xffff; dev->device = (l >> 16) & 0xffff; - pci_set_of_node(dev); - if (pci_setup_device(dev)) { - pci_release_of_node(dev); pci_bus_put(dev->bus); kfree(dev); return NULL;