| Message ID | 1443811443-18878-5-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[+cc Arnd] And this one? On Fri, Oct 02, 2015 at 11:44:02AM -0700, 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 V3: Add to explanation of "reg" property in > host-generic-pci.txt. Add error message if "reg" property is too big. > > Documentation/devicetree/bindings/pci/host-generic-pci.txt | 6 +++++- > drivers/pci/host/pci-host-generic.c | 12 +++++++++--- > 2 files changed, 14 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..42303bb 100644 > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > @@ -34,7 +34,11 @@ 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. The driver must only map > + or access the portion of the Configuration Space that > + corresponds to the "bus-range" > > > Properties of the /chosen node: > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 216ded5..5cce837 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,14 @@ 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) { > + dev_err(dev, > + "\"reg\" size corresponds to bus %d, truncating to 255\n", > + bus_max); > + bus_max = 255; > + } > + > pci->cfg.bus_range->end = min_t(resource_size_t, > pci->cfg.bus_range->end, bus_max); > > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Thursday 08 October 2015 10:02:43 Bjorn Helgaas wrote: > [+cc Arnd] > > And this one? Not so good. > > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt > > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > > @@ -34,7 +34,11 @@ 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. The driver must only map > > + or access the portion of the Configuration Space that > > + corresponds to the "bus-range" I thought we had reached an agreement that it is a bad idea to have a 'reg' property that lists registers belonging to a different device. Arnd
On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote: > > > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt > > > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > > > @@ -34,7 +34,11 @@ 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. The driver must only map > > > + or access the portion of the Configuration Space that > > > + corresponds to the "bus-range" > > I thought we had reached an agreement that it is a bad idea to have a 'reg' > property that lists registers belonging to a different device. > > To further clarify: the argument was to make it mirror what ACPI does for PCI. However, this is unlike what all other devices do with DT, where you have non-overlapping register ranges (start, length) for each device. ACPI as far as I understand it does not give a range for a PCIe host, but instead provides a way to get the start address of the ECAM register area for the domain that the host is part of, and that needs to be the same address for each host in the domain. Arnd
On 10/08/2015 08:18 AM, Arnd Bergmann wrote: > On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote: >>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt >>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt >>>> @@ -34,7 +34,11 @@ 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. The driver must only map >>>> + or access the portion of the Configuration Space that >>>> + corresponds to the "bus-range" >> >> I thought we had reached an agreement that it is a bad idea to have a 'reg' >> property that lists registers belonging to a different device. >> >> > > To further clarify: the argument was to make it mirror what ACPI does for > PCI. However, this is unlike what all other devices do with DT, where you > have non-overlapping register ranges (start, length) for each device. > ACPI as far as I understand it does not give a range for a PCIe host, but > instead provides a way to get the start address of the ECAM register area > for the domain that the host is part of, and that needs to be the same > address for each host in the domain. > We are agreed that it is a bad thing to do. There is disagreement about if we should do it. I think there are two schools of thought (I will attribute them to their proponents and my apologies if I misrepresent someone's stance): 1) (Arnd) Don't make the the "reg" ranges overlap because it is ugly, dangerous and arguably incorrect in general. 2) (Me, Will Deacon, Lorenzo Pieralisi) Overlapping "reg" properties should be maintained, as that is the current behavior and seems to agree with legacy OF device-tree specifications (although there is some debate about this). Because the driver is broken in this area (thus the patch), it indicates that there are probably no users with non-zero starting bus numbers. So, we may have some latitude to change it. I will generate another patch that does it Arnd's way, and if Will is OK with it, we might be able to do that instead. One thing is certain: The driver is currently broken for non-zero starting bus numbers. David Daney
On Thu, Oct 08, 2015 at 08:39:58AM -0700, David Daney wrote: > On 10/08/2015 08:18 AM, Arnd Bergmann wrote: > >On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote: > >>>>--- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt > >>>>+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > >>>>@@ -34,7 +34,11 @@ 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. The driver must only map > >>>>+ or access the portion of the Configuration Space that > >>>>+ corresponds to the "bus-range" > >> > >>I thought we had reached an agreement that it is a bad idea to have a 'reg' > >>property that lists registers belonging to a different device. > >> > >> > > > >To further clarify: the argument was to make it mirror what ACPI does for > >PCI. However, this is unlike what all other devices do with DT, where you > >have non-overlapping register ranges (start, length) for each device. > >ACPI as far as I understand it does not give a range for a PCIe host, but > >instead provides a way to get the start address of the ECAM register area > >for the domain that the host is part of, and that needs to be the same > >address for each host in the domain. > > > > We are agreed that it is a bad thing to do. There is disagreement > about if we should do it. > > I think there are two schools of thought (I will attribute them to > their proponents and my apologies if I misrepresent someone's > stance): > > 1) (Arnd) Don't make the the "reg" ranges overlap because it is > ugly, dangerous and arguably incorrect in general. > > 2) (Me, Will Deacon, Lorenzo Pieralisi) Overlapping "reg" properties > should be maintained, as that is the current behavior and seems to > agree with legacy OF device-tree specifications (although there is > some debate about this). I have just quoted the PCI FW specification, 4.1.4 section: "System Software Implication of MCFG and _CBA" which defines the ACPI behaviour of MCFG and _CBA methods for hotplug bridges. I understand Arnd's concerns and I do not think the DT bindings are well defined in this respect, it is ok for me to define the DT bindings using the "usual" reg property bindings representation, as long as we document it and we all agree we are deviating from the PCI FW specs (that just cover ACPI, BTW). I think it is worth investigating why the current PCI FW specs were defined that way for ECAM config space before proceeding any further, to avoid pitfalls we might be missing. Thanks, Lorenzo > Because the driver is broken in this area (thus the patch), it > indicates that there are probably no users with non-zero starting > bus numbers. So, we may have some latitude to change it. > > I will generate another patch that does it Arnd's way, and if Will > is OK with it, we might be able to do that instead. One thing is > certain: The driver is currently broken for non-zero starting bus > numbers. > > David Daney > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt index cf3e205..42303bb 100644 --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt @@ -34,7 +34,11 @@ 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. The driver must only map + or access the portion of the Configuration Space that + corresponds to the "bus-range" Properties of the /chosen node: diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index 216ded5..5cce837 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,14 @@ 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) { + dev_err(dev, + "\"reg\" size corresponds to bus %d, truncating to 255\n", + bus_max); + bus_max = 255; + } + pci->cfg.bus_range->end = min_t(resource_size_t, pci->cfg.bus_range->end, bus_max);