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-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html