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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2019-06-17 at 08:53 -0500, Bjorn Helgaas wrote:
> On Fri, May 31, 2019 at 11:12:16AM -0600, Logan Gunthorpe wrote:
> > One odd quirk of PLX switches is that their upstream bridge port has
> > 256K of space allocated behind its BAR0 (most other bridge
> > implementations do not report any BAR space).
> 
> Somewhat unusual, but completely legal, of course.

Ah yes, I've seen these. They have an MMIO path to their internal
registers in addition to cfg. Can be annoying.

> If a bridge has memory BARs, AFAIK it is impossible to enable a memory
> window without also enabling the BARs, so if we want to use the bridge
> at all, we *must* allocate space for its BARs, just like for any other
> device.

Right.

 .../... (agreeing violently)

> 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.

Amen brother. It will take a little while to get there. One thing we
should do is have a clearer way to mark a resource that failed to
assign/allocate (though technically parent=NULL is it really, as long
as all archs these days claim properly, it used not to be the case).

We do wipe *bridge* windows (nor BARs) all over the place, that is less
of an issue I suppose though I would be more comfortable if we also
wrote to the bridge to close those windows as we do so...

The problem of course is how much old weird quirky will break due to
subtle assumptions as we "fix" these things :-)

> 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.

Right, shouldn't it be pci_is_bridge() ?

> 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.
> 
> 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.

Yup.

> 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.

Yeah quite possibly.

Cheers,
Ben.