[PATCH 1/1] PCI: Restore assigned resources fully after release

[PATCH 1/1] PCI: Restore assigned resources fully after release

[Ilpo Järvinen]

PCI resource fitting code in __assign_resources_sorted() runs in
multiple steps. A resource that was successfully assigned may have to
be released before the next step attempts assignment again. The
assign+release cycle is destructive to a start-aligned struct resource
(bridge window or IOV resource) because the start field is overwritten
with the real address when the resource got assigned.

Properly restore the resource after releasing it. The start, end, and
flags fields must be stored into the related struct pci_dev_resource in
order to be able to restore the resource to its original state.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 54d6f4fa3ce1..e994c546422c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -187,6 +187,9 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
 			panic("%s: kzalloc() failed!\n", __func__);
 		tmp->res = r;
 		tmp->dev = dev;
+		tmp->start = r->start;
+		tmp->end = r->end;
+		tmp->flags = r->flags;
 
 		/* Fallback is smallest one or list is empty */
 		n = head;
@@ -545,6 +548,7 @@ static void __assign_resources_sorted(struct list_head *head,
 		pci_dbg(dev, "%s %pR: releasing\n", res_name, res);
 
 		release_resource(res);
+		restore_dev_resource(dev_res);
 	}
 	/* Restore start/end/flags from saved list */
 	list_for_each_entry(save_res, &save_head, list)

base-commit: 7d06015d936c861160803e020f68f413b5c3cd9d
-- 
2.39.5

Re: [PATCH 1/1] PCI: Restore assigned resources fully after release

[Guenter Roeck]

On Thu, Apr 03, 2025 at 12:31:37PM +0300, Ilpo Järvinen wrote:
> PCI resource fitting code in __assign_resources_sorted() runs in
> multiple steps. A resource that was successfully assigned may have to
> be released before the next step attempts assignment again. The
> assign+release cycle is destructive to a start-aligned struct resource
> (bridge window or IOV resource) because the start field is overwritten
> with the real address when the resource got assigned.
> 
> Properly restore the resource after releasing it. The start, end, and
> flags fields must be stored into the related struct pci_dev_resource in
> order to be able to restore the resource to its original state.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

Re: [PATCH 1/1] PCI: Restore assigned resources fully after release

[Ondřej Jirman]

Hello,

On Thu, Apr 03, 2025 at 12:31:37PM +0300, Ilpo Järvinen wrote:
> PCI resource fitting code in __assign_resources_sorted() runs in
> multiple steps. A resource that was successfully assigned may have to
> be released before the next step attempts assignment again. The
> assign+release cycle is destructive to a start-aligned struct resource
> (bridge window or IOV resource) because the start field is overwritten
> with the real address when the resource got assigned.
> 
> Properly restore the resource after releasing it. The start, end, and
> flags fields must be stored into the related struct pci_dev_resource in
> order to be able to restore the resource to its original state.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Tested-by: Guenter Roeck <linux@roeck-us.net>

I've tested this on Orange Pi 5+ where it fixed a regression, too.

Tested-by: Ondrej Jirman <megi@xff.cz>

Thank you,
	Ondrej

> ---
>  drivers/pci/setup-bus.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> 
> base-commit: 7d06015d936c861160803e020f68f413b5c3cd9d
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 54d6f4fa3ce1..e994c546422c 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -187,6 +187,9 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
>  			panic("%s: kzalloc() failed!\n", __func__);
>  		tmp->res = r;
>  		tmp->dev = dev;
> +		tmp->start = r->start;
> +		tmp->end = r->end;
> +		tmp->flags = r->flags;
>  
>  		/* Fallback is smallest one or list is empty */
>  		n = head;
> @@ -545,6 +548,7 @@ static void __assign_resources_sorted(struct list_head *head,
>  		pci_dbg(dev, "%s %pR: releasing\n", res_name, res);
>  
>  		release_resource(res);
> +		restore_dev_resource(dev_res);
>  	}
>  	/* Restore start/end/flags from saved list */
>  	list_for_each_entry(save_res, &save_head, list)

Re: [PATCH 1/1] PCI: Restore assigned resources fully after release

[Nicolas Frattaroli]

On Thursday, 3 April 2025 11:31:37 Central European Summer Time Ilpo Järvinen wrote:
> PCI resource fitting code in __assign_resources_sorted() runs in
> multiple steps. A resource that was successfully assigned may have to
> be released before the next step attempts assignment again. The
> assign+release cycle is destructive to a start-aligned struct resource
> (bridge window or IOV resource) because the start field is overwritten
> with the real address when the resource got assigned.
> 
> Properly restore the resource after releasing it. The start, end, and
> flags fields must be stored into the related struct pci_dev_resource in
> order to be able to restore the resource to its original state.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/setup-bus.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

+Cc: linux-rockchip

Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

This fixes a regression on the RK3588 that I ran into with v6.15-rc<=2.

Specifically, the PCIe3 controller will fail to map the address space
of a
  Non-Volatile memory controller: KIOXIA Corporation NVMe SSD (rev 01)
  (prog-if 02 [NVM Express])

drive on most, but not all, boots, depending on the order initialisation
happens, it seems. A different drive I tested seems to work fine, so not
only does the regression only rear its head based on boot timing, but
also based on the device attached to the PCIe3 controller. Bisecting was
a bit of a tortured affair, as the bad commit 96336ec70264 doesn't seem
to build for me, so I had to `git bisect skip` it.

The specific problematic behaviour fixed by this patch is that we get:

  pci 0000:00:00.0: bridge window [mem size 0x00100000]: can't assign; bogus alignment

It sounds like Ondřej Jirman ran into the same regression, since the
devices he mentioned (Orange Pi 5+ and QuartzPro64) both use the
Rockchip RK3588 SoC.

Kind regards,
Nicolas Frattaroli

Re: [PATCH 1/1] PCI: Restore assigned resources fully after release

[Bjorn Helgaas]

On Thu, Apr 03, 2025 at 12:31:37PM +0300, Ilpo Järvinen wrote:
> PCI resource fitting code in __assign_resources_sorted() runs in
> multiple steps. A resource that was successfully assigned may have to
> be released before the next step attempts assignment again. The
> assign+release cycle is destructive to a start-aligned struct resource
> (bridge window or IOV resource) because the start field is overwritten
> with the real address when the resource got assigned.
> 
> Properly restore the resource after releasing it. The start, end, and
> flags fields must be stored into the related struct pci_dev_resource in
> order to be able to restore the resource to its original state.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Applied to pci/for-linus for v6.15, thanks!

I'd like to add the following to the commit log if it's accurate:

  + One symptom:

  +   pci 0002:00:00.0: bridge window [mem size 0x00100000]: can't assign; bogus alignment

    Reported-by: Guenter Roeck <linux@roeck-us.net>
  + Closes: https://lore.kernel.org/r/01eb7d40-f5b5-4ec5-b390-a5c042c30aff@roeck-us.net/
  + Reported-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
  + Closes: https://lore.kernel.org/r/3578030.5fSG56mABF@workhorse

Let me know if that's wrong or there are additional reports.

> ---
>  drivers/pci/setup-bus.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 54d6f4fa3ce1..e994c546422c 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -187,6 +187,9 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
>  			panic("%s: kzalloc() failed!\n", __func__);
>  		tmp->res = r;
>  		tmp->dev = dev;
> +		tmp->start = r->start;
> +		tmp->end = r->end;
> +		tmp->flags = r->flags;
>  
>  		/* Fallback is smallest one or list is empty */
>  		n = head;
> @@ -545,6 +548,7 @@ static void __assign_resources_sorted(struct list_head *head,
>  		pci_dbg(dev, "%s %pR: releasing\n", res_name, res);
>  
>  		release_resource(res);
> +		restore_dev_resource(dev_res);
>  	}
>  	/* Restore start/end/flags from saved list */
>  	list_for_each_entry(save_res, &save_head, list)
> 
> base-commit: 7d06015d936c861160803e020f68f413b5c3cd9d
> -- 
> 2.39.5
> 

Re: [PATCH 1/1] PCI: Restore assigned resources fully after release

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

On Thu, 17 Apr 2025, Bjorn Helgaas wrote:

> On Thu, Apr 03, 2025 at 12:31:37PM +0300, Ilpo Järvinen wrote:
> > PCI resource fitting code in __assign_resources_sorted() runs in
> > multiple steps. A resource that was successfully assigned may have to
> > be released before the next step attempts assignment again. The
> > assign+release cycle is destructive to a start-aligned struct resource
> > (bridge window or IOV resource) because the start field is overwritten
> > with the real address when the resource got assigned.
> > 
> > Properly restore the resource after releasing it. The start, end, and
> > flags fields must be stored into the related struct pci_dev_resource in
> > order to be able to restore the resource to its original state.
> > 
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Applied to pci/for-linus for v6.15, thanks!
> 
> I'd like to add the following to the commit log if it's accurate:
> 
>   + One symptom:
> 
>   +   pci 0002:00:00.0: bridge window [mem size 0x00100000]: can't assign; bogus alignment

Yes, that is a good addition.

-- 
 i.

>     Reported-by: Guenter Roeck <linux@roeck-us.net>
>   + Closes: https://lore.kernel.org/r/01eb7d40-f5b5-4ec5-b390-a5c042c30aff@roeck-us.net/
>   + Reported-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>   + Closes: https://lore.kernel.org/r/3578030.5fSG56mABF@workhorse
> 
> Let me know if that's wrong or there are additional reports.
> 
> > ---
> >  drivers/pci/setup-bus.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 54d6f4fa3ce1..e994c546422c 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -187,6 +187,9 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
> >  			panic("%s: kzalloc() failed!\n", __func__);
> >  		tmp->res = r;
> >  		tmp->dev = dev;
> > +		tmp->start = r->start;
> > +		tmp->end = r->end;
> > +		tmp->flags = r->flags;
> >  
> >  		/* Fallback is smallest one or list is empty */
> >  		n = head;
> > @@ -545,6 +548,7 @@ static void __assign_resources_sorted(struct list_head *head,
> >  		pci_dbg(dev, "%s %pR: releasing\n", res_name, res);
> >  
> >  		release_resource(res);
> > +		restore_dev_resource(dev_res);
> >  	}
> >  	/* Restore start/end/flags from saved list */
> >  	list_for_each_entry(save_res, &save_head, list)
> > 
> > base-commit: 7d06015d936c861160803e020f68f413b5c3cd9d
> > -- 
> > 2.39.5
> > 
>