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