On Fri, Nov 29, 2024 at 01:28:11PM -0600, Bjorn Helgaas wrote:
> [+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):
>
The internals of PCI pwrctl is not documented clearly in a single place.
Let me try to clarify below.
> - 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().
So we create pwrctl devices starting from PCI bridges. This is because, in order
for the endpoints to be enumerated, the relevant pwrctl drivers need to be
probed first (i.e., pci_bus_add_device() will be called for the endpoints only
when they are detected on the bus, but that cannot happen until the relevant
pwrctl driver is probed). That's why we have the loop in
pci_pwrctrl_create_devices() to iterate over the children of PCI bridges defined
in devicetree.
When the pwrctl devices are created, they will be destroyed in
pci_pwrctrl_unregister(). Now we don't have loop here because, this function
is called from pci_stop_dev() which will be called for each added PCI devices.
So even when the bridge device gets removed, all of its child devices will be
removed first, so we don't need to iterate.
>
> 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."
>
With 'lying around', the commit meant that if the pwrctl device is not
associated with the PCI device (it can happen if the relevant PCI client driver
is not probed), then those dormant pwrctl devices will be removed when their
parent (PCI bridge) gets removed. Because for those PCI devices, pci_stop_dev()
won't be called.
> 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?
>
The idea of pci_pwrctrl_unregister() is to remove the pwrctl device when the
associated PCI device gets removed. When this happens, the pwrctl driver will
turn off the power to the corresponding PCI device (in other words, it reverses
what it did in probe()). That's the primary reason why we are removing the
pwrctl device when the PCI device gets removed.
Clearing the OF_POPULATED should also be done separately because of the
shortcomings of the of_platform_depopulate() API. See: commit f1536585588b
("PCI: Don't rely on of_platform_depopulate() for reused OF-nodes").
- Mani
> 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@xxxxxxxxxxxx>
> > ---
> >
> > 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
> >
--
மணிவண்ணன் சதாசிவம்
