| 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: <20140310211828.GA30986@jonmason-lab>
Date: Mon, 10 Mar 2014 14:18:29 -0700
From: Jon Mason <jon.mason@...el.com>
To: Alexander Gordeev <agordeev@...hat.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 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@...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.
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@...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