| Message ID | 1442527332-1174-5-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 17, 2015 at 11:02:11PM +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. > > Update host-generic-pci.txt to clarify the semantics of the "reg" > property with respect to non-zero starting bus numbers. > > Signed-off-by: David Daney <david.daney@cavium.com> > --- > Change from v1: Added text to host-generic-pci.txt > > Documentation/devicetree/bindings/pci/host-generic-pci.txt | 4 +++- > drivers/pci/host/pci-host-generic.c | 7 ++++--- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > index cf3e205..105a968 100644 > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > @@ -34,7 +34,9 @@ Properties of the host controller node: > - #size-cells : Must be 2. > > - reg : The Configuration Space base address and size, as accessed > - from the parent bus. > + from the parent bus. The base address corresponds to > + bus zero, even though the "bus-range" property may specify > + a different starting bus number. That's a useful clarification, thanks. > Properties of the /chosen node: > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 77cf4bd..0a9c453 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; I still don't understand the need for this part. If the cfg space is bigger than bus_max, isn't that simply an invalid resource? Given that the resource could be broken in other ways too, this check feels more like a specific workaround rather than generally useful code. Will
On 09/23/2015 11:01 AM, Will Deacon wrote: > On Thu, Sep 17, 2015 at 11:02:11PM +0100, David Daney wrote: [...] > >> Properties of the /chosen node: >> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c >> index 77cf4bd..0a9c453 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; > > I still don't understand the need for this part. If the cfg space is bigger > than bus_max, isn't that simply an invalid resource? Given that the resource > could be broken in other ways too, this check feels more like a specific > workaround rather than generally useful code. Imagine... bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the entire range of 0..0xff. according to the calculations above, (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so... bus_max = 0x80 + 0xff -> OVERFLOW of u8! That is not useful. bus_max should represent the largest bus number that can be covered by cfg.res. That is what my patch is attempting to accomplish. Calculate the largest bus number that can be accommodated by cfg.res, and then clamp it to 0xff. David Daney.
On Wednesday 23 September 2015 11:21:56 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; > > > > I still don't understand the need for this part. If the cfg space is bigger > > than bus_max, isn't that simply an invalid resource? Given that the resource > > could be broken in other ways too, this check feels more like a specific > > workaround rather than generally useful code. > > Imagine... > > bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the > entire range of 0..0xff. > > according to the calculations above, (resource_size(&pci->cfg.res) >> > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so... Extending the computation to 32 bit seems fine, but I'd rather warn loudly if the bus range does not fit within the registers. Also note that the computation is already correct with my interpretation of the reg property. Arnd
On Wed, Sep 23, 2015 at 07:21:56PM +0100, David Daney wrote: > On 09/23/2015 11:01 AM, Will Deacon wrote: > > On Thu, Sep 17, 2015 at 11:02:11PM +0100, David Daney wrote: > [...] > > > >> Properties of the /chosen node: > >> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > >> index 77cf4bd..0a9c453 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; > > > > I still don't understand the need for this part. If the cfg space is bigger > > than bus_max, isn't that simply an invalid resource? Given that the resource > > could be broken in other ways too, this check feels more like a specific > > workaround rather than generally useful code. > > Imagine... > > bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the > entire range of 0..0xff. > > according to the calculations above, (resource_size(&pci->cfg.res) >> > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so... > > bus_max = 0x80 + 0xff -> OVERFLOW of u8! > > That is not useful. bus_max should represent the largest bus number > that can be covered by cfg.res. That is what my patch is attempting to > accomplish. Calculate the largest bus number that can be accommodated > by cfg.res, and then clamp it to 0xff. Sorry, I should've been more specific. The only part I don't like is the 'if (bus_max > 255)' check. Will
On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote: > On Wednesday 23 September 2015 11:21:56 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; > > > > > > I still don't understand the need for this part. If the cfg space is bigger > > > than bus_max, isn't that simply an invalid resource? Given that the resource > > > could be broken in other ways too, this check feels more like a specific > > > workaround rather than generally useful code. > > > > Imagine... > > > > bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the > > entire range of 0..0xff. > > > > according to the calculations above, (resource_size(&pci->cfg.res) >> > > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so... > > Extending the computation to 32 bit seems fine, but I'd rather warn loudly > if the bus range does not fit within the registers. > > Also note that the computation is already correct with my interpretation > of the reg property. From what Lorenzo was saying, ACPI shares the interpretation that David is implementing here and, given that the DT version seems to be subjective, aligning this reg property with MMCFG seems to make sense. Will
On Wednesday 23 September 2015 20:35:45 Will Deacon wrote: > On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote: > > On Wednesday 23 September 2015 11:21:56 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; > > > > > > > > I still don't understand the need for this part. If the cfg space is bigger > > > > than bus_max, isn't that simply an invalid resource? Given that the resource > > > > could be broken in other ways too, this check feels more like a specific > > > > workaround rather than generally useful code. > > > > > > Imagine... > > > > > > bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the > > > entire range of 0..0xff. > > > > > > according to the calculations above, (resource_size(&pci->cfg.res) >> > > > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so... > > > > Extending the computation to 32 bit seems fine, but I'd rather warn loudly > > if the bus range does not fit within the registers. > > > > Also note that the computation is already correct with my interpretation > > of the reg property. > > From what Lorenzo was saying, ACPI shares the interpretation that David is > implementing here and, given that the DT version seems to be subjective, > aligning this reg property with MMCFG seems to make sense. We should then make it very clear in the binding that the driver is not allowed to actually map the registers for the buses outside of the bus-range, as that is highly unusual. We would also need a special exception for this if we ever get to implement the DT source checker that we have been talking about for years, as the reg property might then overlap with a property from another device. Arnd
On Wed, Sep 23, 2015 at 08:39:27PM +0100, Arnd Bergmann wrote: > On Wednesday 23 September 2015 20:35:45 Will Deacon wrote: > > On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote: > > > On Wednesday 23 September 2015 11:21:56 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; > > > > > > > > > > I still don't understand the need for this part. If the cfg space is bigger > > > > > than bus_max, isn't that simply an invalid resource? Given that the resource > > > > > could be broken in other ways too, this check feels more like a specific > > > > > workaround rather than generally useful code. > > > > > > > > Imagine... > > > > > > > > bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the > > > > entire range of 0..0xff. > > > > > > > > according to the calculations above, (resource_size(&pci->cfg.res) >> > > > > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so... > > > > > > Extending the computation to 32 bit seems fine, but I'd rather warn loudly > > > if the bus range does not fit within the registers. > > > > > > Also note that the computation is already correct with my interpretation > > > of the reg property. > > > > From what Lorenzo was saying, ACPI shares the interpretation that David is > > implementing here and, given that the DT version seems to be subjective, > > aligning this reg property with MMCFG seems to make sense. > > We should then make it very clear in the binding that the driver > is not allowed to actually map the registers for the buses outside > of the bus-range, as that is highly unusual. > > We would also need a special exception for this if we ever get to > implement the DT source checker that we have been talking about for > years, as the reg property might then overlap with a property from > another device. Completely agreed. Having a base that isn't actually safe to map is horrible and should be called out. Will
On Wednesday 23 September 2015 21:39:27 Arnd Bergmann wrote: > On Wednesday 23 September 2015 20:35:45 Will Deacon wrote: > > > > From what Lorenzo was saying, ACPI shares the interpretation that David is > > implementing here and, given that the DT version seems to be subjective, > > aligning this reg property with MMCFG seems to make sense. > > We should then make it very clear in the binding that the driver > is not allowed to actually map the registers for the buses outside > of the bus-range, as that is highly unusual. One more point here: I think ACPI does something different here, it lists the base address of mmconfig space of the the PCI domain that the host bridge belongs to. In contrast, in DT the 'reg' property is defined as the registers belonging to the host bridge itself, and the DT does not say anything about domains at all (aside from the linux,pci-domain property that is not mandatory and a Linux-specific extension). Arnd
diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt index cf3e205..105a968 100644 --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt @@ -34,7 +34,9 @@ Properties of the host controller node: - #size-cells : Must be 2. - reg : The Configuration Space base address and size, as accessed - from the parent bus. + from the parent bus. The base address corresponds to + bus zero, even though the "bus-range" property may specify + a different starting bus number. Properties of the /chosen node: diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index 77cf4bd..0a9c453 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);