[PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary

[PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary

[Nicholas Johnson]

Remove checks for resource size in extend_bridge_window(). This is
necessary to allow the pci_bus_distribute_available_resources() to
function when the kernel parameter pci=hpmemsize=nn[KMG] is used to
allocate resources. Because the kernel parameter sets the size of all
hotplug bridges to be the same, there are problems when nested hotplug
bridges are encountered. Fitting a downstream hotplug bridge with size X
and normal bridges with size Y into parent hotplug bridge with size X is
impossible, and hence the downstream hotplug bridge needs to shrink to
fit into its parent.

Add check for if bridge is extended or shrunken and adjust pci_dbg to
reflect this.

Reset the resource if its new size is zero (if we have run out of a
bridge window resource). If it is set to zero size and left, it can
cause significant problems when it comes to enabling devices.

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
---
 drivers/pci/setup-bus.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index a072781ab..7e1dc892a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev *bridge, struct resource *res,
 	if (res->parent)
 		return;
 
-	if (resource_size(res) >= new_size)
-		return;
-
-	add_size = new_size - resource_size(res);
-	pci_dbg(bridge, "bridge window %pR extended by %pa\n", res, &add_size);
+	if (new_size > resource_size(res)) {
+		add_size = new_size - resource_size(res);
+		pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+			&add_size);
+	} else if (new_size < resource_size(res)) {
+		add_size = resource_size(res) - new_size;
+		pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+			&add_size);
+	}
 	res->end = res->start + new_size - 1;
 	remove_from_list(add_list, res);
+	if (!new_size)
+		reset_resource(res);
 }
 
 static void pci_bus_distribute_available_resources(struct pci_bus *bus,
-- 
2.22.0

Re: [PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary

[mika.westerberg]

On Fri, Jul 26, 2019 at 12:54:22PM +0000, Nicholas Johnson wrote:
> Remove checks for resource size in extend_bridge_window(). This is
> necessary to allow the pci_bus_distribute_available_resources() to
> function when the kernel parameter pci=hpmemsize=nn[KMG] is used to
> allocate resources. Because the kernel parameter sets the size of all
> hotplug bridges to be the same, there are problems when nested hotplug
> bridges are encountered. Fitting a downstream hotplug bridge with size X
> and normal bridges with size Y into parent hotplug bridge with size X is
> impossible, and hence the downstream hotplug bridge needs to shrink to
> fit into its parent.

Maybe you could show the topology here which needs shrinking.

> Add check for if bridge is extended or shrunken and adjust pci_dbg to
> reflect this.
> 
> Reset the resource if its new size is zero (if we have run out of a
> bridge window resource). If it is set to zero size and left, it can
> cause significant problems when it comes to enabling devices.

Same comment here about explaining the "significant problems".
> 
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> ---
>  drivers/pci/setup-bus.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index a072781ab..7e1dc892a 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev *bridge, struct resource *res,

Since it is also shrinking now maybe name it adjust_bridge_window() instead?

Re: [PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary

[Nicholas Johnson]

On Tue, Oct 08, 2019 at 03:09:07PM +0300, mika.westerberg@linux.intel.com wrote:
> On Fri, Jul 26, 2019 at 12:54:22PM +0000, Nicholas Johnson wrote:
> > Remove checks for resource size in extend_bridge_window(). This is
> > necessary to allow the pci_bus_distribute_available_resources() to
> > function when the kernel parameter pci=hpmemsize=nn[KMG] is used to
> > allocate resources. Because the kernel parameter sets the size of all
> > hotplug bridges to be the same, there are problems when nested hotplug
> > bridges are encountered. Fitting a downstream hotplug bridge with size X
> > and normal bridges with size Y into parent hotplug bridge with size X is
> > impossible, and hence the downstream hotplug bridge needs to shrink to
> > fit into its parent.
> 
> Maybe you could show the topology here which needs shrinking.
> 
> > Add check for if bridge is extended or shrunken and adjust pci_dbg to
> > reflect this.
> > 
> > Reset the resource if its new size is zero (if we have run out of a
> > bridge window resource). If it is set to zero size and left, it can
> > cause significant problems when it comes to enabling devices.
> 
> Same comment here about explaining the "significant problems".
I have in the past, but because the problems are very hard to describe succinctly, it just turns into a 
nightmare. I can try to do it again.

> > 
> > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > ---
> >  drivers/pci/setup-bus.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index a072781ab..7e1dc892a 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev *bridge, struct resource *res,
> 
> Since it is also shrinking now maybe name it adjust_bridge_window() instead?
I am happy to do this.

If we can drop the pci_dbg() calls, then I might be able to drop this 
function entirely. During the development of this patch, that is exactly 
what I did. How important are the pci_dbg calls to you?

Another option is to simply print something with pci_dbg that simply 
says the bridge size has been set to maximum possible while still 
fitting in parent. That will remove the need for logic to detect if it 
shrunk or extended.

Re: [PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary

[mika.westerberg]

On Wed, Oct 23, 2019 at 09:16:10AM +0000, Nicholas Johnson wrote:
> On Tue, Oct 08, 2019 at 03:09:07PM +0300, mika.westerberg@linux.intel.com wrote:
> > On Fri, Jul 26, 2019 at 12:54:22PM +0000, Nicholas Johnson wrote:
> > > Remove checks for resource size in extend_bridge_window(). This is
> > > necessary to allow the pci_bus_distribute_available_resources() to
> > > function when the kernel parameter pci=hpmemsize=nn[KMG] is used to
> > > allocate resources. Because the kernel parameter sets the size of all
> > > hotplug bridges to be the same, there are problems when nested hotplug
> > > bridges are encountered. Fitting a downstream hotplug bridge with size X
> > > and normal bridges with size Y into parent hotplug bridge with size X is
> > > impossible, and hence the downstream hotplug bridge needs to shrink to
> > > fit into its parent.
> > 
> > Maybe you could show the topology here which needs shrinking.
> > 
> > > Add check for if bridge is extended or shrunken and adjust pci_dbg to
> > > reflect this.
> > > 
> > > Reset the resource if its new size is zero (if we have run out of a
> > > bridge window resource). If it is set to zero size and left, it can
> > > cause significant problems when it comes to enabling devices.
> > 
> > Same comment here about explaining the "significant problems".
> I have in the past, but because the problems are very hard to describe succinctly, it just turns into a 
> nightmare. I can try to do it again.
> 
> > > 
> > > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > > ---
> > >  drivers/pci/setup-bus.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > > index a072781ab..7e1dc892a 100644
> > > --- a/drivers/pci/setup-bus.c
> > > +++ b/drivers/pci/setup-bus.c
> > > @@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev *bridge, struct resource *res,
> > 
> > Since it is also shrinking now maybe name it adjust_bridge_window() instead?
> I am happy to do this.
> 
> If we can drop the pci_dbg() calls, then I might be able to drop this 
> function entirely. During the development of this patch, that is exactly 
> what I did. How important are the pci_dbg calls to you?

Well they are still useful when debugging resource allocation issues
(and they match similar we do when extending number of buses). I would
like to keep them if possible.