| Message ID | 1467978595-25386-1-git-send-email-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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
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 >
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
[+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
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
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 >
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 *);
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(-)