Re: [PATCH 6.13] PCI/pwrctrl: Skip NULL of_node when unregistering

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Jon, Saurabh]

On Tue, Nov 26, 2024 at 01:04:34PM -0800, Brian Norris wrote:
> of_find_device_by_node() doesn't like a NULL pointer, and may end up
> identifying an arbitrary device, which we then start tearing down. We
> should check for NULL first.

I assume pci_pwrctrl_unregister() is unregistering something added by
pci_pwrctrl_create_devices() in pci_bus_add_device()?

There are a couple things I don't like about this code (these are
unrelated to this particular regression fix, but they make me worry
that I'm misunderstanding how this all works):

  - pci_pwrctrl_create_devices() and pci_pwrctrl_unregister() are not
    parallel names.

  - pci_pwrctrl_create_devices() has a loop and potentially creates
    several pwrctrl devices, one for each child that has a "*-supply"
    property, but there is no corresponding loop in
    pci_pwrctrl_unregister().

I see that 681725afb6b9 ("PCI/pwrctl: Remove pwrctl device without
iterating over all children of pwrctl parent") is at least partly
*about* removing without iterating, and the commit log has some
explanation about how pwrctrl devices lying around will get removed,
but the lack of parallelism is a hassle for readers.

The subject and commit log also refer to "*the* pwrctl device," which
suggests there is only one, but that seems slightly misleading even
though it goes on to mention "any pwrctl devices lying around."

If they get removed when the parent gets removed, why do we need to
remove *any* of them?  Is the real point that we need to clear
OF_POPULATED?

Maybe I'm missing the whole point here?

> [  222.965216] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000c0
> ...
> [  223.107395] CPU: 3 UID: 0 PID: 5095 Comm: bash Tainted: G        WC         6.12.0-rc1 #3
> ...
> [  223.227750] Call trace:
> [  223.230501]  pci_stop_bus_device+0x190/0x1b0
> [  223.235314]  pci_stop_and_remove_bus_device_locked+0x28/0x48
> [  223.241672]  remove_store+0xa0/0xb8
> [  223.245616]  dev_attr_store+0x20/0x40
> [  223.249737]  sysfs_kf_write+0x4c/0x68
> [  223.253859]  kernfs_fop_write_iter+0x128/0x200
> [  223.253887]  do_iter_readv_writev+0xdc/0x1e0
> [  223.263631]  vfs_writev+0x100/0x2a0
> [  223.267550]  do_writev+0x84/0x130
> [  223.271273]  __arm64_sys_writev+0x28/0x40
> [  223.275774]  invoke_syscall+0x50/0x120
> [  223.279988]  el0_svc_common.constprop.0+0x48/0xf0
> [  223.285270]  do_el0_svc+0x24/0x38
> [  223.288993]  el0_svc+0x34/0xe0
> [  223.292426]  el0t_64_sync_handler+0x120/0x138
> [  223.297311]  el0t_64_sync+0x190/0x198
> [  223.301423] Code: 17fffff8 91030000 d2800101 f9800011 (c85f7c02)
> [  223.308248] ---[ end trace 0000000000000000 ]---
> 
> Fixes: 681725afb6b9 ("PCI/pwrctl: Remove pwrctl device without iterating over all children of pwrctl parent")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  drivers/pci/remove.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 963b8d2855c1..efc37fcb73e2 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -19,14 +19,19 @@ static void pci_free_resources(struct pci_dev *dev)
>  
>  static void pci_pwrctrl_unregister(struct device *dev)
>  {
> +	struct device_node *np;
>  	struct platform_device *pdev;
>  
> -	pdev = of_find_device_by_node(dev_of_node(dev));
> +	np = dev_of_node(dev);
> +	if (!np)
> +		return;
> +
> +	pdev = of_find_device_by_node(np);
>  	if (!pdev)
>  		return;
>  
>  	of_device_unregister(pdev);
> -	of_node_clear_flag(dev_of_node(dev), OF_POPULATED);
> +	of_node_clear_flag(np, OF_POPULATED);
>  }
>  
>  static void pci_stop_dev(struct pci_dev *dev)
> -- 
> 2.47.0.338
>