Messages in this thread | | | Date | Wed, 16 Sep 2015 10:39:23 -0700 | From | David Daney <> | Subject | Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation. |
| |
On 09/16/2015 10:29 AM, Will Deacon wrote: > Hi Lorenzo, > > On Wed, Sep 16, 2015 at 12:28:52PM +0100, Lorenzo Pieralisi wrote: >> On Wed, Sep 16, 2015 at 11:41:53AM +0100, Will Deacon wrote: >>>> Here is the current code: >>>> >>>>>> bus_range = pci->cfg.bus_range; >>>>>> for (busn = bus_range->start; busn <= bus_range->end; ++busn) { >>>>>> u32 idx = busn - bus_range->start; >>>> >>>> The index is offset by the bus range start... >>>> >>>>>> u32 sz = 1 << pci->cfg.ops.bus_shift; >>>>>> >>>>>> pci->cfg.win[idx] = devm_ioremap(dev, >>>>>> pci->cfg.res.start + busn * sz, >>>>>> sz); >>>> >>>> But, the offset into the "reg" property is the raw bus number. >>>> >>>> >>>>>> if (!pci->cfg.win[idx]) >>>>>> return -ENOMEM; >>>>>> } >>>> >>>> >>>> I hope that makes it more clear. >>> >>> Got it. So we should be using idx instead of busn in the devm_ioremap >>> call above. >> >> I think that's not what's specified in the PCI firmware specification, >> at least for the MMCFG regions. For MMCFG regions (quoting the specs) >> the "base address of the memory mapped configuration space always >> corresponds to bus number 0 (regardless of the start bus number decoded >> by the host bridge)..." >> >> For the x86 implementation have a look at: >> >> arch/x86/pci/mmconfig_64.c mcfg_ioremap() >> >> static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg) >> { >> void __iomem *addr; >> u64 start, size; >> int num_buses; >> >> start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus); >> num_buses = cfg->end_bus - cfg->start_bus + 1; >> size = PCI_MMCFG_BUS_OFFSET(num_buses); >> addr = ioremap_nocache(start, size); >> if (addr) >> addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus); >> return addr; >> } >> >> The MCFG config accessors add back the PCI_MMCFG_BUS_OFFSET(cfg->start_bus) >> to the virtual address so that the proper virtual address is used when >> issuing the config cycles, that's my understanding. > > Ok. I think that whether the config space mapping or the config accessors > do the fixup should remain an implementation detail, but the resource > identifying config space should be dealt with consistently. > > So that means the reg property should describe everything from bus 0, > but then we only map the region corresponding to the bus-range. > >> So IMO we have to define what "reg" represents for ECAM in DT, we can't >> leave this open to interpretation (and I think makng MCFG and DT config >> work the same way would be ideal).
I will update the Documentation/devicetree/bindings/pci/host-generic-pci.txt to reflect this interpretation.
> > If we define reg to cover the whole config space from bus 0 onwards, > then I think the driver should work as-is today. It's slightly odd, in > that there may be a prefix of config space that maps to god-knows-where, > but it's consistent with ACPI and doesn't require us to change the driver. > > David?
I agree with this approach for two reasons:
1) My interpretation of relevant specifications agrees with Lorenzo's
2) It is less work for me, as this is how my firmware is currently configured.
This patch 4/6 is still necessary, as the bus_max calculation is broken for non-zero start_bus.
I anticipate sending a new version of the patch set later today (PDT). I will add any Acked-by/Reviewed-by that I receive to the new set.
David Daney
| |