Re: [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes

[Bartosz Golaszewski <brgl@bgdev.pl>]

On Wed, Jun 26, 2024 at 2:38 PM Krishna chaitanya chundru
<quic_krichai@quicinc.com> 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@quicinc.com>
> ---
>  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
>