[PATCH v3 0/6] PCI: generic: Misc. bug fixes/enhancements

[PATCH v3 0/6] PCI: generic: Misc. bug fixes/enhancements

[David Daney]

From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

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 v2:

  - Added " PCI: generic: Claim device resources if PCI_PROBE_ONLY"

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 (6):
  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().
  PCI: generic: Claim device resources if PCI_PROBE_ONLY

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

-- 
1.9.1

--
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 v3 1/6] 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>
---
No change from v2.

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 v3 2/6] 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>
---
No change from v2.

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

[PATCH v3 3/6] 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 or v2.

 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 v3 4/6] 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>
---
No change from v2.
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

[PATCH v3 5/6] 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 or v2.

 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

[PATCH v3 6/6] PCI: generic: Claim device resources if PCI_PROBE_ONLY

[David Daney]

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

In the case where the PCI_PROBE_ONLY flag is set, we need to claim the
resources for all PCI devices added to the bus.  Failure to claim
SRIOV BAR resources prevents SRIOV devices from being being enabled.

So, when the PCI_PROBE_ONLY flag is set, claim MEM and IO resources
for all devices that were added to the bus.

Signed-off-by: David Daney <david.daney@cavium.com>
---
Added this patch in v3.

 drivers/pci/host/pci-host-generic.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index e364232..b163fdc 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -209,6 +209,24 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 	return 0;
 }
 
+static int gen_pci_claim_resource_cb(struct pci_dev *dev, void *arg)
+{
+	int resno;
+
+	for (resno = 0; resno < PCI_BRIDGE_RESOURCES; resno++) {
+		struct resource *res = &dev->resource[resno];
+
+		/* If it already has a parent, don't claim it. */
+		if (res->parent)
+			continue;
+
+		if (resource_type(res) == IORESOURCE_MEM ||
+		    resource_type(res) == IORESOURCE_IO)
+			pci_claim_resource(dev, resno);
+	}
+	return 0;
+}
+
 static int gen_pci_probe(struct platform_device *pdev)
 {
 	int err;
@@ -277,6 +295,9 @@ static int gen_pci_probe(struct platform_device *pdev)
 	}
 
 	pci_bus_add_devices(bus);
+
+	if (pci_has_flag(PCI_PROBE_ONLY))
+		pci_walk_bus(bus, gen_pci_claim_resource_cb, NULL);
 	return 0;
 }
 
-- 
1.9.1

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

[Arnd Bergmann]

On Tuesday 22 September 2015 16:49:15 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>

Not sure about this one

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

This sounds like very unusual behavior. If you have a system with faked
bus numbers where the registers only physically exist for a subset of the
buses, this requires defining a reg property that contains MMIO space
which is outside of the device and potentially contains other devices.

What would break if we instead defined it the expected way and only
list the registers for the bus numbers in the "bus-range" property?

	Arnd

Re: [PATCH v3 0/6] PCI: generic: Misc. bug fixes/enhancements

[Arnd Bergmann]

On Tuesday 22 September 2015 16:49:11 David Daney wrote:
> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> 
> 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.
> 

Looks good overall, except for one patch that I commented on.

	Arnd
--
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 v3 3/6] PCI: generic: Quit clobbering our pci_ops.

[Arnd Bergmann]

On Tuesday 22 September 2015 16:49:14 David Daney wrote:
> 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,

This is a very useful change.

> to future proof against the addition of bus specific elements to
> struct pci_ops.

but I don't like this part. We should just not have bus specific
elements in pci_ops. We don't really have that here either, except
that the gen_pci driver had a hack for reusing the same operations
for things that are actually different.

It's an established practice that anything named '*_operations' is
meant to be constant and ideally defined as 'static const ... *_ops;'
in the driver. We could try to enforce this better by marking
bus->ops as 'const' and changing all the instances of this structure
accordingly.

> @@ -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,
> +	}
>  };

So this is good. We could in theory unify the map_bus functions
like this now:

static void __iomem *gen_pci_map_cfg_bus(struct pci_bus *bus,
                                         unsigned int devfn,
                                         int where)
{
        struct gen_pci *pci = bus->sysdata;
	struct gen_pci_cfg_bus_ops *ops;
        resource_size_t idx;

	ops = container_of(bus->ops, struct gen_pci_cfg_bus_ops, ops);
	idx = bus->number - pci->cfg.bus_range->start;

        return pci->cfg.win[idx] + ((devfn << ops->dev_shift) | where);
}

Not sure if that improves clarity or not, up to Will.

> @@ -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;
> +	pci->cfg.ops = *(struct gen_pci_cfg_bus_ops *)of_id->data;

This is the part that grabbed my attention, we should not do it like this.

	Arnd

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

[David Daney]

On 09/23/2015 01:01 AM, Arnd Bergmann wrote:
> On Tuesday 22 September 2015 16:49:15 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>
>
> Not sure about this one
>
>> 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.
>
> This sounds like very unusual behavior. If you have a system with faked
> bus numbers where the registers only physically exist for a subset of the
> buses, this requires defining a reg property that contains MMIO space
> which is outside of the device and potentially contains other devices.

The pci-host-generic driver only maps the ranges that correspond to the 
"bus-range" buses, so mapping of illegal address ranges should not be a 
problem.

>
> What would break if we instead defined it the expected way and only
> list the registers for the bus numbers in the "bus-range" property?

I'm not sure if we have the luxury of being able to change the 
definition, although the existing code only works with a starting bus 
number of zero.  From this we might conclude that non-zero starting bus 
numbers cannot exist in the wild, so changing the the definition of 
"reg" so that it starts at the starting bus number might be possible.

My reading of:

http://www.o3one.org/hwdocs/openfirmware/pci_supplement_2_1.pdf

Section 3.1.1, does not preclude your interpretation.  Although that is 
for PCI-PCI bridges, and not this pci-host-generic root complex.

If we really want to go with a different definition of what the "reg" 
property means, then actual code has to change, and we risk breaking 
something.

David Daney

Re: [PATCH v3 3/6] PCI: generic: Quit clobbering our pci_ops.

[David Daney]

On 09/23/2015 01:21 AM, Arnd Bergmann wrote:
> On Tuesday 22 September 2015 16:49:14 David Daney wrote:
>> 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,
>
> This is a very useful change.
>
>> to future proof against the addition of bus specific elements to
>> struct pci_ops.
>
> but I don't like this part. We should just not have bus specific
> elements in pci_ops. We don't really have that here either, except
> that the gen_pci driver had a hack for reusing the same operations
> for things that are actually different.
>
> It's an established practice that anything named '*_operations' is
> meant to be constant and ideally defined as 'static const ... *_ops;'
> in the driver. We could try to enforce this better by marking
> bus->ops as 'const' and changing all the instances of this structure
> accordingly.
>
>> @@ -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,
>> +	}
>>   };
>
> So this is good. We could in theory unify the map_bus functions
> like this now:
>
> static void __iomem *gen_pci_map_cfg_bus(struct pci_bus *bus,
>                                           unsigned int devfn,
>                                           int where)
> {
>          struct gen_pci *pci = bus->sysdata;
> 	struct gen_pci_cfg_bus_ops *ops;
>          resource_size_t idx;
>
> 	ops = container_of(bus->ops, struct gen_pci_cfg_bus_ops, ops);
> 	idx = bus->number - pci->cfg.bus_range->start;
>
>          return pci->cfg.win[idx] + ((devfn << ops->dev_shift) | where);
> }
>
> Not sure if that improves clarity or not, up to Will.
>
>> @@ -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;
>> +	pci->cfg.ops = *(struct gen_pci_cfg_bus_ops *)of_id->data;
>
> This is the part that grabbed my attention, we should not do it like this.
>

I will consider changing this so that a structure copy is not used, 
perhaps as you suggest above.

David Daney.


> 	Arnd
>

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

[Arnd Bergmann]

On Wednesday 23 September 2015 08:50:45 David Daney wrote:
> On 09/23/2015 01:01 AM, Arnd Bergmann wrote:
> > On Tuesday 22 September 2015 16:49:15 David Daney wrote:
> >> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >> 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.
> >
> > This sounds like very unusual behavior. If you have a system with faked
> > bus numbers where the registers only physically exist for a subset of the
> > buses, this requires defining a reg property that contains MMIO space
> > which is outside of the device and potentially contains other devices.
> 
> The pci-host-generic driver only maps the ranges that correspond to the 
> "bus-range" buses, so mapping of illegal address ranges should not be a 
> problem.

There is still a problem with what the driver is allowed to map,
the current behavior is just an implementation detail that should
not mean the binding can rely on that.

We do the per-bus mapping because the vmalloc space on 32-bit systems
is very limited. A driver for a purely 64-bit OS could simplify this
by just mapping the entire 'reg' property as most drivers do, and
then use the device as an offset into that.

> > What would break if we instead defined it the expected way and only
> > list the registers for the bus numbers in the "bus-range" property?
> 
> I'm not sure if we have the luxury of being able to change the 
> definition, although the existing code only works with a starting bus 
> number of zero.  From this we might conclude that non-zero starting bus 
> numbers cannot exist in the wild, so changing the the definition of 
> "reg" so that it starts at the starting bus number might be possible.
>
> My reading of:
> 
> http://www.o3one.org/hwdocs/openfirmware/pci_supplement_2_1.pdf
> 
> Section 3.1.1, does not preclude your interpretation.  Although that is 
> for PCI-PCI bridges, and not this pci-host-generic root complex.
> 
> If we really want to go with a different definition of what the "reg" 
> property means, then actual code has to change, and we risk breaking 
> something.

My understanding is that the code has to change anyway in one place
or another. The change you did in this patch is not needed then, but
you say that something else has to change. Is this the only change
we'd need?

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 265dd25169bf..d8a5c0047155 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -196,7 +196,7 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 		u32 sz = 1 << pci->cfg.ops->bus_shift;
 
 		pci->cfg.win[idx] = devm_ioremap(dev,
-						 pci->cfg.res.start + busn * sz,
+						 pci->cfg.res.start + idx * sz,
 						 sz);
 		if (!pci->cfg.win[idx])
 			return -ENOMEM;

	Arnd
--
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