| Message ID | ef40dbdc4eae32490caec47bed5b57eeb438dd80.1567029860.git.sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Fix PF/VF dependency issue | expand |
On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Since pci_prg_resp_pasid_required() function has dependency on both > PASID and PRI, define it only if both CONFIG_PCI_PRI and > CONFIG_PCI_PASID config options are enabled. > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required() > interface.") [Don't split tags, including "Fixes:" across lines] This definitely doesn't fix e5567f5f6762. That commit added pci_prg_resp_pasid_required(), but with no dependency on CONFIG_PCI_PRI or CONFIG_PCI_PASID. This patch is only required when a subsequent patch is applied. It should be squashed into the commit that requires it so it's obvious why it's needed. I've been poking at this series, and I'll post a v8 soon with this and other fixes. > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > drivers/pci/ats.c | 10 ++++++---- > include/linux/pci-ats.h | 12 +++++++++--- > 2 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index e18499243f84..cdd936d10f68 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev) > } > EXPORT_SYMBOL_GPL(pci_pasid_features); > > +#ifdef CONFIG_PCI_PRI > + > /** > * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit > * status. > @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features); > * > * Returns 1 if PASID is required in PRG Response Message, 0 otherwise. > * > - * Even though the PRG response PASID status is read from PRI Status > - * Register, since this API will mainly be used by PASID users, this > - * function is defined within #ifdef CONFIG_PCI_PASID instead of > - * CONFIG_PCI_PRI. > + * Since this API has dependency on both PRI and PASID, protect it > + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID. > */ > int pci_prg_resp_pasid_required(struct pci_dev *pdev) > { > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev) > } > EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); > > +#endif > + > #define PASID_NUMBER_SHIFT 8 > #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT) > /** > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h > index 1ebb88e7c184..1a0bdaee2f32 100644 > --- a/include/linux/pci-ats.h > +++ b/include/linux/pci-ats.h > @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev); > void pci_restore_pasid_state(struct pci_dev *pdev); > int pci_pasid_features(struct pci_dev *pdev); > int pci_max_pasids(struct pci_dev *pdev); > -int pci_prg_resp_pasid_required(struct pci_dev *pdev); > > #else /* CONFIG_PCI_PASID */ > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev) > return -EINVAL; > } > > +#endif /* CONFIG_PCI_PASID */ > + > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID) > + > +int pci_prg_resp_pasid_required(struct pci_dev *pdev); > + > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */ > + > static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev) > { > return 0; > } > -#endif /* CONFIG_PCI_PASID */ > - > +#endif > > #endif /* LINUX_PCI_ATS_H*/ > -- > 2.21.0 >
Hi Bjorn, Thanks for looking into this patch set. On 9/5/19 12:18 PM, Bjorn Helgaas wrote: > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> >> Since pci_prg_resp_pasid_required() function has dependency on both >> PASID and PRI, define it only if both CONFIG_PCI_PRI and >> CONFIG_PCI_PASID config options are enabled. >> >> Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required() >> interface.") > [Don't split tags, including "Fixes:" across lines] > > This definitely doesn't fix e5567f5f6762. That commit added > pci_prg_resp_pasid_required(), but with no dependency on > CONFIG_PCI_PRI or CONFIG_PCI_PASID. > > This patch is only required when a subsequent patch is applied. It > should be squashed into the commit that requires it so it's obvious > why it's needed. > > I've been poking at this series, and I'll post a v8 soon with this and > other fixes. In your v8 submission you did not merge this patch. You did not use pri_cap or pasid_cap cached values. Instead you have re-read the value from register. Is this intentional? Since this function will be called for every VF device we might loose some performance benefit. > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> drivers/pci/ats.c | 10 ++++++---- >> include/linux/pci-ats.h | 12 +++++++++--- >> 2 files changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c >> index e18499243f84..cdd936d10f68 100644 >> --- a/drivers/pci/ats.c >> +++ b/drivers/pci/ats.c >> @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev) >> } >> EXPORT_SYMBOL_GPL(pci_pasid_features); >> >> +#ifdef CONFIG_PCI_PRI >> + >> /** >> * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit >> * status. >> @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features); >> * >> * Returns 1 if PASID is required in PRG Response Message, 0 otherwise. >> * >> - * Even though the PRG response PASID status is read from PRI Status >> - * Register, since this API will mainly be used by PASID users, this >> - * function is defined within #ifdef CONFIG_PCI_PASID instead of >> - * CONFIG_PCI_PRI. >> + * Since this API has dependency on both PRI and PASID, protect it >> + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID. >> */ >> int pci_prg_resp_pasid_required(struct pci_dev *pdev) >> { >> @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev) >> } >> EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); >> >> +#endif >> + >> #define PASID_NUMBER_SHIFT 8 >> #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT) >> /** >> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h >> index 1ebb88e7c184..1a0bdaee2f32 100644 >> --- a/include/linux/pci-ats.h >> +++ b/include/linux/pci-ats.h >> @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev); >> void pci_restore_pasid_state(struct pci_dev *pdev); >> int pci_pasid_features(struct pci_dev *pdev); >> int pci_max_pasids(struct pci_dev *pdev); >> -int pci_prg_resp_pasid_required(struct pci_dev *pdev); >> >> #else /* CONFIG_PCI_PASID */ >> >> @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev) >> return -EINVAL; >> } >> >> +#endif /* CONFIG_PCI_PASID */ >> + >> +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID) >> + >> +int pci_prg_resp_pasid_required(struct pci_dev *pdev); >> + >> +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */ >> + >> static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev) >> { >> return 0; >> } >> -#endif /* CONFIG_PCI_PASID */ >> - >> +#endif >> >> #endif /* LINUX_PCI_ATS_H*/ >> -- >> 2.21.0 >>
On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote: > Hi Bjorn, > > Thanks for looking into this patch set. > > On 9/5/19 12:18 PM, Bjorn Helgaas wrote: > > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > Since pci_prg_resp_pasid_required() function has dependency on both > > > PASID and PRI, define it only if both CONFIG_PCI_PRI and > > > CONFIG_PCI_PASID config options are enabled. > > > > > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required() > > > interface.") > > [Don't split tags, including "Fixes:" across lines] > > > > This definitely doesn't fix e5567f5f6762. That commit added > > pci_prg_resp_pasid_required(), but with no dependency on > > CONFIG_PCI_PRI or CONFIG_PCI_PASID. > > > > This patch is only required when a subsequent patch is applied. It > > should be squashed into the commit that requires it so it's obvious > > why it's needed. > > > > I've been poking at this series, and I'll post a v8 soon with this and > > other fixes. > In your v8 submission you did not merge this patch. You did not use > pri_cap or pasid_cap cached values. Instead you have re-read the > value from register. Is this intentional? > > Since this function will be called for every VF device we might loose some > performance benefit. This particular patch doesn't do any caching. IIRC it fiddles with ifdefs to solve a problem that would be introduced by a future patch. I don't remember the exact details, but I think the series I merged doesn't have that problem. If it does, let me know the details and we can fix it. I did include the caching patches for both PRI and PASID capabilities, but they're only performance optimizations so I moved them to the end so the functional fixes would be smaller and earlier in the series. > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > --- > > > drivers/pci/ats.c | 10 ++++++---- > > > include/linux/pci-ats.h | 12 +++++++++--- > > > 2 files changed, 15 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > > > index e18499243f84..cdd936d10f68 100644 > > > --- a/drivers/pci/ats.c > > > +++ b/drivers/pci/ats.c > > > @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev) > > > } > > > EXPORT_SYMBOL_GPL(pci_pasid_features); > > > +#ifdef CONFIG_PCI_PRI > > > + > > > /** > > > * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit > > > * status. > > > @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features); > > > * > > > * Returns 1 if PASID is required in PRG Response Message, 0 otherwise. > > > * > > > - * Even though the PRG response PASID status is read from PRI Status > > > - * Register, since this API will mainly be used by PASID users, this > > > - * function is defined within #ifdef CONFIG_PCI_PASID instead of > > > - * CONFIG_PCI_PRI. > > > + * Since this API has dependency on both PRI and PASID, protect it > > > + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID. > > > */ > > > int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > { > > > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > } > > > EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); > > > +#endif > > > + > > > #define PASID_NUMBER_SHIFT 8 > > > #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT) > > > /** > > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h > > > index 1ebb88e7c184..1a0bdaee2f32 100644 > > > --- a/include/linux/pci-ats.h > > > +++ b/include/linux/pci-ats.h > > > @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev); > > > void pci_restore_pasid_state(struct pci_dev *pdev); > > > int pci_pasid_features(struct pci_dev *pdev); > > > int pci_max_pasids(struct pci_dev *pdev); > > > -int pci_prg_resp_pasid_required(struct pci_dev *pdev); > > > #else /* CONFIG_PCI_PASID */ > > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev) > > > return -EINVAL; > > > } > > > +#endif /* CONFIG_PCI_PASID */ > > > + > > > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID) > > > + > > > +int pci_prg_resp_pasid_required(struct pci_dev *pdev); > > > + > > > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */ > > > + > > > static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > { > > > return 0; > > > } > > > -#endif /* CONFIG_PCI_PASID */ > > > - > > > +#endif > > > #endif /* LINUX_PCI_ATS_H*/ > > > -- > > > 2.21.0 > > > > -- > Sathyanarayanan Kuppuswamy > Linux kernel developer >
On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote: > On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote: > > Hi Bjorn, > > > > Thanks for looking into this patch set. > > > > On 9/5/19 12:18 PM, Bjorn Helgaas wrote: > > > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > > > Since pci_prg_resp_pasid_required() function has dependency on both > > > > PASID and PRI, define it only if both CONFIG_PCI_PRI and > > > > CONFIG_PCI_PASID config options are enabled. > > > > > > > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required() > > > > interface.") > > > [Don't split tags, including "Fixes:" across lines] > > > > > > This definitely doesn't fix e5567f5f6762. That commit added > > > pci_prg_resp_pasid_required(), but with no dependency on > > > CONFIG_PCI_PRI or CONFIG_PCI_PASID. > > > > > > This patch is only required when a subsequent patch is applied. It > > > should be squashed into the commit that requires it so it's obvious > > > why it's needed. > > > > > > I've been poking at this series, and I'll post a v8 soon with this and > > > other fixes. > > In your v8 submission you did not merge this patch. You did not use > > pri_cap or pasid_cap cached values. Instead you have re-read the > > value from register. Is this intentional? > > > > Since this function will be called for every VF device we might loose some > > performance benefit. > > This particular patch doesn't do any caching. IIRC it fiddles with > ifdefs to solve a problem that would be introduced by a future patch. > I don't remember the exact details, but I think the series I merged > doesn't have that problem. If it does, let me know the details and we > can fix it. This patch by itself does not do any caching. But your caching patch missed modifying this function to use cached values. Please check the current implementation of this function. It still reads PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let me know your comments. int pci_prg_resp_pasid_required(struct pci_dev *pdev) { u16 status; int pri; if (pdev->is_virtfn) pdev = pci_physfn(pdev); pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI); if (!pri) return 0; pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status); if (status & PCI_PRI_STATUS_PASID) return 1; return 0; } EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); If caching is applied to this function then we need this #ifdef dependency correction patch. > > I did include the caching patches for both PRI and PASID capabilities, > but they're only performance optimizations so I moved them to the end > so the functional fixes would be smaller and earlier in the series. > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > --- > > > > drivers/pci/ats.c | 10 ++++++---- > > > > include/linux/pci-ats.h | 12 +++++++++--- > > > > 2 files changed, 15 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > > > > index e18499243f84..cdd936d10f68 100644 > > > > --- a/drivers/pci/ats.c > > > > +++ b/drivers/pci/ats.c > > > > @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev) > > > > } > > > > EXPORT_SYMBOL_GPL(pci_pasid_features); > > > > +#ifdef CONFIG_PCI_PRI > > > > + > > > > /** > > > > * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit > > > > * status. > > > > @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features); > > > > * > > > > * Returns 1 if PASID is required in PRG Response Message, 0 otherwise. > > > > * > > > > - * Even though the PRG response PASID status is read from PRI Status > > > > - * Register, since this API will mainly be used by PASID users, this > > > > - * function is defined within #ifdef CONFIG_PCI_PASID instead of > > > > - * CONFIG_PCI_PRI. > > > > + * Since this API has dependency on both PRI and PASID, protect it > > > > + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID. > > > > */ > > > > int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > > { > > > > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > > } > > > > EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); > > > > +#endif > > > > + > > > > #define PASID_NUMBER_SHIFT 8 > > > > #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT) > > > > /** > > > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h > > > > index 1ebb88e7c184..1a0bdaee2f32 100644 > > > > --- a/include/linux/pci-ats.h > > > > +++ b/include/linux/pci-ats.h > > > > @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev); > > > > void pci_restore_pasid_state(struct pci_dev *pdev); > > > > int pci_pasid_features(struct pci_dev *pdev); > > > > int pci_max_pasids(struct pci_dev *pdev); > > > > -int pci_prg_resp_pasid_required(struct pci_dev *pdev); > > > > #else /* CONFIG_PCI_PASID */ > > > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev) > > > > return -EINVAL; > > > > } > > > > +#endif /* CONFIG_PCI_PASID */ > > > > + > > > > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID) > > > > + > > > > +int pci_prg_resp_pasid_required(struct pci_dev *pdev); > > > > + > > > > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */ > > > > + > > > > static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > > { > > > > return 0; > > > > } > > > > -#endif /* CONFIG_PCI_PASID */ > > > > - > > > > +#endif > > > > #endif /* LINUX_PCI_ATS_H*/ > > > > -- > > > > 2.21.0 > > > > > > -- > > Sathyanarayanan Kuppuswamy > > Linux kernel developer > >
On Thu, Oct 03, 2019 at 01:37:26PM -0700, Kuppuswamy Sathyanarayanan wrote: > On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote: > > On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote: > > > Hi Bjorn, > > > > > > Thanks for looking into this patch set. > > > > > > On 9/5/19 12:18 PM, Bjorn Helgaas wrote: > > > > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > > > > > Since pci_prg_resp_pasid_required() function has dependency on both > > > > > PASID and PRI, define it only if both CONFIG_PCI_PRI and > > > > > CONFIG_PCI_PASID config options are enabled. > > > > > > > > > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required() > > > > > interface.") > > > > [Don't split tags, including "Fixes:" across lines] > > > > > > > > This definitely doesn't fix e5567f5f6762. That commit added > > > > pci_prg_resp_pasid_required(), but with no dependency on > > > > CONFIG_PCI_PRI or CONFIG_PCI_PASID. > > > > > > > > This patch is only required when a subsequent patch is applied. It > > > > should be squashed into the commit that requires it so it's obvious > > > > why it's needed. > > > > > > > > I've been poking at this series, and I'll post a v8 soon with this and > > > > other fixes. > > > In your v8 submission you did not merge this patch. You did not use > > > pri_cap or pasid_cap cached values. Instead you have re-read the > > > value from register. Is this intentional? > > > > > > Since this function will be called for every VF device we might loose some > > > performance benefit. > > > > This particular patch doesn't do any caching. IIRC it fiddles with > > ifdefs to solve a problem that would be introduced by a future patch. > > I don't remember the exact details, but I think the series I merged > > doesn't have that problem. If it does, let me know the details and we > > can fix it. > This patch by itself does not do any caching. But your caching patch > missed modifying this function to use cached values. Please check the > current implementation of this function. It still reads > PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let > me know your comments. > > int pci_prg_resp_pasid_required(struct pci_dev *pdev) > { > u16 status; > int pri; > > if (pdev->is_virtfn) > pdev = pci_physfn(pdev); > > pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI); > if (!pri) > return 0; > > pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status); > > if (status & PCI_PRI_STATUS_PASID) > return 1; > > return 0; > } > EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); > > If caching is applied to this function then we need this #ifdef > dependency correction patch. IIRC this #ifdef patch wasn't connected to the actual *need* for the #ifdef, so it was very difficult to review. I thought this function would be infrequently used and it wasn't worth trying to sort out the #ifdef muddle to do the caching. But it does seem sort of pointless to chase the capability list again here, so maybe it *is* worth optimizing. The PRG Response PASID Required bit is read-only, so I wonder if it would be simpler if we just read PCI_PRI_STATUS once and save the bit in the struct pci_dev? We could do that in pci_enable_pri(), or if we might need the value before that's called, we could add a pci_pri_init() and do it there. > > I did include the caching patches for both PRI and PASID capabilities, > > but they're only performance optimizations so I moved them to the end > > so the functional fixes would be smaller and earlier in the series. > > > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > --- > > > > > drivers/pci/ats.c | 10 ++++++---- > > > > > include/linux/pci-ats.h | 12 +++++++++--- > > > > > 2 files changed, 15 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > > > > > index e18499243f84..cdd936d10f68 100644 > > > > > --- a/drivers/pci/ats.c > > > > > +++ b/drivers/pci/ats.c > > > > > @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev) > > > > > } > > > > > EXPORT_SYMBOL_GPL(pci_pasid_features); > > > > > +#ifdef CONFIG_PCI_PRI > > > > > + > > > > > /** > > > > > * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit > > > > > * status. > > > > > @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features); > > > > > * > > > > > * Returns 1 if PASID is required in PRG Response Message, 0 otherwise. > > > > > * > > > > > - * Even though the PRG response PASID status is read from PRI Status > > > > > - * Register, since this API will mainly be used by PASID users, this > > > > > - * function is defined within #ifdef CONFIG_PCI_PASID instead of > > > > > - * CONFIG_PCI_PRI. > > > > > + * Since this API has dependency on both PRI and PASID, protect it > > > > > + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID. > > > > > */ > > > > > int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > > > { > > > > > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > > > } > > > > > EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); > > > > > +#endif > > > > > + > > > > > #define PASID_NUMBER_SHIFT 8 > > > > > #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT) > > > > > /** > > > > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h > > > > > index 1ebb88e7c184..1a0bdaee2f32 100644 > > > > > --- a/include/linux/pci-ats.h > > > > > +++ b/include/linux/pci-ats.h > > > > > @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev); > > > > > void pci_restore_pasid_state(struct pci_dev *pdev); > > > > > int pci_pasid_features(struct pci_dev *pdev); > > > > > int pci_max_pasids(struct pci_dev *pdev); > > > > > -int pci_prg_resp_pasid_required(struct pci_dev *pdev); > > > > > #else /* CONFIG_PCI_PASID */ > > > > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev) > > > > > return -EINVAL; > > > > > } > > > > > +#endif /* CONFIG_PCI_PASID */ > > > > > + > > > > > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID) > > > > > + > > > > > +int pci_prg_resp_pasid_required(struct pci_dev *pdev); > > > > > + > > > > > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */ > > > > > + > > > > > static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > > > { > > > > > return 0; > > > > > } > > > > > -#endif /* CONFIG_PCI_PASID */ > > > > > - > > > > > +#endif > > > > > #endif /* LINUX_PCI_ATS_H*/ > > > > > -- > > > > > 2.21.0 > > > > > > > > -- > > > Sathyanarayanan Kuppuswamy > > > Linux kernel developer > > > > > -- > Sathyanarayanan Kuppuswamy > Linux kernel developer
On 10/3/19 2:01 PM, Bjorn Helgaas wrote: > On Thu, Oct 03, 2019 at 01:37:26PM -0700, Kuppuswamy Sathyanarayanan wrote: >> On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote: >>> On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote: >>>> Hi Bjorn, >>>> >>>> Thanks for looking into this patch set. >>>> >>>> On 9/5/19 12:18 PM, Bjorn Helgaas wrote: >>>>> On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: >>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >>>>>> >>>>>> Since pci_prg_resp_pasid_required() function has dependency on both >>>>>> PASID and PRI, define it only if both CONFIG_PCI_PRI and >>>>>> CONFIG_PCI_PASID config options are enabled. >>>>>> >>>>>> Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required() >>>>>> interface.") >>>>> [Don't split tags, including "Fixes:" across lines] >>>>> >>>>> This definitely doesn't fix e5567f5f6762. That commit added >>>>> pci_prg_resp_pasid_required(), but with no dependency on >>>>> CONFIG_PCI_PRI or CONFIG_PCI_PASID. >>>>> >>>>> This patch is only required when a subsequent patch is applied. It >>>>> should be squashed into the commit that requires it so it's obvious >>>>> why it's needed. >>>>> >>>>> I've been poking at this series, and I'll post a v8 soon with this and >>>>> other fixes. >>>> In your v8 submission you did not merge this patch. You did not use >>>> pri_cap or pasid_cap cached values. Instead you have re-read the >>>> value from register. Is this intentional? >>>> >>>> Since this function will be called for every VF device we might loose some >>>> performance benefit. >>> This particular patch doesn't do any caching. IIRC it fiddles with >>> ifdefs to solve a problem that would be introduced by a future patch. >>> I don't remember the exact details, but I think the series I merged >>> doesn't have that problem. If it does, let me know the details and we >>> can fix it. >> This patch by itself does not do any caching. But your caching patch >> missed modifying this function to use cached values. Please check the >> current implementation of this function. It still reads >> PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let >> me know your comments. >> >> int pci_prg_resp_pasid_required(struct pci_dev *pdev) >> { >> u16 status; >> int pri; >> >> if (pdev->is_virtfn) >> pdev = pci_physfn(pdev); >> >> pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI); >> if (!pri) >> return 0; >> >> pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status); >> >> if (status & PCI_PRI_STATUS_PASID) >> return 1; >> >> return 0; >> } >> EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); >> >> If caching is applied to this function then we need this #ifdef >> dependency correction patch. > IIRC this #ifdef patch wasn't connected to the actual *need* for the > #ifdef, so it was very difficult to review. I thought this function > would be infrequently used and it wasn't worth trying to sort out the > #ifdef muddle to do the caching. But it does seem sort of pointless > to chase the capability list again here, so maybe it *is* worth > optimizing. > > The PRG Response PASID Required bit is read-only, so I wonder if it > would be simpler if we just read PCI_PRI_STATUS once and save the bit > in the struct pci_dev? We could do that in pci_enable_pri(), or if we > might need the value before that's called, we could add a > pci_pri_init() and do it there. Yes, caching PASID Required bit in pci_pri_init() function would provide performance benefits. But another thing to consider is, since this bit is same for both PF/VF, is it worth to add this bit it to struct pci_dev?or struct pci_sriov is the more appropriate place? > >>> I did include the caching patches for both PRI and PASID capabilities, >>> but they're only performance optimizations so I moved them to the end >>> so the functional fixes would be smaller and earlier in the series. >>> >>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >>>>>> --- >>>>>> drivers/pci/ats.c | 10 ++++++---- >>>>>> include/linux/pci-ats.h | 12 +++++++++--- >>>>>> 2 files changed, 15 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c >>>>>> index e18499243f84..cdd936d10f68 100644 >>>>>> --- a/drivers/pci/ats.c >>>>>> +++ b/drivers/pci/ats.c >>>>>> @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev) >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(pci_pasid_features); >>>>>> +#ifdef CONFIG_PCI_PRI >>>>>> + >>>>>> /** >>>>>> * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit >>>>>> * status. >>>>>> @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features); >>>>>> * >>>>>> * Returns 1 if PASID is required in PRG Response Message, 0 otherwise. >>>>>> * >>>>>> - * Even though the PRG response PASID status is read from PRI Status >>>>>> - * Register, since this API will mainly be used by PASID users, this >>>>>> - * function is defined within #ifdef CONFIG_PCI_PASID instead of >>>>>> - * CONFIG_PCI_PRI. >>>>>> + * Since this API has dependency on both PRI and PASID, protect it >>>>>> + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID. >>>>>> */ >>>>>> int pci_prg_resp_pasid_required(struct pci_dev *pdev) >>>>>> { >>>>>> @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev) >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); >>>>>> +#endif >>>>>> + >>>>>> #define PASID_NUMBER_SHIFT 8 >>>>>> #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT) >>>>>> /** >>>>>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h >>>>>> index 1ebb88e7c184..1a0bdaee2f32 100644 >>>>>> --- a/include/linux/pci-ats.h >>>>>> +++ b/include/linux/pci-ats.h >>>>>> @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev); >>>>>> void pci_restore_pasid_state(struct pci_dev *pdev); >>>>>> int pci_pasid_features(struct pci_dev *pdev); >>>>>> int pci_max_pasids(struct pci_dev *pdev); >>>>>> -int pci_prg_resp_pasid_required(struct pci_dev *pdev); >>>>>> #else /* CONFIG_PCI_PASID */ >>>>>> @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev) >>>>>> return -EINVAL; >>>>>> } >>>>>> +#endif /* CONFIG_PCI_PASID */ >>>>>> + >>>>>> +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID) >>>>>> + >>>>>> +int pci_prg_resp_pasid_required(struct pci_dev *pdev); >>>>>> + >>>>>> +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */ >>>>>> + >>>>>> static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev) >>>>>> { >>>>>> return 0; >>>>>> } >>>>>> -#endif /* CONFIG_PCI_PASID */ >>>>>> - >>>>>> +#endif >>>>>> #endif /* LINUX_PCI_ATS_H*/ >>>>>> -- >>>>>> 2.21.0 >>>>>> >>>> -- >>>> Sathyanarayanan Kuppuswamy >>>> Linux kernel developer >>>> >> -- >> Sathyanarayanan Kuppuswamy >> Linux kernel developer
On Thu, Oct 03, 2019 at 02:11:28PM -0700, Kuppuswamy Sathyanarayanan wrote: > > On 10/3/19 2:01 PM, Bjorn Helgaas wrote: > > On Thu, Oct 03, 2019 at 01:37:26PM -0700, Kuppuswamy Sathyanarayanan wrote: > > > On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote: > > > > On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote: > > > > > Hi Bjorn, > > > > > > > > > > Thanks for looking into this patch set. > > > > > > > > > > On 9/5/19 12:18 PM, Bjorn Helgaas wrote: > > > > > > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > > > > > > > > > Since pci_prg_resp_pasid_required() function has dependency on both > > > > > > > PASID and PRI, define it only if both CONFIG_PCI_PRI and > > > > > > > CONFIG_PCI_PASID config options are enabled. > > > > > > > > > > > > > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required() > > > > > > > interface.") > > > > > > [Don't split tags, including "Fixes:" across lines] > > > > > > > > > > > > This definitely doesn't fix e5567f5f6762. That commit added > > > > > > pci_prg_resp_pasid_required(), but with no dependency on > > > > > > CONFIG_PCI_PRI or CONFIG_PCI_PASID. > > > > > > > > > > > > This patch is only required when a subsequent patch is applied. It > > > > > > should be squashed into the commit that requires it so it's obvious > > > > > > why it's needed. > > > > > > > > > > > > I've been poking at this series, and I'll post a v8 soon with this and > > > > > > other fixes. > > > > > In your v8 submission you did not merge this patch. You did not use > > > > > pri_cap or pasid_cap cached values. Instead you have re-read the > > > > > value from register. Is this intentional? > > > > > > > > > > Since this function will be called for every VF device we might loose some > > > > > performance benefit. > > > > This particular patch doesn't do any caching. IIRC it fiddles with > > > > ifdefs to solve a problem that would be introduced by a future patch. > > > > I don't remember the exact details, but I think the series I merged > > > > doesn't have that problem. If it does, let me know the details and we > > > > can fix it. > > > This patch by itself does not do any caching. But your caching patch > > > missed modifying this function to use cached values. Please check the > > > current implementation of this function. It still reads > > > PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let > > > me know your comments. > > > > > > int pci_prg_resp_pasid_required(struct pci_dev *pdev) > > > { > > > u16 status; > > > int pri; > > > > > > if (pdev->is_virtfn) > > > pdev = pci_physfn(pdev); > > > > > > pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI); > > > if (!pri) > > > return 0; > > > > > > pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status); > > > > > > if (status & PCI_PRI_STATUS_PASID) > > > return 1; > > > > > > return 0; > > > } > > > EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); > > > > > > If caching is applied to this function then we need this #ifdef > > > dependency correction patch. > > IIRC this #ifdef patch wasn't connected to the actual *need* for the > > #ifdef, so it was very difficult to review. I thought this function > > would be infrequently used and it wasn't worth trying to sort out the > > #ifdef muddle to do the caching. But it does seem sort of pointless > > to chase the capability list again here, so maybe it *is* worth > > optimizing. > > > > The PRG Response PASID Required bit is read-only, so I wonder if it > > would be simpler if we just read PCI_PRI_STATUS once and save the bit > > in the struct pci_dev? We could do that in pci_enable_pri(), or if we > > might need the value before that's called, we could add a > > pci_pri_init() and do it there. > > Yes, caching PASID Required bit in pci_pri_init() function would > provide performance benefits. But another thing to consider is, > since this bit is same for both PF/VF, is it worth to add this bit > it to struct pci_dev?or struct pci_sriov is the more appropriate > place? IIUC, the PRI capability is not specific to SR-IOV, so I don't think it would make sense to cache PRG Response PASID Required in pci_sriov. PFs may implement PRI; VFs do not (PCIe r5.0, sec 10.5.2), so I think the bit should be cached in the pci_dev, and if we want to know the value for a VF, we should read it from the PF's pci_dev. Bjorn
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index e18499243f84..cdd936d10f68 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev) } EXPORT_SYMBOL_GPL(pci_pasid_features); +#ifdef CONFIG_PCI_PRI + /** * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit * status. @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features); * * Returns 1 if PASID is required in PRG Response Message, 0 otherwise. * - * Even though the PRG response PASID status is read from PRI Status - * Register, since this API will mainly be used by PASID users, this - * function is defined within #ifdef CONFIG_PCI_PASID instead of - * CONFIG_PCI_PRI. + * Since this API has dependency on both PRI and PASID, protect it + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID. */ int pci_prg_resp_pasid_required(struct pci_dev *pdev) { @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev) } EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required); +#endif + #define PASID_NUMBER_SHIFT 8 #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT) /** diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h index 1ebb88e7c184..1a0bdaee2f32 100644 --- a/include/linux/pci-ats.h +++ b/include/linux/pci-ats.h @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev); void pci_restore_pasid_state(struct pci_dev *pdev); int pci_pasid_features(struct pci_dev *pdev); int pci_max_pasids(struct pci_dev *pdev); -int pci_prg_resp_pasid_required(struct pci_dev *pdev); #else /* CONFIG_PCI_PASID */ @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev) return -EINVAL; } +#endif /* CONFIG_PCI_PASID */ + +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID) + +int pci_prg_resp_pasid_required(struct pci_dev *pdev); + +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */ + static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev) { return 0; } -#endif /* CONFIG_PCI_PASID */ - +#endif #endif /* LINUX_PCI_ATS_H*/