Re: [PATCH v3 2/5] PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()

[Lukas Wunner]

On Thu, Jan 16, 2025 at 07:39:12PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> PCI core will try to access the devices even after pci_stop_dev() for
> things like Data Object Exchange (DOE), ASPM etc... So move
> pci_pwrctrl_unregister() to the near end of pci_destroy_dev() to make sure
> that the devices are powered down only after the PCI core is done with
> them.
[...]
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -41,7 +41,6 @@ static void pci_stop_dev(struct pci_dev *dev)
>  	if (!pci_dev_test_and_clear_added(dev))
>  		return;
>  
> -	pci_pwrctrl_unregister(&dev->dev);
>  	device_release_driver(&dev->dev);
>  	pci_proc_detach_device(dev);
>  	pci_remove_sysfs_dev_files(dev);
> @@ -64,6 +63,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
>  	pci_doe_destroy(dev);
>  	pcie_aspm_exit_link_state(dev);
>  	pci_bridge_d3_update(dev);
> +	pci_pwrctrl_unregister(&dev->dev);
>  	pci_free_resources(dev);
>  	put_device(&dev->dev);
>  }

The above is now commit 2d923930f2e3 ("PCI/pwrctrl: Move
pci_pwrctrl_unregister() to pci_destroy_dev()"), which went
into v6.15-rc1.

While inspecting the code for an unrelated issue, I noticed a
potential ordering problem here:

Prior to 2d923930f2e3, pci_pwrctrl_unregister() was called before
of_pci_remove_node().  The order is reversed now.

So if the of_node of a PCI device was created dynamically (note the
OF_DYNAMIC check in of_pci_remove_node()), the of_node may now be
destroyed and pdev->dev.of_node may be set to NULL.

Afterwards pci_pwrctrl_unregister() bails out for lack of an of_node.

It's a change of behavior vis-à-vis what the code did prior to
2d923930f2e3.

I don't have a board using a power controller for a PCI device,
so I'm not really sure if this is an issue in practice and if so,
how it should be solved.  One obvious solution would be to move the
invocation of of_pci_remove_node() to pci_destroy_dev(), after the
invocation of pci_pwrctrl_unregister().

However I'm confused that of_pci_remove_node() is currently gated by
the PCI_DEV_ADDED flag.  Well I guess the reason is that the counterpart,
of_pci_make_dev_node(), is gated by the flag as well.  But why?
And why is the latter additionally gated by pci_is_bridge() but the
former is not?

Thanks,

Lukas