On Wed, Jun 26, 2024 at 2:38 PM Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx> wrote: > > Currently the power control driver is child of pci-pci bridge driver, > this will cause issue when suspend resume is introduced in the pwr > control driver. If the supply is removed to the endpoint in the > power control driver then the config space access initaited by the > pci-pci bridge driver can cause issues like Timeouts. > > For this reason change the parent to controller from pci-pci bridge. Newline before trailers please. > Signed-off-by: Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx> > --- > drivers/pci/bus.c | 5 +++-- > drivers/pci/pwrctl/core.c | 7 ++++++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 3e3517567721..eedab4aabd81 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -335,6 +335,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { } > void pci_bus_add_device(struct pci_dev *dev) > { > struct device_node *dn = dev->dev.of_node; > + struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > int retval; > > /* > @@ -356,9 +357,9 @@ void pci_bus_add_device(struct pci_dev *dev) > > pci_dev_assign_added(dev, true); > > - if (pci_is_bridge(dev)) { > + if (pci_is_bridge(dev) && host) { I know I told you to check the return value of pci_find_host_bridge() in private but now after a second look I see it's just a multi-layer wrapper around container_of() and it looks like it cannot fail so - correct me if I'm wrong - this can be dropped after all. > retval = of_platform_populate(dev->dev.of_node, NULL, NULL, > - &dev->dev); > + host->dev.parent); > if (retval) > pci_err(dev, "failed to populate child OF nodes (%d)\n", > retval); > diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c > index feca26ad2f6a..4c0d0f3b15f8 100644 > --- a/drivers/pci/pwrctl/core.c > +++ b/drivers/pci/pwrctl/core.c > @@ -10,6 +10,7 @@ > #include <linux/pci-pwrctl.h> > #include <linux/property.h> > #include <linux/slab.h> New line here, please. > +#include "../pci.h" > > static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action, > void *data) > @@ -64,18 +65,22 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action, > */ > int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl) > { > + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(pwrctl->dev->parent->of_node), 0); > int ret; > > if (!pwrctl->dev) > return -ENODEV; > > + if (!bus) > + return -ENODEV; This - on the other hand - can fail, so the check if valid. Could you assign it and then test it in a single spot for better readability? Bart > + > pwrctl->nb.notifier_call = pci_pwrctl_notify; > ret = bus_register_notifier(&pci_bus_type, &pwrctl->nb); > if (ret) > return ret; > > pci_lock_rescan_remove(); > - pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus); > + pci_rescan_bus(bus); > pci_unlock_rescan_remove(); > > return 0; > > -- > 2.42.0 >