Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Lorenzo Pieralisi]

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.

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

Thoughts appreciated, I will countercheck on my side.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/