Re: [PATCH 1/4] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

[Lukas Wunner <lukas@wunner.de>]

On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2446,6 +2448,36 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>  }
>  EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
>  
> +/*
> + * Create pwrctrl devices (if required) for the PCI devices to handle the power
> + * state.
> + */
> +static void pci_pwrctrl_create_devices(struct pci_bus *bus, int devfn)

Nit:  AFAICS this only creates a *single* platform device, so the
"devices" (plural) in the function name and in the code comment
doesn't seem to be accurate anymore.


> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 2fb174db91e5..9cc7e2b7f2b5 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -44,7 +44,7 @@ static void rescan_work_func(struct work_struct *work)
>  						   struct pci_pwrctrl, work);
>  
>  	pci_lock_rescan_remove();
> -	pci_rescan_bus(to_pci_dev(pwrctrl->dev->parent)->bus);
> +	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
>  	pci_unlock_rescan_remove();
>  }

Remind me, what's the purpose of this?  I'm guessing that it
recursively creates the platform devices below the newly
powered up device, is that correct?  If so, is it still
necessary?  Doesn't the new approach automatically create
those devices upon their enumeration?

Overall it looks like a significant improvement, thanks for doing this!

Lukas