| Message ID | 20120530044834.1164.9081.stgit@bling.home |
|---|---|
| State | Accepted |
| Headers | show |
Hi Alex, >DMA transactions are tagged with the source ID of the device making >the request. Occasionally hardware screws this up and uses the >source ID of a different device (often the wrong function number of >a multifunction device). A specific Ricoh multifunction device is >a prime example of this problem and included in this patch. The >purpose of this function is that given a pci_dev, return the pci_dev >to use as the source ID for DMA. When hardware works correctly, >this returns the input device. For the components of the Ricoh >multifunction device, return the pci_dev for function 0. > >This will be used by IOMMU drivers for determining the boundaries >of IOMMU groups as multiple devices using the same source ID must >be contained within the same group. This can also be used by >existing streaming DMA paths for the same purpose. > >Signed-off-by: Alex Williamson <alex.williamson@redhat.com> >--- > > drivers/pci/quirks.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 5 +++++ > 2 files changed, 45 insertions(+) > >diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >index 2a75216..004e167 100644 >--- a/drivers/pci/quirks.c >+++ b/drivers/pci/quirks.c >@@ -3179,3 +3179,43 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe) > > return -ENOTTY; > } >+ >+static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev) >+{ >+ return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); >+} I think we needn't retrieve the PCI device passed in again from the list of PCI devices if its function number is zero. if (!PCI_FUNC(dev->devfn)) return dev; Thanks, Gavin >+ >+static const struct pci_dev_dma_source { >+ u16 vendor; >+ u16 device; >+ struct pci_dev *(*dma_source)(struct pci_dev *dev); >+} pci_dev_dma_source[] = { >+ /* >+ * https://bugzilla.redhat.com/show_bug.cgi?id=605888 >+ * >+ * Some Ricoh devices use the function 0 source ID for DMA on >+ * other functions of a multifunction device. The DMA devices >+ * is therefore function 0, which will have implications of the >+ * iommu grouping of these devices. >+ */ >+ { PCI_VENDOR_ID_RICOH, 0xe822, pci_func_0_dma_source }, >+ { PCI_VENDOR_ID_RICOH, 0xe230, pci_func_0_dma_source }, >+ { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, >+ { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, >+ { 0 } >+}; >+ >+struct pci_dev *pci_dma_source(struct pci_dev *dev) >+{ >+ const struct pci_dev_dma_source *i; >+ >+ for (i = pci_dev_dma_source; i->dma_source; i++) { >+ if ((i->vendor == dev->vendor || >+ i->vendor == (u16)PCI_ANY_ID) && >+ (i->device == dev->device || >+ i->device == (u16)PCI_ANY_ID)) >+ return i->dma_source(dev); >+ } >+ >+ return dev; >+} >diff --git a/include/linux/pci.h b/include/linux/pci.h >index d8c379d..5bc7502 100644 >--- a/include/linux/pci.h >+++ b/include/linux/pci.h >@@ -1486,9 +1486,14 @@ enum pci_fixup_pass { > > #ifdef CONFIG_PCI_QUIRKS > void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev); >+struct pci_dev *pci_dma_source(struct pci_dev *dev); > #else > static inline void pci_fixup_device(enum pci_fixup_pass pass, > struct pci_dev *dev) {} >+static inline struct pci_dev *pci_dma_source(struct pci_dev *dev) >+{ >+ return dev; >+} > #endif > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen); > >-- >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 Alex, pci_get_slot() will hold a reference count on the returned pci device, so need to release the reference count for balance. According to patches "[PATCH 5/7] amd_iommu: Make use of DMA quirks and ACS checks in IOMMU groups" and "[PATCH 6/7] intel-iommu: Make use of DMA quirks and ACS checks in IOMMU groups", current patches failed to release the reference count gained by pci_get_slot(). Thanks Gerry On 05/30/2012 12:48 PM, Alex Williamson wrote: > DMA transactions are tagged with the source ID of the device making > the request. Occasionally hardware screws this up and uses the > source ID of a different device (often the wrong function number of > a multifunction device). A specific Ricoh multifunction device is > a prime example of this problem and included in this patch. The > purpose of this function is that given a pci_dev, return the pci_dev > to use as the source ID for DMA. When hardware works correctly, > this returns the input device. For the components of the Ricoh > multifunction device, return the pci_dev for function 0. > > This will be used by IOMMU drivers for determining the boundaries > of IOMMU groups as multiple devices using the same source ID must > be contained within the same group. This can also be used by > existing streaming DMA paths for the same purpose. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > drivers/pci/quirks.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 5 +++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 2a75216..004e167 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3179,3 +3179,43 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe) > > return -ENOTTY; > } > + > +static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev) > +{ > + return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); > +} > + > +static const struct pci_dev_dma_source { > + u16 vendor; > + u16 device; > + struct pci_dev *(*dma_source)(struct pci_dev *dev); > +} pci_dev_dma_source[] = { > + /* > + * https://bugzilla.redhat.com/show_bug.cgi?id=605888 > + * > + * Some Ricoh devices use the function 0 source ID for DMA on > + * other functions of a multifunction device. The DMA devices > + * is therefore function 0, which will have implications of the > + * iommu grouping of these devices. > + */ > + { PCI_VENDOR_ID_RICOH, 0xe822, pci_func_0_dma_source }, > + { PCI_VENDOR_ID_RICOH, 0xe230, pci_func_0_dma_source }, > + { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, > + { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, > + { 0 } > +}; > + > +struct pci_dev *pci_dma_source(struct pci_dev *dev) > +{ > + const struct pci_dev_dma_source *i; > + > + for (i = pci_dev_dma_source; i->dma_source; i++) { > + if ((i->vendor == dev->vendor || > + i->vendor == (u16)PCI_ANY_ID) && > + (i->device == dev->device || > + i->device == (u16)PCI_ANY_ID)) > + return i->dma_source(dev); > + } > + > + return dev; > +} > diff --git a/include/linux/pci.h b/include/linux/pci.h > index d8c379d..5bc7502 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1486,9 +1486,14 @@ enum pci_fixup_pass { > > #ifdef CONFIG_PCI_QUIRKS > void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev); > +struct pci_dev *pci_dma_source(struct pci_dev *dev); > #else > static inline void pci_fixup_device(enum pci_fixup_pass pass, > struct pci_dev *dev) {} > +static inline struct pci_dev *pci_dma_source(struct pci_dev *dev) > +{ > + return dev; > +} > #endif > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen); > > -- > 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
On Wed, 2012-05-30 at 16:34 +0800, Gavin Shan wrote: > Hi Alex, > > >DMA transactions are tagged with the source ID of the device making > >the request. Occasionally hardware screws this up and uses the > >source ID of a different device (often the wrong function number of > >a multifunction device). A specific Ricoh multifunction device is > >a prime example of this problem and included in this patch. The > >purpose of this function is that given a pci_dev, return the pci_dev > >to use as the source ID for DMA. When hardware works correctly, > >this returns the input device. For the components of the Ricoh > >multifunction device, return the pci_dev for function 0. > > > >This will be used by IOMMU drivers for determining the boundaries > >of IOMMU groups as multiple devices using the same source ID must > >be contained within the same group. This can also be used by > >existing streaming DMA paths for the same purpose. > > > >Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > >--- > > > > drivers/pci/quirks.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 5 +++++ > > 2 files changed, 45 insertions(+) > > > >diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > >index 2a75216..004e167 100644 > >--- a/drivers/pci/quirks.c > >+++ b/drivers/pci/quirks.c > >@@ -3179,3 +3179,43 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe) > > > > return -ENOTTY; > > } > >+ > >+static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev) > >+{ > >+ return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); > >+} > > I think we needn't retrieve the PCI device passed in again from the list > of PCI devices if its function number is zero. > > if (!PCI_FUNC(dev->devfn)) > return dev; I guess I was expecting we wouldn't quirk a function 0 device, but looking again at the bug below I see that I am including the func 0 vendor/device. I've also got a bad cut-n-paste as I'm including the same device twice. I'll take your suggestion. Thanks, Alex > >+ > >+static const struct pci_dev_dma_source { > >+ u16 vendor; > >+ u16 device; > >+ struct pci_dev *(*dma_source)(struct pci_dev *dev); > >+} pci_dev_dma_source[] = { > >+ /* > >+ * https://bugzilla.redhat.com/show_bug.cgi?id=605888 > >+ * > >+ * Some Ricoh devices use the function 0 source ID for DMA on > >+ * other functions of a multifunction device. The DMA devices > >+ * is therefore function 0, which will have implications of the > >+ * iommu grouping of these devices. > >+ */ > >+ { PCI_VENDOR_ID_RICOH, 0xe822, pci_func_0_dma_source }, > >+ { PCI_VENDOR_ID_RICOH, 0xe230, pci_func_0_dma_source }, > >+ { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, > >+ { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, > >+ { 0 } > >+}; > >+ > >+struct pci_dev *pci_dma_source(struct pci_dev *dev) > >+{ > >+ const struct pci_dev_dma_source *i; > >+ > >+ for (i = pci_dev_dma_source; i->dma_source; i++) { > >+ if ((i->vendor == dev->vendor || > >+ i->vendor == (u16)PCI_ANY_ID) && > >+ (i->device == dev->device || > >+ i->device == (u16)PCI_ANY_ID)) > >+ return i->dma_source(dev); > >+ } > >+ > >+ return dev; > >+} > >diff --git a/include/linux/pci.h b/include/linux/pci.h > >index d8c379d..5bc7502 100644 > >--- a/include/linux/pci.h > >+++ b/include/linux/pci.h > >@@ -1486,9 +1486,14 @@ enum pci_fixup_pass { > > > > #ifdef CONFIG_PCI_QUIRKS > > void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev); > >+struct pci_dev *pci_dma_source(struct pci_dev *dev); > > #else > > static inline void pci_fixup_device(enum pci_fixup_pass pass, > > struct pci_dev *dev) {} > >+static inline struct pci_dev *pci_dma_source(struct pci_dev *dev) > >+{ > >+ return dev; > >+} > > #endif > > > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen); > > > >-- > >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
On Wed, 2012-05-30 at 21:27 +0800, Jiang Liu wrote: > Hi Alex, > pci_get_slot() will hold a reference count on the returned > pci device, so need to release the reference count for balance. > According to patches "[PATCH 5/7] amd_iommu: Make use of DMA quirks > and ACS checks in IOMMU groups" and "[PATCH 6/7] intel-iommu: Make > use of DMA quirks and ACS checks in IOMMU groups", current patches > failed to release the reference count gained by pci_get_slot(). Thanks for pointing this out Gerry. I think I'll rename pci_dma_source() to pci_get_dma_source() which will then increment the reference of the device it returns, whether it's a quirk or the original device. The caller will then be responsible for doing a pci_dev_put(). Thanks, Alex > On 05/30/2012 12:48 PM, Alex Williamson wrote: > > DMA transactions are tagged with the source ID of the device making > > the request. Occasionally hardware screws this up and uses the > > source ID of a different device (often the wrong function number of > > a multifunction device). A specific Ricoh multifunction device is > > a prime example of this problem and included in this patch. The > > purpose of this function is that given a pci_dev, return the pci_dev > > to use as the source ID for DMA. When hardware works correctly, > > this returns the input device. For the components of the Ricoh > > multifunction device, return the pci_dev for function 0. > > > > This will be used by IOMMU drivers for determining the boundaries > > of IOMMU groups as multiple devices using the same source ID must > > be contained within the same group. This can also be used by > > existing streaming DMA paths for the same purpose. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > > > drivers/pci/quirks.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 5 +++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index 2a75216..004e167 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -3179,3 +3179,43 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe) > > > > return -ENOTTY; > > } > > + > > +static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev) > > +{ > > + return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); > > +} > > + > > +static const struct pci_dev_dma_source { > > + u16 vendor; > > + u16 device; > > + struct pci_dev *(*dma_source)(struct pci_dev *dev); > > +} pci_dev_dma_source[] = { > > + /* > > + * https://bugzilla.redhat.com/show_bug.cgi?id=605888 > > + * > > + * Some Ricoh devices use the function 0 source ID for DMA on > > + * other functions of a multifunction device. The DMA devices > > + * is therefore function 0, which will have implications of the > > + * iommu grouping of these devices. > > + */ > > + { PCI_VENDOR_ID_RICOH, 0xe822, pci_func_0_dma_source }, > > + { PCI_VENDOR_ID_RICOH, 0xe230, pci_func_0_dma_source }, > > + { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, > > + { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, > > + { 0 } > > +}; > > + > > +struct pci_dev *pci_dma_source(struct pci_dev *dev) > > +{ > > + const struct pci_dev_dma_source *i; > > + > > + for (i = pci_dev_dma_source; i->dma_source; i++) { > > + if ((i->vendor == dev->vendor || > > + i->vendor == (u16)PCI_ANY_ID) && > > + (i->device == dev->device || > > + i->device == (u16)PCI_ANY_ID)) > > + return i->dma_source(dev); > > + } > > + > > + return dev; > > +} > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index d8c379d..5bc7502 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -1486,9 +1486,14 @@ enum pci_fixup_pass { > > > > #ifdef CONFIG_PCI_QUIRKS > > void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev); > > +struct pci_dev *pci_dma_source(struct pci_dev *dev); > > #else > > static inline void pci_fixup_device(enum pci_fixup_pass pass, > > struct pci_dev *dev) {} > > +static inline struct pci_dev *pci_dma_source(struct pci_dev *dev) > > +{ > > + return dev; > > +} > > #endif > > > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen); > > > > -- > > 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/quirks.c b/drivers/pci/quirks.c index 2a75216..004e167 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3179,3 +3179,43 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe) return -ENOTTY; } + +static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev) +{ + return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); +} + +static const struct pci_dev_dma_source { + u16 vendor; + u16 device; + struct pci_dev *(*dma_source)(struct pci_dev *dev); +} pci_dev_dma_source[] = { + /* + * https://bugzilla.redhat.com/show_bug.cgi?id=605888 + * + * Some Ricoh devices use the function 0 source ID for DMA on + * other functions of a multifunction device. The DMA devices + * is therefore function 0, which will have implications of the + * iommu grouping of these devices. + */ + { PCI_VENDOR_ID_RICOH, 0xe822, pci_func_0_dma_source }, + { PCI_VENDOR_ID_RICOH, 0xe230, pci_func_0_dma_source }, + { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, + { PCI_VENDOR_ID_RICOH, 0xe832, pci_func_0_dma_source }, + { 0 } +}; + +struct pci_dev *pci_dma_source(struct pci_dev *dev) +{ + const struct pci_dev_dma_source *i; + + for (i = pci_dev_dma_source; i->dma_source; i++) { + if ((i->vendor == dev->vendor || + i->vendor == (u16)PCI_ANY_ID) && + (i->device == dev->device || + i->device == (u16)PCI_ANY_ID)) + return i->dma_source(dev); + } + + return dev; +} diff --git a/include/linux/pci.h b/include/linux/pci.h index d8c379d..5bc7502 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1486,9 +1486,14 @@ enum pci_fixup_pass { #ifdef CONFIG_PCI_QUIRKS void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev); +struct pci_dev *pci_dma_source(struct pci_dev *dev); #else static inline void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev) {} +static inline struct pci_dev *pci_dma_source(struct pci_dev *dev) +{ + return dev; +} #endif void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);
DMA transactions are tagged with the source ID of the device making the request. Occasionally hardware screws this up and uses the source ID of a different device (often the wrong function number of a multifunction device). A specific Ricoh multifunction device is a prime example of this problem and included in this patch. The purpose of this function is that given a pci_dev, return the pci_dev to use as the source ID for DMA. When hardware works correctly, this returns the input device. For the components of the Ricoh multifunction device, return the pci_dev for function 0. This will be used by IOMMU drivers for determining the boundaries of IOMMU groups as multiple devices using the same source ID must be contained within the same group. This can also be used by existing streaming DMA paths for the same purpose. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/pci/quirks.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 5 +++++ 2 files changed, 45 insertions(+) -- 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