Received-SPF: pass (google.com: domain of linux-kernel+bounces-87602-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240228062138.1275542-1-peng.fan@oss.nxp.com>
 <CAL_JsqJktYv+sSfwtT6ExsES5+HqB2BStnEMMRKVqTO3dFJm2Q@mail.gmail.com> <DU0PR04MB94177FFE211711E9DE4E9E51885F2@DU0PR04MB9417.eurprd04.prod.outlook.com>
In-Reply-To: <DU0PR04MB94177FFE211711E9DE4E9E51885F2@DU0PR04MB9417.eurprd04.prod.outlook.com>
From: Rob Herring <robh@kernel.org>
Date: Thu, 29 Feb 2024 15:50:04 -0600
Message-ID: <CAL_JsqL4cb8NVOV9QF5__PtjyDUQd-MnuhJhbTHmUF3qr3x47w@mail.gmail.com>
Subject: Re: [PATCH] of: dynamic: notify before revert
To: Peng Fan <peng.fan@nxp.com>
Cc: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>, "saravanak@google.com" <saravanak@google.com>, 
	"bhelgaas@google.com" <bhelgaas@google.com>, "pali@kernel.org" <pali@kernel.org>, 
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 29, 2024 at 2:01=E2=80=AFAM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH] of: dynamic: notify before revert
> >
> > On Wed, Feb 28, 2024 at 12:13=E2=80=AFAM Peng Fan (OSS) <peng.fan@oss.n=
xp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > 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 sta=
tic 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 i=
t.
>
> 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 devi=
ce *parent)
> {
>         if (bus->domain_nr < 0)
>                 return;
>
>         /* Release domain from IDA where it was allocated. */
>         if (of_get_pci_domain_nr(parent->of_node) =3D=3D 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 [jai=
lhouse]

$ 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