Message ID | 20150721001511.28145.29144.stgit@bhelgaas-glaptop2.roam.corp.google.com (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Bjorn Helgaas |
Headers | show |
On Mon, Jul 20, 2015 at 07:15:11PM -0500, Bjorn Helgaas wrote: > Stop caching the Invalidate Queue Depth in struct pci_dev. > pci_ats_queue_depth() is typically called only once per device, and it > returns a fixed value per-device, so callers who need the value frequently > can cache it themselves. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/ats.c | 9 ++++----- > include/linux/pci.h | 1 - > 2 files changed, 4 insertions(+), 6 deletions(-) Reviewed-by: Joerg Roedel <jroedel@suse.de> -- 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 07/20/2015 08:15 PM, Bjorn Helgaas wrote: > Stop caching the Invalidate Queue Depth in struct pci_dev. > pci_ats_queue_depth() is typically called only once per device, and it > returns a fixed value per-device, so callers who need the value frequently > can cache it themselves. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/ats.c | 9 ++++----- > include/linux/pci.h | 1 - > 2 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index 67524a7..bdb1383 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -20,16 +20,12 @@ > void pci_ats_init(struct pci_dev *dev) > { > int pos; > - u16 cap; > > pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS); > if (!pos) > return; > > dev->ats_cap = pos; > - pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); > - dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : > - PCI_ATS_MAX_QDEP; > } > > /** > @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state); > */ > int pci_ats_queue_depth(struct pci_dev *dev) > { > + u16 cap; > + > if (!dev->ats_cap) > return -EINVAL; > > if (dev->is_virtfn) > return 0; > hmmm, isn't one of the fixes in this patch set to change the caching of ats queue depth ? won't above make it 0 for all VF's ? is that correct, or desired (virtual) value? > - return dev->ats_qdep; > + pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); > + return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP; > } > EXPORT_SYMBOL_GPL(pci_ats_queue_depth); > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 307f96a..4b484fd 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -378,7 +378,6 @@ struct pci_dev { > }; > u16 ats_cap; /* ATS Capability offset */ > u8 ats_stu; /* ATS Smallest Translation Unit */ > - u8 ats_qdep; /* ATS Invalidate Queue Depth */ > atomic_t ats_ref_cnt; /* number of VFs with ATS enabled */ > #endif > phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */ > > -- > 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 Don, On Mon, Jul 27, 2015 at 10:00:53AM -0400, Don Dutile wrote: > On 07/20/2015 08:15 PM, Bjorn Helgaas wrote: > >Stop caching the Invalidate Queue Depth in struct pci_dev. > >pci_ats_queue_depth() is typically called only once per device, and it > >returns a fixed value per-device, so callers who need the value frequently > >can cache it themselves. > > > >Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > >--- > > drivers/pci/ats.c | 9 ++++----- > > include/linux/pci.h | 1 - > > 2 files changed, 4 insertions(+), 6 deletions(-) > > > >diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > >index 67524a7..bdb1383 100644 > >--- a/drivers/pci/ats.c > >+++ b/drivers/pci/ats.c > >@@ -20,16 +20,12 @@ > > void pci_ats_init(struct pci_dev *dev) > > { > > int pos; > >- u16 cap; > > > > pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS); > > if (!pos) > > return; > > > > dev->ats_cap = pos; > >- pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); > >- dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : > >- PCI_ATS_MAX_QDEP; > > } > > > > /** > >@@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state); > > */ > > int pci_ats_queue_depth(struct pci_dev *dev) > > { > >+ u16 cap; > >+ > > if (!dev->ats_cap) > > return -EINVAL; > > > > if (dev->is_virtfn) > > return 0; > > > hmmm, isn't one of the fixes in this patch set to > change the caching of ats queue depth ? won't above > make it 0 for all VF's ? is that correct, or desired (virtual) value? Per spec (SR-IOV r1.1., sec 3.7.4), the ATS queue depth register on a VF always contains zero. Here's the v4.1 code: int pci_ats_queue_depth(struct pci_dev *dev) { if (dev->is_virtfn) return 0; if (dev->ats) return dev->ats->qdep; ... In v4.1, pci_ats_queue_depth() always returned 0 for a VF. For VFs, we didn't look at the dev->ats->qdep cache. So I don't think this changes anything for the caller. A previous patch changed this path so we return -EINVAL instead of 0 for VFs that don't support ATS. I think the previous behavior there was wrong, but I doubt anybody noticed. Bjorn > >- return dev->ats_qdep; > >+ pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); > >+ return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP; > > } > > EXPORT_SYMBOL_GPL(pci_ats_queue_depth); > > > >diff --git a/include/linux/pci.h b/include/linux/pci.h > >index 307f96a..4b484fd 100644 > >--- a/include/linux/pci.h > >+++ b/include/linux/pci.h > >@@ -378,7 +378,6 @@ struct pci_dev { > > }; > > u16 ats_cap; /* ATS Capability offset */ > > u8 ats_stu; /* ATS Smallest Translation Unit */ > >- u8 ats_qdep; /* ATS Invalidate Queue Depth */ > > atomic_t ats_ref_cnt; /* number of VFs with ATS enabled */ > > #endif > > phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */ > > > >-- > >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 07/27/2015 06:27 PM, Bjorn Helgaas wrote: > Hi Don, > > On Mon, Jul 27, 2015 at 10:00:53AM -0400, Don Dutile wrote: >> On 07/20/2015 08:15 PM, Bjorn Helgaas wrote: >>> Stop caching the Invalidate Queue Depth in struct pci_dev. >>> pci_ats_queue_depth() is typically called only once per device, and it >>> returns a fixed value per-device, so callers who need the value frequently >>> can cache it themselves. >>> >>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >>> --- >>> drivers/pci/ats.c | 9 ++++----- >>> include/linux/pci.h | 1 - >>> 2 files changed, 4 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c >>> index 67524a7..bdb1383 100644 >>> --- a/drivers/pci/ats.c >>> +++ b/drivers/pci/ats.c >>> @@ -20,16 +20,12 @@ >>> void pci_ats_init(struct pci_dev *dev) >>> { >>> int pos; >>> - u16 cap; >>> >>> pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS); >>> if (!pos) >>> return; >>> >>> dev->ats_cap = pos; >>> - pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); >>> - dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : >>> - PCI_ATS_MAX_QDEP; >>> } >>> >>> /** >>> @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state); >>> */ >>> int pci_ats_queue_depth(struct pci_dev *dev) >>> { >>> + u16 cap; >>> + >>> if (!dev->ats_cap) >>> return -EINVAL; >>> >>> if (dev->is_virtfn) >>> return 0; >>> >> hmmm, isn't one of the fixes in this patch set to >> change the caching of ats queue depth ? won't above >> make it 0 for all VF's ? is that correct, or desired (virtual) value? > > Per spec (SR-IOV r1.1., sec 3.7.4), the ATS queue depth register on a VF > always contains zero. > > Here's the v4.1 code: > > int pci_ats_queue_depth(struct pci_dev *dev) > { > if (dev->is_virtfn) > return 0; > > if (dev->ats) > return dev->ats->qdep; > > ... > > > In v4.1, pci_ats_queue_depth() always returned 0 for a VF. For VFs, we > didn't look at the dev->ats->qdep cache. So I don't think this changes > anything for the caller. > > A previous patch changed this path so we return -EINVAL instead of 0 for > VFs that don't support ATS. I think the previous behavior there was wrong, > but I doubt anybody noticed. > > Bjorn > ok. thanks for the sanity check. >>> - return dev->ats_qdep; >>> + pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); >>> + return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP; >>> } >>> EXPORT_SYMBOL_GPL(pci_ats_queue_depth); >>> >>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>> index 307f96a..4b484fd 100644 >>> --- a/include/linux/pci.h >>> +++ b/include/linux/pci.h >>> @@ -378,7 +378,6 @@ struct pci_dev { >>> }; >>> u16 ats_cap; /* ATS Capability offset */ >>> u8 ats_stu; /* ATS Smallest Translation Unit */ >>> - u8 ats_qdep; /* ATS Invalidate Queue Depth */ >>> atomic_t ats_ref_cnt; /* number of VFs with ATS enabled */ >>> #endif >>> phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */ >>> >>> -- >>> 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/ats.c b/drivers/pci/ats.c index 67524a7..bdb1383 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -20,16 +20,12 @@ void pci_ats_init(struct pci_dev *dev) { int pos; - u16 cap; pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS); if (!pos) return; dev->ats_cap = pos; - pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); - dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : - PCI_ATS_MAX_QDEP; } /** @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state); */ int pci_ats_queue_depth(struct pci_dev *dev) { + u16 cap; + if (!dev->ats_cap) return -EINVAL; if (dev->is_virtfn) return 0; - return dev->ats_qdep; + pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); + return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP; } EXPORT_SYMBOL_GPL(pci_ats_queue_depth); diff --git a/include/linux/pci.h b/include/linux/pci.h index 307f96a..4b484fd 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -378,7 +378,6 @@ struct pci_dev { }; u16 ats_cap; /* ATS Capability offset */ u8 ats_stu; /* ATS Smallest Translation Unit */ - u8 ats_qdep; /* ATS Invalidate Queue Depth */ atomic_t ats_ref_cnt; /* number of VFs with ATS enabled */ #endif phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
Stop caching the Invalidate Queue Depth in struct pci_dev. pci_ats_queue_depth() is typically called only once per device, and it returns a fixed value per-device, so callers who need the value frequently can cache it themselves. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/ats.c | 9 ++++----- include/linux/pci.h | 1 - 2 files changed, 4 insertions(+), 6 deletions(-) -- 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