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
