Re: [PATCH v2 3/4] ntb: Split ntb_setup_msix() into separate BWD/SNB routines

[Jon Mason]

On Tue, Mar 04, 2014 at 05:41:28PM +0100, Alexander Gordeev wrote:
> On Mon, Mar 03, 2014 at 05:12:25PM -0700, Jon Mason wrote:
> > On Fri, Feb 21, 2014 at 04:49:31PM +0100, Alexander Gordeev wrote:
> > > This is an cleanup effort to make ntb_setup_msix() more
> > > readable - use ntb_setup_bwd_msix() to init MSI-Xs on
> > > BWD hardware and ntb_setup_snb_msix() - on SNB hardware.
> > > 
> > > Function ntb_setup_snb_msix() also initializes MSI-Xs the
> > > way it should has been done - looping pci_enable_msix()
> > > until success or failure.
> > > 
> > > Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> > > Cc: Jon Mason <jon.mason@xxxxxxxxx>
> > > Cc: linux-pci@xxxxxxxxxxxxxxx
> > > ---
> > >  drivers/ntb/ntb_hw.c |  170 +++++++++++++++++++++++++++++++-------------------
> > >  1 files changed, 106 insertions(+), 64 deletions(-)
> > > 
> > > diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
> > > index 53468f4..0472045 100644
> > > --- a/drivers/ntb/ntb_hw.c
> > > +++ b/drivers/ntb/ntb_hw.c
> > > @@ -1079,6 +1079,107 @@ static irqreturn_t ntb_interrupt(int irq, void *dev)
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > +static int ntb_setup_snb_msix(struct ntb_device *ndev, int msix_entries)
> > > +{
> > > +	struct pci_dev *pdev = ndev->pdev;
> > > +	struct msix_entry *msix;
> > > +	int rc, i;
> > > +
> > > +	if (msix_entries < SNB_MSIX_CNT)
> > 
> > I would prefer ndev->limits.msix_cnt here to keep it generic.  I could
> > foresee this number changing on upcoming hardware with a similar
> > design.
> > 
> > > +		return -ENOSPC;
> > > +
> > > +	rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
> > > +	if (rc < 0)
> > > +		return rc;
> > > +	else if (rc > 0)
> > > +		return -ENOSPC;
> > > +
> > > +	for (i = 0; i < msix_entries; i++) {
> > > +		msix = &ndev->msix_entries[i];
> > > +		WARN_ON(!msix->vector);
> > > +
> > > +		if (i == msix_entries - 1) {
> > > +			rc = request_irq(msix->vector,
> > > +					 xeon_event_msix_irq, 0,
> > > +					 "ntb-event-msix", ndev);
> > > +			if (rc)
> > > +				goto err;
> > > +		} else {
> > > +			rc = request_irq(msix->vector,
> > > +					 xeon_callback_msix_irq, 0,
> > > +					 "ntb-callback-msix",
> > > +					 &ndev->db_cb[i]);
> > > +			if (rc)
> > > +				goto err;
> > > +		}
> > > +	}
> > > +
> > > +	ndev->num_msix = msix_entries;
> > > +	ndev->max_cbs = msix_entries - 1;
> > > +
> > > +	return 0;
> > > +
> > > +err:
> > > +	while (--i >= 0) {
> > > +		msix = &ndev->msix_entries[i];
> > > +		WARN_ON(i == ndev->num_msix - 1);
> > > +
> > > +		if (i == ndev->num_msix - 1)
> > 
> > Why have a WARN followed by a if statement on the same condition?  As
> > it stands right now, there is no way it could be hit.  So I'd say
> > remove both of them.
> 
> Right, WARN_ON() is stupid here. But we can not remove the condition,
> because of 'dev_id' parameter for free_irq() is 'ndev' for the last
> MSI-X, unlike &ndev->db_cb[i] for all other MSI-Xs, unless I am
> missing something.

The "i == ndev->num_msix - 1" cannot be hit.  If a failure hits in
request_irq with i < msix_entries - 1, then there is no need to free
the irq for xeon_event_msix_irq allocation because it was never
allocated.  If i == msix_entries - 1 when it fails, there is no need
to free the irq because the allocation failed.  These 2 cases being
true, it's never possible to hit the case and the clean-up code is
unnecessary.

Thanks,
Jon

> 
> > 
> > Aside from that it looks good.  Thanks for taking the time to cleanup
> > the code!
> > 
> > Thanks,
> > Jon
> > 
> > > +			free_irq(msix->vector, ndev);
> > > +		else
> > > +			free_irq(msix->vector, &ndev->db_cb[i]);
> > > +	}
> > > +
> > > +	pci_disable_msix(pdev);
> > > +	ndev->num_msix = 0;
> > > +
> > > +	return rc;
> > > +}
> > > +
> > > +static int ntb_setup_bwd_msix(struct ntb_device *ndev, int msix_entries)
> > > +{
> > > +	struct pci_dev *pdev = ndev->pdev;
> > > +	struct msix_entry *msix;
> > > +	int rc, i;
> > > +
> > > +retry:
> > > +	rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
> > > +	if (rc < 0)
> > > +		return rc;
> > > +	else if (rc > 0) {
> > > +		dev_warn(&pdev->dev,
> > > +			 "Only %d MSI-X vectors. "
> > > +			 "Limiting the number of queues to that number.\n",
> > > +			 rc);
> > > +		msix_entries = rc;
> > > +		goto retry;
> > > +	}
> > > +
> > > +	for (i = 0; i < msix_entries; i++) {
> > > +		msix = &ndev->msix_entries[i];
> > > +		WARN_ON(!msix->vector);
> > > +
> > > +		rc = request_irq(msix->vector, bwd_callback_msix_irq, 0,
> > > +				 "ntb-callback-msix", &ndev->db_cb[i]);
> > > +		if (rc)
> > > +			goto err;
> > > +	}
> > > +
> > > +	ndev->num_msix = msix_entries;
> > > +	ndev->max_cbs = msix_entries;
> > > +
> > > +	return 0;
> > > +
> > > +err:
> > > +	while (--i >= 0)
> > > +		free_irq(msix->vector, &ndev->db_cb[i]);
> > > +
> > > +	pci_disable_msix(pdev);
> > > +	ndev->num_msix = 0;
> > > +
> > > +	return rc;
> > > +}
> > > +
> > >  static int ntb_setup_msix(struct ntb_device *ndev)
> > >  {
> > >  	struct pci_dev *pdev = ndev->pdev;
> > > @@ -1105,78 +1206,19 @@ static int ntb_setup_msix(struct ntb_device *ndev)
> > >  	for (i = 0; i < msix_entries; i++)
> > >  		ndev->msix_entries[i].entry = i;
> > >  
> > > -	rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
> > > -	if (rc < 0)
> > > -		goto err1;
> > > -	if (rc > 0) {
> > > -		/* On SNB, the link interrupt is always tied to 4th vector.  If
> > > -		 * we can't get all 4, then we can't use MSI-X.
> > > -		 */
> > > -		if (ndev->hw_type != BWD_HW) {
> > > -			rc = -EIO;
> > > -			goto err1;
> > > -		}
> > > -
> > > -		dev_warn(&pdev->dev,
> > > -			 "Only %d MSI-X vectors.  Limiting the number of queues to that number.\n",
> > > -			 rc);
> > > -		msix_entries = rc;
> > > -
> > > -		rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
> > > -		if (rc)
> > > -			goto err1;
> > > -	}
> > > -
> > > -	for (i = 0; i < msix_entries; i++) {
> > > -		msix = &ndev->msix_entries[i];
> > > -		WARN_ON(!msix->vector);
> > > -
> > > -		/* Use the last MSI-X vector for Link status */
> > > -		if (ndev->hw_type == BWD_HW) {
> > > -			rc = request_irq(msix->vector, bwd_callback_msix_irq, 0,
> > > -					 "ntb-callback-msix", &ndev->db_cb[i]);
> > > -			if (rc)
> > > -				goto err2;
> > > -		} else {
> > > -			if (i == msix_entries - 1) {
> > > -				rc = request_irq(msix->vector,
> > > -						 xeon_event_msix_irq, 0,
> > > -						 "ntb-event-msix", ndev);
> > > -				if (rc)
> > > -					goto err2;
> > > -			} else {
> > > -				rc = request_irq(msix->vector,
> > > -						 xeon_callback_msix_irq, 0,
> > > -						 "ntb-callback-msix",
> > > -						 &ndev->db_cb[i]);
> > > -				if (rc)
> > > -					goto err2;
> > > -			}
> > > -		}
> > > -	}
> > > -
> > > -	ndev->num_msix = msix_entries;
> > >  	if (ndev->hw_type == BWD_HW)
> > > -		ndev->max_cbs = msix_entries;
> > > +		rc = ntb_setup_bwd_msix(ndev, msix_entries);
> > >  	else
> > > -		ndev->max_cbs = msix_entries - 1;
> > > +		rc = ntb_setup_snb_msix(ndev, msix_entries);
> > > +	if (rc)
> > > +		goto err1;
> > >  
> > >  	return 0;
> > >  
> > > -err2:
> > > -	while (--i >= 0) {
> > > -		msix = &ndev->msix_entries[i];
> > > -		if (ndev->hw_type != BWD_HW && i == ndev->num_msix - 1)
> > > -			free_irq(msix->vector, ndev);
> > > -		else
> > > -			free_irq(msix->vector, &ndev->db_cb[i]);
> > > -	}
> > > -	pci_disable_msix(pdev);
> > >  err1:
> > >  	kfree(ndev->msix_entries);
> > > -	dev_err(&pdev->dev, "Error allocating MSI-X interrupt\n");
> > >  err:
> > > -	ndev->num_msix = 0;
> > > +	dev_err(&pdev->dev, "Error allocating MSI-X interrupt\n");
> > >  	return rc;
> > >  }
> > >  
> > > -- 
> > > 1.7.7.6
> > > 
> 
> -- 
> Regards,
> Alexander Gordeev
> agordeev@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/