lkml.org 
[lkml]   [2014]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 3/4] ntb: Split ntb_setup_msix() into separate BWD/SNB routines
    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@redhat.com>
    > > Cc: Jon Mason <jon.mason@intel.com>
    > > Cc: linux-pci@vger.kernel.org
    > > ---
    > > 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.

    >
    > 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@redhat.com


    \
     
     \ /
      Last update: 2014-03-04 18:21    [W:4.473 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site