[PATCH v2 0/5] PCI: generic: Misc. bug fixes

[PATCH v2 0/5] PCI: generic: Misc. bug fixes

[David Daney]

From: David Daney <david.daney@cavium.com>

While using the pci-host-generic driver to add PCI support for the
Cavium ThunderX processors, several bugs were discovered.  This patch
set fixes the bugs, a follow-on set will add the ThunderX support.

Changes from v1:

  - "PCI: generic: Allow bus default MSI controller to be specified."
    patch was dropped as it is no longer necessary.

  - "PCI: Make global and export pdev_fixup_irq()." and "PCI: generic:
    Only fixup irqs for bus we are creating." were rewritten to move
    the support into a somewhat more generic form in setup-irq.c.

  - Add some clarifying text to host-generic-pci.txt

  - Add some Acked-by:

David Daney (5):
  PCI: Add pci_bus_fixup_irqs().
  PCI: generic: Only fixup irqs for bus we are creating.
  PCI: generic: Quit clobbering our pci_ops.
  PCI: generic: Correct, and avoid overflow, in bus_max calculation.
  PCI: generic: Pass proper starting bus number to pci_scan_root_bus().

 .../devicetree/bindings/pci/host-generic-pci.txt   |  4 ++-
 drivers/pci/host/pci-host-generic.c                | 38 ++++++++++++----------
 drivers/pci/setup-irq.c                            | 30 +++++++++++++++++
 include/linux/pci.h                                |  4 +++
 4 files changed, 58 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] PCI: Add pci_bus_fixup_irqs().

[David Daney]

From: David Daney <david.daney@cavium.com>

pci_bus_fixup_irqs() works like pci_fixup_irqs(), except it only does
the fixups for devices on the specified bus.

Follow-on patch will use the new function.

Signed-off-by: David Daney <david.daney@cavium.com>
---
This patch didn't exist in v1 of the set.

 drivers/pci/setup-irq.c | 30 ++++++++++++++++++++++++++++++
 include/linux/pci.h     |  4 ++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index 95c225b..189ad17 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -66,3 +66,33 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
 		pdev_fixup_irq(dev, swizzle, map_irq);
 }
 EXPORT_SYMBOL_GPL(pci_fixup_irqs);
+
+struct pci_bus_fixup_cb_info {
+	u8 (*swizzle)(struct pci_dev *, u8 *);
+	int (*map_irq)(const struct pci_dev *, u8, u8);
+};
+
+static int pci_bus_fixup_irq_cb(struct pci_dev *dev, void *arg)
+{
+	struct pci_bus_fixup_cb_info *info = arg;
+
+	pdev_fixup_irq(dev, info->swizzle, info->map_irq);
+	return 0;
+}
+
+/*
+ * Fixup the irqs only for devices on the given bus using supplied
+ * swizzle and map_irq function pointers
+ */
+void pci_bus_fixup_irqs(struct pci_bus *bus,
+			u8 (*swizzle)(struct pci_dev *, u8 *),
+			int (*map_irq)(const struct pci_dev *, u8, u8))
+{
+	struct pci_bus_fixup_cb_info info;
+
+	info.swizzle = swizzle;
+	info.map_irq = map_irq;
+	pci_walk_bus(bus, pci_bus_fixup_irq_cb, &info);
+
+}
+EXPORT_SYMBOL_GPL(pci_bus_fixup_irqs);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e90eb22..b505b50 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1120,6 +1120,10 @@ void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
 		    int (*)(const struct pci_dev *, u8, u8));
+void pci_bus_fixup_irqs(struct pci_bus *,
+			u8 (*)(struct pci_dev *, u8 *),
+			int (*)(const struct pci_dev *, u8, u8));
+
 #define HAVE_PCI_REQ_REGIONS	2
 int __must_check pci_request_regions(struct pci_dev *, const char *);
 int __must_check pci_request_regions_exclusive(struct pci_dev *, const char *);
-- 
1.9.1

[PATCH v2 2/5] PCI: generic: Only fixup irqs for bus we are creating.

[David Daney]

From: David Daney <david.daney@cavium.com>

If we create multiple buses with pci-host-generic, or there are buses
created by other drivers, we don't want to call pci_fixup_irqs() which
operates on all devices, not just the devices on the bus being added.
The consequence is that either the fixups are done more than once, or
in some cases incorrect fixups could be applied.

Call pci_bus_fixup_irqs() instead of pci_fixup_irqs().

Signed-off-by: David Daney <david.daney@cavium.com>
---
Changes from v1: Moved most of the code to pci_bus_fixup_irqs(),
making this patch very simple.

 drivers/pci/host/pci-host-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 265dd25..9e9f1c3 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -262,7 +262,7 @@ static int gen_pci_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+	pci_bus_fixup_irqs(bus, pci_common_swizzle, of_irq_parse_and_map_pci);
 
 	if (!pci_has_flag(PCI_PROBE_ONLY)) {
 		pci_bus_size_bridges(bus);
-- 
1.9.1

Re: [PATCH v2 2/5] PCI: generic: Only fixup irqs for bus we are creating.

[Will Deacon]

On Thu, Sep 17, 2015 at 11:02:09PM +0100, David Daney wrote:
> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> 
> If we create multiple buses with pci-host-generic, or there are buses
> created by other drivers, we don't want to call pci_fixup_irqs() which
> operates on all devices, not just the devices on the bus being added.
> The consequence is that either the fixups are done more than once, or
> in some cases incorrect fixups could be applied.
> 
> Call pci_bus_fixup_irqs() instead of pci_fixup_irqs().
> 
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
> Changes from v1: Moved most of the code to pci_bus_fixup_irqs(),
> making this patch very simple.

Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/5] PCI: generic: Quit clobbering our pci_ops.

[David Daney]

From: David Daney <david.daney@cavium.com>

The pci-host-generic driver keeps a global struct pci_ops which it
then patches with the .map_bus method appropriate for the bus device.
A problem arises when the driver is used for two different types of
bus devices, the .map_bus method for the last device probed clobbers
the method for all previous devices.  The result, only the last bus
device probed has the proper .map_bus, and the others fail.

Move the struct pci_ops into the bus specific structure, and
initialize it when the bus device is probed.  Keep a copy of the
gen_pci_cfg_bus_ops structure, instead of a pointer to a global copy,
to future proof against the addition of bus specific elements to
struct pci_ops.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
No change from v1.

 drivers/pci/host/pci-host-generic.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 9e9f1c3..77cf4bd 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -27,7 +27,7 @@
 
 struct gen_pci_cfg_bus_ops {
 	u32 bus_shift;
-	void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int);
+	struct pci_ops ops;
 };
 
 struct gen_pci_cfg_windows {
@@ -35,7 +35,7 @@ struct gen_pci_cfg_windows {
 	struct resource				*bus_range;
 	void __iomem				**win;
 
-	const struct gen_pci_cfg_bus_ops	*ops;
+	struct gen_pci_cfg_bus_ops		ops;
 };
 
 /*
@@ -65,7 +65,11 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
 
 static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
 	.bus_shift	= 16,
-	.map_bus	= gen_pci_map_cfg_bus_cam,
+	.ops		= {
+		.map_bus	= gen_pci_map_cfg_bus_cam,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
 };
 
 static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
@@ -80,12 +84,11 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
 
 static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
 	.bus_shift	= 20,
-	.map_bus	= gen_pci_map_cfg_bus_ecam,
-};
-
-static struct pci_ops gen_pci_ops = {
-	.read	= pci_generic_config_read,
-	.write	= pci_generic_config_write,
+	.ops		= {
+		.map_bus	= gen_pci_map_cfg_bus_ecam,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
 };
 
 static const struct of_device_id gen_pci_of_match[] = {
@@ -175,7 +178,7 @@ 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;
+		  (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
 	pci->cfg.bus_range->end = min_t(resource_size_t,
 					pci->cfg.bus_range->end, bus_max);
 
@@ -193,7 +196,7 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 	bus_range = pci->cfg.bus_range;
 	for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
 		u32 idx = busn - bus_range->start;
-		u32 sz = 1 << pci->cfg.ops->bus_shift;
+		u32 sz = 1 << pci->cfg.ops.bus_shift;
 
 		pci->cfg.win[idx] = devm_ioremap(dev,
 						 pci->cfg.res.start + busn * sz,
@@ -234,8 +237,7 @@ static int gen_pci_probe(struct platform_device *pdev)
 	}
 
 	of_id = of_match_node(gen_pci_of_match, np);
-	pci->cfg.ops = of_id->data;
-	gen_pci_ops.map_bus = pci->cfg.ops->map_bus;
+	pci->cfg.ops = *(struct gen_pci_cfg_bus_ops *)of_id->data;
 	pci->host.dev.parent = dev;
 	INIT_LIST_HEAD(&pci->host.windows);
 	INIT_LIST_HEAD(&pci->resources);
@@ -256,7 +258,8 @@ static int gen_pci_probe(struct platform_device *pdev)
 	if (!pci_has_flag(PCI_PROBE_ONLY))
 		pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
 
-	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
+	bus = pci_scan_root_bus(dev, 0,
+				&pci->cfg.ops.ops, pci, &pci->resources);
 	if (!bus) {
 		dev_err(dev, "Scanning rootbus failed");
 		return -ENODEV;
-- 
1.9.1

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

[David Daney]

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

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

[Will Deacon]

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

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

[David Daney]

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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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

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

[Will Deacon]

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

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

[Arnd Bergmann]

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

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

[Will Deacon]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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

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

[Will Deacon]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] PCI: generic: Pass proper starting bus number to pci_scan_root_bus().

[David Daney]

From: David Daney <david.daney@cavium.com>

If the bus is being configured with a bus-range that does not start at
zero, pass that starting bus number to pci_scan_root_bus().  Passing
the incorrect value of zero causes attempted config accesses outside
of the supported range, which cascades to an OOPs spew and eventual
kernel panic.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
No change from v1.

 drivers/pci/host/pci-host-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 0a9c453..e364232 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -259,7 +259,7 @@ static int gen_pci_probe(struct platform_device *pdev)
 	if (!pci_has_flag(PCI_PROBE_ONLY))
 		pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
 
-	bus = pci_scan_root_bus(dev, 0,
+	bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start,
 				&pci->cfg.ops.ops, pci, &pci->resources);
 	if (!bus) {
 		dev_err(dev, "Scanning rootbus failed");
-- 
1.9.1