[PATCH v2 0/9] Phase out pci_enable_msi_block()

[PATCH v2 0/9] Phase out pci_enable_msi_block()

[Alexander Gordeev]

This series is against "next" branch in Bjorn's repo:
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git

Changes from v1 to v2:
  - added a regression fix "ahci: Fix broken fallback to single
    MSI mode" as patch 1/9;
  - the series is reordered to move the regression fix in front;
  - at Bjorn's request pci_enable_msi() is un-deprecated;
  - as result, pci_enable_msi_range(pdev, 1, 1) styled calls
    rolled back to pci_enable_msi(pdev);
  - nvme bug fix moved out as a separate patch 5/9 "nvme: Fix
    invalid call to irq_set_affinity_hint()"
  - patches changelog elaborated a bit;

Bjorn,

As the release is supposedly this weekend, do you prefer
the patches to go to your tree or to individual trees after
the release?

Thanks!

Alexander Gordeev (9):
  ahci: Fix broken fallback to single MSI mode
  ahci: Use pci_enable_msi_range()
  ipr: Get rid of superfluous call to pci_disable_msi/msix()
  ipr: Use pci_enable_msi_range() and pci_enable_msix_range()
  nvme: Fix invalid call to irq_set_affinity_hint()
  nvme: Use pci_enable_msi_range() and pci_enable_msix_range()
  vfio: Use pci_enable_msi_range() and pci_enable_msix_range()
  ath10k: Use pci_enable_msi_range()
  wil6210: Use pci_enable_msi_range()

 drivers/ata/ahci.c                          |   18 +++++-----
 drivers/block/nvme-core.c                   |   33 ++++-------------
 drivers/net/wireless/ath/ath10k/pci.c       |   20 +++++-----
 drivers/net/wireless/ath/wil6210/pcie_bus.c |   36 ++++++++++---------
 drivers/scsi/ipr.c                          |   51 +++++++++-----------------
 drivers/vfio/pci/vfio_pci_intrs.c           |   12 ++++--
 6 files changed, 72 insertions(+), 98 deletions(-)

-- 
1.7.7.6

[PATCH v2 8/9] ath10k: Use pci_enable_msi_range()

[Alexander Gordeev]

As result 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() and pci_enable_msix_range()
interfaces.

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 <agordeev@redhat.com>
Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 9e86a81..873f50c 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,11 +2111,11 @@ 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;
@@ -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

[PATCH v2 9/9] wil6210: Use pci_enable_msi_range()

[Alexander Gordeev]

As result 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() and pci_enable_msix_range()
interfaces.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Acked-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 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..4e1bf54 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(pdev))
+			break;
+		wil_err(wil, "pci_enable_msi 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

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 9:02 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> This series is against "next" branch in Bjorn's repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
>
> Changes from v1 to v2:
>   - added a regression fix "ahci: Fix broken fallback to single
>     MSI mode" as patch 1/9;
>   - the series is reordered to move the regression fix in front;
>   - at Bjorn's request pci_enable_msi() is un-deprecated;
>   - as result, pci_enable_msi_range(pdev, 1, 1) styled calls
>     rolled back to pci_enable_msi(pdev);
>   - nvme bug fix moved out as a separate patch 5/9 "nvme: Fix
>     invalid call to irq_set_affinity_hint()"
>   - patches changelog elaborated a bit;
>
> Bjorn,
>
> As the release is supposedly this weekend, do you prefer
> the patches to go to your tree or to individual trees after
> the release?

I'd be happy to merge them, except for the fact that they probably
wouldn't have any time in -next before I ask Linus to pull them.  So
how about if we wait until after the release, ask the area maintainers
to take them, and if they don't take them, I'll put them in my tree
for v3.15?

Bjorn

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Alexander Gordeev]

On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> > As the release is supposedly this weekend, do you prefer
> > the patches to go to your tree or to individual trees after
> > the release?
> 
> I'd be happy to merge them, except for the fact that they probably
> wouldn't have any time in -next before I ask Linus to pull them.  So
> how about if we wait until after the release, ask the area maintainers
> to take them, and if they don't take them, I'll put them in my tree
> for v3.15?

Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
Whatever works for you ;)

I am only concerned with a regression fix "ahci: Fix broken fallback to
single MSI mode" which would be nice to have in 3.14. But it seems pretty
much too late.

> Bjorn

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Sat, Jan 18, 2014 at 12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
>> > As the release is supposedly this weekend, do you prefer
>> > the patches to go to your tree or to individual trees after
>> > the release?
>>
>> I'd be happy to merge them, except for the fact that they probably
>> wouldn't have any time in -next before I ask Linus to pull them.  So
>> how about if we wait until after the release, ask the area maintainers
>> to take them, and if they don't take them, I'll put them in my tree
>> for v3.15?
>
> Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
> Whatever works for you ;)
>
> I am only concerned with a regression fix "ahci: Fix broken fallback to
> single MSI mode" which would be nice to have in 3.14. But it seems pretty
> much too late.

Tejun, if you want to ack that one, I can put it in either the first
3.14 pull request or a subsequent one.  Either way, since it's a
regression fix, we should be able to get it in 3.14.

Bjorn

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Tejun Heo]

On Sat, Jan 18, 2014 at 07:38:55AM -0700, Bjorn Helgaas wrote:
> On Sat, Jan 18, 2014 at 12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> >> > As the release is supposedly this weekend, do you prefer
> >> > the patches to go to your tree or to individual trees after
> >> > the release?
> >>
> >> I'd be happy to merge them, except for the fact that they probably
> >> wouldn't have any time in -next before I ask Linus to pull them.  So
> >> how about if we wait until after the release, ask the area maintainers
> >> to take them, and if they don't take them, I'll put them in my tree
> >> for v3.15?
> >
> > Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
> > Whatever works for you ;)
> >
> > I am only concerned with a regression fix "ahci: Fix broken fallback to
> > single MSI mode" which would be nice to have in 3.14. But it seems pretty
> > much too late.
> 
> Tejun, if you want to ack that one, I can put it in either the first
> 3.14 pull request or a subsequent one.  Either way, since it's a
> regression fix, we should be able to get it in 3.14.

Acked-by: Tejun Heo <tj@kernel.org>

Please feel free to route it any way you see fit.

Thanks!

-- 
tejun

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Alexander Gordeev]

On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> > Bjorn,
> >
> > As the release is supposedly this weekend, do you prefer
> > the patches to go to your tree or to individual trees after
> > the release?
> 
> I'd be happy to merge them, except for the fact that they probably
> wouldn't have any time in -next before I ask Linus to pull them.  So
> how about if we wait until after the release, ask the area maintainers
> to take them, and if they don't take them, I'll put them in my tree
> for v3.15?

Hi Gentleman,

As the prerequisite commit 302a252 ("PCI/MSI: Add pci_enable_msi_range()
and pci_enable_msix_range()") is in mainline now, could you please take
the ACKed patches to your trees?

Thanks!

> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 9/9] wil6210: Use pci_enable_msi_range()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 05:02:23PM +0100, Alexander Gordeev wrote:
> As result 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() and pci_enable_msix_range()
> interfaces.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Acked-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>

Applied to my pci/msi branch, thanks!

> ---
>  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..4e1bf54 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(pdev))
> +			break;
> +		wil_err(wil, "pci_enable_msi 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Sat, Jan 18, 2014 at 09:59:40AM -0500, Tejun Heo wrote:
> On Sat, Jan 18, 2014 at 07:38:55AM -0700, Bjorn Helgaas wrote:
> > On Sat, Jan 18, 2014 at 12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > > On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> > >> > As the release is supposedly this weekend, do you prefer
> > >> > the patches to go to your tree or to individual trees after
> > >> > the release?
> > >>
> > >> I'd be happy to merge them, except for the fact that they probably
> > >> wouldn't have any time in -next before I ask Linus to pull them.  So
> > >> how about if we wait until after the release, ask the area maintainers
> > >> to take them, and if they don't take them, I'll put them in my tree
> > >> for v3.15?
> > >
> > > Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
> > > Whatever works for you ;)
> > >
> > > I am only concerned with a regression fix "ahci: Fix broken fallback to
> > > single MSI mode" which would be nice to have in 3.14. But it seems pretty
> > > much too late.
> > 
> > Tejun, if you want to ack that one, I can put it in either the first
> > 3.14 pull request or a subsequent one.  Either way, since it's a
> > regression fix, we should be able to get it in 3.14.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Please feel free to route it any way you see fit.

I applied the following to my pci/msi branch, since they had acks from
maintainers (Tejun, I assumed your ack applies to both ahci patches):

    ahci: Fix broken fallback to single MSI mode
    ahci: Use pci_enable_msi_range()
    vfio: Use pci_enable_msi_range() and pci_enable_msix_range()
    wil6210: Use pci_enable_msi_range()

I didn't do anything with these:

    ipr: Get rid of superfluous call to pci_disable_msi/msix()
    ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

	The conflict with "ipr: Handle early EEH" needs to get resolved
	first.  Either Alexander's patches need to go via the same tree as
	the EEH change, or the EEH change needs to be in some published
	tree so I can cherry-pick it.

    nvme: Fix invalid call to irq_set_affinity_hint()
    nvme: Use pci_enable_msi_range() and pci_enable_msix_range()

	These don't seem fully baked yet.  If/when Keith acks them, I (or
	he) can merge them.

    ath10k: Use pci_enable_msi_range()

	This has been acked, but no longer applies to mainline (I'm
	currently at 0e47c969c65e).

Bjorn

[PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 29fd197..6525e1f 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2414,8 +2414,6 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
 		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
 		if (ret == 0)
 			return 0;
-		if (ret > 0)
-			pci_disable_msi(ar_pci->pdev);
 
 		/* fall-through */
 	}
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 2/3] ath10k: Disable MSI in case IRQ configuration is unknown

[Alexander Gordeev]

In case IRQ configuration is unknown possibly enabled MSIs
are left enabled in ath10k_pci_deinit_irq(). This update
fixes the described misbehaviour.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 6525e1f..563ce77 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2480,6 +2480,8 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar)
 	case MSI_NUM_REQUEST:
 		pci_disable_msi(ar_pci->pdev);
 		return 0;
+	default:
+		pci_disable_msi(ar_pci->pdev);
 	}
 
 	ath10k_warn("unknown irq configuration upon deinit\n");
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v3 3/3] ath10k: Use pci_enable_msi_range()

[Alexander Gordeev]

As result 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() and pci_enable_msix_range()
interfaces.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 563ce77..ad2a6cf 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2411,8 +2411,9 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
 	/* Try MSI-X */
 	if (ath10k_pci_irq_mode == ATH10K_PCI_IRQ_AUTO && msix_supported) {
 		ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
-		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
-		if (ret == 0)
+		ret = pci_enable_msi_range(ar_pci->pdev, ar_pci->num_msi_intrs,
+							 ar_pci->num_msi_intrs);
+		if (ret > 0)
 			return 0;
 
 		/* fall-through */
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 8/9] ath10k: Use pci_enable_msi_range()

[Alexander Gordeev]

Hi Kalle,

Could you please review the three updated patches?

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/net/wireless/ath/ath10k/pci.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 29fd197..6525e1f 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2414,8 +2414,6 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
>  		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
>  		if (ret == 0)
>  			return 0;
> -		if (ret > 0)
> -			pci_disable_msi(ar_pci->pdev);

I don't understand how this is superfluous. When I read the
documentation for pci_enable_msi_block() it states that if it can't
allocate all requests, it will return the number requests it could
allocate. And in that case we want to fall back other modes.

Am I missing something?

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Tue, Feb 04, 2014 at 08:32:12PM +0200, Kalle Valo wrote:
> Alexander Gordeev <agordeev@redhat.com> writes:
> 
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  drivers/net/wireless/ath/ath10k/pci.c |    2 --
> >  1 files changed, 0 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > index 29fd197..6525e1f 100644
> > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > @@ -2414,8 +2414,6 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
> >  		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
> >  		if (ret == 0)
> >  			return 0;
> > -		if (ret > 0)
> > -			pci_disable_msi(ar_pci->pdev);
> 
> I don't understand how this is superfluous. When I read the
> documentation for pci_enable_msi_block() it states that if it can't
> allocate all requests, it will return the number requests it could
> allocate. And in that case we want to fall back other modes.
> 
> Am I missing something?

Yep. The documentation states 'could have been allocated', not 'could
allocate'. IOW, MSIs are *not* enabled if a positive value returned.
The code I changed tries to disable MSIs in such case, although it is
not necessary, nor required. Just superfluous.

HTH.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

> On Tue, Feb 04, 2014 at 08:32:12PM +0200, Kalle Valo wrote:
>> Alexander Gordeev <agordeev@redhat.com> writes:
>> 
>> I don't understand how this is superfluous. When I read the
>> documentation for pci_enable_msi_block() it states that if it can't
>> allocate all requests, it will return the number requests it could
>> allocate. And in that case we want to fall back other modes.
>> 
>> Am I missing something?
>
> Yep. The documentation states 'could have been allocated', not 'could
> allocate'. IOW, MSIs are *not* enabled if a positive value returned.
> The code I changed tries to disable MSIs in such case, although it is
> not necessary, nor required. Just superfluous.

Ah, thanks for explaining that. I added this to the commit log (I hate
empty commit logs anyway):

    ath10k: Get rid of superfluous call to pci_disable_msi()
    
    The documentation states that pci_enable_msi_block() returns the number of
    requests 'could have been allocated', not 'could allocate'. IOW, MSIs are *not*
    enabled if a positive value returned.
    
    kvalo: add commit log based on Alexander's email
    
    Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
    Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

Is it ok for me to take these patches to my ath.git tree or would you
prefer to route them some other way?

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Wed, Feb 05, 2014 at 10:21:28AM +0200, Kalle Valo wrote:
> Is it ok for me to take these patches to my ath.git tree or would you
> prefer to route them some other way?

Yeah, Bjorn has indicated he would pull it to his tree.
I get it you are fine with 2/3 and 3/3?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

> On Wed, Feb 05, 2014 at 10:21:28AM +0200, Kalle Valo wrote:
>> Is it ok for me to take these patches to my ath.git tree or would you
>> prefer to route them some other way?
>
> Yeah, Bjorn has indicated he would pull it to his tree.

Ok, I'll drop these from my pending branch then. I just think it would
have been easier if I would take these, smaller chance of conflicts that
way.

> I get it you are fine with 2/3 and 3/3?

Yes, with the addition of the commit log to 1/3 I'll give:

Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>

-- 
Kalle Valo

[PATCH] wil6210: Fix switch operator "missing break?" warning

[Alexander Gordeev]

This update fixes a warning introduced with commit bc977ba1
("wil6210: Use pci_enable_msi_range() instead of pci_enable_msi_block()")

drivers/net/wireless/ath/wil6210/pcie_bus.c:65 wil_if_pcie_enable()
warn: missing break? reassigning 'use_msi'

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/wireless/ath/wil6210/pcie_bus.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index 4e1bf54..e1c8cc4 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -51,16 +51,12 @@ static int wil_if_pcie_enable(struct wil6210_priv *wil)
 		use_msi = 1;
 	}
 
-	switch (use_msi) {
-	case 3:
-		if (pci_enable_msi_range(pdev, 3, 3) > 0)
-			break;
+	if (use_msi == 3 && pci_enable_msi_range(pdev, 3, 3) < 0) {
 		wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
 		use_msi = 1;
-		/* fallthrough */
-	case 1:
-		if (!pci_enable_msi(pdev))
-			break;
+	}
+
+	if (use_msi == 1 && pci_enable_msi(pdev)) {
 		wil_err(wil, "pci_enable_msi failed, use INTx\n");
 		use_msi = 0;
 	}
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] wil6210: Fix switch operator "missing break?" warning

[Vladimir Kondratiev]

On Friday, February 07, 2014 03:46:40 PM Alexander Gordeev wrote:
> This update fixes a warning introduced with commit bc977ba1
> ("wil6210: Use pci_enable_msi_range() instead of pci_enable_msi_block()")
> 
> drivers/net/wireless/ath/wil6210/pcie_bus.c:65 wil_if_pcie_enable()
> warn: missing break? reassigning 'use_msi'
> 

I can't reproduce this warning. What tools used to get it?
Neither gcc (I have 4.8.1) nor sparse report it.

Anyway, I am fine with both 'switch' and 'if'.

Thanks, Vladimir.

Re: [PATCH] wil6210: Fix switch operator "missing break?" warning

[Alexander Gordeev]

On Mon, Feb 10, 2014 at 12:54:13PM +0200, Vladimir Kondratiev wrote:
> I can't reproduce this warning. What tools used to get it?

Fengguang?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] wil6210: Fix switch operator "missing break?" warning

[Fengguang Wu]

Hi Alexander,

On Mon, Feb 10, 2014 at 01:16:38PM +0100, Alexander Gordeev wrote:
> On Mon, Feb 10, 2014 at 12:54:13PM +0200, Vladimir Kondratiev wrote:
> > I can't reproduce this warning. What tools used to get it?
> 
> Fengguang?

This is a smatch warning:

        http://smatch.sourceforge.net/

Thanks,
Fengguang

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Bjorn Helgaas]

On Wed, Feb 05, 2014 at 10:54:37AM +0200, Kalle Valo wrote:
> Alexander Gordeev <agordeev@redhat.com> writes:
> 
> > On Wed, Feb 05, 2014 at 10:21:28AM +0200, Kalle Valo wrote:
> >> Is it ok for me to take these patches to my ath.git tree or would you
> >> prefer to route them some other way?
> >
> > Yeah, Bjorn has indicated he would pull it to his tree.
> 
> Ok, I'll drop these from my pending branch then. I just think it would
> have been easier if I would take these, smaller chance of conflicts that
> way.
> 
> > I get it you are fine with 2/3 and 3/3?
> 
> Yes, with the addition of the commit log to 1/3 I'll give:
> 
> Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>

I haven't put these in my branch, so you can take them.

Alexander has a whole batch of network driver updates that I think David
Miller is going to apply; would it make sense to include these in that
batch?

There's also the wil6210 patch for
drivers/net/wireless/ath/wil6210/pcie_bus.c, which seems like it maybe
could be in that batch, too.

Bjorn

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Tue, Feb 11, 2014 at 05:31:43PM -0700, Bjorn Helgaas wrote:
> I haven't put these in my branch, so you can take them.
> 
> Alexander has a whole batch of network driver updates that I think David
> Miller is going to apply; would it make sense to include these in that
> batch?
> 
> There's also the wil6210 patch for
> drivers/net/wireless/ath/wil6210/pcie_bus.c, which seems like it maybe
> could be in that batch, too.

Well, as this series is small I thought it could quickly go thru your
tree. But since ipr had conflicts, there is no point routing all patches
altogether, so up to you guys. The wil6210 patch is already in your pci/msi
branch though.

> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Bjorn Helgaas]

On Wed, Feb 12, 2014 at 6:38 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Tue, Feb 11, 2014 at 05:31:43PM -0700, Bjorn Helgaas wrote:
>> I haven't put these in my branch, so you can take them.
>>
>> Alexander has a whole batch of network driver updates that I think David
>> Miller is going to apply; would it make sense to include these in that
>> batch?
>>
>> There's also the wil6210 patch for
>> drivers/net/wireless/ath/wil6210/pcie_bus.c, which seems like it maybe
>> could be in that batch, too.
>
> Well, as this series is small I thought it could quickly go thru your
> tree. But since ipr had conflicts, there is no point routing all patches
> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
> branch though.

It's in pci/msi, but that's not in my -next branch yet, so I can
easily drop it.  Do drivers/net/wireless patches normally follow a
different path than the other drivers/net patches?  The wil6210 and
ath10k patches look just like the others in the 34-patch series (bnx2,
bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
to include them there.

I think I need to put the ahci regression fix in v3.14, so I'll move
that to my for-linus branch and just keep the other odds and ends
(ahci and vfio) for v3.15.

Bjorn

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Bjorn Helgaas <bhelgaas@google.com> writes:

>> Well, as this series is small I thought it could quickly go thru your
>> tree. But since ipr had conflicts, there is no point routing all patches
>> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
>> branch though.
>
> It's in pci/msi, but that's not in my -next branch yet, so I can
> easily drop it.  Do drivers/net/wireless patches normally follow a
> different path than the other drivers/net patches?  The wil6210 and
> ath10k patches look just like the others in the 34-patch series (bnx2,
> bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
> to include them there.

ath10k patches normally go through my ath.git tree to Linville and then
to David Miller. To avoid conflicts I would prefer to take ath10k
patches to my tree whenever possible.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Bjorn Helgaas]

On Wed, Feb 12, 2014 at 2:30 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
>
>>> Well, as this series is small I thought it could quickly go thru your
>>> tree. But since ipr had conflicts, there is no point routing all patches
>>> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
>>> branch though.
>>
>> It's in pci/msi, but that's not in my -next branch yet, so I can
>> easily drop it.  Do drivers/net/wireless patches normally follow a
>> different path than the other drivers/net patches?  The wil6210 and
>> ath10k patches look just like the others in the 34-patch series (bnx2,
>> bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
>> to include them there.
>
> ath10k patches normally go through my ath.git tree to Linville and then
> to David Miller. To avoid conflicts I would prefer to take ath10k
> patches to my tree whenever possible.

OK, I won't do anything with ath10k (I haven't applied it anywhere).
And if Alexander re-posts the networking series (I think he might, to
add a pci_enable_msix_exact() interface), maybe he can include wil6210
with that series.

Bjorn

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Bjorn Helgaas <bhelgaas@google.com> writes:

> On Wed, Feb 12, 2014 at 2:30 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Bjorn Helgaas <bhelgaas@google.com> writes:
>>
>>>> Well, as this series is small I thought it could quickly go thru your
>>>> tree. But since ipr had conflicts, there is no point routing all patches
>>>> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
>>>> branch though.
>>>
>>> It's in pci/msi, but that's not in my -next branch yet, so I can
>>> easily drop it.  Do drivers/net/wireless patches normally follow a
>>> different path than the other drivers/net patches?  The wil6210 and
>>> ath10k patches look just like the others in the 34-patch series (bnx2,
>>> bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
>>> to include them there.
>>
>> ath10k patches normally go through my ath.git tree to Linville and then
>> to David Miller. To avoid conflicts I would prefer to take ath10k
>> patches to my tree whenever possible.
>
> OK, I won't do anything with ath10k (I haven't applied it anywhere).

I have now taken the ath10k patches to my pending branch, will apply
them soon.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Wed, Feb 12, 2014 at 11:30:44PM +0200, Kalle Valo wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
> 
> >> Well, as this series is small I thought it could quickly go thru your
> >> tree. But since ipr had conflicts, there is no point routing all patches
> >> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
> >> branch though.
> >
> > It's in pci/msi, but that's not in my -next branch yet, so I can
> > easily drop it.  Do drivers/net/wireless patches normally follow a
> > different path than the other drivers/net patches?  The wil6210 and
> > ath10k patches look just like the others in the 34-patch series (bnx2,
> > bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
> > to include them there.
> 
> ath10k patches normally go through my ath.git tree to Linville and then
> to David Miller. To avoid conflicts I would prefer to take ath10k
> patches to my tree whenever possible.

CC'ing Vladimir, in case he decides to do the same with wil6210.

> -- 
> Kalle Valo

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

Thanks, all three patches applied to my ath.git tree.

-- 
Kalle Valo