Re: [PATCH 1/8] PCI: Allocate ATS struct during enumeration

[Bjorn Helgaas <bhelgaas@google.com>]

On Mon, Jul 20, 2015 at 03:55:16PM +0200, Joerg Roedel wrote:
> Hi Bjorn,
> 
> On Fri, Jul 17, 2015 at 04:31:52PM -0500, Bjorn Helgaas wrote:
> > -static int ats_alloc_one(struct pci_dev *dev, int ps)
> > +static void ats_alloc_one(struct pci_dev *dev)
> >  {
> >  	int pos;
> >  	u16 cap;
> > @@ -25,20 +25,17 @@ static int ats_alloc_one(struct pci_dev *dev, int ps)
> >  
> >  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
> >  	if (!pos)
> > -		return -ENODEV;
> > +		return;
> >  
> >  	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
> >  	if (!ats)
> > -		return -ENOMEM;
> > +		return;
> 
> I think we should print a warning here when the allocation fails.
> Otherwise the user wonders why ATS can't be used despite the available
> capability.

Good idea, done (although it gets removed right away in the next patch :))
> 
> > -	if (dev->is_physfn || dev->is_virtfn) {
> > -		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
> > -
> > -		mutex_lock(&pdev->sriov->lock);
> > -		if (pdev->ats)
> > -			rc = pdev->ats->stu == ps ? 0 : -EINVAL;
> > -		else
> > -			rc = ats_alloc_one(pdev, ps);
> > +	if (ps < PCI_ATS_MIN_STU)
> > +		return -EINVAL;
> >  
> > -		if (!rc)
> > -			pdev->ats->ref_cnt++;
> > -		mutex_unlock(&pdev->sriov->lock);
> > -		if (rc)
> > -			return rc;
> > -	}
> > +	ctrl = PCI_ATS_CTRL_ENABLE;
> > +	if (dev->is_virtfn) {
> > +		struct pci_dev *pdev = dev->physfn;
> >  
> > -	if (!dev->is_physfn) {
> > -		rc = ats_alloc_one(dev, ps);
> > -		if (rc)
> > -			return rc;
> > +		if (pdev->ats->stu != ps)
> > +			return -EINVAL;
> > +	} else {
> > +		dev->ats->stu = ps;
> > +		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
> >  	}
> 
> Okay, enabling ATS for the virt_fn will now fail when it is not
> already enabled for the phys_fn. The drivers probe function, which might
> enable SR-IOV, runs after BUS_NOTIFY_ADD_DEVICE has finished, so this
> should be safe.
> 
> But I think a comment explaining these dependencies would be good here.

Comment added, thanks!

Bjorn