| Message ID | bb2b61719e4dd15c29bc8022e76a9463d548f561.1392996332.git.agordeev@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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. 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 > -- 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, 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 > >
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@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. 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@redhat.com -- 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/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) + 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) + 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; }
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(-)