Hi Lorenzo, Thanks for the link, I'll wait to see how this pans out. Thanks Phil > -----Original Message----- > From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci- > owner@xxxxxxxxxxxxxxx] On Behalf Of Lorenzo Pieralisi > Sent: 21 July 2016 10:35 > To: Phil Edworthy <phil.edworthy@xxxxxxxxxxx> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx; linux- > renesas-soc@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html
