Re: [PATCH 7/8] PCI: Clean up ATS error handling

[Bjorn Helgaas <bhelgaas@google.com>]

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