Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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
> > 
> > --
> > 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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux