| Message ID | 1442013719-5001-5-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote: > From: David Daney <david.daney@cavium.com> > > There are two problems with the bus_max calculation: > > 1) The u8 data type can overflow for large config space windows. > > 2) The calculation is incorrect for a bus range that doesn't start at > zero. > > Since the configuration space is relative to bus zero, make bus_max > just be the size of the config window scaled by bus_shift. Then clamp > it to a maximum of 255, per PCI. Use a data type of int to avoid > overflow problems. > > Signed-off-by: David Daney <david.daney@cavium.com> > --- > drivers/pci/host/pci-host-generic.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index cd6f898..fce5bf7 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -164,7 +164,7 @@ out_release_res: > static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > { > int err; > - u8 bus_max; > + int bus_max; > resource_size_t busn; > struct resource *bus_range; > struct device *dev = pci->host.dev.parent; > @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > } > > /* Limit the bus-range to fit within reg */ > - bus_max = pci->cfg.bus_range->start + > - (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; > + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; > + if (bus_max > 255) > + bus_max = 255; > pci->cfg.bus_range->end = min_t(resource_size_t, > pci->cfg.bus_range->end, bus_max); Hmm, this is changing the meaning of the bus-range property in the device-tree, which really needs to match what IEEE Std 1275-1994 requires. My understanding was that the bus-range could be used to offset the config space, which is why it's subtracted from the bus number in gen_pci_map_cfg_bus_[e]cam. Also, why is your config space so large that we end up overflowing bus_max? Will
On 09/15/2015 10:49 AM, Will Deacon wrote: > On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote: >> From: David Daney <david.daney@cavium.com> >> >> There are two problems with the bus_max calculation: >> >> 1) The u8 data type can overflow for large config space windows. >> >> 2) The calculation is incorrect for a bus range that doesn't start at >> zero. >> >> Since the configuration space is relative to bus zero, make bus_max >> just be the size of the config window scaled by bus_shift. Then clamp >> it to a maximum of 255, per PCI. Use a data type of int to avoid >> overflow problems. >> >> Signed-off-by: David Daney <david.daney@cavium.com> >> --- >> drivers/pci/host/pci-host-generic.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c >> index cd6f898..fce5bf7 100644 >> --- a/drivers/pci/host/pci-host-generic.c >> +++ b/drivers/pci/host/pci-host-generic.c >> @@ -164,7 +164,7 @@ out_release_res: >> static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) >> { >> int err; >> - u8 bus_max; >> + int bus_max; >> resource_size_t busn; >> struct resource *bus_range; >> struct device *dev = pci->host.dev.parent; >> @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) >> } >> >> /* Limit the bus-range to fit within reg */ >> - bus_max = pci->cfg.bus_range->start + >> - (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; >> + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; >> + if (bus_max > 255) >> + bus_max = 255; >> pci->cfg.bus_range->end = min_t(resource_size_t, >> pci->cfg.bus_range->end, bus_max); > > Hmm, this is changing the meaning of the bus-range property in the > device-tree, which really needs to match what IEEE Std 1275-1994 requires. I doesn't change the bus-range. > > My understanding was that the bus-range could be used to offset the config > space, which is why it's subtracted from the bus number in > gen_pci_map_cfg_bus_[e]cam. There is an inconsistency in the current code. The calculation of the cfg.win[?] pointers is done such that the beginning of the config space specified in the "reg" property corresponds to bus 0. The calculation that I am changing, was done such that the beginning of the config space specified in the "reg" property corresponds to the first bus of the "bus-range" Which is correct? I assumed that the config space specified in the "reg" property corresponds to bus 0. Based on this assumption, I made the bus_max calculation match. Due to hardware peculiarities, our bus-range starts at a non-zero bus number. So, something has to be done to make all the code agree on a single interpretation of the meaning "reg" property. > Also, why is your config space so large that > we end up overflowing bus_max? It isn't. The part of the patch that changes the type from u8 to int was just to add some sanity. The code was easily susceptible to overflow failures, it seemed best to change to int. David Daney
On Tue, Sep 15, 2015 at 07:02:54PM +0100, David Daney wrote: > On 09/15/2015 10:49 AM, Will Deacon wrote: > > On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote: > >> /* Limit the bus-range to fit within reg */ > >> - bus_max = pci->cfg.bus_range->start + > >> - (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; > >> + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; > >> + if (bus_max > 255) > >> + bus_max = 255; > >> pci->cfg.bus_range->end = min_t(resource_size_t, > >> pci->cfg.bus_range->end, bus_max); > > > > Hmm, this is changing the meaning of the bus-range property in the > > device-tree, which really needs to match what IEEE Std 1275-1994 requires. > > I doesn't change the bus-range. Not directly, but pci->cfg.bus_range is a resource populated from the "bus-range" property in the device-tree, so it's changing how the driver uses that property. > > My understanding was that the bus-range could be used to offset the config > > space, which is why it's subtracted from the bus number in > > gen_pci_map_cfg_bus_[e]cam. > > There is an inconsistency in the current code. The calculation of the > cfg.win[?] pointers is done such that the beginning of the config space > specified in the "reg" property corresponds to bus 0. I don't follow you here. The mapping functions explicitly subtract the start of the bus range when calculating the window offset: resource_size_t idx = bus->number - pci->cfg.bus_range->start; so if I have bus-range = <128 255>; then bus 128 lives at the start of the configuration space described by the reg property, not bus 0. Sorry if I'm being thick; I just can't see the inconsistency. > The calculation that I am changing, was done such that the beginning of > the config space specified in the "reg" property corresponds to the > first bus of the "bus-range" > > Which is correct? I assumed that the config space specified in the > "reg" property corresponds to bus 0. Based on this assumption, I made > the bus_max calculation match. > > Due to hardware peculiarities, our bus-range starts at a non-zero bus > number. So, something has to be done to make all the code agree on a > single interpretation of the meaning "reg" property. I think you're the first to exercise this code, so it's definitely worth us fixing whatever's going wrong. > > Also, why is your config space so large that > > we end up overflowing bus_max? > > It isn't. The part of the patch that changes the type from u8 to int > was just to add some sanity. The code was easily susceptible to > overflow failures, it seemed best to change to int. Can we drop this part for now if it's not actually needed? Will
On 09/15/2015 11:35 AM, Will Deacon wrote: > On Tue, Sep 15, 2015 at 07:02:54PM +0100, David Daney wrote: >> On 09/15/2015 10:49 AM, Will Deacon wrote: >>> On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote: >>>> /* Limit the bus-range to fit within reg */ >>>> - bus_max = pci->cfg.bus_range->start + >>>> - (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; >>>> + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; >>>> + if (bus_max > 255) >>>> + bus_max = 255; >>>> pci->cfg.bus_range->end = min_t(resource_size_t, >>>> pci->cfg.bus_range->end, bus_max); >>> >>> Hmm, this is changing the meaning of the bus-range property in the >>> device-tree, which really needs to match what IEEE Std 1275-1994 requires. >> >> I doesn't change the bus-range. > > Not directly, but pci->cfg.bus_range is a resource populated from the > "bus-range" property in the device-tree, so it's changing how the driver > uses that property. > >>> My understanding was that the bus-range could be used to offset the config >>> space, which is why it's subtracted from the bus number in >>> gen_pci_map_cfg_bus_[e]cam. >> >> There is an inconsistency in the current code. The calculation of the >> cfg.win[?] pointers is done such that the beginning of the config space >> specified in the "reg" property corresponds to bus 0. > > I don't follow you here. The mapping functions explicitly subtract the > start of the bus range when calculating the window offset: > > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > so if I have bus-range = <128 255>; then bus 128 lives at the start of > the configuration space described by the reg property, not bus 0. > > Sorry if I'm being thick; I just can't see the inconsistency. > 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. >> The calculation that I am changing, was done such that the beginning of >> the config space specified in the "reg" property corresponds to the >> first bus of the "bus-range" >> >> Which is correct? I assumed that the config space specified in the >> "reg" property corresponds to bus 0. Based on this assumption, I made >> the bus_max calculation match. >> >> Due to hardware peculiarities, our bus-range starts at a non-zero bus >> number. So, something has to be done to make all the code agree on a >> single interpretation of the meaning "reg" property. > > I think you're the first to exercise this code, so it's definitely worth > us fixing whatever's going wrong. > >>> Also, why is your config space so large that >>> we end up overflowing bus_max? >> >> It isn't. The part of the patch that changes the type from u8 to int >> was just to add some sanity. The code was easily susceptible to >> overflow failures, it seemed best to change to int. > > Can we drop this part for now if it's not actually needed? > > Will >
On Tue, Sep 15, 2015 at 07:45:56PM +0100, David Daney wrote: > On 09/15/2015 11:35 AM, Will Deacon wrote: > > On Tue, Sep 15, 2015 at 07:02:54PM +0100, David Daney wrote: > >> On 09/15/2015 10:49 AM, Will Deacon wrote: > >>> On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote: > >>>> /* Limit the bus-range to fit within reg */ > >>>> - bus_max = pci->cfg.bus_range->start + > >>>> - (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; > >>>> + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; > >>>> + if (bus_max > 255) > >>>> + bus_max = 255; > >>>> pci->cfg.bus_range->end = min_t(resource_size_t, > >>>> pci->cfg.bus_range->end, bus_max); > >>> > >>> Hmm, this is changing the meaning of the bus-range property in the > >>> device-tree, which really needs to match what IEEE Std 1275-1994 requires. > >> > >> I doesn't change the bus-range. > > > > Not directly, but pci->cfg.bus_range is a resource populated from the > > "bus-range" property in the device-tree, so it's changing how the driver > > uses that property. > > > >>> My understanding was that the bus-range could be used to offset the config > >>> space, which is why it's subtracted from the bus number in > >>> gen_pci_map_cfg_bus_[e]cam. > >> > >> There is an inconsistency in the current code. The calculation of the > >> cfg.win[?] pointers is done such that the beginning of the config space > >> specified in the "reg" property corresponds to bus 0. > > > > I don't follow you here. The mapping functions explicitly subtract the > > start of the bus range when calculating the window offset: > > > > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > > > so if I have bus-range = <128 255>; then bus 128 lives at the start of > > the configuration space described by the reg property, not bus 0. > > > > Sorry if I'm being thick; I just can't see the inconsistency. > > > > 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. Will
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
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). 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? Will
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
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index cd6f898..fce5bf7 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -164,7 +164,7 @@ out_release_res: static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) { int err; - u8 bus_max; + int bus_max; resource_size_t busn; struct resource *bus_range; struct device *dev = pci->host.dev.parent; @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) } /* Limit the bus-range to fit within reg */ - bus_max = pci->cfg.bus_range->start + - (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; + if (bus_max > 255) + bus_max = 255; pci->cfg.bus_range->end = min_t(resource_size_t, pci->cfg.bus_range->end, bus_max);