* [RFC] pci: Provide a domain limited version of pdev_fixup_irq
@ 2016-07-08 11:49 Phil Edworthy
2016-07-11 8:39 ` Phil Edworthy
2016-07-21 9:35 ` Lorenzo Pieralisi
0 siblings, 2 replies; 7+ messages in thread
From: Phil Edworthy @ 2016-07-08 11:49 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-renesas-soc, Phil Edworthy
Hi Bjorn,
I've marked this as RFC as I guess there might be a better way to do
this, but I'm not sure how. I would appreciate your thoughts on this.
Thanks
Phil
---
pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
no matter that the device may be on a completely unrelated PCI
Host controller.
When you have multiple PCI Host controllers, pdev_fixup_irq() can
clobber the dev->irq of devices it should not be changing.
This has been seen when performing suspend/resume on boards with
two R-Car PCIe Host controllers, resulting in a NULL ptr access
in __pci_restore_msi_state. This happens because dev->irq has been
overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
This patch introduces a new function, pci_fixup_irqs_local(), that
performs the same operation as pdev_fixup_irq(), but only changes
the dev->irq of device on the same domain.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
drivers/pci/setup-irq.c | 25 ++++++++++++++++++++++---
include/linux/pci.h | 3 +++
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index 95c225b..90ea8fa 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -22,7 +22,8 @@ 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,
+static void pdev_fixup_irq(int domain_nr,
+ struct pci_dev *dev,
u8 (*swizzle)(struct pci_dev *, u8 *),
int (*map_irq)(const struct pci_dev *, u8, u8))
{
@@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
if (irq == -1)
irq = 0;
}
- dev->irq = irq;
+ /* Since pci_fixup_irqs() can be called more than once due to multiple
+ * host controllers, and we scan all PCI devices, not just those
+ * attached to this controller, make sure we don't clobber dev->irq
+ * that has nothing to do with this domain.
+ */
+ if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
+ return;
+ dev->irq = irq;
dev_dbg(&dev->dev, "fixup irq: got %d\n", dev->irq);
/* Always tell the device, so the driver knows what is
@@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
struct pci_dev *dev = NULL;
for_each_pci_dev(dev)
- pdev_fixup_irq(dev, swizzle, map_irq);
+ pdev_fixup_irq(-1, dev, swizzle, map_irq);
}
EXPORT_SYMBOL_GPL(pci_fixup_irqs);
+
+void pci_fixup_irqs_local(struct pci_bus *bus,
+ u8 (*swizzle)(struct pci_dev *, u8 *),
+ int (*map_irq)(const struct pci_dev *, u8, u8))
+{
+ struct pci_dev *dev = NULL;
+
+ for_each_pci_dev(dev)
+ pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
+}
+EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8badb66..37a97df 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1134,6 +1134,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 pci_fixup_irqs_local(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 *);
--
2.5.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
2016-07-08 11:49 [RFC] pci: Provide a domain limited version of pdev_fixup_irq Phil Edworthy
@ 2016-07-11 8:39 ` Phil Edworthy
2016-07-21 9:35 ` Lorenzo Pieralisi
1 sibling, 0 replies; 7+ messages in thread
From: Phil Edworthy @ 2016-07-11 8:39 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-renesas-soc
Note that kbuild test failed because domain_nr is only available when we
have CONFIG_PCI_DOMAINS_GENERIC defined, so the patch will definitely
have to change.
Phil
On 08 July 2016 12:50, Phil Edworthy wrote:
> Hi Bjorn,
>
> I've marked this as RFC as I guess there might be a better way to do
> this, but I'm not sure how. I would appreciate your thoughts on this.
>
> Thanks
> Phil
>
> ---
> pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> no matter that the device may be on a completely unrelated PCI
> Host controller.
>
> When you have multiple PCI Host controllers, pdev_fixup_irq() can
> clobber the dev->irq of devices it should not be changing.
> This has been seen when performing suspend/resume on boards with
> two R-Car PCIe Host controllers, resulting in a NULL ptr access
> in __pci_restore_msi_state. This happens because dev->irq has been
> overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
>
> This patch introduces a new function, pci_fixup_irqs_local(), that
> performs the same operation as pdev_fixup_irq(), but only changes
> the dev->irq of device on the same domain.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> drivers/pci/setup-irq.c | 25 ++++++++++++++++++++++---
> include/linux/pci.h | 3 +++
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index 95c225b..90ea8fa 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -22,7 +22,8 @@ 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,
> +static void pdev_fixup_irq(int domain_nr,
> + struct pci_dev *dev,
> u8 (*swizzle)(struct pci_dev *, u8 *),
> int (*map_irq)(const struct pci_dev *, u8, u8))
> {
> @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
> if (irq == -1)
> irq = 0;
> }
> - dev->irq = irq;
> + /* Since pci_fixup_irqs() can be called more than once due to multiple
> + * host controllers, and we scan all PCI devices, not just those
> + * attached to this controller, make sure we don't clobber dev->irq
> + * that has nothing to do with this domain.
> + */
> + if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> + return;
>
> + dev->irq = irq;
> dev_dbg(&dev->dev, "fixup irq: got %d\n", dev->irq);
>
> /* Always tell the device, so the driver knows what is
> @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
> struct pci_dev *dev = NULL;
>
> for_each_pci_dev(dev)
> - pdev_fixup_irq(dev, swizzle, map_irq);
> + pdev_fixup_irq(-1, dev, swizzle, map_irq);
> }
> EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> +
> +void pci_fixup_irqs_local(struct pci_bus *bus,
> + u8 (*swizzle)(struct pci_dev *, u8 *),
> + int (*map_irq)(const struct pci_dev *, u8, u8))
> +{
> + struct pci_dev *dev = NULL;
> +
> + for_each_pci_dev(dev)
> + pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> +}
> +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8badb66..37a97df 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1134,6 +1134,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 pci_fixup_irqs_local(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 *);
> --
> 2.5.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
2016-07-08 11:49 [RFC] pci: Provide a domain limited version of pdev_fixup_irq Phil Edworthy
2016-07-11 8:39 ` Phil Edworthy
@ 2016-07-21 9:35 ` Lorenzo Pieralisi
2016-08-08 9:31 ` Phil Edworthy
1 sibling, 1 reply; 7+ messages in thread
From: Lorenzo Pieralisi @ 2016-07-21 9:35 UTC (permalink / raw)
To: Phil Edworthy; +Cc: Bjorn Helgaas, linux-pci, linux-renesas-soc
Hi Phil,
On Fri, Jul 08, 2016 at 12:49:55PM +0100, Phil Edworthy wrote:
> Hi Bjorn,
>
> I've marked this as RFC as I guess there might be a better way to do
> this, but I'm not sure how. I would appreciate your thoughts on this.
There is a more generic solution to this issue, that has been
hanging in the balance for a while:
http://marc.info/?l=linux-pci&m=144557674601373&w=2
We should bring it to completion since pci_fixup_irqs() should
really be replaced, this is just another example of the bugs
it can trigger.
Thanks,
Lorenzo
> Thanks
> Phil
>
> ---
> pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> no matter that the device may be on a completely unrelated PCI
> Host controller.
>
> When you have multiple PCI Host controllers, pdev_fixup_irq() can
> clobber the dev->irq of devices it should not be changing.
> This has been seen when performing suspend/resume on boards with
> two R-Car PCIe Host controllers, resulting in a NULL ptr access
> in __pci_restore_msi_state. This happens because dev->irq has been
> overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
>
> This patch introduces a new function, pci_fixup_irqs_local(), that
> performs the same operation as pdev_fixup_irq(), but only changes
> the dev->irq of device on the same domain.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> drivers/pci/setup-irq.c | 25 ++++++++++++++++++++++---
> include/linux/pci.h | 3 +++
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index 95c225b..90ea8fa 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -22,7 +22,8 @@ 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,
> +static void pdev_fixup_irq(int domain_nr,
> + struct pci_dev *dev,
> u8 (*swizzle)(struct pci_dev *, u8 *),
> int (*map_irq)(const struct pci_dev *, u8, u8))
> {
> @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
> if (irq == -1)
> irq = 0;
> }
> - dev->irq = irq;
> + /* Since pci_fixup_irqs() can be called more than once due to multiple
> + * host controllers, and we scan all PCI devices, not just those
> + * attached to this controller, make sure we don't clobber dev->irq
> + * that has nothing to do with this domain.
> + */
> + if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> + return;
>
> + dev->irq = irq;
> dev_dbg(&dev->dev, "fixup irq: got %d\n", dev->irq);
>
> /* Always tell the device, so the driver knows what is
> @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
> struct pci_dev *dev = NULL;
>
> for_each_pci_dev(dev)
> - pdev_fixup_irq(dev, swizzle, map_irq);
> + pdev_fixup_irq(-1, dev, swizzle, map_irq);
> }
> EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> +
> +void pci_fixup_irqs_local(struct pci_bus *bus,
> + u8 (*swizzle)(struct pci_dev *, u8 *),
> + int (*map_irq)(const struct pci_dev *, u8, u8))
> +{
> + struct pci_dev *dev = NULL;
> +
> + for_each_pci_dev(dev)
> + pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> +}
> +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8badb66..37a97df 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1134,6 +1134,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 pci_fixup_irqs_local(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 *);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
2016-07-21 9:35 ` Lorenzo Pieralisi
@ 2016-08-08 9:31 ` Phil Edworthy
2016-08-08 23:19 ` Bjorn Helgaas
0 siblings, 1 reply; 7+ messages in thread
From: Phil Edworthy @ 2016-08-08 9:31 UTC (permalink / raw)
To: Lorenzo Pieralisi; +Cc: Bjorn Helgaas, linux-pci, linux-renesas-soc
Hi Lorenzo,
Thanks for the link, I'll wait to see how this pans out.
Thanks
Phil
> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Lorenzo Pieralisi
> Sent: 21 July 2016 10:35
> To: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> renesas-soc@vger.kernel.org
> Subject: Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
>
> Hi Phil,
>
> On Fri, Jul 08, 2016 at 12:49:55PM +0100, Phil Edworthy wrote:
> > Hi Bjorn,
> >
> > I've marked this as RFC as I guess there might be a better way to do
> > this, but I'm not sure how. I would appreciate your thoughts on this.
>
> There is a more generic solution to this issue, that has been
> hanging in the balance for a while:
>
> http://marc.info/?l=linux-pci&m=144557674601373&w=2
>
> We should bring it to completion since pci_fixup_irqs() should
> really be replaced, this is just another example of the bugs
> it can trigger.
>
> Thanks,
> Lorenzo
>
> > Thanks
> > Phil
> >
> > ---
> > pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> > no matter that the device may be on a completely unrelated PCI
> > Host controller.
> >
> > When you have multiple PCI Host controllers, pdev_fixup_irq() can
> > clobber the dev->irq of devices it should not be changing.
> > This has been seen when performing suspend/resume on boards with
> > two R-Car PCIe Host controllers, resulting in a NULL ptr access
> > in __pci_restore_msi_state. This happens because dev->irq has been
> > overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
> >
> > This patch introduces a new function, pci_fixup_irqs_local(), that
> > performs the same operation as pdev_fixup_irq(), but only changes
> > the dev->irq of device on the same domain.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > drivers/pci/setup-irq.c | 25 ++++++++++++++++++++++---
> > include/linux/pci.h | 3 +++
> > 2 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> > index 95c225b..90ea8fa 100644
> > --- a/drivers/pci/setup-irq.c
> > +++ b/drivers/pci/setup-irq.c
> > @@ -22,7 +22,8 @@ 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,
> > +static void pdev_fixup_irq(int domain_nr,
> > + struct pci_dev *dev,
> > u8 (*swizzle)(struct pci_dev *, u8 *),
> > int (*map_irq)(const struct pci_dev *, u8, u8))
> > {
> > @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
> > if (irq == -1)
> > irq = 0;
> > }
> > - dev->irq = irq;
> > + /* Since pci_fixup_irqs() can be called more than once due to multiple
> > + * host controllers, and we scan all PCI devices, not just those
> > + * attached to this controller, make sure we don't clobber dev->irq
> > + * that has nothing to do with this domain.
> > + */
> > + if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> > + return;
> >
> > + dev->irq = irq;
> > dev_dbg(&dev->dev, "fixup irq: got %d\n", dev->irq);
> >
> > /* Always tell the device, so the driver knows what is
> > @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
> > struct pci_dev *dev = NULL;
> >
> > for_each_pci_dev(dev)
> > - pdev_fixup_irq(dev, swizzle, map_irq);
> > + pdev_fixup_irq(-1, dev, swizzle, map_irq);
> > }
> > EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> > +
> > +void pci_fixup_irqs_local(struct pci_bus *bus,
> > + u8 (*swizzle)(struct pci_dev *, u8 *),
> > + int (*map_irq)(const struct pci_dev *, u8, u8))
> > +{
> > + struct pci_dev *dev = NULL;
> > +
> > + for_each_pci_dev(dev)
> > + pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 8badb66..37a97df 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1134,6 +1134,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 pci_fixup_irqs_local(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
> *);
> > --
> > 2.5.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
2016-08-08 9:31 ` Phil Edworthy
@ 2016-08-08 23:19 ` Bjorn Helgaas
2016-08-09 3:40 ` Matthew Minter
2016-08-09 9:14 ` Lorenzo Pieralisi
0 siblings, 2 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2016-08-08 23:19 UTC (permalink / raw)
To: Phil Edworthy
Cc: Lorenzo Pieralisi, Bjorn Helgaas, linux-pci, linux-renesas-soc,
Matthew Minter
[+cc Matthew]
On Mon, Aug 08, 2016 at 09:31:05AM +0000, Phil Edworthy wrote:
> Hi Lorenzo,
>
> Thanks for the link, I'll wait to see how this pans out.
I'm not sure anything is happening there. Last time I looked
(http://lkml.kernel.org/r/20151223230433.GA27708@localhost), I thought
there were some issues.
>From Lorenzo's response, it sounds like he thought maybe I should
apply it anyway, but I dropped the ball and haven't done anything with
it.
I'll take another look at it.
> > -----Original Message-----
> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > owner@vger.kernel.org] On Behalf Of Lorenzo Pieralisi
> > Sent: 21 July 2016 10:35
> > To: Phil Edworthy <phil.edworthy@renesas.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> > renesas-soc@vger.kernel.org
> > Subject: Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
> >
> > Hi Phil,
> >
> > On Fri, Jul 08, 2016 at 12:49:55PM +0100, Phil Edworthy wrote:
> > > Hi Bjorn,
> > >
> > > I've marked this as RFC as I guess there might be a better way to do
> > > this, but I'm not sure how. I would appreciate your thoughts on this.
> >
> > There is a more generic solution to this issue, that has been
> > hanging in the balance for a while:
> >
> > http://marc.info/?l=linux-pci&m=144557674601373&w=2
> >
> > We should bring it to completion since pci_fixup_irqs() should
> > really be replaced, this is just another example of the bugs
> > it can trigger.
> >
> > Thanks,
> > Lorenzo
> >
> > > Thanks
> > > Phil
> > >
> > > ---
> > > pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> > > no matter that the device may be on a completely unrelated PCI
> > > Host controller.
> > >
> > > When you have multiple PCI Host controllers, pdev_fixup_irq() can
> > > clobber the dev->irq of devices it should not be changing.
> > > This has been seen when performing suspend/resume on boards with
> > > two R-Car PCIe Host controllers, resulting in a NULL ptr access
> > > in __pci_restore_msi_state. This happens because dev->irq has been
> > > overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
> > >
> > > This patch introduces a new function, pci_fixup_irqs_local(), that
> > > performs the same operation as pdev_fixup_irq(), but only changes
> > > the dev->irq of device on the same domain.
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > > ---
> > > drivers/pci/setup-irq.c | 25 ++++++++++++++++++++++---
> > > include/linux/pci.h | 3 +++
> > > 2 files changed, 25 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> > > index 95c225b..90ea8fa 100644
> > > --- a/drivers/pci/setup-irq.c
> > > +++ b/drivers/pci/setup-irq.c
> > > @@ -22,7 +22,8 @@ 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,
> > > +static void pdev_fixup_irq(int domain_nr,
> > > + struct pci_dev *dev,
> > > u8 (*swizzle)(struct pci_dev *, u8 *),
> > > int (*map_irq)(const struct pci_dev *, u8, u8))
> > > {
> > > @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
> > > if (irq == -1)
> > > irq = 0;
> > > }
> > > - dev->irq = irq;
> > > + /* Since pci_fixup_irqs() can be called more than once due to multiple
> > > + * host controllers, and we scan all PCI devices, not just those
> > > + * attached to this controller, make sure we don't clobber dev->irq
> > > + * that has nothing to do with this domain.
> > > + */
> > > + if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> > > + return;
> > >
> > > + dev->irq = irq;
> > > dev_dbg(&dev->dev, "fixup irq: got %d\n", dev->irq);
> > >
> > > /* Always tell the device, so the driver knows what is
> > > @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
> > > struct pci_dev *dev = NULL;
> > >
> > > for_each_pci_dev(dev)
> > > - pdev_fixup_irq(dev, swizzle, map_irq);
> > > + pdev_fixup_irq(-1, dev, swizzle, map_irq);
> > > }
> > > EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> > > +
> > > +void pci_fixup_irqs_local(struct pci_bus *bus,
> > > + u8 (*swizzle)(struct pci_dev *, u8 *),
> > > + int (*map_irq)(const struct pci_dev *, u8, u8))
> > > +{
> > > + struct pci_dev *dev = NULL;
> > > +
> > > + for_each_pci_dev(dev)
> > > + pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 8badb66..37a97df 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -1134,6 +1134,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 pci_fixup_irqs_local(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
> > *);
> > > --
> > > 2.5.0
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
2016-08-08 23:19 ` Bjorn Helgaas
@ 2016-08-09 3:40 ` Matthew Minter
2016-08-09 9:14 ` Lorenzo Pieralisi
1 sibling, 0 replies; 7+ messages in thread
From: Matthew Minter @ 2016-08-09 3:40 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Phil Edworthy, Lorenzo Pieralisi, Bjorn Helgaas, linux-pci,
linux-renesas-soc
Hi Bjorn and all involved here,
I have not really been able to do much work on the patchset in question
lately. A version (which I think is up to date but am not 100% sure is here
https://github.com/Vality/linux/commits/pci-fixes though I somehow thought I
rebased it onto a slightly newer kernel at some point.
Perhaps that is in a mailing list as a patchset somewhere.
I am not currently able to dedicate as much time as I would like to this but
am happy to do what I can to support any merging efforts for this if that is
what is propsed. Particularly if there are any questions or small fixes.
However I do not think I have the time to single-handedly do a major rewrite
at this time if that is needed. But am happy to help any other efforts.
Thanks for CCing me in. I would be happy to see this go somewhere if possible.
Matthew
On Monday, August 8, 2016 6:19:22 PM MDT Bjorn Helgaas wrote:
> [+cc Matthew]
>
> On Mon, Aug 08, 2016 at 09:31:05AM +0000, Phil Edworthy wrote:
> > Hi Lorenzo,
> >
> > Thanks for the link, I'll wait to see how this pans out.
>
> I'm not sure anything is happening there. Last time I looked
> (http://lkml.kernel.org/r/20151223230433.GA27708@localhost), I thought
> there were some issues.
>
> From Lorenzo's response, it sounds like he thought maybe I should
> apply it anyway, but I dropped the ball and haven't done anything with
> it.
>
> I'll take another look at it.
>
> > > -----Original Message-----
> > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > > owner@vger.kernel.org] On Behalf Of Lorenzo Pieralisi
> > > Sent: 21 July 2016 10:35
> > > To: Phil Edworthy <phil.edworthy@renesas.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org;
> > > linux-
> > > renesas-soc@vger.kernel.org
> > > Subject: Re: [RFC] pci: Provide a domain limited version of
> > > pdev_fixup_irq
> > >
> > > Hi Phil,
> > >
> > > On Fri, Jul 08, 2016 at 12:49:55PM +0100, Phil Edworthy wrote:
> > > > Hi Bjorn,
> > > >
> > > > I've marked this as RFC as I guess there might be a better way to do
> > > > this, but I'm not sure how. I would appreciate your thoughts on this.
> > >
> > > There is a more generic solution to this issue, that has been
> > > hanging in the balance for a while:
> > >
> > > http://marc.info/?l=linux-pci&m=144557674601373&w=2
> > >
> > > We should bring it to completion since pci_fixup_irqs() should
> > > really be replaced, this is just another example of the bugs
> > > it can trigger.
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > > Thanks
> > > > Phil
> > > >
> > > > ---
> > > > pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> > > > no matter that the device may be on a completely unrelated PCI
> > > > Host controller.
> > > >
> > > > When you have multiple PCI Host controllers, pdev_fixup_irq() can
> > > > clobber the dev->irq of devices it should not be changing.
> > > > This has been seen when performing suspend/resume on boards with
> > > > two R-Car PCIe Host controllers, resulting in a NULL ptr access
> > > > in __pci_restore_msi_state. This happens because dev->irq has been
> > > > overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
> > > >
> > > > This patch introduces a new function, pci_fixup_irqs_local(), that
> > > > performs the same operation as pdev_fixup_irq(), but only changes
> > > > the dev->irq of device on the same domain.
> > > >
> > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > > > ---
> > > >
> > > > drivers/pci/setup-irq.c | 25 ++++++++++++++++++++++---
> > > > include/linux/pci.h | 3 +++
> > > > 2 files changed, 25 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> > > > index 95c225b..90ea8fa 100644
> > > > --- a/drivers/pci/setup-irq.c
> > > > +++ b/drivers/pci/setup-irq.c
> > > > @@ -22,7 +22,8 @@ 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,
> > > > +static void pdev_fixup_irq(int domain_nr,
> > > > + struct pci_dev *dev,
> > > >
> > > > u8 (*swizzle)(struct pci_dev *, u8 *),
> > > > int (*map_irq)(const struct pci_dev *, u8, u8))
> > > >
> > > > {
> > > >
> > > > @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
> > > >
> > > > if (irq == -1)
> > > >
> > > > irq = 0;
> > > >
> > > > }
> > > >
> > > > - dev->irq = irq;
> > > > + /* Since pci_fixup_irqs() can be called more than once due to
> > > > multiple
> > > > + * host controllers, and we scan all PCI devices, not just those
> > > > + * attached to this controller, make sure we don't clobber dev-
>irq
> > > > + * that has nothing to do with this domain.
> > > > + */
> > > > + if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> > > > + return;
> > > >
> > > > + dev->irq = irq;
> > > >
> > > > dev_dbg(&dev->dev, "fixup irq: got %d\n", dev->irq);
> > > >
> > > > /* Always tell the device, so the driver knows what is
> > > >
> > > > @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *,
> > > > u8 *),> > >
> > > > struct pci_dev *dev = NULL;
> > > >
> > > > for_each_pci_dev(dev)
> > > >
> > > > - pdev_fixup_irq(dev, swizzle, map_irq);
> > > > + pdev_fixup_irq(-1, dev, swizzle, map_irq);
> > > >
> > > > }
> > > > EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> > > >
> > > > +
> > > > +void pci_fixup_irqs_local(struct pci_bus *bus,
> > > > + u8 (*swizzle)(struct pci_dev *, u8 *),
> > > > + int (*map_irq)(const struct pci_dev *, u8, u8))
> > > > +{
> > > > + struct pci_dev *dev = NULL;
> > > > +
> > > > + for_each_pci_dev(dev)
> > > > + pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 8badb66..37a97df 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -1134,6 +1134,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 pci_fixup_irqs_local(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
> > >
> > > *);
> > >
> > > > --
> > > > 2.5.0
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> > > > in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
2016-08-08 23:19 ` Bjorn Helgaas
2016-08-09 3:40 ` Matthew Minter
@ 2016-08-09 9:14 ` Lorenzo Pieralisi
1 sibling, 0 replies; 7+ messages in thread
From: Lorenzo Pieralisi @ 2016-08-09 9:14 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Phil Edworthy, Bjorn Helgaas, linux-pci, linux-renesas-soc,
Matthew Minter
Hi Bjorn, all,
On Mon, Aug 08, 2016 at 06:19:22PM -0500, Bjorn Helgaas wrote:
> [+cc Matthew]
>
> On Mon, Aug 08, 2016 at 09:31:05AM +0000, Phil Edworthy wrote:
> > Hi Lorenzo,
> >
> > Thanks for the link, I'll wait to see how this pans out.
>
> I'm not sure anything is happening there. Last time I looked
> (http://lkml.kernel.org/r/20151223230433.GA27708@localhost), I thought
> there were some issues.
>
> From Lorenzo's response, it sounds like he thought maybe I should
> apply it anyway, but I dropped the ball and haven't done anything with
> it.
There are pending issues to be sorted in particular for x86, but
given how Matthew's patchset is structured we could merge the code
as an opt-in (we could initialize the bridge->map_irq function pointer
only for the cases we know that are safe; granted we won't be able to
remove pci_fixup_irqs() till all its users are removed, but at least
on ARM/ARM64 we would be able to switch over to the new bridge->map_irq
allocation approach instead of relying on pci_fixup_irqs()).
I am not sure we need to wait for all arches to be converted before
merging Matt's code, that was my point, but I need to have another
thorough look myself before commenting any further.
Thanks,
Lorenzo
> I'll take another look at it.
>
> > > -----Original Message-----
> > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > > owner@vger.kernel.org] On Behalf Of Lorenzo Pieralisi
> > > Sent: 21 July 2016 10:35
> > > To: Phil Edworthy <phil.edworthy@renesas.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> > > renesas-soc@vger.kernel.org
> > > Subject: Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
> > >
> > > Hi Phil,
> > >
> > > On Fri, Jul 08, 2016 at 12:49:55PM +0100, Phil Edworthy wrote:
> > > > Hi Bjorn,
> > > >
> > > > I've marked this as RFC as I guess there might be a better way to do
> > > > this, but I'm not sure how. I would appreciate your thoughts on this.
> > >
> > > There is a more generic solution to this issue, that has been
> > > hanging in the balance for a while:
> > >
> > > http://marc.info/?l=linux-pci&m=144557674601373&w=2
> > >
> > > We should bring it to completion since pci_fixup_irqs() should
> > > really be replaced, this is just another example of the bugs
> > > it can trigger.
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > > Thanks
> > > > Phil
> > > >
> > > > ---
> > > > pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> > > > no matter that the device may be on a completely unrelated PCI
> > > > Host controller.
> > > >
> > > > When you have multiple PCI Host controllers, pdev_fixup_irq() can
> > > > clobber the dev->irq of devices it should not be changing.
> > > > This has been seen when performing suspend/resume on boards with
> > > > two R-Car PCIe Host controllers, resulting in a NULL ptr access
> > > > in __pci_restore_msi_state. This happens because dev->irq has been
> > > > overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
> > > >
> > > > This patch introduces a new function, pci_fixup_irqs_local(), that
> > > > performs the same operation as pdev_fixup_irq(), but only changes
> > > > the dev->irq of device on the same domain.
> > > >
> > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > > > ---
> > > > drivers/pci/setup-irq.c | 25 ++++++++++++++++++++++---
> > > > include/linux/pci.h | 3 +++
> > > > 2 files changed, 25 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> > > > index 95c225b..90ea8fa 100644
> > > > --- a/drivers/pci/setup-irq.c
> > > > +++ b/drivers/pci/setup-irq.c
> > > > @@ -22,7 +22,8 @@ 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,
> > > > +static void pdev_fixup_irq(int domain_nr,
> > > > + struct pci_dev *dev,
> > > > u8 (*swizzle)(struct pci_dev *, u8 *),
> > > > int (*map_irq)(const struct pci_dev *, u8, u8))
> > > > {
> > > > @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
> > > > if (irq == -1)
> > > > irq = 0;
> > > > }
> > > > - dev->irq = irq;
> > > > + /* Since pci_fixup_irqs() can be called more than once due to multiple
> > > > + * host controllers, and we scan all PCI devices, not just those
> > > > + * attached to this controller, make sure we don't clobber dev->irq
> > > > + * that has nothing to do with this domain.
> > > > + */
> > > > + if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> > > > + return;
> > > >
> > > > + dev->irq = irq;
> > > > dev_dbg(&dev->dev, "fixup irq: got %d\n", dev->irq);
> > > >
> > > > /* Always tell the device, so the driver knows what is
> > > > @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
> > > > struct pci_dev *dev = NULL;
> > > >
> > > > for_each_pci_dev(dev)
> > > > - pdev_fixup_irq(dev, swizzle, map_irq);
> > > > + pdev_fixup_irq(-1, dev, swizzle, map_irq);
> > > > }
> > > > EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> > > > +
> > > > +void pci_fixup_irqs_local(struct pci_bus *bus,
> > > > + u8 (*swizzle)(struct pci_dev *, u8 *),
> > > > + int (*map_irq)(const struct pci_dev *, u8, u8))
> > > > +{
> > > > + struct pci_dev *dev = NULL;
> > > > +
> > > > + for_each_pci_dev(dev)
> > > > + pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 8badb66..37a97df 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -1134,6 +1134,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 pci_fixup_irqs_local(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
> > > *);
> > > > --
> > > > 2.5.0
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-09 9:14 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-08 11:49 [RFC] pci: Provide a domain limited version of pdev_fixup_irq Phil Edworthy
2016-07-11 8:39 ` Phil Edworthy
2016-07-21 9:35 ` Lorenzo Pieralisi
2016-08-08 9:31 ` Phil Edworthy
2016-08-08 23:19 ` Bjorn Helgaas
2016-08-09 3:40 ` Matthew Minter
2016-08-09 9:14 ` Lorenzo Pieralisi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.