As result of recent deprecation of MSI-X/MSI enablement
interfaces pci_enable_msi_block(), pci_enable_msi() and
pci_enable_msix() all drivers need to be updated to use
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.
This is the first in a series of updates, to phase out
pci_enable_msi_block() function.
This series is against pci/msi branch in Bjorn Helgaas's repo:
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
Thanks!
Alexander Gordeev (7):
ipr: Do not call pci_disable_msi/msix() if pci_enable_msi/msix()
failed
ipr: Use new interfaces for MSI/MSI-X enablement
ahci: Use new interfaces for MSI/MSI-X enablement
nvme: Use new interfaces for MSI/MSI-X enablement
vfio: Use new interfaces for MSI/MSI-X enablement
ath10k: Use new interfaces for MSI enablement
wil6210: Use new interfaces for MSI enablement
drivers/ata/ahci.c | 15 +++-----
drivers/block/nvme-core.c | 33 ++++-------------
drivers/net/wireless/ath/ath10k/pci.c | 22 ++++++------
drivers/net/wireless/ath/wil6210/pcie_bus.c | 36 ++++++++++---------
drivers/scsi/ipr.c | 51 +++++++++-----------------
drivers/vfio/pci/vfio_pci_intrs.c | 8 ++--
6 files changed, 66 insertions(+), 99 deletions(-)
--
1.7.7.6
On Tue, 2014-01-07 at 19:05 +0100, Alexander Gordeev wrote:
> This update also fixes a bug when deprecated pci_enable_msix()
> and pci_enable_msi_block() functions return a positive return
> value which indicats the number of interrupts that could have
> been allocated rather than a successful allocation. The driver
> misinterpreted this value and assumed MSI-X/MSIs are enabled,
> although in fact it were not.
No, the driver interpreted it correctly, which is why anything other
than zero is handled as an error. This patch looks incorrect if the new
interfaces follow the same return convention. Thanks,
Alex
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 641bc87..66d1746 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
> for (i = 0; i < nvec; i++)
> vdev->msix[i].entry = i;
>
> - ret = pci_enable_msix(pdev, vdev->msix, nvec);
> - if (ret) {
> + ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
> + if (ret < 0) {
> kfree(vdev->msix);
> kfree(vdev->ctx);
> return ret;
> }
> } else {
> - ret = pci_enable_msi_block(pdev, nvec);
> - if (ret) {
> + ret = pci_enable_msi_range(pdev, nvec, nvec);
> + if (ret < 0) {
> kfree(vdev->ctx);
> return ret;
> }
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/scsi/ipr.c | 8 ++------
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 36ac1c3..fb57e21 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -9255,10 +9255,8 @@ static int ipr_enable_msix(struct ipr_ioa_cfg *ioa_cfg)
while ((err = pci_enable_msix(ioa_cfg->pdev, entries, vectors)) > 0)
vectors = err;
- if (err < 0) {
- pci_disable_msix(ioa_cfg->pdev);
+ if (err < 0)
return err;
- }
if (!err) {
for (i = 0; i < vectors; i++)
@@ -9278,10 +9276,8 @@ static int ipr_enable_msi(struct ipr_ioa_cfg *ioa_cfg)
while ((err = pci_enable_msi_block(ioa_cfg->pdev, vectors)) > 0)
vectors = err;
- if (err < 0) {
- pci_disable_msi(ioa_cfg->pdev);
+ if (err < 0)
return err;
- }
if (!err) {
for (i = 0; i < vectors; i++)
--
1.7.7.6
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/block/nvme-core.c | 33 ++++++++-------------------------
1 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 26d03fa..adf26c2 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1774,32 +1774,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
/* Deregister the admin queue's interrupt */
free_irq(dev->entry[0].vector, dev->queues[0]);
- vecs = nr_io_queues;
- for (i = 0; i < vecs; i++)
+ for (i = 0; i < nr_io_queues; i++)
dev->entry[i].entry = i;
- for (;;) {
- result = pci_enable_msix(pdev, dev->entry, vecs);
- if (result <= 0)
- break;
- vecs = result;
- }
-
- if (result < 0) {
- vecs = nr_io_queues;
- if (vecs > 32)
- vecs = 32;
- for (;;) {
- result = pci_enable_msi_block(pdev, vecs);
- if (result == 0) {
- for (i = 0; i < vecs; i++)
- dev->entry[i].vector = i + pdev->irq;
- break;
- } else if (result < 0) {
- vecs = 1;
- break;
- }
- vecs = result;
- }
+ vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
+ if (vecs < 0) {
+ vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
+ if (vecs < 0)
+ vecs = 1;
+ for (i = 0; i < vecs; i++)
+ dev->entry[i].vector = i + pdev->irq;
}
/*
--
1.7.7.6
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/scsi/ipr.c | 47 ++++++++++++++++++-----------------------------
1 files changed, 18 insertions(+), 29 deletions(-)
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index fb57e21..3841298 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -9245,47 +9245,36 @@ ipr_get_chip_info(const struct pci_device_id *dev_id)
static int ipr_enable_msix(struct ipr_ioa_cfg *ioa_cfg)
{
struct msix_entry entries[IPR_MAX_MSIX_VECTORS];
- int i, err, vectors;
+ int i, vectors;
for (i = 0; i < ARRAY_SIZE(entries); ++i)
entries[i].entry = i;
- vectors = ipr_number_of_msix;
+ vectors = pci_enable_msix_range(ioa_cfg->pdev, entries,
+ 1, ipr_number_of_msix);
+ if (vectors < 0)
+ return vectors;
- while ((err = pci_enable_msix(ioa_cfg->pdev, entries, vectors)) > 0)
- vectors = err;
+ for (i = 0; i < vectors; i++)
+ ioa_cfg->vectors_info[i].vec = entries[i].vector;
+ ioa_cfg->nvectors = vectors;
- if (err < 0)
- return err;
-
- if (!err) {
- for (i = 0; i < vectors; i++)
- ioa_cfg->vectors_info[i].vec = entries[i].vector;
- ioa_cfg->nvectors = vectors;
- }
-
- return err;
+ return 0;
}
static int ipr_enable_msi(struct ipr_ioa_cfg *ioa_cfg)
{
- int i, err, vectors;
+ int i, vectors;
- vectors = ipr_number_of_msix;
+ vectors = pci_enable_msi_range(ioa_cfg->pdev, 1, ipr_number_of_msix);
+ if (vectors < 0)
+ return vectors;
- while ((err = pci_enable_msi_block(ioa_cfg->pdev, vectors)) > 0)
- vectors = err;
+ for (i = 0; i < vectors; i++)
+ ioa_cfg->vectors_info[i].vec = ioa_cfg->pdev->irq + i;
+ ioa_cfg->nvectors = vectors;
- if (err < 0)
- return err;
-
- if (!err) {
- for (i = 0; i < vectors; i++)
- ioa_cfg->vectors_info[i].vec = ioa_cfg->pdev->irq + i;
- ioa_cfg->nvectors = vectors;
- }
-
- return err;
+ return 0;
}
static void name_msi_vectors(struct ipr_ioa_cfg *ioa_cfg)
@@ -9350,7 +9339,7 @@ static irqreturn_t ipr_test_intr(int irq, void *devp)
* ipr_test_msi - Test for Message Signaled Interrupt (MSI) support.
* @pdev: PCI device struct
*
- * Description: The return value from pci_enable_msi() can not always be
+ * Description: The return value from pci_enable_msi_range() can not always be
* trusted. This routine sets up and initiates a test interrupt to determine
* if the interrupt is received via the ipr_test_intr() service routine.
* If the tests fails, the driver will fall back to LSI.
--
1.7.7.6
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/ata/ahci.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 8516f4d..cfdb079 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1098,13 +1098,13 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
struct ahci_host_priv *hpriv)
{
- int rc, nvec;
+ int nvec;
if (hpriv->flags & AHCI_HFLAG_NO_MSI)
goto intx;
- rc = pci_msi_vec_count(pdev);
- if (rc < 0)
+ nvec = pci_msi_vec_count(pdev);
+ if (nvec < 0)
goto intx;
/*
@@ -1112,19 +1112,16 @@ int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
* Message mode could be enforced. In this case assume that advantage
* of multipe MSIs is negated and use single MSI mode instead.
*/
- if (rc < n_ports)
+ if (nvec < n_ports)
goto single_msi;
- nvec = rc;
- rc = pci_enable_msi_block(pdev, nvec);
- if (rc)
+ if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
goto intx;
return nvec;
single_msi:
- rc = pci_enable_msi(pdev);
- if (rc)
+ if (pci_enable_msi_range(pdev, 1, 1) < 0)
goto intx;
return 1;
--
1.7.7.6
This update also fixes a stylistic (naming and messaging only)
confusion of MSI-X vs multiple MSIs which are not the same.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 9e86a81..08807fe 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2073,14 +2073,14 @@ static void ath10k_pci_tasklet(unsigned long data)
}
}
-static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
+static int ath10k_pci_start_intr_multi_msi(struct ath10k *ar, int num)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;
int i;
- ret = pci_enable_msi_block(ar_pci->pdev, num);
- if (ret)
+ ret = pci_enable_msi_range(ar_pci->pdev, num, num);
+ if (ret < 0)
return ret;
ret = request_irq(ar_pci->pdev->irq + MSI_ASSIGN_FW,
@@ -2111,16 +2111,16 @@ static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
}
}
- ath10k_info("MSI-X interrupt handling (%d intrs)\n", num);
+ ath10k_info("Multi MSI interrupt handling (%d intrs)\n", num);
return 0;
}
-static int ath10k_pci_start_intr_msi(struct ath10k *ar)
+static int ath10k_pci_start_intr_single_msi(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;
- ret = pci_enable_msi(ar_pci->pdev);
+ ret = pci_enable_msi_range(ar_pci->pdev, 1, 1);
if (ret < 0)
return ret;
@@ -2132,7 +2132,7 @@ static int ath10k_pci_start_intr_msi(struct ath10k *ar)
return ret;
}
- ath10k_info("MSI interrupt handling\n");
+ ath10k_info("Single MSI interrupt handling\n");
return 0;
}
@@ -2199,20 +2199,20 @@ static int ath10k_pci_start_intr(struct ath10k *ar)
num = 1;
if (num > 1) {
- ret = ath10k_pci_start_intr_msix(ar, num);
+ ret = ath10k_pci_start_intr_multi_msi(ar, num);
if (ret == 0)
goto exit;
- ath10k_warn("MSI-X didn't succeed (%d), trying MSI\n", ret);
+ ath10k_warn("Multi MSI failed (%d), trying single MSI\n", ret);
num = 1;
}
if (num == 1) {
- ret = ath10k_pci_start_intr_msi(ar);
+ ret = ath10k_pci_start_intr_single_msi(ar);
if (ret == 0)
goto exit;
- ath10k_warn("MSI didn't succeed (%d), trying legacy INTR\n",
+ ath10k_warn("Single MSI failed (%d), trying legacy INTR\n",
ret);
num = 0;
}
--
1.7.7.6
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/net/wireless/ath/wil6210/pcie_bus.c | 36 ++++++++++++++------------
1 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index eeceab3..022dfe4 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -41,30 +41,32 @@ static int wil_if_pcie_enable(struct wil6210_priv *wil)
switch (use_msi) {
case 3:
case 1:
+ wil_dbg_misc(wil, "Setup %d MSI interrupts\n", use_msi);
+ break;
case 0:
+ wil_dbg_misc(wil, "MSI interrupts disabled, use INTx\n");
break;
default:
- wil_err(wil, "Invalid use_msi=%d, default to 1\n",
- use_msi);
+ wil_err(wil, "Invalid use_msi=%d, default to 1\n", use_msi);
use_msi = 1;
}
- wil->n_msi = use_msi;
- if (wil->n_msi) {
- wil_dbg_misc(wil, "Setup %d MSI interrupts\n", use_msi);
- rc = pci_enable_msi_block(pdev, wil->n_msi);
- if (rc && (wil->n_msi == 3)) {
- wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
- wil->n_msi = 1;
- rc = pci_enable_msi_block(pdev, wil->n_msi);
- }
- if (rc) {
- wil_err(wil, "pci_enable_msi failed, use INTx\n");
- wil->n_msi = 0;
- }
- } else {
- wil_dbg_misc(wil, "MSI interrupts disabled, use INTx\n");
+
+ switch (use_msi) {
+ case 3:
+ if (pci_enable_msi_range(pdev, 3, 3) > 0)
+ break;
+ wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
+ use_msi = 1;
+ /* fallthrough */
+ case 1:
+ if (pci_enable_msi_range(pdev, 1, 1) > 0)
+ break;
+ wil_err(wil, "pci_enable_msi_range failed, use INTx\n");
+ use_msi = 0;
}
+ wil->n_msi = use_msi;
+
rc = wil6210_init_irq(wil, pdev->irq);
if (rc)
goto stop_master;
--
1.7.7.6
This update also fixes a bug when deprecated pci_enable_msix()
and pci_enable_msi_block() functions return a positive return
value which indicats the number of interrupts that could have
been allocated rather than a successful allocation. The driver
misinterpreted this value and assumed MSI-X/MSIs are enabled,
although in fact it were not.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/vfio/pci/vfio_pci_intrs.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 641bc87..66d1746 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
for (i = 0; i < nvec; i++)
vdev->msix[i].entry = i;
- ret = pci_enable_msix(pdev, vdev->msix, nvec);
- if (ret) {
+ ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
+ if (ret < 0) {
kfree(vdev->msix);
kfree(vdev->ctx);
return ret;
}
} else {
- ret = pci_enable_msi_block(pdev, nvec);
- if (ret) {
+ ret = pci_enable_msi_range(pdev, nvec, nvec);
+ if (ret < 0) {
kfree(vdev->ctx);
return ret;
}
--
1.7.7.6
On Tue, Jan 07, 2014 at 11:34:13AM -0700, Alex Williamson wrote:
> On Tue, 2014-01-07 at 19:05 +0100, Alexander Gordeev wrote:
> > This update also fixes a bug when deprecated pci_enable_msix()
> > and pci_enable_msi_block() functions return a positive return
> > value which indicats the number of interrupts that could have
> > been allocated rather than a successful allocation. The driver
> > misinterpreted this value and assumed MSI-X/MSIs are enabled,
> > although in fact it were not.
>
> No, the driver interpreted it correctly, which is why anything other
> than zero is handled as an error. This patch looks incorrect if the new
> interfaces follow the same return convention. Thanks,
The new interfaces differ wrt the return value - it is eigher a negative
error code or a positive number of successfuly allocated vectors.
If the user level makes use of a number of vectors that could have been
allocated then it should cease doing it, since only 0 or a negative error
code is returned after this update.
The changelog is incorrect as the driver indeed bailes out on positive
return values. I will send a updated version.
> Alex
--
Regards,
Alexander Gordeev
[email protected]
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/vfio/pci/vfio_pci_intrs.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 641bc87..66d1746 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
for (i = 0; i < nvec; i++)
vdev->msix[i].entry = i;
- ret = pci_enable_msix(pdev, vdev->msix, nvec);
- if (ret) {
+ ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
+ if (ret < 0) {
kfree(vdev->msix);
kfree(vdev->ctx);
return ret;
}
} else {
- ret = pci_enable_msi_block(pdev, nvec);
- if (ret) {
+ ret = pci_enable_msi_range(pdev, nvec, nvec);
+ if (ret < 0) {
kfree(vdev->ctx);
return ret;
}
--
1.7.7.6
--
Regards,
Alexander Gordeev
[email protected]
Alexander Gordeev <[email protected]> writes:
> This update also fixes a stylistic (naming and messaging only)
> confusion of MSI-X vs multiple MSIs which are not the same.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
Looks good to me.
Acked-by: Kalle Valo <[email protected]>
Do you want me to take this patch to my ath.git tree or how were you
planning to handle it?
--
Kalle Valo
On Wed, Jan 08, 2014 at 10:23:18AM +0200, Kalle Valo wrote:
> Looks good to me.
>
> Acked-by: Kalle Valo <[email protected]>
Thanks, Kalle.
> Do you want me to take this patch to my ath.git tree or how were you
> planning to handle it?
I see no option other than pushing it thru pci.git tree at this stage.
> --
> Kalle Valo
--
Regards,
Alexander Gordeev
[email protected]
On Tuesday, January 07, 2014 07:05:42 PM Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/net/wireless/ath/wil6210/pcie_bus.c | 36 ++++++++++++++------------
> 1 files changed, 19 insertions(+), 17 deletions(-)
>
Patch looks fine, but I can't validate it as I can't find patch introducing
pci_enable_msi_range(). Where this patch landed on?
I am working with:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.git
I also checked
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
and, of course
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
Thanks, Vladimir
On Wed, Jan 08, 2014 at 01:30:45PM +0200, Vladimir Kondratiev wrote:
> On Tuesday, January 07, 2014 07:05:42 PM Alexander Gordeev wrote:
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > drivers/net/wireless/ath/wil6210/pcie_bus.c | 36 ++++++++++++++------------
> > 1 files changed, 19 insertions(+), 17 deletions(-)
> >
>
> Patch looks fine, but I can't validate it as I can't find patch introducing
> pci_enable_msi_range(). Where this patch landed on?
Vladimir,
This series is against pci/msi branch in Bjorn Helgaas's repo:
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
Commit 302a252 "PCI/MSI: Add pci_enable_msi_range() and pci_enable_msix_range()"
> I am working with:
> git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.git
>
> I also checked
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> and, of course
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>
> Thanks, Vladimir
--
Regards,
Alexander Gordeev
[email protected]
On Wednesday, January 08, 2014 12:54:01 PM Alexander Gordeev wrote:
> Vladimir,
>
> This series is against pci/msi branch in Bjorn Helgaas's repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
>
> Commit 302a252 "PCI/MSI: Add pci_enable_msi_range() and pci_enable_msix_range()"
>
Thanks, see it. New code don't distinguish between negative and positive error
values, same as old code. I'll fix it.
Meanwhile, below my
Acked-by: Vladimir Kondratiev <[email protected]>
On Wed, Jan 08, 2014 at 02:19:01PM +0200, Vladimir Kondratiev wrote:
> Thanks, see it. New code don't distinguish between negative and positive error
> values, same as old code. I'll fix it.
As the patch seems okay for you, I am not quite getting what else needs
to be fixed? :)
> Meanwhile, below my
>
> Acked-by: Vladimir Kondratiev <[email protected]>
Thanks, Vladimir!
--
Regards,
Alexander Gordeev
[email protected]
Alexander Gordeev <[email protected]> writes:
> On Wed, Jan 08, 2014 at 10:23:18AM +0200, Kalle Valo wrote:
>
>> Do you want me to take this patch to my ath.git tree or how were you
>> planning to handle it?
>
> I see no option other than pushing it thru pci.git tree at this stage.
Thanks, I'll then just drop it from my queue.
--
Kalle Valo
On Wed, 2014-01-08 at 08:57 +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 641bc87..66d1746 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
> for (i = 0; i < nvec; i++)
> vdev->msix[i].entry = i;
>
> - ret = pci_enable_msix(pdev, vdev->msix, nvec);
> - if (ret) {
> + ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
> + if (ret < 0) {
> kfree(vdev->msix);
> kfree(vdev->ctx);
> return ret;
> }
> } else {
> - ret = pci_enable_msi_block(pdev, nvec);
> - if (ret) {
> + ret = pci_enable_msi_range(pdev, nvec, nvec);
> + if (ret < 0) {
> kfree(vdev->ctx);
> return ret;
> }
Based on your description, this is a user visible API change. We now
return success so long as we allocated at least a single vector and the
user has no way to know that they didn't get all the vectors they
requested. That's unacceptable, userspace expects the old API - setup
the requested vectors or setup none and tell me how many to retry with.
To maintain the same API exposed to userspace, I'd think we need
something like:
if (ret != nvec) {
if (ret > 0)
pci_disable...
kfree(...
kfree(...
return ret;
}
Thanks,
Alex
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/vfio/pci/vfio_pci_intrs.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 641bc87..4a9db1d 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -482,15 +482,19 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
for (i = 0; i < nvec; i++)
vdev->msix[i].entry = i;
- ret = pci_enable_msix(pdev, vdev->msix, nvec);
- if (ret) {
+ ret = pci_enable_msix_range(pdev, vdev->msix, 1, nvec);
+ if (ret < nvec) {
+ if (ret > 0)
+ pci_disable_msix(pdev);
kfree(vdev->msix);
kfree(vdev->ctx);
return ret;
}
} else {
- ret = pci_enable_msi_block(pdev, nvec);
- if (ret) {
+ ret = pci_enable_msi_range(pdev, 1, nvec);
+ if (ret < nvec) {
+ if (ret > 0)
+ pci_disable_msi(pdev);
kfree(vdev->ctx);
return ret;
}
--
1.7.7.6
--
Regards,
Alexander Gordeev
[email protected]
On Fri, 2014-01-10 at 08:42 +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <[email protected]>
Thanks for the changes.
Acked-by: Alex Williamson <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 641bc87..4a9db1d 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -482,15 +482,19 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
> for (i = 0; i < nvec; i++)
> vdev->msix[i].entry = i;
>
> - ret = pci_enable_msix(pdev, vdev->msix, nvec);
> - if (ret) {
> + ret = pci_enable_msix_range(pdev, vdev->msix, 1, nvec);
> + if (ret < nvec) {
> + if (ret > 0)
> + pci_disable_msix(pdev);
> kfree(vdev->msix);
> kfree(vdev->ctx);
> return ret;
> }
> } else {
> - ret = pci_enable_msi_block(pdev, nvec);
> - if (ret) {
> + ret = pci_enable_msi_range(pdev, 1, nvec);
> + if (ret < nvec) {
> + if (ret > 0)
> + pci_disable_msi(pdev);
> kfree(vdev->ctx);
> return ret;
> }
> --
> 1.7.7.6
>
On Tue, Jan 07, 2014 at 07:05:38PM +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/ata/ahci.c | 15 ++++++---------
> 1 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 8516f4d..cfdb079 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1098,13 +1098,13 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
> int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> struct ahci_host_priv *hpriv)
> {
> - int rc, nvec;
> + int nvec;
>
> if (hpriv->flags & AHCI_HFLAG_NO_MSI)
> goto intx;
>
> - rc = pci_msi_vec_count(pdev);
> - if (rc < 0)
> + nvec = pci_msi_vec_count(pdev);
> + if (nvec < 0)
> goto intx;
>
> /*
> @@ -1112,19 +1112,16 @@ int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> * Message mode could be enforced. In this case assume that advantage
> * of multipe MSIs is negated and use single MSI mode instead.
> */
> - if (rc < n_ports)
> + if (nvec < n_ports)
> goto single_msi;
>
> - nvec = rc;
> - rc = pci_enable_msi_block(pdev, nvec);
> - if (rc)
> + if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
> goto intx;
>
> return nvec;
>
> single_msi:
> - rc = pci_enable_msi(pdev);
> - if (rc)
> + if (pci_enable_msi_range(pdev, 1, 1) < 0)
This part doesn't seem like an improvement. There are a hundred or so
callers of pci_enable_msi() that only want a single MSI. Is there any
benefit in changing them to use pci_enable_msi_range()?
I guess I agreed (maybe even suggested) to deprecate pci_enable_msi(),
but it doesn't suffer from the tri-state return value problem, and I'm
having second thoughts.
> goto intx;
> return 1;
>
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 13, 2014 at 12:12:20PM -0700, Bjorn Helgaas wrote:
> > - nvec = rc;
> > - rc = pci_enable_msi_block(pdev, nvec);
> > - if (rc)
> > + if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
> > goto intx;
> >
> > return nvec;
> >
> > single_msi:
> > - rc = pci_enable_msi(pdev);
> > - if (rc)
> > + if (pci_enable_msi_range(pdev, 1, 1) < 0)
>
> This part doesn't seem like an improvement. There are a hundred or so
> callers of pci_enable_msi() that only want a single MSI. Is there any
> benefit in changing them to use pci_enable_msi_range()?
In this particular case it reads better to me as one sees on the screen
pci_enable_msi_range(pdev, nvec, nvec) and pci_enable_msi_range(pdev, 1, 1)
calls. That allows to avoid switching in mind between negative-or-positive
return in the former call and negative-or-zero return from pci_enable_msi()
if we had it.
But in most cases when single MSI is enabled we do cause complication
with the patterns below (which I expect I am going be hated for ;) ):
- rc = pci_enable_msi(pdev);
- if (rc)
+ rc = pci_enable_msi_range(pdev, 1, 1);
+ if (rc < 0)
...
- rc = pci_enable_msi(pdev);
- if (!rc)
+ rc = pci_enable_msi_range(pdev, 1, 1);
+ if (rc > 0)
...
I think we have a tradeoff between the interface simplicity and code clarity.
What if we try to address both goals by making pci_enable_msi() a helper over
pci_enable_msi_range(pdev, 1, 1)? In this case the return value will match
negative-or-positive binary semantics while reads almost as good as it used to:
- rc = pci_enable_msi(pdev);
- if (rc)
+ rc = pci_enable_msi(pdev);
+ if (rc < 0)
...
- rc = pci_enable_msi(pdev);
- if (!rc)
+ rc = pci_enable_msi(pdev);
+ if (rc > 0)
...
The whole interface would not be inflated as well, with just:
diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index a8d0100..fa0b27d 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -158,6 +158,9 @@ static int foo_driver_enable_single_msi(struct pci_dev *pdev)
return pci_enable_msi_range(pdev, 1, 1);
}
+A helper function pci_enable_msi() could be used instead. Note, as just
+one MSI is requested it could return either a negative errno or 1.
+
4.2.2 pci_disable_msi
void pci_disable_msi(struct pci_dev *dev)
--
Regards,
Alexander Gordeev
[email protected]
On Tue, Jan 14, 2014 at 09:50:40AM +0100, Alexander Gordeev wrote:
> What if we try to address both goals by making pci_enable_msi() a helper over
> pci_enable_msi_range(pdev, 1, 1)?
Or pci_enable_msi_single() to help reading and avoid drivers conversion mess.
--
Regards,
Alexander Gordeev
[email protected]
On Tue, Jan 14, 2014 at 1:50 AM, Alexander Gordeev <[email protected]> wrote:
> On Mon, Jan 13, 2014 at 12:12:20PM -0700, Bjorn Helgaas wrote:
>> > - nvec = rc;
>> > - rc = pci_enable_msi_block(pdev, nvec);
>> > - if (rc)
>> > + if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
>> > goto intx;
>> >
>> > return nvec;
>> >
>> > single_msi:
>> > - rc = pci_enable_msi(pdev);
>> > - if (rc)
>> > + if (pci_enable_msi_range(pdev, 1, 1) < 0)
>>
>> This part doesn't seem like an improvement. There are a hundred or so
>> callers of pci_enable_msi() that only want a single MSI. Is there any
>> benefit in changing them to use pci_enable_msi_range()?
>
> In this particular case it reads better to me as one sees on the screen
> pci_enable_msi_range(pdev, nvec, nvec) and pci_enable_msi_range(pdev, 1, 1)
> calls. That allows to avoid switching in mind between negative-or-positive
> return in the former call and negative-or-zero return from pci_enable_msi()
> if we had it.
>
> But in most cases when single MSI is enabled we do cause complication
> with the patterns below (which I expect I am going be hated for ;) ):
I don't want to touch those hundred pci_enable_msi() callers at all.
So I think we should have something like this:
/* Return 0 for success (one MSI enabled) or negative errno */
static inline int pci_enable_msi(struct pci_dev *dev)
{
int rc;
rc = pci_enable_msi_range(pdev, 1, 1);
if (rc == 1)
return 0;
return rc;
}