[v9,08/30] PCI: Update pci_host_bridge bus resource

Commit Message

Yijing Wang April 3, 2015, 9:25 a.m. UTC

From: Yijing Wang <[e-mail]>

Sometimes, the bus resource start number is not equal to
root bus number. For example, in pci_scan_bus(), we always
add the default bus resource which start bus number is 0,
but the root bus number callers given may != 0, so
we need to update pci_host_bridge bus resource, because we
would check whether host bridge bus resoruce is confict
in later patch.

Signed-off-by: Yijing Wang <[e-mail]>
---
 drivers/pci/host-bridge.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Comments

Bjorn Helgaas April 7, 2015, 10:42 p.m. UTC | #1

On Fri, Apr 03, 2015 at 05:25:42PM +0800, Yijing Wang wrote:
> From: Yijing Wang <[e-mail]>
> 
> Sometimes, the bus resource start number is not equal to
> root bus number. For example, in pci_scan_bus(), we always
> add the default bus resource which start bus number is 0,
> but the root bus number callers given may != 0, so
> we need to update pci_host_bridge bus resource, because we
> would check whether host bridge bus resoruce is confict
> in later patch.

It's true that pci_scan_bus() always inserts [bus 00-ff].  But I think
that's completely bogus.  The caller of pci_scan_bus() supplies a root bus
number X.  Any bus numbers below X are useless as far as this host bridge
is concerned, and it's pointless to include them in the range inserted by
pci_scan_bus().

I think we'd be better off if we forced every pci_scan_bus() caller to
supply a real non-overlapping bus number range.  We probably can't do that
easily because the arch knows the beginning bus number, but some don't know
how to figure out the ending bus number.

Do we have a per-domain structure that tracks the bus numbers in use in the
domain?  It seems like if we had one, we could use that to approximate the
end.  For example, if the arch scans three root buses, at bus 00, bus 40,
and bus 80, we could start with the first one at [bus 00-ff], and when we
scan the one at bus 40, we could either report a conflict (if the bus 00
tree included a bus 40) or reduce the first range to [bus 00-3f].

> Signed-off-by: Yijing Wang <[e-mail]>
> ---
>  drivers/pci/host-bridge.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index ecc1a7c..1a9834b 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -26,8 +26,11 @@ static void pci_host_update_busn_res(
>  	struct resource_entry *window;
>  
>  	resource_list_for_each_entry(window, resources)
> -		if (window->res->flags & IORESOURCE_BUS)
> +		if (window->res->flags & IORESOURCE_BUS) {
> +			if (bus > window->res->start)
> +				window->res->start = bus;

I see what you're trying to do here, but I think this is the wrong place to
do it.  I'd rather figure out a way to insert something other than
busn_resource in pci_scan_bus().  That probably means we need to
dynamically allocate a new busn_res struct.

>  			return;
> +		}
>  
>  	pr_info(
>  	 "No busn resource found for pci%04x:%02x, will use [bus %02x-ff]\n",
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yijing Wang April 8, 2015, 9:22 a.m. UTC | #2

On 2015/4/8 6:42, Bjorn Helgaas wrote:
> On Fri, Apr 03, 2015 at 05:25:42PM +0800, Yijing Wang wrote:
>> From: Yijing Wang <[e-mail]>
>>
>> Sometimes, the bus resource start number is not equal to
>> root bus number. For example, in pci_scan_bus(), we always
>> add the default bus resource which start bus number is 0,
>> but the root bus number callers given may != 0, so
>> we need to update pci_host_bridge bus resource, because we
>> would check whether host bridge bus resoruce is confict
>> in later patch.
> 
> It's true that pci_scan_bus() always inserts [bus 00-ff].  But I think
> that's completely bogus.  The caller of pci_scan_bus() supplies a root bus
> number X.  Any bus numbers below X are useless as far as this host bridge
> is concerned, and it's pointless to include them in the range inserted by
> pci_scan_bus().

Agree, I think it's better to use the [bus start - FF] instead of [00-FF].

> 
> I think we'd be better off if we forced every pci_scan_bus() caller to
> supply a real non-overlapping bus number range.  We probably can't do that
> easily because the arch knows the beginning bus number, but some don't know
> how to figure out the ending bus number.

It seems to have some problems in pci_scan_bus() if we have more than one pci host bridge
in a domain. Because we add the default busn_res[00-FF] to resource list in pci_scan_bus(),
and pass the resource list to pci_create_root_bus().  The pci root bus would
consume the bus range from the start to the 0xFF, and request it from the per-domain resource[00-FF].
So if we have another pci host bridge in this domain, and pass another start bus number, it would
fail when the root bus try to request bus resource from per-domain resource. Now only several archs
call pci_scan_bus(), I guess they didn't run the code in above case, so we didn't find
the bus report.

> 
> Do we have a per-domain structure that tracks the bus numbers in use in the
> domain?  It seems like if we had one, we could use that to approximate the
> end.  For example, if the arch scans three root buses, at bus 00, bus 40,
> and bus 80, we could start with the first one at [bus 00-ff], and when we
> scan the one at bus 40, we could either report a conflict (if the bus 00
> tree included a bus 40) or reduce the first range to [bus 00-3f].

Per-domain resource pci_domain_busn_res is what we are looking for.
For simplicity, we could report a conflict and fail when we find a conflict bus
resource in system.


>>  	resource_list_for_each_entry(window, resources)
>> -		if (window->res->flags & IORESOURCE_BUS)
>> +		if (window->res->flags & IORESOURCE_BUS) {
>> +			if (bus > window->res->start)
>> +				window->res->start = bus;
> 
> I see what you're trying to do here, but I think this is the wrong place to
> do it.  I'd rather figure out a way to insert something other than
> busn_resource in pci_scan_bus().  That probably means we need to
> dynamically allocate a new busn_res struct.

Allocate a new busn_res maybe unnecessary, the bus resource we added to resource list
and passed to pci_create_root_bus() will never be used again after we call
pci_bus_insert_busn_res(), we may could consider to use local busn_res in pci_scan_bus().

Thanks!
Yijing.

> 
>>  			return;
>> +		}
>>  
>>  	pr_info(
>>  	 "No busn resource found for pci%04x:%02x, will use [bus %02x-ff]\n",
>> -- 
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
>

Patch

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index ecc1a7c..1a9834b 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -26,8 +26,11 @@  static void pci_host_update_busn_res(
 	struct resource_entry *window;
 
 	resource_list_for_each_entry(window, resources)
-		if (window->res->flags & IORESOURCE_BUS)
+		if (window->res->flags & IORESOURCE_BUS) {
+			if (bus > window->res->start)
+				window->res->start = bus;
 			return;
+		}
 
 	pr_info(
 	 "No busn resource found for pci%04x:%02x, will use [bus %02x-ff]\n",