Hi Jon,
This is an attempt to make ntb_setup_msix() more readable ( to me :) )
The idea is to get rid of confusing branching between SNB and BWD
and split MSI-X ininialization into ntb_setup_bwd_msix() and
ntb_setup_snb_msix() - both small and straight.
Cc: Jon Mason <[email protected]>
Cc: [email protected]
Alexander Gordeev (4):
ntb: Fix leakage of ntb_device::msix_entries[] array
ntb: Use pci_msix_vec_count() to obtain number of MSI-Xs
ntb: Split ntb_setup_msix() into separate BWD/SNB routines
ntb: Use pci_enable_msix_range() instead of pci_enable_msix()
drivers/ntb/ntb_hw.c | 174 +++++++++++++++++++++++++++++---------------------
drivers/ntb/ntb_hw.h | 2 -
2 files changed, 100 insertions(+), 76 deletions(-)
--
1.7.7.6
Signed-off-by: Alexander Gordeev <[email protected]>
Cc: Jon Mason <[email protected]>
Cc: [email protected]
---
drivers/ntb/ntb_hw.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index 170e8e6..487169dd 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1281,6 +1281,7 @@ static void ntb_free_interrupts(struct ntb_device *ndev)
free_irq(msix->vector, &ndev->db_cb[i]);
}
pci_disable_msix(pdev);
+ kfree(ndev->msix_entries);
} else {
free_irq(pdev->irq, ndev);
--
1.7.7.6
As result of deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() or pci_enable_msi_exact()
and pci_enable_msix_range() or pci_enable_msix_exact()
interfaces.
Signed-off-by: Alexander Gordeev <[email protected]>
Cc: Jon Mason <[email protected]>
Cc: [email protected]
---
drivers/ntb/ntb_hw.c | 20 +++++---------------
1 files changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index 0472045..9f2005d 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1088,11 +1088,9 @@ static int ntb_setup_snb_msix(struct ntb_device *ndev, int msix_entries)
if (msix_entries < SNB_MSIX_CNT)
return -ENOSPC;
- rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
+ rc = pci_enable_msix_exact(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];
@@ -1142,18 +1140,10 @@ static int ntb_setup_bwd_msix(struct ntb_device *ndev, int msix_entries)
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;
- }
+ msix_entries = pci_enable_msix_range(pdev, ndev->msix_entries,
+ 1, msix_entries);
+ if (msix_entries < 0)
+ return msix_entries;
for (i = 0; i < msix_entries; i++) {
msix = &ndev->msix_entries[i];
--
1.7.7.6
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 <[email protected]>
Cc: Jon Mason <[email protected]>
Cc: [email protected]
---
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)
+ 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;
}
--
1.7.7.6
Signed-off-by: Alexander Gordeev <[email protected]>
Cc: Jon Mason <[email protected]>
Cc: [email protected]
---
drivers/ntb/ntb_hw.c | 15 ++++-----------
drivers/ntb/ntb_hw.h | 2 --
2 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index 487169dd..53468f4 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1085,19 +1085,12 @@ static int ntb_setup_msix(struct ntb_device *ndev)
struct msix_entry *msix;
int msix_entries;
int rc, i;
- u16 val;
- if (!pdev->msix_cap) {
- rc = -EIO;
- goto err;
- }
-
- rc = pci_read_config_word(pdev, pdev->msix_cap + PCI_MSIX_FLAGS, &val);
- if (rc)
+ msix_entries = pci_msix_vec_count(pdev);
+ if (msix_entries < 0) {
+ rc = msix_entries;
goto err;
-
- msix_entries = msix_table_size(val);
- if (msix_entries > ndev->limits.msix_cnt) {
+ } else if (msix_entries > ndev->limits.msix_cnt) {
rc = -EINVAL;
goto err;
}
diff --git a/drivers/ntb/ntb_hw.h b/drivers/ntb/ntb_hw.h
index bbdb7ed..d307107 100644
--- a/drivers/ntb/ntb_hw.h
+++ b/drivers/ntb/ntb_hw.h
@@ -60,8 +60,6 @@
#define PCI_DEVICE_ID_INTEL_NTB_SS_HSX 0x2F0F
#define PCI_DEVICE_ID_INTEL_NTB_B2B_BWD 0x0C4E
-#define msix_table_size(control) ((control & PCI_MSIX_FLAGS_QSIZE)+1)
-
#ifndef readq
static inline u64 readq(void __iomem *addr)
{
--
1.7.7.6
On Fri, Feb 21, 2014 at 04:49:28PM +0100, Alexander Gordeev wrote:
> Hi Jon,
>
> This is an attempt to make ntb_setup_msix() more readable ( to me :) )
>
> The idea is to get rid of confusing branching between SNB and BWD
> and split MSI-X ininialization into ntb_setup_bwd_msix() and
> ntb_setup_snb_msix() - both small and straight.
Ouch, I was too fast. Patch #3 introduces warning "unused variable ‘msix’"
I can re-post if you want, but the rest is still is up to review ;)
Thanks!
--
Regards,
Alexander Gordeev
[email protected]
On Fri, Feb 21, 2014 at 04:49:29PM +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <[email protected]>
> Cc: Jon Mason <[email protected]>
> Cc: [email protected]
Good catch. Applied.
> ---
> drivers/ntb/ntb_hw.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
> index 170e8e6..487169dd 100644
> --- a/drivers/ntb/ntb_hw.c
> +++ b/drivers/ntb/ntb_hw.c
> @@ -1281,6 +1281,7 @@ static void ntb_free_interrupts(struct ntb_device *ndev)
> free_irq(msix->vector, &ndev->db_cb[i]);
> }
> pci_disable_msix(pdev);
> + kfree(ndev->msix_entries);
> } else {
> free_irq(pdev->irq, ndev);
>
> --
> 1.7.7.6
>
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 <[email protected]>
Cc: Jon Mason <[email protected]>
Cc: [email protected]
---
drivers/ntb/ntb_hw.c | 171 +++++++++++++++++++++++++++++++-------------------
1 files changed, 106 insertions(+), 65 deletions(-)
diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index 53468f4..679d4d0 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1079,10 +1079,110 @@ static irqreturn_t ntb_interrupt(int irq, void *dev)
return IRQ_HANDLED;
}
-static int ntb_setup_msix(struct ntb_device *ndev)
+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;
int msix_entries;
int rc, i;
@@ -1105,78 +1205,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
[email protected]
On Fri, Feb 21, 2014 at 05:09:37PM +0100, Alexander Gordeev wrote:
> Ouch, I was too fast. Patch #3 introduces warning "unused variable ‘msix’"
> I can re-post if you want, but the rest is still is up to review ;)
Resent.
--
Regards,
Alexander Gordeev
[email protected]
On Fri, Feb 21, 2014 at 04:33:25PM -0700, Jon Mason wrote:
> On Fri, Feb 21, 2014 at 04:49:29PM +0100, Alexander Gordeev wrote:
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > Cc: Jon Mason <[email protected]>
> > Cc: [email protected]
>
> Good catch. Applied.
Hi Jon,
If the other three patches are okay?
--
Regards,
Alexander Gordeev
[email protected]