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

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

[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 v3:

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

  - Add some Acked-by:

  - Add to explanation of "reg" property in host-generic-pci.txt.

  - Add error message if "reg" property is too big.

  - Use pointer to ops rather than make a copy.

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 (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   |  6 +++-
 drivers/pci/host/pci-host-generic.c                | 42 +++++++++++++---------
 drivers/pci/setup-irq.c                            | 30 ++++++++++++++++
 include/linux/pci.h                                |  4 +++
 4 files changed, 65 insertions(+), 17 deletions(-)

-- 
1.9.1

[PATCH v4 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>
---
No change from v2.

 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

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

[Bjorn Helgaas]

Hi David,

On Fri, Oct 02, 2015 at 11:43:59AM -0700, David Daney wrote:
> 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.
> 
>  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);

I don't like the existing pci_fixup_irqs(), so by transitivity, I
don't like pci_bus_fixup_irqs() either.  The problem is that in both
cases this is a one-time pass over the tree, so we don't handle
hot-added devices correctly.

I think we need to get rid of pci_fixup_irqs() and somehow integrate
it into the pci_device_add() path, where it would be done once for
every device we enumerate.  If we did that, I don't think you would
need to add pci_bus_fixup_irqs(), would you?

Bjorn

> +
> +}
> +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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[David Daney]

On 10/07/2015 12:44 PM, Bjorn Helgaas wrote:
> Hi David,
>
> On Fri, Oct 02, 2015 at 11:43:59AM -0700, David Daney wrote:
>> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>
>> 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-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>> ---
>> No change from v2.
>>
>>   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);
>
> I don't like the existing pci_fixup_irqs(), so by transitivity, I
> don't like pci_bus_fixup_irqs() either.

We are in agreement with respect to this point.


>  The problem is that in both
> cases this is a one-time pass over the tree, so we don't handle
> hot-added devices correctly.
>
> I think we need to get rid of pci_fixup_irqs() and somehow integrate
> it into the pci_device_add() path, where it would be done once for
> every device we enumerate.

I also agree with this point.

>  If we did that, I don't think you would
> need to add pci_bus_fixup_irqs(), would you?

Nope.

However, such a change is essentially untestable by me.  So, I didn't 
attempt it.   pci_fixup_irqs() is used by alpha, arm, m68k, mips, sh, 
sparc, tile, unicore32 and other things as well.  If the core 
pci_device_add() code were to suddenly start doing the fixup, there 
would be the potential to break all these things I cannot test.

The new pci_bus_fixup_irqs() is really an optimization so that if we 
have multiple buses created by pci-host-generic.c, that we only iterate 
over each device once.  I believe that pci-host-generic.c would still 
operate without these patches 1/5 and 2/5, and could test that if you 
are OK with the remaining three patches.  Or we could merge all 5 and 
live a while longer with the ugliness that is already there.

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 v4 1/5] PCI: Add pci_bus_fixup_irqs().

[Bjorn Helgaas]

[+cc Matthew]

On Wed, Oct 07, 2015 at 01:08:40PM -0700, David Daney wrote:
> On 10/07/2015 12:44 PM, Bjorn Helgaas wrote:
> >Hi David,
> >
> >On Fri, Oct 02, 2015 at 11:43:59AM -0700, David Daney wrote:
> >>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.
> >>
> >>  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);
> >
> >I don't like the existing pci_fixup_irqs(), so by transitivity, I
> >don't like pci_bus_fixup_irqs() either.
> 
> We are in agreement with respect to this point.
> 
> > The problem is that in both
> >cases this is a one-time pass over the tree, so we don't handle
> >hot-added devices correctly.
> >
> >I think we need to get rid of pci_fixup_irqs() and somehow integrate
> >it into the pci_device_add() path, where it would be done once for
> >every device we enumerate.
> 
> I also agree with this point.
> 
> > If we did that, I don't think you would
> >need to add pci_bus_fixup_irqs(), would you?
> 
> Nope.
> 
> However, such a change is essentially untestable by me.  So, I
> didn't attempt it.   pci_fixup_irqs() is used by alpha, arm, m68k,
> mips, sh, sparc, tile, unicore32 and other things as well.  If the
> core pci_device_add() code were to suddenly start doing the fixup,
> there would be the potential to break all these things I cannot
> test.

Yep, that's certainly a risk.  I can't test all those arches either,
but I think it's a risk worth taking because the end result is more
maintainable.

Matthew Minter did some really nice work on this last year, but it got
stalled somehow.  I wonder if we can resurrect it?  It seems like it
was pretty close to being ready.  Here's a pointer to the last posting
I saw:

http://lkml.kernel.org/r/1412222866-21068-1-git-send-email-matt@masarand.com

Bjorn

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

[Matthew Minter]

On Wednesday 07 October 2015 18:08:47 Bjorn Helgaas wrote:
> [+cc Matthew]
> 
> On Wed, Oct 07, 2015 at 01:08:40PM -0700, David Daney wrote:
> > On 10/07/2015 12:44 PM, Bjorn Helgaas wrote:
> > >Hi David,
> > >
> > >On Fri, Oct 02, 2015 at 11:43:59AM -0700, David Daney wrote:
> > >>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.
> > >>
> > >>  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);
> > >
> > >I don't like the existing pci_fixup_irqs(), so by transitivity, I
> > >don't like pci_bus_fixup_irqs() either.
> > 
> > We are in agreement with respect to this point.
> > 
> > > The problem is that in both
> > >
> > >cases this is a one-time pass over the tree, so we don't handle
> > >hot-added devices correctly.
> > >
> > >I think we need to get rid of pci_fixup_irqs() and somehow integrate
> > >it into the pci_device_add() path, where it would be done once for
> > >every device we enumerate.
> > 
> > I also agree with this point.
> > 
> > > If we did that, I don't think you would
> > >
> > >need to add pci_bus_fixup_irqs(), would you?
> > 
> > Nope.
> > 
> > However, such a change is essentially untestable by me.  So, I
> > didn't attempt it.   pci_fixup_irqs() is used by alpha, arm, m68k,
> > mips, sh, sparc, tile, unicore32 and other things as well.  If the
> > core pci_device_add() code were to suddenly start doing the fixup,
> > there would be the potential to break all these things I cannot
> > test.
> 
> Yep, that's certainly a risk.  I can't test all those arches either,
> but I think it's a risk worth taking because the end result is more
> maintainable.
> 
> Matthew Minter did some really nice work on this last year, but it got
> stalled somehow.  I wonder if we can resurrect it?  It seems like it
> was pretty close to being ready.  Here's a pointer to the last posting
> I saw:
> 
> http://lkml.kernel.org/r/1412222866-21068-1-git-send-email-matt@masarand.com
> 
> Bjorn

Thanks for adding me into the loop,

Yes, I had been working on this last year, and got a patchset that was tested 
on arm, x86 (and amd64), and slightly tested on powerpc. However I was not 
able to test other architectures as they were not available in the software 
lab I work in but should in theory work on all arches the kernel runs on.

I can say that that patchset is being used by several projects out of tree 
currently but unfortunately due to a shift in priorities in the lab I was 
working for I lost access to the resources to develop and test the patch 
easily.

I have done some additional work personally on it but so far have not got 
anything that I am happy to submit for inclusion in tree. (due to a number of 
issues in structure and  a complication relating to weak functions where 
multiple variations of the same arch exist.

You can see in thread that is linked above 
(http://lkml.kernel.org/r/1412222866-21068-1-git-send-email-matt@masarand.com)
some feedback on the issues that need to be solved.

I also expect that the patchset needs to be pulled forward to a newer kernel 
as I have been working in a frozen tree without rebasing to reduce test 
complexity.

I would be happy to put some time this weekend if possible into reviewing the 
state of this and seeing if I can at least put together a version running on a 
recent kernel. I can also go over the issues again which were proving 
problematic and see if any of them are easy to fix.

However I can only work on this in my own time for now and on my own boxes 
(which are all x86 and amd64) so the amount I can do will be limited. However 
any assistance in fixing the issues would be appreciated, I will try and throw 
up a git repo somewhere this weekend with the latest patches if anyone is able 
to take a look.

In the mean time, the biggest issues with the current iteration and the full 
thread are here:

http://comments.gmane.org/gmane.linux.kernel.pci/35756

I will get a repo somewhere online for browsing soon but cannot quite yet as I 
need to clean up the repo a little first.

Kind regards to all,
Matthew Minter

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

[Lorenzo Pieralisi]

Hi Matthew,

On Thu, Oct 08, 2015 at 03:07:34AM +0100, Matthew Minter wrote:
> On Wednesday 07 October 2015 18:08:47 Bjorn Helgaas wrote:

[...]

> Yes, I had been working on this last year, and got a patchset that was tested 
> on arm, x86 (and amd64), and slightly tested on powerpc. However I was not 
> able to test other architectures as they were not available in the software 
> lab I work in but should in theory work on all arches the kernel runs on.
> 
> I can say that that patchset is being used by several projects out of tree 
> currently but unfortunately due to a shift in priorities in the lab I was 
> working for I lost access to the resources to develop and test the patch 
> easily.
> 
> I have done some additional work personally on it but so far have not got 
> anything that I am happy to submit for inclusion in tree. (due to a number of 
> issues in structure and  a complication relating to weak functions where 
> multiple variations of the same arch exist.
> 
> You can see in thread that is linked above 
> (http://lkml.kernel.org/r/1412222866-21068-1-git-send-email-matt@masarand.com)
> some feedback on the issues that need to be solved.
> 
> I also expect that the patchset needs to be pulled forward to a newer kernel 
> as I have been working in a frozen tree without rebasing to reduce test 
> complexity.
> 
> I would be happy to put some time this weekend if possible into reviewing the 
> state of this and seeing if I can at least put together a version running on a 
> recent kernel. I can also go over the issues again which were proving 
> problematic and see if any of them are easy to fix.
> 
> However I can only work on this in my own time for now and on my own boxes 
> (which are all x86 and amd64) so the amount I can do will be limited. 

I was not aware of your patchset but I am happy to see that code since it
is definitely the right approach and I am willing to help you test it on
arm with the HW I have (probably it will also help remove some code
from arm64 too), let me know when you have a branch ready to test and
if you need any help in rebasing the set.

Thanks,
Lorenzo

[PATCH v4 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().

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
No change from 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 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 v4 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 a pointer to it when the bus device is probed.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
Change from v3: Use pointer to ops rather than make a copy.

 drivers/pci/host/pci-host-generic.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 9e9f1c3..216ded5 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[] = {
@@ -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

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

[Bjorn Helgaas]

[+cc Arnd]

Arnd, you had comments on the previous version.  What do you think of
this one?

On Fri, Oct 02, 2015 at 11:44:01AM -0700, David Daney wrote:
> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> 
> 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 a pointer to it when the bus device is probed.
> 
> Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
> Change from v3: Use pointer to ops rather than make a copy.
> 
>  drivers/pci/host/pci-host-generic.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 9e9f1c3..216ded5 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[] = {
> @@ -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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
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 v4 3/5] PCI: generic: Quit clobbering our pci_ops.

[Arnd Bergmann]

On Thursday 08 October 2015 10:02:16 Bjorn Helgaas wrote:
> [+cc Arnd]
> 
> Arnd, you had comments on the previous version.  What do you think of
> this one?

Looks good to me

> On Fri, Oct 02, 2015 at 11:44:01AM -0700, 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 a pointer to it when the bus device is probed.
> > 
> > Acked-by: Will Deacon <will.deacon@arm.com>
> > Signed-off-by: David Daney <david.daney@cavium.com>
> 

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

[PATCH v4 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 V3: Add to explanation of "reg" property in
host-generic-pci.txt.  Add error message if "reg" property is too big.

 Documentation/devicetree/bindings/pci/host-generic-pci.txt |  6 +++++-
 drivers/pci/host/pci-host-generic.c                        | 12 +++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
index cf3e205..42303bb 100644
--- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
@@ -34,7 +34,11 @@ Properties of the host controller node:
 - #size-cells    : Must be 2.
 
 - reg            : The Configuration Space base address and size, as accessed
-                   from the parent bus.
+                   from the parent bus.  The base address corresponds to
+                   bus zero, even though the "bus-range" property may specify
+                   a different starting bus number.  The driver must only map
+                   or access the portion of the Configuration Space that
+                   corresponds to the "bus-range"
 
 
 Properties of the /chosen node:
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 216ded5..5cce837 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -164,7 +164,7 @@ out_release_res:
 static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 {
 	int err;
-	u8 bus_max;
+	int bus_max;
 	resource_size_t busn;
 	struct resource *bus_range;
 	struct device *dev = pci->host.dev.parent;
@@ -177,8 +177,14 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 	}
 
 	/* Limit the bus-range to fit within reg */
-	bus_max = pci->cfg.bus_range->start +
-		  (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
+	bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
+	if (bus_max > 255) {
+		dev_err(dev,
+			"\"reg\" size corresponds to bus %d, truncating to 255\n",
+			bus_max);
+		bus_max = 255;
+	}
+
 	pci->cfg.bus_range->end = min_t(resource_size_t,
 					pci->cfg.bus_range->end, bus_max);
 
-- 
1.9.1

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

[Bjorn Helgaas]

[+cc Arnd]

And this one?

On Fri, Oct 02, 2015 at 11:44:02AM -0700, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> There are two problems with the bus_max calculation:
> 
> 1) The u8 data type can overflow for large config space windows.
> 
> 2) The calculation is incorrect for a bus range that doesn't start at
>    zero.
> 
> Since the configuration space is relative to bus zero, make bus_max
> just be the size of the config window scaled by bus_shift.  Then clamp
> it to a maximum of 255, per PCI.  Use a data type of int to avoid
> overflow problems.
> 
> Update host-generic-pci.txt to clarify the semantics of the "reg"
> property with respect to non-zero starting bus numbers.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
> Change from V3: Add to explanation of "reg" property in
> host-generic-pci.txt.  Add error message if "reg" property is too big.
> 
>  Documentation/devicetree/bindings/pci/host-generic-pci.txt |  6 +++++-
>  drivers/pci/host/pci-host-generic.c                        | 12 +++++++++---
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> index cf3e205..42303bb 100644
> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> @@ -34,7 +34,11 @@ Properties of the host controller node:
>  - #size-cells    : Must be 2.
>  
>  - reg            : The Configuration Space base address and size, as accessed
> -                   from the parent bus.
> +                   from the parent bus.  The base address corresponds to
> +                   bus zero, even though the "bus-range" property may specify
> +                   a different starting bus number.  The driver must only map
> +                   or access the portion of the Configuration Space that
> +                   corresponds to the "bus-range"
>  
>  
>  Properties of the /chosen node:
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 216ded5..5cce837 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -164,7 +164,7 @@ out_release_res:
>  static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  {
>  	int err;
> -	u8 bus_max;
> +	int bus_max;
>  	resource_size_t busn;
>  	struct resource *bus_range;
>  	struct device *dev = pci->host.dev.parent;
> @@ -177,8 +177,14 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  	}
>  
>  	/* Limit the bus-range to fit within reg */
> -	bus_max = pci->cfg.bus_range->start +
> -		  (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
> +	bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
> +	if (bus_max > 255) {
> +		dev_err(dev,
> +			"\"reg\" size corresponds to bus %d, truncating to 255\n",
> +			bus_max);
> +		bus_max = 255;
> +	}
> +
>  	pci->cfg.bus_range->end = min_t(resource_size_t,
>  					pci->cfg.bus_range->end, bus_max);
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Arnd Bergmann]

On Thursday 08 October 2015 10:02:43 Bjorn Helgaas wrote:
> [+cc Arnd]
> 
> And this one?

Not so good.

> > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> > @@ -34,7 +34,11 @@ Properties of the host controller node:
> >  - #size-cells    : Must be 2.
> >  
> >  - reg            : The Configuration Space base address and size, as accessed
> > -                   from the parent bus.
> > +                   from the parent bus.  The base address corresponds to
> > +                   bus zero, even though the "bus-range" property may specify
> > +                   a different starting bus number.  The driver must only map
> > +                   or access the portion of the Configuration Space that
> > +                   corresponds to the "bus-range"

I thought we had reached an agreement that it is a bad idea to have a 'reg'
property that lists registers belonging to a different device.

	Arnd

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

[Arnd Bergmann]

On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote:
> > > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> > > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> > > @@ -34,7 +34,11 @@ Properties of the host controller node:
> > >  - #size-cells    : Must be 2.
> > >  
> > >  - reg            : The Configuration Space base address and size, as accessed
> > > -                   from the parent bus.
> > > +                   from the parent bus.  The base address corresponds to
> > > +                   bus zero, even though the "bus-range" property may specify
> > > +                   a different starting bus number.  The driver must only map
> > > +                   or access the portion of the Configuration Space that
> > > +                   corresponds to the "bus-range"
> 
> I thought we had reached an agreement that it is a bad idea to have a 'reg'
> property that lists registers belonging to a different device.
> 
> 

To further clarify: the argument was to make it mirror what ACPI does for
PCI. However, this is unlike what all other devices do with DT, where you
have non-overlapping register ranges (start, length) for each device.
ACPI as far as I understand it does not give a range for a PCIe host, but
instead provides a way to get the start address of the ECAM register area
for the domain that the host is part of, and that needs to be the same
address for each host in the domain.

	Arnd

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

[David Daney]

On 10/08/2015 08:18 AM, Arnd Bergmann wrote:
> On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote:
>>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> @@ -34,7 +34,11 @@ Properties of the host controller node:
>>>>   - #size-cells    : Must be 2.
>>>>
>>>>   - reg            : The Configuration Space base address and size, as accessed
>>>> -                   from the parent bus.
>>>> +                   from the parent bus.  The base address corresponds to
>>>> +                   bus zero, even though the "bus-range" property may specify
>>>> +                   a different starting bus number.  The driver must only map
>>>> +                   or access the portion of the Configuration Space that
>>>> +                   corresponds to the "bus-range"
>>
>> I thought we had reached an agreement that it is a bad idea to have a 'reg'
>> property that lists registers belonging to a different device.
>>
>>
>
> To further clarify: the argument was to make it mirror what ACPI does for
> PCI. However, this is unlike what all other devices do with DT, where you
> have non-overlapping register ranges (start, length) for each device.
> ACPI as far as I understand it does not give a range for a PCIe host, but
> instead provides a way to get the start address of the ECAM register area
> for the domain that the host is part of, and that needs to be the same
> address for each host in the domain.
>

We are agreed that it is a bad thing to do.  There is disagreement about 
if we should do it.

I think there are two schools of thought (I will attribute them to their 
proponents and my apologies if I misrepresent someone's stance):

1) (Arnd) Don't make the the "reg" ranges overlap because it is ugly, 
dangerous and arguably incorrect in general.

2) (Me, Will Deacon, Lorenzo Pieralisi) Overlapping "reg" properties 
should be maintained, as that is the current behavior and seems to agree 
with legacy OF device-tree specifications (although there is some debate 
about this).

Because the driver is broken in this area (thus the patch), it indicates 
that there are probably no users with non-zero starting bus numbers. 
So, we may have some latitude to change it.

I will generate another patch that does it Arnd's way, and if Will is OK 
with it, we might be able to do that instead.  One thing is certain: 
The driver is currently broken for non-zero starting bus numbers.

David Daney

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

[Lorenzo Pieralisi]

On Thu, Oct 08, 2015 at 08:39:58AM -0700, David Daney wrote:
> On 10/08/2015 08:18 AM, Arnd Bergmann wrote:
> >On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote:
> >>>>--- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> >>>>+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> >>>>@@ -34,7 +34,11 @@ Properties of the host controller node:
> >>>>  - #size-cells    : Must be 2.
> >>>>
> >>>>  - reg            : The Configuration Space base address and size, as accessed
> >>>>-                   from the parent bus.
> >>>>+                   from the parent bus.  The base address corresponds to
> >>>>+                   bus zero, even though the "bus-range" property may specify
> >>>>+                   a different starting bus number.  The driver must only map
> >>>>+                   or access the portion of the Configuration Space that
> >>>>+                   corresponds to the "bus-range"
> >>
> >>I thought we had reached an agreement that it is a bad idea to have a 'reg'
> >>property that lists registers belonging to a different device.
> >>
> >>
> >
> >To further clarify: the argument was to make it mirror what ACPI does for
> >PCI. However, this is unlike what all other devices do with DT, where you
> >have non-overlapping register ranges (start, length) for each device.
> >ACPI as far as I understand it does not give a range for a PCIe host, but
> >instead provides a way to get the start address of the ECAM register area
> >for the domain that the host is part of, and that needs to be the same
> >address for each host in the domain.
> >
> 
> We are agreed that it is a bad thing to do.  There is disagreement
> about if we should do it.
> 
> I think there are two schools of thought (I will attribute them to
> their proponents and my apologies if I misrepresent someone's
> stance):
> 
> 1) (Arnd) Don't make the the "reg" ranges overlap because it is
> ugly, dangerous and arguably incorrect in general.
> 
> 2) (Me, Will Deacon, Lorenzo Pieralisi) Overlapping "reg" properties
> should be maintained, as that is the current behavior and seems to
> agree with legacy OF device-tree specifications (although there is
> some debate about this).

I have just quoted the PCI FW specification, 4.1.4 section:

"System Software Implication of MCFG and _CBA"

which defines the ACPI behaviour of MCFG and _CBA methods for
hotplug bridges.

I understand Arnd's concerns and I do not think the DT bindings are well
defined in this respect, it is ok for me to define the DT bindings
using the "usual" reg property bindings representation, as long as we
document it and we all agree we are deviating from the PCI FW specs
(that just cover ACPI, BTW).

I think it is worth investigating why the current PCI FW specs were
defined that way for ECAM config space before proceeding any further,
to avoid pitfalls we might be missing.

Thanks,
Lorenzo

> Because the driver is broken in this area (thus the patch), it
> indicates that there are probably no users with non-zero starting
> bus numbers. So, we may have some latitude to change it.
> 
> I will generate another patch that does it Arnd's way, and if Will
> is OK with it, we might be able to do that instead.  One thing is
> certain: The driver is currently broken for non-zero starting bus
> numbers.
> 
> David Daney
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
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 v4 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 v3.

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

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 5cce837..eb4b7c7 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -264,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,
+
+	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

Re: [PATCH v4 0/5] PCI: generic: Misc. bug fixes/enhancements

[Bjorn Helgaas]

On Fri, Oct 02, 2015 at 11:43:58AM -0700, 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.  This patch
> set fixes the bugs, a follow-on set will add the ThunderX support.
> 
> Changes from v3:
> 
>   - Drop "PCI: generic: Claim device resources if PCI_PROBE_ONLY"
> 
>   - Add some Acked-by:
> 
>   - Add to explanation of "reg" property in host-generic-pci.txt.
> 
>   - Add error message if "reg" property is too big.
> 
>   - Use pointer to ops rather than make a copy.
> 
> 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 (5):
>   PCI: Add pci_bus_fixup_irqs().
>   PCI: generic: Only fixup irqs for bus we are creating.

I'm hoping we won't need the above if we can resurrect Matthew's
patches.

>   PCI: generic: Quit clobbering our pci_ops.

Applied to pci/host-generic for v4.4 with Arnd's Reviewed-by and minor
changelog tweaks (as below).

>   PCI: generic: Correct, and avoid overflow, in bus_max calculation.

Seems not settled yet.

>   PCI: generic: Pass proper starting bus number to pci_scan_root_bus().

Also applied to pci/host-generic, thanks!

commit 9a28033753ca
Author: David Daney <david.daney@cavium.com>
Date:   Thu Oct 8 10:15:10 2015 -0500

    PCI: generic: Allow multiple hosts with different map_bus() methods
    
    The generic driver kept a global struct pci_ops ("gen_pci_ops") which it
    patched with the .map_bus() method appropriate for the bus device.  This is
    a problem when we have two different types of bus devices: the .map_bus()
    method for the last device probed clobbers the method for previous devices.
    The result is that only the last bus device probed has the correct
    .map_bus(), and the others fail.
    
    Move the struct pci_ops into the bus-specific structure and initialize a
    pointer to it when the bus device is probed.
    
    Signed-off-by: David Daney <david.daney@cavium.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Arnd Bergmann <arnd@arndb.de>
    Acked-by: Will Deacon <will.deacon@arm.com>

commit 47ddb949029f
Author: David Daney <david.daney@cavium.com>
Date:   Thu Oct 8 10:24:41 2015 -0500

    PCI: generic: Pass starting bus number to pci_scan_root_bus()
    
    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>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Will Deacon <will.deacon@arm.com>

Re: [PATCH v4 0/5] PCI: generic: Misc. bug fixes/enhancements

[David Daney]

On 10/08/2015 08:28 AM, Bjorn Helgaas wrote:
> On Fri, Oct 02, 2015 at 11:43:58AM -0700, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
[...]
>>
>> David Daney (5):
>>    PCI: Add pci_bus_fixup_irqs().
>>    PCI: generic: Only fixup irqs for bus we are creating.
>
> I'm hoping we won't need the above if we can resurrect Matthew's
> patches.

I will test some things with this today.


>
>>    PCI: generic: Quit clobbering our pci_ops.
>
> Applied to pci/host-generic for v4.4 with Arnd's Reviewed-by and minor
> changelog tweaks (as below).
>
>>    PCI: generic: Correct, and avoid overflow, in bus_max calculation.
>
> Seems not settled yet.
>
>>    PCI: generic: Pass proper starting bus number to pci_scan_root_bus().
>
> Also applied to pci/host-generic, thanks!
>

Many thanks for getting those two in,


David Daney