| Message ID | 20150717213240.18379.42110.stgit@bhelgaas-glaptop2.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Jul 17, 2015 at 04:32:40PM -0500, Bjorn Helgaas wrote: > There's no need to BUG() if we enable ATS when it's already enabled. We > don't need to BUG() when disabling ATS on a device that doesn't support ATS > or if it's already disabled. If ATS is enabled, certainly we found an ATS > capability in the past, so it should still be there now. > > Clean up these error paths. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/ats.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index c35de8e..9069126 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -43,8 +43,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps) > { > u16 ctrl; > > - BUG_ON(dev->ats_cap && dev->ats_enabled); > - > if (!dev->ats_cap) > return -EINVAL; Hmm, what happens if pci_enable_ats is called twice for the same device? Can we change the STU without disabling ATS first, for example? The function should probably return if it finds ATS already enabled (or at least WARN when it is enabled and ps != last ps value). Joerg -- 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 Mon, Jul 20, 2015 at 04:24:00PM +0200, Joerg Roedel wrote: > On Fri, Jul 17, 2015 at 04:32:40PM -0500, Bjorn Helgaas wrote: > > There's no need to BUG() if we enable ATS when it's already enabled. We > > don't need to BUG() when disabling ATS on a device that doesn't support ATS > > or if it's already disabled. If ATS is enabled, certainly we found an ATS > > capability in the past, so it should still be there now. > > > > Clean up these error paths. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > drivers/pci/ats.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > > index c35de8e..9069126 100644 > > --- a/drivers/pci/ats.c > > +++ b/drivers/pci/ats.c > > @@ -43,8 +43,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps) > > { > > u16 ctrl; > > > > - BUG_ON(dev->ats_cap && dev->ats_enabled); > > - > > if (!dev->ats_cap) > > return -EINVAL; > > Hmm, what happens if pci_enable_ats is called twice for the same > device? Can we change the STU without disabling ATS first, for example? > > The function should probably return if it finds ATS already enabled (or > at least WARN when it is enabled and ps != last ps value). It would probably be nicest if: - enabling PF fails if ATS is already enabled - enabling VF fails if (PF ATS is disabled || PS != PF PS) - disabling PF fails if ATS is enabled on any VF I added the first (which we basically had before in the BUG_ON()), and I already had the second. The third is where it gets messy because we have either add a refcount and associated locking, or we have to iterate through all the VFs. I did consider both of those but thought it was a lot of work for the way it's currently used: it looks like ATS is enabled for every device when it is enumerated, with a fixed PS, and never changed. But I guess a refcount with atomic_inc() probably wouldn't be too hard. Let me poke at that. Bjorn -- 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 c35de8e..9069126 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -43,8 +43,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps) { u16 ctrl; - BUG_ON(dev->ats_cap && dev->ats_enabled); - if (!dev->ats_cap) return -EINVAL; @@ -74,7 +72,8 @@ void pci_disable_ats(struct pci_dev *dev) { u16 ctrl; - BUG_ON(!dev->ats_cap || !dev->ats_enabled); + if (!dev->ats_cap) + return; pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, &ctrl); ctrl &= ~PCI_ATS_CTRL_ENABLE; @@ -90,8 +89,6 @@ void pci_restore_ats_state(struct pci_dev *dev) if (!pci_ats_enabled(dev)) return; - if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS)) - BUG(); ctrl = PCI_ATS_CTRL_ENABLE; if (!dev->is_virtfn)
There's no need to BUG() if we enable ATS when it's already enabled. We don't need to BUG() when disabling ATS on a device that doesn't support ATS or if it's already disabled. If ATS is enabled, certainly we found an ATS capability in the past, so it should still be there now. Clean up these error paths. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/ats.c | 7 ++----- 1 file changed, 2 insertions(+), 5 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