From: Peng Fan <[email protected]>
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.
Introduce a static_nr field in pci_bus to indicate whether the
IDA is a dynamic or static in order to free the correct one.
Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
Signed-off-by: Peng Fan <[email protected]>
---
V2:
Update commit message
Move static_nr:1 to stay besides others :1 fields.
drivers/pci/pci.c | 22 ++++++++++++++--------
include/linux/pci.h | 1 +
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0..5c98502bcda6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6881,10 +6881,10 @@ static void of_pci_reserve_static_domain_nr(void)
}
}
-static int of_pci_bus_find_domain_nr(struct device *parent)
+static int of_pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
{
static bool static_domains_reserved = false;
- int domain_nr;
+ int domain_nr, ret;
/* On the first call scan device tree for static allocations. */
if (!static_domains_reserved) {
@@ -6892,6 +6892,8 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
static_domains_reserved = true;
}
+ bus->static_nr = 0;
+
if (parent) {
/*
* If domain is in DT, allocate it in static IDA. This
@@ -6899,10 +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
* in DT.
*/
domain_nr = of_get_pci_domain_nr(parent->of_node);
- if (domain_nr >= 0)
- return ida_alloc_range(&pci_domain_nr_static_ida,
- domain_nr, domain_nr,
- GFP_KERNEL);
+ if (domain_nr >= 0) {
+ ret = ida_alloc_range(&pci_domain_nr_static_ida,
+ domain_nr, domain_nr, GFP_KERNEL);
+ if (ret >= 0)
+ bus->static_nr = 1;
+
+ return ret;
+ }
}
/*
@@ -6920,7 +6926,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *par
return;
/* Release domain from IDA where it was allocated. */
- if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
+ if (bus->static_nr)
ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
else
ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
@@ -6928,7 +6934,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *par
int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
{
- return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
+ return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
acpi_pci_bus_find_domain_nr(bus);
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index eeb2e6f6130f..222a1729ea7e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -677,6 +677,7 @@ struct pci_bus {
struct bin_attribute *legacy_mem; /* Legacy mem */
unsigned int is_added:1;
unsigned int unsafe_warn:1; /* warned about RW1C config write */
+ unsigned int static_nr:1;
};
#define to_pci_bus(n) container_of(n, struct pci_bus, dev)
--
2.37.1
In subject, run "git log --oneline drivers/pci/pci.c" and match the
capitalization style.
On Tue, Aug 15, 2023 at 09:37:44AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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.
Is there any user-visible effect of this issue? For example, is there
any hint in the dmesg log? If so, we should include that hint here.
> Introduce a static_nr field in pci_bus to indicate whether the
> IDA is a dynamic or static in order to free the correct one.
>
> Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> Signed-off-by: Peng Fan <[email protected]>
> ---
>
> V2:
> Update commit message
> Move static_nr:1 to stay besides others :1 fields.
>
> drivers/pci/pci.c | 22 ++++++++++++++--------
> include/linux/pci.h | 1 +
> 2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60230da957e0..5c98502bcda6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6881,10 +6881,10 @@ static void of_pci_reserve_static_domain_nr(void)
> }
> }
>
> -static int of_pci_bus_find_domain_nr(struct device *parent)
> +static int of_pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> {
> static bool static_domains_reserved = false;
> - int domain_nr;
> + int domain_nr, ret;
>
> /* On the first call scan device tree for static allocations. */
> if (!static_domains_reserved) {
> @@ -6892,6 +6892,8 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> static_domains_reserved = true;
> }
>
> + bus->static_nr = 0;
> +
> if (parent) {
> /*
> * If domain is in DT, allocate it in static IDA. This
> @@ -6899,10 +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> * in DT.
> */
> domain_nr = of_get_pci_domain_nr(parent->of_node);
> - if (domain_nr >= 0)
> - return ida_alloc_range(&pci_domain_nr_static_ida,
> - domain_nr, domain_nr,
> - GFP_KERNEL);
> + if (domain_nr >= 0) {
> + ret = ida_alloc_range(&pci_domain_nr_static_ida,
> + domain_nr, domain_nr, GFP_KERNEL);
> + if (ret >= 0)
> + bus->static_nr = 1;
There are a couple things that make this a little messy:
1) The "static_nr" name isn't great because the struct pci_bus
declaration doesn't have any clue about what it's for. "nr" doesn't
suggest "PCI domain".
2) More importantly, the "static_nr = 1" assignment here doesn't fit
with the design where pci_bus_find_domain_nr() allocates the domain
number and pci_register_host_bridge() stores it in the struct
pci_bus. The bus->domain_nr assignment and the bus->static_nr
assignment really should be in the same place.
I do see the problem that it's hard to return the "static alloc"
info back up the call chain to pci_register_host_bridge() where
bus->domain_nr is assigned, but setting bus->static_nr here in
of_pci_bus_find_domain_nr() seems a little kludgy. I don't have any
good ideas yet, but I'll think about it.
> + return ret;
> + }
> }
>
> /*
> @@ -6920,7 +6926,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *par
> return;
>
> /* Release domain from IDA where it was allocated. */
> - if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
> + if (bus->static_nr)
> ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
> else
> ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
> @@ -6928,7 +6934,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *par
>
> int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> {
> - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
> acpi_pci_bus_find_domain_nr(bus);
> }
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index eeb2e6f6130f..222a1729ea7e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -677,6 +677,7 @@ struct pci_bus {
> struct bin_attribute *legacy_mem; /* Legacy mem */
> unsigned int is_added:1;
> unsigned int unsafe_warn:1; /* warned about RW1C config write */
> + unsigned int static_nr:1;
> };
>
> #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
> --
> 2.37.1
>
On Tue, Aug 15, 2023 at 09:37:44AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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.
I'm not familiar with how overlays work. What's the call path where
the overlay is removed? I see an of_overlay_remove(), but I don't see
any callers except test cases.
I guess the problem happens in a PCI host bridge remove path, e.g.,
pci_host_common_remove
pci_remove_root_bus
pci_bus_release_domain_nr
of_pci_bus_release_domain_nr
But I don't know how that's related to the overlay removal.
Is this an ordering issue? It seems possibly problematic that the OF
overlay is destroyed before the device it describes (e.g., the host
bridge) is removed. I would expect the device to be removed before
the description of the device is removed.
> Then of_pci_bus_release_domain_nr will actually use the dynamic IDA,
> even if the IDA was allocated in static IDA.
>
> Introduce a static_nr field in pci_bus to indicate whether the
> IDA is a dynamic or static in order to free the correct one.
>
> Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> Signed-off-by: Peng Fan <[email protected]>
> ---
>
> V2:
> Update commit message
> Move static_nr:1 to stay besides others :1 fields.
>
> drivers/pci/pci.c | 22 ++++++++++++++--------
> include/linux/pci.h | 1 +
> 2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60230da957e0..5c98502bcda6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6881,10 +6881,10 @@ static void of_pci_reserve_static_domain_nr(void)
> }
> }
>
> -static int of_pci_bus_find_domain_nr(struct device *parent)
> +static int of_pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> {
> static bool static_domains_reserved = false;
> - int domain_nr;
> + int domain_nr, ret;
>
> /* On the first call scan device tree for static allocations. */
> if (!static_domains_reserved) {
> @@ -6892,6 +6892,8 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> static_domains_reserved = true;
> }
>
> + bus->static_nr = 0;
> +
> if (parent) {
> /*
> * If domain is in DT, allocate it in static IDA. This
> @@ -6899,10 +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> * in DT.
> */
> domain_nr = of_get_pci_domain_nr(parent->of_node);
> - if (domain_nr >= 0)
> - return ida_alloc_range(&pci_domain_nr_static_ida,
> - domain_nr, domain_nr,
> - GFP_KERNEL);
> + if (domain_nr >= 0) {
> + ret = ida_alloc_range(&pci_domain_nr_static_ida,
> + domain_nr, domain_nr, GFP_KERNEL);
> + if (ret >= 0)
> + bus->static_nr = 1;
> +
> + return ret;
> + }
> }
>
> /*
> @@ -6920,7 +6926,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *par
> return;
>
> /* Release domain from IDA where it was allocated. */
> - if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
> + if (bus->static_nr)
> ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
> else
> ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
> @@ -6928,7 +6934,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *par
>
> int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> {
> - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
> acpi_pci_bus_find_domain_nr(bus);
> }
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index eeb2e6f6130f..222a1729ea7e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -677,6 +677,7 @@ struct pci_bus {
> struct bin_attribute *legacy_mem; /* Legacy mem */
> unsigned int is_added:1;
> unsigned int unsafe_warn:1; /* warned about RW1C config write */
> + unsigned int static_nr:1;
> };
>
> #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
> --
> 2.37.1
>
On Wed, Sep 13, 2023 at 01:24:26AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V2] pci: introduce static_nr to indicate domain_nr from
> > which IDA
> >
> > On Tue, Aug 15, 2023 at 09:37:44AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <[email protected]>
> > >
> > > 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.
> >
> > I'm not familiar with how overlays work. What's the call path
> > where the overlay is removed? I see an of_overlay_remove(), but I
> > don't see any callers except test cases.
>
> We are using an out of tree hypervisor driver:
> https://github.com/siemens/jailhouse/blob/master/driver/pci.c#L483
> >
> > I guess the problem happens in a PCI host bridge remove path, e.g.,
> >
> > pci_host_common_remove
> > pci_remove_root_bus
> > pci_bus_release_domain_nr
> > of_pci_bus_release_domain_nr
> >
> > But I don't know how that's related to the overlay removal.
>
> When the overlay node got removed, the device removal will be invoked and
> the domain number indicated by linux,pci-domain should also get freed.
>
> But actually the domain number not got freed because of bug as this patch
> shows. "of_pci_bus_release_domain_nr will actually use the dynamic IDA,
> even if the IDA was allocated in static IDA."
>
> So after the overlay node got destroyed and our test recreate the overlay node
> with same domain number, issue triggered, the device could not be
> created.
>
> > Is this an ordering issue? It seems possibly problematic that the
> > OF overlay is destroyed before the device it describes (e.g., the
> > host
>
> No. it is "of_pci_bus_release_domain_nr will actually use the
> dynamic IDA, even if the IDA was allocated in static IDA "
Yes, that's the *symptom*, to be sure. My question was why the
overlay was removed before the host bridge.
The current ordering is this, where A happens before B:
A overlay is removed
B pci_host_common_remove
pci_bus_release_domain_nr
of_pci_bus_release_domain_nr
of_get_pci_domain_nr # fails because overlay is gone
ida_free(&pci_domain_nr_dynamic_ida)
But if the host bridge were removed first, the ordering would be as
follows, and the problem would not occur:
B pci_host_common_remove
pci_bus_release_domain_nr
of_pci_bus_release_domain_nr
of_get_pci_domain_nr # succeeds in this order
ida_free(&pci_domain_nr_static_ida)
A overlay is removed
Is there a reason the overlay should be removed before the host
bridge?
Obviously the overlay is loaded before the host bridge is created, and
symmetry would suggest that the overlay should be removed after the
host bridge is removed.
> > bridge) is removed. I would expect the device to be removed before the
> > description of the device is removed.
> >
> > > Then of_pci_bus_release_domain_nr will actually use the dynamic IDA,
> > > even if the IDA was allocated in static IDA.
> > >
> > > Introduce a static_nr field in pci_bus to indicate whether the IDA is
> > > a dynamic or static in order to free the correct one.
> > >
> > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> > > Signed-off-by: Peng Fan <[email protected]>
> > > ---
> > >
> > > V2:
> > > Update commit message
> > > Move static_nr:1 to stay besides others :1 fields.
> > >
> > > drivers/pci/pci.c | 22 ++++++++++++++--------
> > > include/linux/pci.h | 1 +
> > > 2 files changed, 15 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index
> > > 60230da957e0..5c98502bcda6 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -6881,10 +6881,10 @@ static void
> > of_pci_reserve_static_domain_nr(void)
> > > }
> > > }
> > >
> > > -static int of_pci_bus_find_domain_nr(struct device *parent)
> > > +static int of_pci_bus_find_domain_nr(struct pci_bus *bus, struct
> > > +device *parent)
> > > {
> > > static bool static_domains_reserved = false;
> > > - int domain_nr;
> > > + int domain_nr, ret;
> > >
> > > /* On the first call scan device tree for static allocations. */
> > > if (!static_domains_reserved) {
> > > @@ -6892,6 +6892,8 @@ static int of_pci_bus_find_domain_nr(struct
> > device *parent)
> > > static_domains_reserved = true;
> > > }
> > >
> > > + bus->static_nr = 0;
> > > +
> > > if (parent) {
> > > /*
> > > * If domain is in DT, allocate it in static IDA. This @@ -
> > 6899,10
> > > +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> > > * in DT.
> > > */
> > > domain_nr = of_get_pci_domain_nr(parent->of_node);
> > > - if (domain_nr >= 0)
> > > - return ida_alloc_range(&pci_domain_nr_static_ida,
> > > - domain_nr, domain_nr,
> > > - GFP_KERNEL);
> > > + if (domain_nr >= 0) {
> > > + ret = ida_alloc_range(&pci_domain_nr_static_ida,
> > > + domain_nr, domain_nr,
> > GFP_KERNEL);
> > > + if (ret >= 0)
> > > + bus->static_nr = 1;
> > > +
> > > + return ret;
> > > + }
> > > }
> > >
> > > /*
> > > @@ -6920,7 +6926,7 @@ static void of_pci_bus_release_domain_nr(struct
> > pci_bus *bus, struct device *par
> > > return;
> > >
> > > /* Release domain from IDA where it was allocated. */
> > > - if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
> > > + if (bus->static_nr)
> > > ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
> > > else
> > > ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
> > @@ -6928,7
> > > +6934,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus
> > > *bus, struct device *par
> > >
> > > int pci_bus_find_domain_nr(struct pci_bus *bus, struct device
> > > *parent) {
> > > - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> > > + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
> > > acpi_pci_bus_find_domain_nr(bus); }
> > >
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > > eeb2e6f6130f..222a1729ea7e 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -677,6 +677,7 @@ struct pci_bus {
> > > struct bin_attribute *legacy_mem; /* Legacy mem */
> > > unsigned int is_added:1;
> > > unsigned int unsafe_warn:1; /* warned about RW1C
> > config write */
> > > + unsigned int static_nr:1;
> > > };
> > >
> > > #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
> > > --
> > > 2.37.1
> > >
> Subject: Re: [PATCH V2] pci: introduce static_nr to indicate domain_nr from
> which IDA
>
> On Wed, Sep 13, 2023 at 01:24:26AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH V2] pci: introduce static_nr to indicate
> > > domain_nr from which IDA
> > >
> > > On Tue, Aug 15, 2023 at 09:37:44AM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <[email protected]>
> > > >
> > > > 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.
> > >
> > > I'm not familiar with how overlays work. What's the call path where
> > > the overlay is removed? I see an of_overlay_remove(), but I don't
> > > see any callers except test cases.
> >
> > We are using an out of tree hypervisor driver:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> >
> ub.com%2Fsiemens%2Fjailhouse%2Fblob%2Fmaster%2Fdriver%2Fpci.c%23L
> 483&d
> >
> ata=05%7C01%7Cpeng.fan%40nxp.com%7C1bf0364c585f42dae7f108dbb3fe
> fcc4%7C
> >
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63830167996269745
> 6%7CUnknow
> >
> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLC
> >
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=raIsz2HIH%2FQ0nmolBBwd81
> WxQh2vmFeXN
> > t3n6OIQaSU%3D&reserved=0
> > >
> > > I guess the problem happens in a PCI host bridge remove path, e.g.,
> > >
> > > pci_host_common_remove
> > > pci_remove_root_bus
> > > pci_bus_release_domain_nr
> > > of_pci_bus_release_domain_nr
> > >
> > > But I don't know how that's related to the overlay removal.
> >
> > When the overlay node got removed, the device removal will be invoked
> > and the domain number indicated by linux,pci-domain should also get freed.
> >
> > But actually the domain number not got freed because of bug as this
> > patch shows. "of_pci_bus_release_domain_nr will actually use the
> > dynamic IDA, even if the IDA was allocated in static IDA."
> >
> > So after the overlay node got destroyed and our test recreate the
> > overlay node with same domain number, issue triggered, the device
> > could not be created.
> >
> > > Is this an ordering issue? It seems possibly problematic that the
> > > OF overlay is destroyed before the device it describes (e.g., the
> > > host
> >
> > No. it is "of_pci_bus_release_domain_nr will actually use the dynamic
> > IDA, even if the IDA was allocated in static IDA "
>
> Yes, that's the *symptom*, to be sure. My question was why the overlay was
> removed before the host bridge.
I am not sure.
I added a dump_stack, would this help to understand?
root@imx93evk:~# jailhouse disable
[ 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]
[ 595.260484] jailhouse_cmd_disable+0x70/0x1ac [jailhouse]
[ 595.265883] jailhouse_ioctl+0x60/0xf0 [jailhouse]
[ 595.270674] __arm64_sys_ioctl+0xac/0xf0
[ 595.274595] invoke_syscall+0x48/0x114
[ 595.278336] el0_svc_common.constprop.0+0xc4/0xe4
>
> The current ordering is this, where A happens before B:
>
> A overlay is removed
> B pci_host_common_remove
> pci_bus_release_domain_nr
> of_pci_bus_release_domain_nr
> of_get_pci_domain_nr # fails because overlay is gone
> ida_free(&pci_domain_nr_dynamic_ida)
>
> But if the host bridge were removed first, the ordering would be as follows,
> and the problem would not occur:
>
> B pci_host_common_remove
> pci_bus_release_domain_nr
> of_pci_bus_release_domain_nr
> of_get_pci_domain_nr # succeeds in this order
> ida_free(&pci_domain_nr_static_ida)
> A overlay is removed
>
> Is there a reason the overlay should be removed before the host bridge?
>
> Obviously the overlay is loaded before the host bridge is created, and
> symmetry would suggest that the overlay should be removed after the host
> bridge is removed.
Hmm, I need check more. But do you think this patch is fix a real issue or
If order is did as you say above, this patch is not required anymore?
Thanks,
Peng.
>
> > > bridge) is removed. I would expect the device to be removed before
> > > the description of the device is removed.
> > >
> > > > Then of_pci_bus_release_domain_nr will actually use the dynamic
> > > > IDA, even if the IDA was allocated in static IDA.
> > > >
> > > > Introduce a static_nr field in pci_bus to indicate whether the IDA
> > > > is a dynamic or static in order to free the correct one.
> > > >
> > > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> > > > Signed-off-by: Peng Fan <[email protected]>
> > > > ---
> > > >
> > > > V2:
> > > > Update commit message
> > > > Move static_nr:1 to stay besides others :1 fields.
> > > >
> > > > drivers/pci/pci.c | 22 ++++++++++++++--------
> > > > include/linux/pci.h | 1 +
> > > > 2 files changed, 15 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index
> > > > 60230da957e0..5c98502bcda6 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -6881,10 +6881,10 @@ static void
> > > of_pci_reserve_static_domain_nr(void)
> > > > }
> > > > }
> > > >
> > > > -static int of_pci_bus_find_domain_nr(struct device *parent)
> > > > +static int of_pci_bus_find_domain_nr(struct pci_bus *bus, struct
> > > > +device *parent)
> > > > {
> > > > static bool static_domains_reserved = false;
> > > > - int domain_nr;
> > > > + int domain_nr, ret;
> > > >
> > > > /* On the first call scan device tree for static allocations. */
> > > > if (!static_domains_reserved) {
> > > > @@ -6892,6 +6892,8 @@ static int of_pci_bus_find_domain_nr(struct
> > > device *parent)
> > > > static_domains_reserved = true;
> > > > }
> > > >
> > > > + bus->static_nr = 0;
> > > > +
> > > > if (parent) {
> > > > /*
> > > > * If domain is in DT, allocate it in static IDA. This @@ -
> > > 6899,10
> > > > +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device
> > > > +*parent)
> > > > * in DT.
> > > > */
> > > > domain_nr = of_get_pci_domain_nr(parent->of_node);
> > > > - if (domain_nr >= 0)
> > > > - return ida_alloc_range(&pci_domain_nr_static_ida,
> > > > - domain_nr, domain_nr,
> > > > - GFP_KERNEL);
> > > > + if (domain_nr >= 0) {
> > > > + ret = ida_alloc_range(&pci_domain_nr_static_ida,
> > > > + domain_nr, domain_nr,
> > > GFP_KERNEL);
> > > > + if (ret >= 0)
> > > > + bus->static_nr = 1;
> > > > +
> > > > + return ret;
> > > > + }
> > > > }
> > > >
> > > > /*
> > > > @@ -6920,7 +6926,7 @@ static void
> > > > of_pci_bus_release_domain_nr(struct
> > > pci_bus *bus, struct device *par
> > > > return;
> > > >
> > > > /* Release domain from IDA where it was allocated. */
> > > > - if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
> > > > + if (bus->static_nr)
> > > > ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
> > > > else
> > > > ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
> > > @@ -6928,7
> > > > +6934,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus
> > > > *bus, struct device *par
> > > >
> > > > int pci_bus_find_domain_nr(struct pci_bus *bus, struct device
> > > > *parent) {
> > > > - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> > > > + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
> > > > acpi_pci_bus_find_domain_nr(bus); }
> > > >
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > > > eeb2e6f6130f..222a1729ea7e 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -677,6 +677,7 @@ struct pci_bus {
> > > > struct bin_attribute *legacy_mem; /* Legacy mem */
> > > > unsigned int is_added:1;
> > > > unsigned int unsafe_warn:1; /* warned about RW1C
> > > config write */
> > > > + unsigned int static_nr:1;
> > > > };
> > > >
> > > > #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
> > > > --
> > > > 2.37.1
> > > >
> Subject: Re: [PATCH V2] pci: introduce static_nr to indicate domain_nr from
> which IDA
>
> On Tue, Aug 15, 2023 at 09:37:44AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > 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.
>
> I'm not familiar with how overlays work. What's the call path where the
> overlay is removed? I see an of_overlay_remove(), but I don't see any callers
> except test cases.
We are using an out of tree hypervisor driver:
https://github.com/siemens/jailhouse/blob/master/driver/pci.c#L483
>
> I guess the problem happens in a PCI host bridge remove path, e.g.,
>
> pci_host_common_remove
> pci_remove_root_bus
> pci_bus_release_domain_nr
> of_pci_bus_release_domain_nr
>
> But I don't know how that's related to the overlay removal.
When the overlay node got removed, the device removal will be invoked and
the domain number indicated by linux,pci-domain should also get freed.
But actually the domain number not got freed because of bug as this patch
shows. "of_pci_bus_release_domain_nr will actually use the dynamic IDA,
even if the IDA was allocated in static IDA."
So after the overlay node got destroyed and our test recreate the overlay node
with same domain number, issue triggered, the device could not be
created.
>
> Is this an ordering issue? It seems possibly problematic that the OF overlay is
> destroyed before the device it describes (e.g., the host
No. it is " of_pci_bus_release_domain_nr will actually use the dynamic IDA,
even if the IDA was allocated in static IDA "
Thanks,
Peng.
> bridge) is removed. I would expect the device to be removed before the
> description of the device is removed.
>
> > Then of_pci_bus_release_domain_nr will actually use the dynamic IDA,
> > even if the IDA was allocated in static IDA.
> >
> > Introduce a static_nr field in pci_bus to indicate whether the IDA is
> > a dynamic or static in order to free the correct one.
> >
> > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> >
> > V2:
> > Update commit message
> > Move static_nr:1 to stay besides others :1 fields.
> >
> > drivers/pci/pci.c | 22 ++++++++++++++--------
> > include/linux/pci.h | 1 +
> > 2 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index
> > 60230da957e0..5c98502bcda6 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6881,10 +6881,10 @@ static void
> of_pci_reserve_static_domain_nr(void)
> > }
> > }
> >
> > -static int of_pci_bus_find_domain_nr(struct device *parent)
> > +static int of_pci_bus_find_domain_nr(struct pci_bus *bus, struct
> > +device *parent)
> > {
> > static bool static_domains_reserved = false;
> > - int domain_nr;
> > + int domain_nr, ret;
> >
> > /* On the first call scan device tree for static allocations. */
> > if (!static_domains_reserved) {
> > @@ -6892,6 +6892,8 @@ static int of_pci_bus_find_domain_nr(struct
> device *parent)
> > static_domains_reserved = true;
> > }
> >
> > + bus->static_nr = 0;
> > +
> > if (parent) {
> > /*
> > * If domain is in DT, allocate it in static IDA. This @@ -
> 6899,10
> > +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> > * in DT.
> > */
> > domain_nr = of_get_pci_domain_nr(parent->of_node);
> > - if (domain_nr >= 0)
> > - return ida_alloc_range(&pci_domain_nr_static_ida,
> > - domain_nr, domain_nr,
> > - GFP_KERNEL);
> > + if (domain_nr >= 0) {
> > + ret = ida_alloc_range(&pci_domain_nr_static_ida,
> > + domain_nr, domain_nr,
> GFP_KERNEL);
> > + if (ret >= 0)
> > + bus->static_nr = 1;
> > +
> > + return ret;
> > + }
> > }
> >
> > /*
> > @@ -6920,7 +6926,7 @@ static void of_pci_bus_release_domain_nr(struct
> pci_bus *bus, struct device *par
> > return;
> >
> > /* Release domain from IDA where it was allocated. */
> > - if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
> > + if (bus->static_nr)
> > ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
> > else
> > ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
> @@ -6928,7
> > +6934,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus
> > *bus, struct device *par
> >
> > int pci_bus_find_domain_nr(struct pci_bus *bus, struct device
> > *parent) {
> > - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> > + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
> > acpi_pci_bus_find_domain_nr(bus); }
> >
> > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > eeb2e6f6130f..222a1729ea7e 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -677,6 +677,7 @@ struct pci_bus {
> > struct bin_attribute *legacy_mem; /* Legacy mem */
> > unsigned int is_added:1;
> > unsigned int unsafe_warn:1; /* warned about RW1C
> config write */
> > + unsigned int static_nr:1;
> > };
> >
> > #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
> > --
> > 2.37.1
> >
On Wed, Sep 13, 2023 at 02:49:37AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V2] pci: introduce static_nr to indicate domain_nr from
> > which IDA
> >
> > On Wed, Sep 13, 2023 at 01:24:26AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH V2] pci: introduce static_nr to indicate
> > > > domain_nr from which IDA
> > > >
> > > > On Tue, Aug 15, 2023 at 09:37:44AM +0800, Peng Fan (OSS) wrote:
> > > > > From: Peng Fan <[email protected]>
> > > > >
> > > > > 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.
> > > >
> > > > I'm not familiar with how overlays work. What's the call path where
> > > > the overlay is removed? I see an of_overlay_remove(), but I don't
> > > > see any callers except test cases.
> > >
> > > We are using an out of tree hypervisor driver:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> > >
> > ub.com%2Fsiemens%2Fjailhouse%2Fblob%2Fmaster%2Fdriver%2Fpci.c%23L
> > 483&d
> > >
> > ata=05%7C01%7Cpeng.fan%40nxp.com%7C1bf0364c585f42dae7f108dbb3fe
> > fcc4%7C
> > >
> > 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63830167996269745
> > 6%7CUnknow
> > >
> > n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > WwiLC
> > >
> > JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=raIsz2HIH%2FQ0nmolBBwd81
> > WxQh2vmFeXN
> > > t3n6OIQaSU%3D&reserved=0
> > > >
> > > > I guess the problem happens in a PCI host bridge remove path, e.g.,
> > > >
> > > > pci_host_common_remove
> > > > pci_remove_root_bus
> > > > pci_bus_release_domain_nr
> > > > of_pci_bus_release_domain_nr
> > > >
> > > > But I don't know how that's related to the overlay removal.
> > >
> > > When the overlay node got removed, the device removal will be invoked
> > > and the domain number indicated by linux,pci-domain should also get freed.
> > >
> > > But actually the domain number not got freed because of bug as this
> > > patch shows. "of_pci_bus_release_domain_nr will actually use the
> > > dynamic IDA, even if the IDA was allocated in static IDA."
> > >
> > > So after the overlay node got destroyed and our test recreate the
> > > overlay node with same domain number, issue triggered, the device
> > > could not be created.
> > >
> > > > Is this an ordering issue? It seems possibly problematic that the
> > > > OF overlay is destroyed before the device it describes (e.g., the
> > > > host
> > >
> > > No. it is "of_pci_bus_release_domain_nr will actually use the dynamic
> > > IDA, even if the IDA was allocated in static IDA "
> >
> > Yes, that's the *symptom*, to be sure. My question was why the overlay was
> > removed before the host bridge.
>
> I am not sure.
> I added a dump_stack, would this help to understand?
> root@imx93evk:~# jailhouse disable
> [ 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]
> [ 595.260484] jailhouse_cmd_disable+0x70/0x1ac [jailhouse]
> [ 595.265883] jailhouse_ioctl+0x60/0xf0 [jailhouse]
> [ 595.270674] __arm64_sys_ioctl+0xac/0xf0
> [ 595.274595] invoke_syscall+0x48/0x114
> [ 595.278336] el0_svc_common.constprop.0+0xc4/0xe4
> >
> > The current ordering is this, where A happens before B:
> >
> > A overlay is removed
> > B pci_host_common_remove
> > pci_bus_release_domain_nr
> > of_pci_bus_release_domain_nr
> > of_get_pci_domain_nr # fails because overlay is gone
> > ida_free(&pci_domain_nr_dynamic_ida)
> >
> > But if the host bridge were removed first, the ordering would be as follows,
> > and the problem would not occur:
> >
> > B pci_host_common_remove
> > pci_bus_release_domain_nr
> > of_pci_bus_release_domain_nr
> > of_get_pci_domain_nr # succeeds in this order
> > ida_free(&pci_domain_nr_static_ida)
> > A overlay is removed
> >
> > Is there a reason the overlay should be removed before the host bridge?
> >
> > Obviously the overlay is loaded before the host bridge is created, and
> > symmetry would suggest that the overlay should be removed after the host
> > bridge is removed.
>
> Hmm, I need check more. But do you think this patch is fix a real issue or
> If order is did as you say above, this patch is not required anymore?
It looks like you're using a notifier call chain to remove the devices
when the of_changeset is removed. I think that's the wrong approach.
The only in-tree user of of_changeset_revert() is i2c-demux-pinctrl.c,
which uses the ordering I suggested:
i2c_demux_activate_master
of_changeset_apply # add OF changeset
i2c_add_adapter # add device
i2c_demux_deactivate_master
i2c_del_adapter # remove device
of_changeset_revert # remove OF changeset
> > > > bridge) is removed. I would expect the device to be removed before
> > > > the description of the device is removed.
> > > >
> > > > > Then of_pci_bus_release_domain_nr will actually use the dynamic
> > > > > IDA, even if the IDA was allocated in static IDA.
> > > > >
> > > > > Introduce a static_nr field in pci_bus to indicate whether the IDA
> > > > > is a dynamic or static in order to free the correct one.
> > > > >
> > > > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> > > > > Signed-off-by: Peng Fan <[email protected]>
> > > > > ---
> > > > >
> > > > > V2:
> > > > > Update commit message
> > > > > Move static_nr:1 to stay besides others :1 fields.
> > > > >
> > > > > drivers/pci/pci.c | 22 ++++++++++++++--------
> > > > > include/linux/pci.h | 1 +
> > > > > 2 files changed, 15 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index
> > > > > 60230da957e0..5c98502bcda6 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -6881,10 +6881,10 @@ static void
> > > > of_pci_reserve_static_domain_nr(void)
> > > > > }
> > > > > }
> > > > >
> > > > > -static int of_pci_bus_find_domain_nr(struct device *parent)
> > > > > +static int of_pci_bus_find_domain_nr(struct pci_bus *bus, struct
> > > > > +device *parent)
> > > > > {
> > > > > static bool static_domains_reserved = false;
> > > > > - int domain_nr;
> > > > > + int domain_nr, ret;
> > > > >
> > > > > /* On the first call scan device tree for static allocations. */
> > > > > if (!static_domains_reserved) {
> > > > > @@ -6892,6 +6892,8 @@ static int of_pci_bus_find_domain_nr(struct
> > > > device *parent)
> > > > > static_domains_reserved = true;
> > > > > }
> > > > >
> > > > > + bus->static_nr = 0;
> > > > > +
> > > > > if (parent) {
> > > > > /*
> > > > > * If domain is in DT, allocate it in static IDA. This @@ -
> > > > 6899,10
> > > > > +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device
> > > > > +*parent)
> > > > > * in DT.
> > > > > */
> > > > > domain_nr = of_get_pci_domain_nr(parent->of_node);
> > > > > - if (domain_nr >= 0)
> > > > > - return ida_alloc_range(&pci_domain_nr_static_ida,
> > > > > - domain_nr, domain_nr,
> > > > > - GFP_KERNEL);
> > > > > + if (domain_nr >= 0) {
> > > > > + ret = ida_alloc_range(&pci_domain_nr_static_ida,
> > > > > + domain_nr, domain_nr,
> > > > GFP_KERNEL);
> > > > > + if (ret >= 0)
> > > > > + bus->static_nr = 1;
> > > > > +
> > > > > + return ret;
> > > > > + }
> > > > > }
> > > > >
> > > > > /*
> > > > > @@ -6920,7 +6926,7 @@ static void
> > > > > of_pci_bus_release_domain_nr(struct
> > > > pci_bus *bus, struct device *par
> > > > > return;
> > > > >
> > > > > /* Release domain from IDA where it was allocated. */
> > > > > - if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
> > > > > + if (bus->static_nr)
> > > > > ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
> > > > > else
> > > > > ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
> > > > @@ -6928,7
> > > > > +6934,7 @@ static void of_pci_bus_release_domain_nr(struct pci_bus
> > > > > *bus, struct device *par
> > > > >
> > > > > int pci_bus_find_domain_nr(struct pci_bus *bus, struct device
> > > > > *parent) {
> > > > > - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> > > > > + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
> > > > > acpi_pci_bus_find_domain_nr(bus); }
> > > > >
> > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > > > > eeb2e6f6130f..222a1729ea7e 100644
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -677,6 +677,7 @@ struct pci_bus {
> > > > > struct bin_attribute *legacy_mem; /* Legacy mem */
> > > > > unsigned int is_added:1;
> > > > > unsigned int unsafe_warn:1; /* warned about RW1C
> > > > config write */
> > > > > + unsigned int static_nr:1;
> > > > > };
> > > > >
> > > > > #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
> > > > > --
> > > > > 2.37.1
> > > > >
+DT maintainer and Jailhouse maintainer.
> Subject: Re: [PATCH V2] pci: introduce static_nr to indicate domain_nr from
> which IDA
>
> On Wed, Sep 13, 2023 at 02:49:37AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH V2] pci: introduce static_nr to indicate
> > > domain_nr from which IDA
> > >
> > > On Wed, Sep 13, 2023 at 01:24:26AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [PATCH V2] pci: introduce static_nr to indicate
> > > > > domain_nr from which IDA
> > > > >
> > > > > On Tue, Aug 15, 2023 at 09:37:44AM +0800, Peng Fan (OSS) wrote:
> > > > > > From: Peng Fan <[email protected]>
> > > > > >
> > > > > > 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.
> > > > >
> > > > > I'm not familiar with how overlays work. What's the call path
> > > > > where the overlay is removed? I see an of_overlay_remove(), but
> > > > > I don't see any callers except test cases.
> > > >
> > > > We are using an out of tree hypervisor driver:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> gith%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C8861187acb1644109
> 5b808
> > > >
> dbb450a3f3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63830
> 20306
> > > >
> 79270146%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luM
> > > >
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=55kRQ1
> 0CXF
> > > > vA5LLjO285wkk%2BPrTKdaQYUTrBYF5UkxM%3D&reserved=0
> > > >
> > >
> ub.com%2Fsiemens%2Fjailhouse%2Fblob%2Fmaster%2Fdriver%2Fpci.c%23L
> > > 483&d
> > > >
> > >
> ata=05%7C01%7Cpeng.fan%40nxp.com%7C1bf0364c585f42dae7f108dbb3fe
> > > fcc4%7C
> > > >
> > >
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63830167996269745
> > > 6%7CUnknow
> > > >
> > >
> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > > WwiLC
> > > >
> > >
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=raIsz2HIH%2FQ0nmolBBwd81
> > > WxQh2vmFeXN
> > > > t3n6OIQaSU%3D&reserved=0
> > > > >
> > > > > I guess the problem happens in a PCI host bridge remove path,
> > > > > e.g.,
> > > > >
> > > > > pci_host_common_remove
> > > > > pci_remove_root_bus
> > > > > pci_bus_release_domain_nr
> > > > > of_pci_bus_release_domain_nr
> > > > >
> > > > > But I don't know how that's related to the overlay removal.
> > > >
> > > > When the overlay node got removed, the device removal will be
> > > > invoked and the domain number indicated by linux,pci-domain should
> also get freed.
> > > >
> > > > But actually the domain number not got freed because of bug as
> > > > this patch shows. "of_pci_bus_release_domain_nr will actually use
> > > > the dynamic IDA, even if the IDA was allocated in static IDA."
> > > >
> > > > So after the overlay node got destroyed and our test recreate the
> > > > overlay node with same domain number, issue triggered, the device
> > > > could not be created.
> > > >
> > > > > Is this an ordering issue? It seems possibly problematic that
> > > > > the OF overlay is destroyed before the device it describes
> > > > > (e.g., the host
> > > >
> > > > No. it is "of_pci_bus_release_domain_nr will actually use the
> > > > dynamic IDA, even if the IDA was allocated in static IDA "
> > >
> > > Yes, that's the *symptom*, to be sure. My question was why the
> > > overlay was removed before the host bridge.
> >
> > I am not sure.
> > I added a dump_stack, would this help to understand?
> > root@imx93evk:~# jailhouse disable
> > [ 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] [
> > 595.260484] jailhouse_cmd_disable+0x70/0x1ac [jailhouse] [
> > 595.265883] jailhouse_ioctl+0x60/0xf0 [jailhouse] [ 595.270674]
> > __arm64_sys_ioctl+0xac/0xf0 [ 595.274595] invoke_syscall+0x48/0x114
> > [ 595.278336] el0_svc_common.constprop.0+0xc4/0xe4
> > >
> > > The current ordering is this, where A happens before B:
> > >
> > > A overlay is removed
> > > B pci_host_common_remove
> > > pci_bus_release_domain_nr
> > > of_pci_bus_release_domain_nr
> > > of_get_pci_domain_nr # fails because overlay is gone
> > > ida_free(&pci_domain_nr_dynamic_ida)
> > >
> > > But if the host bridge were removed first, the ordering would be as
> > > follows, and the problem would not occur:
> > >
> > > B pci_host_common_remove
> > > pci_bus_release_domain_nr
> > > of_pci_bus_release_domain_nr
> > > of_get_pci_domain_nr # succeeds in this order
> > > ida_free(&pci_domain_nr_static_ida)
> > > A overlay is removed
> > >
> > > Is there a reason the overlay should be removed before the host bridge?
> > >
> > > Obviously the overlay is loaded before the host bridge is created,
> > > and symmetry would suggest that the overlay should be removed after
> > > the host bridge is removed.
> >
> > Hmm, I need check more. But do you think this patch is fix a real
> > issue or If order is did as you say above, this patch is not required anymore?
>
> It looks like you're using a notifier call chain to remove the devices when the
> of_changeset is removed. I think that's the wrong approach.
> The only in-tree user of of_changeset_revert() is i2c-demux-pinctrl.c, which
> uses the ordering I suggested:
I am not sure whether it is a must to first delete the host bridge then do
the revert from dtb overlay design perspective.
Thanks,
Peng.
>
> i2c_demux_activate_master
> of_changeset_apply # add OF changeset
> i2c_add_adapter # add device
>
> i2c_demux_deactivate_master
> i2c_del_adapter # remove device
> of_changeset_revert # remove OF changeset
>
> > > > > bridge) is removed. I would expect the device to be removed
> > > > > before the description of the device is removed.
> > > > >
> > > > > > Then of_pci_bus_release_domain_nr will actually use the
> > > > > > dynamic IDA, even if the IDA was allocated in static IDA.
> > > > > >
> > > > > > Introduce a static_nr field in pci_bus to indicate whether the
> > > > > > IDA is a dynamic or static in order to free the correct one.
> > > > > >
> > > > > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by
> > > > > > ida_alloc()")
> > > > > > Signed-off-by: Peng Fan <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > V2:
> > > > > > Update commit message
> > > > > > Move static_nr:1 to stay besides others :1 fields.
> > > > > >
> > > > > > drivers/pci/pci.c | 22 ++++++++++++++--------
> > > > > > include/linux/pci.h | 1 +
> > > > > > 2 files changed, 15 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index
> > > > > > 60230da957e0..5c98502bcda6 100644
> > > > > > --- a/drivers/pci/pci.c
> > > > > > +++ b/drivers/pci/pci.c
> > > > > > @@ -6881,10 +6881,10 @@ static void
> > > > > of_pci_reserve_static_domain_nr(void)
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > -static int of_pci_bus_find_domain_nr(struct device *parent)
> > > > > > +static int of_pci_bus_find_domain_nr(struct pci_bus *bus,
> > > > > > +struct device *parent)
> > > > > > {
> > > > > > static bool static_domains_reserved = false;
> > > > > > - int domain_nr;
> > > > > > + int domain_nr, ret;
> > > > > >
> > > > > > /* On the first call scan device tree for static allocations. */
> > > > > > if (!static_domains_reserved) { @@ -6892,6 +6892,8 @@
> static
> > > > > > int of_pci_bus_find_domain_nr(struct
> > > > > device *parent)
> > > > > > static_domains_reserved = true;
> > > > > > }
> > > > > >
> > > > > > + bus->static_nr = 0;
> > > > > > +
> > > > > > if (parent) {
> > > > > > /*
> > > > > > * If domain is in DT, allocate it in static IDA. This
> @@
> > > > > > -
> > > > > 6899,10
> > > > > > +6901,14 @@ static int of_pci_bus_find_domain_nr(struct device
> > > > > > +*parent)
> > > > > > * in DT.
> > > > > > */
> > > > > > domain_nr = of_get_pci_domain_nr(parent-
> >of_node);
> > > > > > - if (domain_nr >= 0)
> > > > > > - return
> ida_alloc_range(&pci_domain_nr_static_ida,
> > > > > > - domain_nr, domain_nr,
> > > > > > - GFP_KERNEL);
> > > > > > + if (domain_nr >= 0) {
> > > > > > + ret =
> ida_alloc_range(&pci_domain_nr_static_ida,
> > > > > > + domain_nr, domain_nr,
> > > > > GFP_KERNEL);
> > > > > > + if (ret >= 0)
> > > > > > + bus->static_nr = 1;
> > > > > > +
> > > > > > + return ret;
> > > > > > + }
> > > > > > }
> > > > > >
> > > > > > /*
> > > > > > @@ -6920,7 +6926,7 @@ static void
> > > > > > of_pci_bus_release_domain_nr(struct
> > > > > pci_bus *bus, struct device *par
> > > > > > return;
> > > > > >
> > > > > > /* Release domain from IDA where it was allocated. */
> > > > > > - if (of_get_pci_domain_nr(parent->of_node) == bus-
> >domain_nr)
> > > > > > + if (bus->static_nr)
> > > > > > ida_free(&pci_domain_nr_static_ida, bus-
> >domain_nr);
> > > > > > else
> > > > > > ida_free(&pci_domain_nr_dynamic_ida, bus-
> >domain_nr);
> > > > > @@ -6928,7
> > > > > > +6934,7 @@ static void of_pci_bus_release_domain_nr(struct
> > > > > > +pci_bus
> > > > > > *bus, struct device *par
> > > > > >
> > > > > > int pci_bus_find_domain_nr(struct pci_bus *bus, struct device
> > > > > > *parent) {
> > > > > > - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> > > > > > + return acpi_disabled ? of_pci_bus_find_domain_nr(bus,
> parent) :
> > > > > > acpi_pci_bus_find_domain_nr(bus); }
> > > > > >
> > > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > > > > > eeb2e6f6130f..222a1729ea7e 100644
> > > > > > --- a/include/linux/pci.h
> > > > > > +++ b/include/linux/pci.h
> > > > > > @@ -677,6 +677,7 @@ struct pci_bus {
> > > > > > struct bin_attribute *legacy_mem; /* Legacy mem */
> > > > > > unsigned int is_added:1;
> > > > > > unsigned int unsafe_warn:1; /* warned about
> RW1C
> > > > > config write */
> > > > > > + unsigned int static_nr:1;
> > > > > > };
> > > > > >
> > > > > > #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
> > > > > > --
> > > > > > 2.37.1
> > > > > >
On 14/09/2023 04:10, Peng Fan wrote:
> +DT maintainer and Jailhouse maintainer.
This does not work like this. Ccing me on some huge bunch of quoted text
without any comment is pointless. Do you expect me to read everything
just because you want? I clearly have not enough of work to do, right?
Ask specific question if you need anything from other people. Direct and
specific question, but not "what do you think?" (the same as this
quote). I am going just to ignore this email.
Best regards,
Krzysztof
Sorry for not being clear when Ccing you.
> Subject: Re: [PATCH V2] pci: introduce static_nr to indicate domain_nr from
The Major question is Bjorn says "
It looks like you're using a notifier call chain to remove the devices
when the of_changeset is removed. I think that's the wrong approach.
The only in-tree user of of_changeset_revert() is i2c-demux-pinctrl.c,
which uses the ordering I suggested:
"
And
"
The current ordering is this, where A happens before B:
A overlay is removed
B pci_host_common_remove
pci_bus_release_domain_nr
of_pci_bus_release_domain_nr
of_get_pci_domain_nr # fails because overlay is gone
ida_free(&pci_domain_nr_dynamic_ida)
But if the host bridge were removed first, the ordering would be as follows,
and the problem would not occur:
B pci_host_common_remove
pci_bus_release_domain_nr
of_pci_bus_release_domain_nr
of_get_pci_domain_nr # succeeds in this order
ida_free(&pci_domain_nr_static_ida)
A overlay is removed
"
I wonder whether it is a must to call pci_host_common_remove, then
remove overlay, or overlay remove automatically trigger
pci_host_common_remove is correct?
Thanks,
Peng.
>
> This does not work like this. Ccing me on some huge bunch of quoted text
> without any comment is pointless. Do you expect me to read everything just
> because you want? I clearly have not enough of work to do, right?
>
> Ask specific question if you need anything from other people. Direct and
> specific question, but not "what do you think?" (the same as this quote). I am
> going just to ignore this email.
>
> Best regards,
> Krzysztof