Re: [PATCH] of: dynamic: notify before revert

[Rob Herring]

On Thu, Feb 29, 2024 at 2:01 AM Peng Fan <peng.fan@xxxxxxx> wrote:
>
> > Subject: Re: [PATCH] of: dynamic: notify before revert
> >
> > On Wed, Feb 28, 2024 at 12:13 AM Peng Fan (OSS) <peng.fan@xxxxxxxxxxx>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@xxxxxxx>
> > >
> > > When PCI node was created using an overlay and the overlay is
> > > reverted/destroyed, the "linux,pci-domain" property no longer exists,
> > > so of_get_pci_domain_nr will return failure. Then
> > > of_pci_bus_release_domain_nr will actually use the dynamic IDA, even
> > > if the IDA was allocated in static IDA.
> >
> > That usage is broken to begin with unless there is a guarantee that static and
> > dynamic domain numbers don't overlap. For example, a dynamic number is
> > assigned and then you load an overlay with the same number defined in it.
>
> I may not describe it clear, the code is here, because overlay property
> removed, of_get_pci_domain_nr will return failure, so the code path
> will goest into free a dynamic ida. But actually there is no such dynamic
> ida, so dump.

I understood the problem.

Your usage of this is broken on applying your overlay. You just got lucky.

> So the problem is overlay was removed, but the notify callback may
> still use the property to do some work.
>
> static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *parent)
> {
>         if (bus->domain_nr < 0)
>                 return;
>
>         /* Release domain from IDA where it was allocated. */
>         if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
>                 ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
>         else
>                 ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
> }
> >
> > > So move the notify before revert to fix the issue.
> >
> > You can't just change the timing. Something might require notify to be after
> > the revert.

Again ^^^

> >
> > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> >
> > I don't see where the notifier is even used.
>
> The stack is as below:
>
>  [  595.150529] CPU: 1 PID: 582 Comm: jailhouse Tainted: G           O       6.5.0-rc4-next-20230804-05021-g3d4cc14b42ef-dirty #355
>  [  595.161998] Hardware name: NXP i.MX93 11X11 EVK board (DT)
>  [  595.167475] Call trace:
>  [  595.169908]  dump_backtrace+0x94/0xec
>  [  595.173573]  show_stack+0x18/0x24
>  [  595.176884]  dump_stack_lvl+0x48/0x60
>  [  595.180541]  dump_stack+0x18/0x24
>  [  595.183843]  pci_bus_release_domain_nr+0x34/0x84
>  [  595.188453]  pci_remove_root_bus+0xa0/0xa4
>  [  595.192544]  pci_host_common_remove+0x28/0x40
>  [  595.196895]  platform_remove+0x54/0x6c
>  [  595.200641]  device_remove+0x4c/0x80
>  [  595.204209]  device_release_driver_internal+0x1d4/0x230
>  [  595.209430]  device_release_driver+0x18/0x24
>  [  595.213691]  bus_remove_device+0xcc/0x10c
>  [  595.217686]  device_del+0x15c/0x41c
>  [  595.221170]  platform_device_del.part.0+0x1c/0x88
>  [  595.225861]  platform_device_unregister+0x24/0x40
>  [  595.230557]  of_platform_device_destroy+0xfc/0x10c
>  [  595.235344]  of_platform_notify+0x13c/0x178
>  [  595.239518]  blocking_notifier_call_chain+0x6c/0xa0
>  [  595.244389]  __of_changeset_entry_notify+0x148/0x16c
>  [  595.249346]  of_changeset_revert+0xa8/0xcc
>  [  595.253437]  jailhouse_pci_virtual_root_devices_remove+0x50/0x74 [jailhouse]

$ git grep jailhouse_pci_virtual_root_devices_remove
(END)

Another out of tree overlay user. I have little interest in worrying
about them. No one wants to step up and solve the problems with
overlays, so I'm not going to worry about them either.

Rob