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 <[email protected]> > --- > 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 [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html >
