diff mbox series

[v5,1/3] PCI: Align extra resources for hotplug bridges properly

Message ID 20230112110000.59974-2-mika.westerberg@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI: distribute resources for root buses | expand

Commit Message

Mika Westerberg Jan. 12, 2023, 10:59 a.m. UTC
After division the extra resource space per hotplug bridge may not be
aligned according to the window alignment so do that before passing it
down for further distribution.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/setup-bus.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Comments

Alexander Motin Jan. 26, 2023, 4:53 p.m. UTC | #1
Hi Mika,

Unfortunately my system was de-racked meanwhile, so it will take few 
more days for me to test this.  So far I only successfully built it on 
my 5.15.79 kernel.  Meanwhile some comments below:

On 12.01.2023 05:59, Mika Westerberg wrote:
> After division the extra resource space per hotplug bridge may not be
> aligned according to the window alignment so do that before passing it
> down for further distribution.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>   drivers/pci/setup-bus.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index b4096598dbcb..34a74bc581b0 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1891,6 +1891,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>   	 * resource space between hotplug bridges.
>   	 */
>   	for_each_pci_bridge(dev, bus) {
> +		struct resource *res;
>   		struct pci_bus *b;
>   
>   		b = dev->subordinate;
> @@ -1902,16 +1903,28 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>   		 * hotplug-capable downstream ports taking alignment into
>   		 * account.
>   		 */
> -		io.end = io.start + io_per_hp - 1;
> -		mmio.end = mmio.start + mmio_per_hp - 1;
> -		mmio_pref.end = mmio_pref.start + mmio_pref_per_hp - 1;
> +		res = &dev->resource[PCI_BRIDGE_IO_WINDOW];
> +		align = pci_resource_alignment(dev, res);
> +		io.end = align ? io.start + ALIGN_DOWN(io_per_hp, align) - 1
> +			       : io.start + io_per_hp - 1;

Not bug probably, but if we align x_per_b down for one bridge, we could 
be able to increase it for other(s).

> +
> +		res = &dev->resource[PCI_BRIDGE_MEM_WINDOW];
> +		align = pci_resource_alignment(dev, res);
> +		mmio.end = align ? mmio.start + ALIGN_DOWN(mmio_per_hp, align) - 1
> +				 : mmio.start + io_per_hp - 1;

Here is a typo, it should be mmio_per_hp here   ^^^.

> +
> +		res = &dev->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
> +		align = pci_resource_alignment(dev, res);
> +		mmio_pref.end = align ? mmio_pref.start +
> +					ALIGN_DOWN(mmio_pref_per_hp, align) - 1
> +				      : mmio_pref.start + mmio_pref_per_hp;
>   
>   		pci_bus_distribute_available_resources(b, add_list, io, mmio,
>   						       mmio_pref);
>   
> -		io.start += io_per_hp;
> -		mmio.start += mmio_per_hp;
> -		mmio_pref.start += mmio_pref_per_hp;
> +		io.start += io.end + 1;
> +		mmio.start += mmio.end + 1;
> +		mmio_pref.start += mmio_pref.end + 1;
>   	}
>   }
>
Mika Westerberg Jan. 27, 2023, 6:45 a.m. UTC | #2
Hi,

On Thu, Jan 26, 2023 at 11:53:55AM -0500, Alexander Motin wrote:
> Hi Mika,
> 
> Unfortunately my system was de-racked meanwhile, so it will take few more
> days for me to test this.  So far I only successfully built it on my 5.15.79
> kernel.  Meanwhile some comments below:

Okay, take your time and thanks for reviewing!

> 
> On 12.01.2023 05:59, Mika Westerberg wrote:
> > After division the extra resource space per hotplug bridge may not be
> > aligned according to the window alignment so do that before passing it
> > down for further distribution.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >   drivers/pci/setup-bus.c | 25 +++++++++++++++++++------
> >   1 file changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index b4096598dbcb..34a74bc581b0 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1891,6 +1891,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> >   	 * resource space between hotplug bridges.
> >   	 */
> >   	for_each_pci_bridge(dev, bus) {
> > +		struct resource *res;
> >   		struct pci_bus *b;
> >   		b = dev->subordinate;
> > @@ -1902,16 +1903,28 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> >   		 * hotplug-capable downstream ports taking alignment into
> >   		 * account.
> >   		 */
> > -		io.end = io.start + io_per_hp - 1;
> > -		mmio.end = mmio.start + mmio_per_hp - 1;
> > -		mmio_pref.end = mmio_pref.start + mmio_pref_per_hp - 1;
> > +		res = &dev->resource[PCI_BRIDGE_IO_WINDOW];
> > +		align = pci_resource_alignment(dev, res);
> > +		io.end = align ? io.start + ALIGN_DOWN(io_per_hp, align) - 1
> > +			       : io.start + io_per_hp - 1;
> 
> Not bug probably, but if we align x_per_b down for one bridge, we could be
> able to increase it for other(s).

Yeah, the down align is just to make sure we don't run over what is
there because of the splitting. We could for example leave the
"leftovers" to the last bridge or so but I don't think we want to do it
in this patch series to avoid any additional bugs creeping in. Unless
you guys think it needs to be done, of course.

> > +
> > +		res = &dev->resource[PCI_BRIDGE_MEM_WINDOW];
> > +		align = pci_resource_alignment(dev, res);
> > +		mmio.end = align ? mmio.start + ALIGN_DOWN(mmio_per_hp, align) - 1
> > +				 : mmio.start + io_per_hp - 1;
> 
> Here is a typo, it should be mmio_per_hp here   ^^^.

Good catch! I went over these specifically for things like this but I
still missed it :( Will fix in next version.

> > +
> > +		res = &dev->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
> > +		align = pci_resource_alignment(dev, res);
> > +		mmio_pref.end = align ? mmio_pref.start +
> > +					ALIGN_DOWN(mmio_pref_per_hp, align) - 1
> > +				      : mmio_pref.start + mmio_pref_per_hp;
> >   		pci_bus_distribute_available_resources(b, add_list, io, mmio,
> >   						       mmio_pref);
> > -		io.start += io_per_hp;
> > -		mmio.start += mmio_per_hp;
> > -		mmio_pref.start += mmio_pref_per_hp;
> > +		io.start += io.end + 1;
> > +		mmio.start += mmio.end + 1;
> > +		mmio_pref.start += mmio_pref.end + 1;
> >   	}
> >   }
> 
> -- 
> Alexander Motin
diff mbox series

Patch

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index b4096598dbcb..34a74bc581b0 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1891,6 +1891,7 @@  static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 	 * resource space between hotplug bridges.
 	 */
 	for_each_pci_bridge(dev, bus) {
+		struct resource *res;
 		struct pci_bus *b;
 
 		b = dev->subordinate;
@@ -1902,16 +1903,28 @@  static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 		 * hotplug-capable downstream ports taking alignment into
 		 * account.
 		 */
-		io.end = io.start + io_per_hp - 1;
-		mmio.end = mmio.start + mmio_per_hp - 1;
-		mmio_pref.end = mmio_pref.start + mmio_pref_per_hp - 1;
+		res = &dev->resource[PCI_BRIDGE_IO_WINDOW];
+		align = pci_resource_alignment(dev, res);
+		io.end = align ? io.start + ALIGN_DOWN(io_per_hp, align) - 1
+			       : io.start + io_per_hp - 1;
+
+		res = &dev->resource[PCI_BRIDGE_MEM_WINDOW];
+		align = pci_resource_alignment(dev, res);
+		mmio.end = align ? mmio.start + ALIGN_DOWN(mmio_per_hp, align) - 1
+				 : mmio.start + io_per_hp - 1;
+
+		res = &dev->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
+		align = pci_resource_alignment(dev, res);
+		mmio_pref.end = align ? mmio_pref.start +
+					ALIGN_DOWN(mmio_pref_per_hp, align) - 1
+				      : mmio_pref.start + mmio_pref_per_hp;
 
 		pci_bus_distribute_available_resources(b, add_list, io, mmio,
 						       mmio_pref);
 
-		io.start += io_per_hp;
-		mmio.start += mmio_per_hp;
-		mmio_pref.start += mmio_pref_per_hp;
+		io.start += io.end + 1;
+		mmio.start += mmio.end + 1;
+		mmio_pref.start += mmio_pref.end + 1;
 	}
 }