| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140304164127.GA18971@dhcp-26-207.brq.redhat.com>
Date: Tue, 4 Mar 2014 17:41:28 +0100
From: Alexander Gordeev <agordeev@...hat.com>
To: Jon Mason <jon.mason@...el.com>
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [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@...hat.com>
> > Cc: Jon Mason <jon.mason@...el.com>
> > Cc: linux-pci@...r.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@...hat.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists