[PATCH 0/6] PCI: generic: Misc. bug fixes and enhanced support for MSI.

[PATCH 0/6] PCI: generic: Misc. bug fixes and enhanced support for MSI.

[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.  We also
need the ability to specify a per-bus MSI controller, so support for
that was added.

David Daney (6):
  PCI: Make global and export pdev_fixup_irq().
  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: Allow bus default MSI controller to be specified.

 .../devicetree/bindings/pci/host-generic-pci.txt   |  5 ++
 drivers/pci/host/pci-host-generic.c                | 55 +++++++++++++++-------
 drivers/pci/setup-irq.c                            |  7 +--
 include/linux/pci.h                                |  3 ++
 4 files changed, 50 insertions(+), 20 deletions(-)

-- 
1.7.11.7

[PATCH 1/6] PCI: Make global and export pdev_fixup_irq().

[David Daney]

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

Follow-on patch will use pdev_fixup_irq().  So, make it visible and
export it.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/pci/setup-irq.c | 7 ++++---
 include/linux/pci.h     | 3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index 95c225b..ac7ffaf 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -22,9 +22,9 @@ void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
 	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
 }
 
-static void pdev_fixup_irq(struct pci_dev *dev,
-			   u8 (*swizzle)(struct pci_dev *, u8 *),
-			   int (*map_irq)(const struct pci_dev *, u8, u8))
+void pdev_fixup_irq(struct pci_dev *dev,
+		    u8 (*swizzle)(struct pci_dev *, u8 *),
+		    int (*map_irq)(const struct pci_dev *, u8, u8))
 {
 	u8 pin, slot;
 	int irq = 0;
@@ -56,6 +56,7 @@ static void pdev_fixup_irq(struct pci_dev *dev,
 	   the real IRQ to use; the device does not use it. */
 	pcibios_update_irq(dev, irq);
 }
+EXPORT_SYMBOL_GPL(pdev_fixup_irq);
 
 void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
 		    int (*map_irq)(const struct pci_dev *, u8, u8))
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e90eb22..50e66ab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1120,6 +1120,9 @@ 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 pdev_fixup_irq(struct pci_dev *,
+		    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.7.11.7

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

[David Daney]

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

Use pci_walk_bus() to restrict the fixup irq actions to only the bus
being created.

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.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/pci/host/pci-host-generic.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 265dd25..a0fb241 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -205,6 +205,12 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 	return 0;
 }
 
+static int gen_pci_fixup_irq_cb(struct pci_dev *dev, void *arg)
+{
+	pdev_fixup_irq(dev, pci_common_swizzle, of_irq_parse_and_map_pci);
+	return 0;
+}
+
 static int gen_pci_probe(struct platform_device *pdev)
 {
 	int err;
@@ -262,7 +268,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_walk_bus(bus, gen_pci_fixup_irq_cb, NULL);
 
 	if (!pci_has_flag(PCI_PROBE_ONLY)) {
 		pci_bus_size_bridges(bus);
-- 
1.7.11.7

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

[Will Deacon]

Hi David,

On Sat, Sep 12, 2015 at 12:21:55AM +0100, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Use pci_walk_bus() to restrict the fixup irq actions to only the bus
> being created.
> 
> 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.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/pci/host/pci-host-generic.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 265dd25..a0fb241 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -205,6 +205,12 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  	return 0;
>  }
>  
> +static int gen_pci_fixup_irq_cb(struct pci_dev *dev, void *arg)
> +{
> +	pdev_fixup_irq(dev, pci_common_swizzle, of_irq_parse_and_map_pci);
> +	return 0;
> +}
> +
>  static int gen_pci_probe(struct platform_device *pdev)
>  {
>  	int err;
> @@ -262,7 +268,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_walk_bus(bus, gen_pci_fixup_irq_cb, NULL);

Any chance we could put something in the core PCI code for this? I think
any host controller wanting to work with arm64 is potentially going to
run into the same problem.

Will

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

[David Daney]

On 09/15/2015 10:36 AM, Will Deacon wrote:
> Hi David,
>
> On Sat, Sep 12, 2015 at 12:21:55AM +0100, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> Use pci_walk_bus() to restrict the fixup irq actions to only the bus
>> being created.
>>
>> 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.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/pci/host/pci-host-generic.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> index 265dd25..a0fb241 100644
>> --- a/drivers/pci/host/pci-host-generic.c
>> +++ b/drivers/pci/host/pci-host-generic.c
>> @@ -205,6 +205,12 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>>   	return 0;
>>   }
>>
>> +static int gen_pci_fixup_irq_cb(struct pci_dev *dev, void *arg)
>> +{
>> +	pdev_fixup_irq(dev, pci_common_swizzle, of_irq_parse_and_map_pci);
>> +	return 0;
>> +}
>> +
>>   static int gen_pci_probe(struct platform_device *pdev)
>>   {
>>   	int err;
>> @@ -262,7 +268,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_walk_bus(bus, gen_pci_fixup_irq_cb, NULL);
>
> Any chance we could put something in the core PCI code for this? I think
> any host controller wanting to work with arm64 is potentially going to
> run into the same problem.

Good idea.

I will move the walking code into setup-irq.c (the current home of 
pci_fixup_irqs()), and make it part of patch 1/6.

Thanks,
David Daney


>
> Will
>

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

[Lorenzo Pieralisi]

On Tue, Sep 15, 2015 at 06:49:24PM +0100, David Daney wrote:
> On 09/15/2015 10:36 AM, Will Deacon wrote:
> > Hi David,
> >
> > On Sat, Sep 12, 2015 at 12:21:55AM +0100, David Daney wrote:
> >> From: David Daney <david.daney@cavium.com>
> >>
> >> Use pci_walk_bus() to restrict the fixup irq actions to only the bus
> >> being created.
> >>
> >> 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.
> >>
> >> Signed-off-by: David Daney <david.daney@cavium.com>
> >> ---
> >>   drivers/pci/host/pci-host-generic.c | 8 +++++++-
> >>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> >> index 265dd25..a0fb241 100644
> >> --- a/drivers/pci/host/pci-host-generic.c
> >> +++ b/drivers/pci/host/pci-host-generic.c
> >> @@ -205,6 +205,12 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> >>   	return 0;
> >>   }
> >>
> >> +static int gen_pci_fixup_irq_cb(struct pci_dev *dev, void *arg)
> >> +{
> >> +	pdev_fixup_irq(dev, pci_common_swizzle, of_irq_parse_and_map_pci);
> >> +	return 0;
> >> +}
> >> +
> >>   static int gen_pci_probe(struct platform_device *pdev)
> >>   {
> >>   	int err;
> >> @@ -262,7 +268,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_walk_bus(bus, gen_pci_fixup_irq_cb, NULL);
> >
> > Any chance we could put something in the core PCI code for this? I think
> > any host controller wanting to work with arm64 is potentially going to
> > run into the same problem.
> 
> Good idea.
> 
> I will move the walking code into setup-irq.c (the current home of 
> pci_fixup_irqs()), and make it part of patch 1/6.

For arm64 we could even compile out pci_fixup_irqs() since the IRQ mapping
is done in pcibios_add_device() through DT, I agree on CONFIG_ARM this
needs fixing though, so yes, make it common code and patch all host
controllers that make use of pci_fixup_irqs() since that's a common
issue as far as I can see (ARM pcibios works around it by having a
IRQ mapping function per bus and it provides an API to initialize
multiple host controllers at once and fixing up IRQs before devices
are added, still, for host controllers that do not use that
functionality - eg drivers/pci/host/pcie-designware.c - if there are
multiple host controllers instances we run into the same problem).

Side note: do you have any PCI host controller where mapping IRQs through
of_irq_parse_and_map_pci() does not work ? If yes, why ?

Cheers,
Lorenzo

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

[David Daney]

On 09/16/2015 03:32 AM, Lorenzo Pieralisi wrote:
> On Tue, Sep 15, 2015 at 06:49:24PM +0100, David Daney wrote:
>> On 09/15/2015 10:36 AM, Will Deacon wrote:
>>> Hi David,
>>>
>>> On Sat, Sep 12, 2015 at 12:21:55AM +0100, David Daney wrote:
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> Use pci_walk_bus() to restrict the fixup irq actions to only the bus
>>>> being created.
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>> ---
>>>>    drivers/pci/host/pci-host-generic.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>>>> index 265dd25..a0fb241 100644
>>>> --- a/drivers/pci/host/pci-host-generic.c
>>>> +++ b/drivers/pci/host/pci-host-generic.c
>>>> @@ -205,6 +205,12 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>>>>    	return 0;
>>>>    }
>>>>
>>>> +static int gen_pci_fixup_irq_cb(struct pci_dev *dev, void *arg)
>>>> +{
>>>> +	pdev_fixup_irq(dev, pci_common_swizzle, of_irq_parse_and_map_pci);
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int gen_pci_probe(struct platform_device *pdev)
>>>>    {
>>>>    	int err;
>>>> @@ -262,7 +268,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_walk_bus(bus, gen_pci_fixup_irq_cb, NULL);
>>>
>>> Any chance we could put something in the core PCI code for this? I think
>>> any host controller wanting to work with arm64 is potentially going to
>>> run into the same problem.
>>
>> Good idea.
>>
>> I will move the walking code into setup-irq.c (the current home of
>> pci_fixup_irqs()), and make it part of patch 1/6.
>
> For arm64 we could even compile out pci_fixup_irqs() since the IRQ mapping
> is done in pcibios_add_device() through DT, I agree on CONFIG_ARM this
> needs fixing though, so yes, make it common code and patch all host
> controllers that make use of pci_fixup_irqs() since that's a common
> issue as far as I can see (ARM pcibios works around it by having a
> IRQ mapping function per bus and it provides an API to initialize
> multiple host controllers at once and fixing up IRQs before devices
> are added, still, for host controllers that do not use that
> functionality - eg drivers/pci/host/pcie-designware.c - if there are
> multiple host controllers instances we run into the same problem).

Patching up other PCI bus drivers would be deferred to a different patch 
set, as I have no way to test it.

>
> Side note: do you have any PCI host controller where mapping IRQs through
> of_irq_parse_and_map_pci() does not work ? If yes, why ?
>

The of_irq_parse_and_map_pci() call in arch/arm64/kernel/pci.c 
(pcibios_add_device) seems to work for me.

I did send a patch recently to make it quite emitting erroneous error 
messages for PCI devices that have no irq pin.

David Daney

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

Signed-off-by: David Daney <david.daney@cavium.com>
---
 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 a0fb241..cd6f898 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,
@@ -240,8 +243,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);
@@ -262,7 +264,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.7.11.7

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

[Will Deacon]

On Sat, Sep 12, 2015 at 12:21:56AM +0100, 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,
> to future proof against the addition of bus specific elements to
> struct pci_ops.

This looks ok to me:

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  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 a0fb241..cd6f898 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,
> @@ -240,8 +243,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);
> @@ -262,7 +264,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.7.11.7
> 

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

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);
 
-- 
1.7.11.7

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

[Will Deacon]

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

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

[David Daney]

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

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

[Will Deacon]

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

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

[David Daney]

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
>

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

[Will Deacon]

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

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

[Lorenzo Pieralisi]

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

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

[Will Deacon]

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

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

[David Daney]

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

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

Signed-off-by: David Daney <david.daney@cavium.com>
---
 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 fce5bf7..8219c0b 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -265,7 +265,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.7.11.7

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

[Will Deacon]

On Sat, Sep 12, 2015 at 12:21:58AM +0100, David Daney wrote:
> 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.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  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 fce5bf7..8219c0b 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -265,7 +265,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");

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH 6/6] PCI: generic: Allow bus default MSI controller to be specified.

[David Daney]

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

If the device tree node for the bus has a "msi-parent" property, use
that as the default MSI controller for devices on the bus.  Add device
tree binding documentation describing the new property.

This allows the pci-host-generic driver to be used in systems with
multiple MSI controllers.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 .../devicetree/bindings/pci/host-generic-pci.txt          |  5 +++++
 drivers/pci/host/pci-host-generic.c                       | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
index cf3e205..daa6942 100644
--- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
@@ -29,6 +29,11 @@ Properties of the host controller node:
                    to indicate the range of bus numbers for this controller.
                    If absent, defaults to <0 255> (i.e. all buses).
 
+- msi-parent     : Optional property to indicate the MSI controller associated
+                   with devices on the bus.  If not present, the default MSI
+                   controller is used.  This property is further discussed in
+                   interrupt-controller/msi.txt
+
 - #address-cells : Must be 3.
 
 - #size-cells    : Must be 2.
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 8219c0b..e0248b4 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -225,6 +225,7 @@ static int gen_pci_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
 	struct pci_bus *bus, *child;
+	struct msi_controller *msi;
 
 	if (!pci)
 		return -ENOMEM;
@@ -265,8 +266,18 @@ 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, pci->cfg.bus_range->start,
-				&pci->cfg.ops.ops, pci, &pci->resources);
+	msi = NULL;
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		struct device_node *msi_node;
+
+		msi_node = of_parse_phandle(np, "msi-parent", 0);
+		if (msi_node)
+			msi = of_pci_find_msi_chip_by_node(msi_node);
+	}
+
+	bus = pci_scan_root_bus_msi(dev, pci->cfg.bus_range->start,
+				    &pci->cfg.ops.ops, pci, &pci->resources,
+				    msi);
 	if (!bus) {
 		dev_err(dev, "Scanning rootbus failed");
 		return -ENODEV;
-- 
1.7.11.7

Re: [PATCH 6/6] PCI: generic: Allow bus default MSI controller to be specified.

[Will Deacon]

On Sat, Sep 12, 2015 at 12:21:59AM +0100, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> If the device tree node for the bus has a "msi-parent" property, use
> that as the default MSI controller for devices on the bus.  Add device
> tree binding documentation describing the new property.
> 
> This allows the pci-host-generic driver to be used in systems with
> multiple MSI controllers.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  .../devicetree/bindings/pci/host-generic-pci.txt          |  5 +++++
>  drivers/pci/host/pci-host-generic.c                       | 15 +++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)

I don't think you need this anymore with 4.3-rc1. The core IRQ subsystem
should take care of parsing and configuring the MSI parents.

Will

Re: [PATCH 6/6] PCI: generic: Allow bus default MSI controller to be specified.

[David Daney]

On 09/15/2015 10:53 AM, Will Deacon wrote:
> On Sat, Sep 12, 2015 at 12:21:59AM +0100, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> If the device tree node for the bus has a "msi-parent" property, use
>> that as the default MSI controller for devices on the bus.  Add device
>> tree binding documentation describing the new property.
>>
>> This allows the pci-host-generic driver to be used in systems with
>> multiple MSI controllers.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   .../devicetree/bindings/pci/host-generic-pci.txt          |  5 +++++
>>   drivers/pci/host/pci-host-generic.c                       | 15 +++++++++++++--
>>   2 files changed, 18 insertions(+), 2 deletions(-)
>
> I don't think you need this anymore with 4.3-rc1. The core IRQ subsystem
> should take care of parsing and configuring the MSI parents.

I will re-test and see if I can make it work.

David Daney


>
> Will
>

Re: [PATCH 0/6] PCI: generic: Misc. bug fixes and enhanced support for MSI.

[David Daney]

On 09/11/2015 04:21 PM, David Daney wrote:
> 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.  We also
> need the ability to specify a per-bus MSI controller, so support for
> that was added.
>
> David Daney (6):
>    PCI: Make global and export pdev_fixup_irq().
>    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: Allow bus default MSI controller to be specified.
>

Based on feedback on this first revision of the patches, I plan on 
sending an updated patch set in a few days.  In the mean time, keep 
sending feedback...

David Daney



>   .../devicetree/bindings/pci/host-generic-pci.txt   |  5 ++
>   drivers/pci/host/pci-host-generic.c                | 55 +++++++++++++++-------
>   drivers/pci/setup-irq.c                            |  7 +--
>   include/linux/pci.h                                |  3 ++
>   4 files changed, 50 insertions(+), 20 deletions(-)
>