Re: [PATCH v3 2/2] PCI: Fix disabling of bridge BARs when assigning bus resources

[Logan Gunthorpe]

On 2019-06-17 7:53 a.m., Bjorn Helgaas wrote:
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 0eb40924169b..7adbd4bedd16 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -1784,11 +1784,16 @@ void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus)
>>  	/* restore size and flags */
>>  	list_for_each_entry(fail_res, &fail_head, list) {
>>  		struct resource *res = fail_res->res;
>> +		int idx;
>>  
>>  		res->start = fail_res->start;
>>  		res->end = fail_res->end;
>>  		res->flags = fail_res->flags;
>> -		if (fail_res->dev->subordinate)
>> +
>> +		idx = res - &fail_res->dev->resource[0];
>> +		if (fail_res->dev->subordinate &&
>> +		    idx >= PCI_BRIDGE_RESOURCES &&
>> +		    idx <= PCI_BRIDGE_RESOURCE_END)
>>  			res->flags = 0;
> 
> In my ideal world we wouldn't zap the flags of any resource.  I think
> we should derive the flags from the device's config space *once*
> during enumeration and remember them for the life of the device.

Yes, I agree. The fact that this code seems to be constantly modifying
everything makes it difficult to follow. When it clears the flags like
this it's not clear if/where/how it will ever put them back.

> This patch preserves res->flags for bridge BARs just like for any
> other device, so I think this is definitely a step in the right
> direction.
> 
> I'm not sure the "dev->subordinate" test is really correct, though.
> I think the original intent of this code was to clear res->flags for
> bridge windows under the assumptions that (a) we can identify bridges
> by "dev->subordinate" being non-zero, and (b) bridges only have
> windows and didn't have BARs.

Yes, I was also unsure of the reasoning behind the dev->subordinate test
as well. But given that I didn't fully understand it, and it wasn't
itself causing any problems, I elected to just change around it only for
the bug I was trying to fix.

> This patch fixes assumption (b), but I think (a) is false, and we
> should fix it as well.  One can imagine a bridge device without a
> subordinate bus (maybe we ran out of bus numbers), so I don't think we
> should test dev->subordinate.
>
> We could test something like pci_is_bridge(), although testing for idx
> being in the PCI_BRIDGE_RESOURCES range should be sufficient because I
> don't think we use those resource for anything other than windows.

Ok, yes, there are a couple possibilities here and I'm unsure of the
best thing to do. I agree that, right now, testing the idx for the range
is probably sufficient. So logically we could probably just remove the
dev->subordinate test. Assuming nobody decides to reuse the bridge
indices for something else (which is probably a safe assumption).
Though, testing for pci_is_bridge() would definitely be an improvement
in terms of readability and the issues you point out.

One way or another I can add a third patch to do this next time I submit
this series.

Thanks,

Logan