[PATCH 0/5] ath10k: fixes 2014-08-07, part 2

[PATCH 0/5] ath10k: fixes 2014-08-07, part 2

[Michal Kazior]

Hi,

This is part 2 (of 3) of my patches. Split for
your reviewing pleasure.

This is related to irq handling only. Prepares for
more start/restart sequences fixes.

This is based on:
  [PATCH 0/6] ath10k: fixes 2014-08-07, part 1


Michal Kazior (5):
  ath10k: fix legacy irq workaround
  ath10k: setup irq method in probe
  ath10k: remove early irq handling
  ath10k: split ce irq/handler setup
  ath10k: unmask ce irqs after posting rx buffers

 drivers/net/wireless/ath/ath10k/ce.c   |  36 ++---
 drivers/net/wireless/ath/ath10k/ce.h   |  12 +-
 drivers/net/wireless/ath/ath10k/core.h |   1 -
 drivers/net/wireless/ath/ath10k/pci.c  | 281 ++++++++++-----------------------
 4 files changed, 99 insertions(+), 231 deletions(-)

-- 
1.8.5.3

[PATCH 0/5] ath10k: fixes 2014-08-07, part 2

[Michal Kazior]

Hi,

This is part 2 (of 3) of my patches. Split for
your reviewing pleasure.

This is related to irq handling only. Prepares for
more start/restart sequences fixes.

This is based on:
  [PATCH 0/6] ath10k: fixes 2014-08-07, part 1


Michal Kazior (5):
  ath10k: fix legacy irq workaround
  ath10k: setup irq method in probe
  ath10k: remove early irq handling
  ath10k: split ce irq/handler setup
  ath10k: unmask ce irqs after posting rx buffers

 drivers/net/wireless/ath/ath10k/ce.c   |  36 ++---
 drivers/net/wireless/ath/ath10k/ce.h   |  12 +-
 drivers/net/wireless/ath/ath10k/core.h |   1 -
 drivers/net/wireless/ath/ath10k/pci.c  | 281 ++++++++++-----------------------
 4 files changed, 99 insertions(+), 231 deletions(-)

-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/5] ath10k: fix legacy irq workaround

[Michal Kazior]

Wrong register was being set up. This could
prevent firmware from booting in some rare cases
when using legacy interrupts.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index cfc1da2..54deea3 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2418,9 +2418,10 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
 
 		if (ar_pci->num_msi_intrs == 0)
 			/* Fix potential race by repeating CORE_BASE writes */
-			ath10k_pci_soc_write32(ar, PCIE_INTR_ENABLE_ADDRESS,
-					       PCIE_INTR_FIRMWARE_MASK |
-					       PCIE_INTR_CE_MASK_ALL);
+			ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS +
+					   PCIE_INTR_ENABLE_ADDRESS,
+					   PCIE_INTR_FIRMWARE_MASK |
+					   PCIE_INTR_CE_MASK_ALL);
 
 		mdelay(10);
 	} while (time_before(jiffies, timeout));
-- 
1.8.5.3

[PATCH 1/5] ath10k: fix legacy irq workaround

[Michal Kazior]

Wrong register was being set up. This could
prevent firmware from booting in some rare cases
when using legacy interrupts.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index cfc1da2..54deea3 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2418,9 +2418,10 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
 
 		if (ar_pci->num_msi_intrs == 0)
 			/* Fix potential race by repeating CORE_BASE writes */
-			ath10k_pci_soc_write32(ar, PCIE_INTR_ENABLE_ADDRESS,
-					       PCIE_INTR_FIRMWARE_MASK |
-					       PCIE_INTR_CE_MASK_ALL);
+			ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS +
+					   PCIE_INTR_ENABLE_ADDRESS,
+					   PCIE_INTR_FIRMWARE_MASK |
+					   PCIE_INTR_CE_MASK_ALL);
 
 		mdelay(10);
 	} while (time_before(jiffies, timeout));
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/5] ath10k: setup irq method in probe

[Michal Kazior]

It doesn't make sense to re-init irqs completely
whenever transport is started/stopped. Do it just
once upon probing/removing.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |  1 -
 drivers/net/wireless/ath/ath10k/pci.c  | 77 +++++++++++++++++++---------------
 2 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 2694bd6..d322eaa 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -344,7 +344,6 @@ enum ath10k_fw_features {
 enum ath10k_dev_flags {
 	/* Indicates that ath10k device is during CAC phase of DFS */
 	ATH10K_CAC_RUNNING,
-	ATH10K_FLAG_FIRST_BOOT_DONE,
 	ATH10K_FLAG_CORE_REGISTERED,
 };
 
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 54deea3..2272f76 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -306,6 +306,18 @@ static void ath10k_pci_free_early_irq(struct ath10k *ar)
 	free_irq(ath10k_pci_priv(ar)->pdev->irq, ar);
 }
 
+static inline const char *ath10k_pci_get_irq_method(struct ath10k *ar)
+{
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+
+	if (ar_pci->num_msi_intrs > 1)
+		return "msi-x";
+	else if (ar_pci->num_msi_intrs == 1)
+		return "msi";
+	else
+		return "legacy";
+}
+
 /*
  * Diagnostic read/write access is provided for startup/config/debug usage.
  * Caller must guarantee proper alignment, when applicable, and single user
@@ -1875,8 +1887,6 @@ static int ath10k_pci_warm_reset(struct ath10k *ar)
 
 static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 {
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	const char *irq_mode;
 	int ret;
 
 	/*
@@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 		goto err;
 	}
 
-	ret = ath10k_ce_disable_interrupts(ar);
-	if (ret) {
-		ath10k_err("failed to disable CE interrupts: %d\n", ret);
-		goto err_ce;
-	}
-
-	ret = ath10k_pci_init_irq(ar);
-	if (ret) {
-		ath10k_err("failed to init irqs: %d\n", ret);
-		goto err_ce;
-	}
-
 	ret = ath10k_pci_request_early_irq(ar);
 	if (ret) {
 		ath10k_err("failed to request early irq: %d\n", ret);
-		goto err_deinit_irq;
+		goto err_ce;
 	}
 
 	ret = ath10k_pci_wait_for_target_init(ar);
@@ -1941,24 +1939,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 		goto err_free_early_irq;
 	}
 
-	if (ar_pci->num_msi_intrs > 1)
-		irq_mode = "MSI-X";
-	else if (ar_pci->num_msi_intrs == 1)
-		irq_mode = "MSI";
-	else
-		irq_mode = "legacy";
-
-	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
-		ath10k_info("pci irq %s irq_mode %d reset_mode %d\n",
-			    irq_mode, ath10k_pci_irq_mode,
-			    ath10k_pci_reset_mode);
-
 	return 0;
 
 err_free_early_irq:
 	ath10k_pci_free_early_irq(ar);
-err_deinit_irq:
-	ath10k_pci_deinit_irq(ar);
 err_ce:
 	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_warm_reset(ar);
@@ -2029,8 +2013,6 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
 
 	ath10k_pci_free_early_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
-	ath10k_pci_deinit_irq(ar);
-	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_warm_reset(ar);
 }
 
@@ -2322,8 +2304,7 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
 
 	ath10k_pci_init_irq_tasklets(ar);
 
-	if (ath10k_pci_irq_mode != ATH10K_PCI_IRQ_AUTO &&
-	    !test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
+	if (ath10k_pci_irq_mode != ATH10K_PCI_IRQ_AUTO)
 		ath10k_info("limiting irq mode to: %d\n", ath10k_pci_irq_mode);
 
 	/* Try MSI-X */
@@ -2608,14 +2589,40 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 		goto err_sleep;
 	}
 
+	ret = ath10k_pci_ce_init(ar);
+	if (ret) {
+		ath10k_err("failed to initialize copy engine: %d\n", ret);
+		goto err_free_ce;
+	}
+
+	ret = ath10k_ce_disable_interrupts(ar);
+	if (ret) {
+		ath10k_err("failed to disable copy engine interrupts: %d\n",
+			   ret);
+		goto err_free_ce;
+	}
+
+	ret = ath10k_pci_init_irq(ar);
+	if (ret) {
+		ath10k_err("failed to init irqs: %d\n", ret);
+		goto err_free_ce;
+	}
+
+	ath10k_info("pci irq %s (num %d) irq_mode %d reset_mode %d\n",
+		    ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
+		    ath10k_pci_irq_mode, ath10k_pci_reset_mode);
+
 	ret = ath10k_core_register(ar, chip_id);
 	if (ret) {
 		ath10k_err("failed to register driver core: %d\n", ret);
-		goto err_free_ce;
+		goto err_deinit_irq;
 	}
 
 	return 0;
 
+err_deinit_irq:
+	ath10k_pci_deinit_irq(ar);
+
 err_free_ce:
 	ath10k_pci_free_ce(ar);
 
@@ -2647,6 +2654,8 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
 		return;
 
 	ath10k_core_unregister(ar);
+	ath10k_pci_deinit_irq(ar);
+	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_free_ce(ar);
 	ath10k_pci_sleep(ar);
 	ath10k_pci_release(ar);
-- 
1.8.5.3

[PATCH 2/5] ath10k: setup irq method in probe

[Michal Kazior]

It doesn't make sense to re-init irqs completely
whenever transport is started/stopped. Do it just
once upon probing/removing.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |  1 -
 drivers/net/wireless/ath/ath10k/pci.c  | 77 +++++++++++++++++++---------------
 2 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 2694bd6..d322eaa 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -344,7 +344,6 @@ enum ath10k_fw_features {
 enum ath10k_dev_flags {
 	/* Indicates that ath10k device is during CAC phase of DFS */
 	ATH10K_CAC_RUNNING,
-	ATH10K_FLAG_FIRST_BOOT_DONE,
 	ATH10K_FLAG_CORE_REGISTERED,
 };
 
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 54deea3..2272f76 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -306,6 +306,18 @@ static void ath10k_pci_free_early_irq(struct ath10k *ar)
 	free_irq(ath10k_pci_priv(ar)->pdev->irq, ar);
 }
 
+static inline const char *ath10k_pci_get_irq_method(struct ath10k *ar)
+{
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+
+	if (ar_pci->num_msi_intrs > 1)
+		return "msi-x";
+	else if (ar_pci->num_msi_intrs == 1)
+		return "msi";
+	else
+		return "legacy";
+}
+
 /*
  * Diagnostic read/write access is provided for startup/config/debug usage.
  * Caller must guarantee proper alignment, when applicable, and single user
@@ -1875,8 +1887,6 @@ static int ath10k_pci_warm_reset(struct ath10k *ar)
 
 static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 {
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	const char *irq_mode;
 	int ret;
 
 	/*
@@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 		goto err;
 	}
 
-	ret = ath10k_ce_disable_interrupts(ar);
-	if (ret) {
-		ath10k_err("failed to disable CE interrupts: %d\n", ret);
-		goto err_ce;
-	}
-
-	ret = ath10k_pci_init_irq(ar);
-	if (ret) {
-		ath10k_err("failed to init irqs: %d\n", ret);
-		goto err_ce;
-	}
-
 	ret = ath10k_pci_request_early_irq(ar);
 	if (ret) {
 		ath10k_err("failed to request early irq: %d\n", ret);
-		goto err_deinit_irq;
+		goto err_ce;
 	}
 
 	ret = ath10k_pci_wait_for_target_init(ar);
@@ -1941,24 +1939,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 		goto err_free_early_irq;
 	}
 
-	if (ar_pci->num_msi_intrs > 1)
-		irq_mode = "MSI-X";
-	else if (ar_pci->num_msi_intrs == 1)
-		irq_mode = "MSI";
-	else
-		irq_mode = "legacy";
-
-	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
-		ath10k_info("pci irq %s irq_mode %d reset_mode %d\n",
-			    irq_mode, ath10k_pci_irq_mode,
-			    ath10k_pci_reset_mode);
-
 	return 0;
 
 err_free_early_irq:
 	ath10k_pci_free_early_irq(ar);
-err_deinit_irq:
-	ath10k_pci_deinit_irq(ar);
 err_ce:
 	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_warm_reset(ar);
@@ -2029,8 +2013,6 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
 
 	ath10k_pci_free_early_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
-	ath10k_pci_deinit_irq(ar);
-	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_warm_reset(ar);
 }
 
@@ -2322,8 +2304,7 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
 
 	ath10k_pci_init_irq_tasklets(ar);
 
-	if (ath10k_pci_irq_mode != ATH10K_PCI_IRQ_AUTO &&
-	    !test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
+	if (ath10k_pci_irq_mode != ATH10K_PCI_IRQ_AUTO)
 		ath10k_info("limiting irq mode to: %d\n", ath10k_pci_irq_mode);
 
 	/* Try MSI-X */
@@ -2608,14 +2589,40 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 		goto err_sleep;
 	}
 
+	ret = ath10k_pci_ce_init(ar);
+	if (ret) {
+		ath10k_err("failed to initialize copy engine: %d\n", ret);
+		goto err_free_ce;
+	}
+
+	ret = ath10k_ce_disable_interrupts(ar);
+	if (ret) {
+		ath10k_err("failed to disable copy engine interrupts: %d\n",
+			   ret);
+		goto err_free_ce;
+	}
+
+	ret = ath10k_pci_init_irq(ar);
+	if (ret) {
+		ath10k_err("failed to init irqs: %d\n", ret);
+		goto err_free_ce;
+	}
+
+	ath10k_info("pci irq %s (num %d) irq_mode %d reset_mode %d\n",
+		    ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
+		    ath10k_pci_irq_mode, ath10k_pci_reset_mode);
+
 	ret = ath10k_core_register(ar, chip_id);
 	if (ret) {
 		ath10k_err("failed to register driver core: %d\n", ret);
-		goto err_free_ce;
+		goto err_deinit_irq;
 	}
 
 	return 0;
 
+err_deinit_irq:
+	ath10k_pci_deinit_irq(ar);
+
 err_free_ce:
 	ath10k_pci_free_ce(ar);
 
@@ -2647,6 +2654,8 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
 		return;
 
 	ath10k_core_unregister(ar);
+	ath10k_pci_deinit_irq(ar);
+	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_free_ce(ar);
 	ath10k_pci_sleep(ar);
 	ath10k_pci_release(ar);
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 3/5] ath10k: remove early irq handling

[Michal Kazior]

It's not really necessary to have a dedicated irq
handler just for the sake of catching early fw
crashes. It is now safe to use one handler even
during early stages of device boot up.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 160 ++++++++--------------------------
 1 file changed, 36 insertions(+), 124 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 2272f76..a61dfcf 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -266,46 +266,6 @@ static void ath10k_pci_enable_legacy_irq(struct ath10k *ar)
 				 PCIE_INTR_ENABLE_ADDRESS);
 }
 
-static irqreturn_t ath10k_pci_early_irq_handler(int irq, void *arg)
-{
-	struct ath10k *ar = arg;
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	if (ar_pci->num_msi_intrs == 0) {
-		if (!ath10k_pci_irq_pending(ar))
-			return IRQ_NONE;
-
-		ath10k_pci_disable_and_clear_legacy_irq(ar);
-	}
-
-	tasklet_schedule(&ar_pci->early_irq_tasklet);
-
-	return IRQ_HANDLED;
-}
-
-static int ath10k_pci_request_early_irq(struct ath10k *ar)
-{
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	int ret;
-
-	/* Regardless whether MSI-X/MSI/legacy irqs have been set up the first
-	 * interrupt from irq vector is triggered in all cases for FW
-	 * indication/errors */
-	ret = request_irq(ar_pci->pdev->irq, ath10k_pci_early_irq_handler,
-			  IRQF_SHARED, "ath10k_pci (early)", ar);
-	if (ret) {
-		ath10k_warn("failed to request early irq: %d\n", ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static void ath10k_pci_free_early_irq(struct ath10k *ar)
-{
-	free_irq(ath10k_pci_priv(ar)->pdev->irq, ar);
-}
-
 static inline const char *ath10k_pci_get_irq_method(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -931,7 +891,6 @@ static void ath10k_pci_kill_tasklet(struct ath10k *ar)
 
 	tasklet_kill(&ar_pci->intr_tq);
 	tasklet_kill(&ar_pci->msi_fw_err);
-	tasklet_kill(&ar_pci->early_irq_tasklet);
 
 	for (i = 0; i < CE_COUNT; i++)
 		tasklet_kill(&ar_pci->pipe_info[i].intr);
@@ -1107,20 +1066,10 @@ static int ath10k_pci_post_rx(struct ath10k *ar)
 static int ath10k_pci_hif_start(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	int ret, ret_early;
+	int ret;
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
 
-	ath10k_pci_free_early_irq(ar);
-	ath10k_pci_kill_tasklet(ar);
-
-	ret = ath10k_pci_request_irq(ar);
-	if (ret) {
-		ath10k_warn("failed to post RX buffers for all pipes: %d\n",
-			    ret);
-		goto err_early_irq;
-	}
-
 	ret = ath10k_pci_setup_ce_irq(ar);
 	if (ret) {
 		ath10k_warn("failed to setup CE interrupts: %d\n", ret);
@@ -1140,15 +1089,7 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
 err_stop:
 	ath10k_ce_disable_interrupts(ar);
-	ath10k_pci_free_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
-err_early_irq:
-	/* Though there should be no interrupts (device was reset)
-	 * power_down() expects the early IRQ to be installed as per the
-	 * driver lifecycle. */
-	ret_early = ath10k_pci_request_early_irq(ar);
-	if (ret_early)
-		ath10k_warn("failed to re-enable early irq: %d\n", ret_early);
 
 	return ret;
 }
@@ -1266,13 +1207,8 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
 	if (ret)
 		ath10k_warn("failed to disable CE interrupts: %d\n", ret);
 
-	ath10k_pci_free_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
 
-	ret = ath10k_pci_request_early_irq(ar);
-	if (ret)
-		ath10k_warn("failed to re-enable early irq: %d\n", ret);
-
 	/* At this point, asynchronous threads are stopped, the target should
 	 * not DMA nor interrupt. We process the leftovers and then free
 	 * everything else up. */
@@ -1760,28 +1696,17 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
 	return 0;
 }
 
-static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
+static bool ath10k_pci_fw_crashed(struct ath10k *ar)
 {
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	u32 fw_indicator;
-
-	fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
-
-	if (fw_indicator & FW_IND_EVENT_PENDING) {
-		/* ACK: clear Target-side pending event */
-		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
-				   fw_indicator & ~FW_IND_EVENT_PENDING);
+	return ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
+	       FW_IND_EVENT_PENDING;
+}
 
-		if (ar_pci->started) {
-			ath10k_pci_hif_dump_area(ar);
-		} else {
-			/*
-			 * Probable Target failure before we're prepared
-			 * to handle it.  Generally unexpected.
-			 */
-			ath10k_warn("early firmware event indicated\n");
-		}
-	}
+static void ath10k_pci_fw_crashed_clear(struct ath10k *ar)
+{
+	ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
+			   (ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
+			    ~FW_IND_EVENT_PENDING));
 }
 
 /* this function effectively clears target memory controller assert line */
@@ -1915,34 +1840,26 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 		goto err;
 	}
 
-	ret = ath10k_pci_request_early_irq(ar);
-	if (ret) {
-		ath10k_err("failed to request early irq: %d\n", ret);
-		goto err_ce;
-	}
-
 	ret = ath10k_pci_wait_for_target_init(ar);
 	if (ret) {
 		ath10k_err("failed to wait for target to init: %d\n", ret);
-		goto err_free_early_irq;
+		goto err_ce;
 	}
 
 	ret = ath10k_pci_init_config(ar);
 	if (ret) {
 		ath10k_err("failed to setup init config: %d\n", ret);
-		goto err_free_early_irq;
+		goto err_ce;
 	}
 
 	ret = ath10k_pci_wake_target_cpu(ar);
 	if (ret) {
 		ath10k_err("could not wake up target CPU: %d\n", ret);
-		goto err_free_early_irq;
+		goto err_ce;
 	}
 
 	return 0;
 
-err_free_early_irq:
-	ath10k_pci_free_early_irq(ar);
 err_ce:
 	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_warm_reset(ar);
@@ -2011,7 +1928,6 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
 {
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif power down\n");
 
-	ath10k_pci_free_early_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
 	ath10k_pci_warm_reset(ar);
 }
@@ -2095,7 +2011,13 @@ static void ath10k_msi_err_tasklet(unsigned long data)
 {
 	struct ath10k *ar = (struct ath10k *)data;
 
-	ath10k_pci_fw_interrupt_handler(ar);
+	if (!ath10k_pci_fw_crashed(ar)) {
+		ath10k_warn("received unsolicited fw crash interrupt\n");
+		return;
+	}
+
+	ath10k_pci_fw_crashed_clear(ar);
+	ath10k_pci_hif_dump_area(ar);
 }
 
 /*
@@ -2156,27 +2078,17 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-static void ath10k_pci_early_irq_tasklet(unsigned long data)
+static void ath10k_pci_tasklet(unsigned long data)
 {
 	struct ath10k *ar = (struct ath10k *)data;
-	u32 fw_ind;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 
-	fw_ind = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
-	if (fw_ind & FW_IND_EVENT_PENDING) {
-		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
-				   fw_ind & ~FW_IND_EVENT_PENDING);
+	if (ath10k_pci_fw_crashed(ar)) {
+		ath10k_pci_fw_crashed_clear(ar);
 		ath10k_pci_hif_dump_area(ar);
+		return;
 	}
 
-	ath10k_pci_enable_legacy_irq(ar);
-}
-
-static void ath10k_pci_tasklet(unsigned long data)
-{
-	struct ath10k *ar = (struct ath10k *)data;
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	ath10k_pci_fw_interrupt_handler(ar); /* FIXME: Handle FW error */
 	ath10k_ce_per_engine_service_any(ar);
 
 	/* Re-enable legacy irq that was disabled in the irq handler */
@@ -2287,8 +2199,6 @@ static void ath10k_pci_init_irq_tasklets(struct ath10k *ar)
 	tasklet_init(&ar_pci->intr_tq, ath10k_pci_tasklet, (unsigned long)ar);
 	tasklet_init(&ar_pci->msi_fw_err, ath10k_msi_err_tasklet,
 		     (unsigned long)ar);
-	tasklet_init(&ar_pci->early_irq_tasklet, ath10k_pci_early_irq_tasklet,
-		     (unsigned long)ar);
 
 	for (i = 0; i < CE_COUNT; i++) {
 		ar_pci->pipe_info[i].ar_pci = ar_pci;
@@ -2412,14 +2322,6 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
 		return -EIO;
 	}
 
-	if (val & FW_IND_EVENT_PENDING) {
-		ath10k_warn("device has crashed during init\n");
-		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
-				   val & ~FW_IND_EVENT_PENDING);
-		ath10k_pci_hif_dump_area(ar);
-		return -ECOMM;
-	}
-
 	if (!(val & FW_IND_INITIALIZED)) {
 		ath10k_err("failed to receive initialized event from target: %08x\n",
 			   val);
@@ -2612,14 +2514,23 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 		    ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
 		    ath10k_pci_irq_mode, ath10k_pci_reset_mode);
 
+	ret = ath10k_pci_request_irq(ar);
+	if (ret) {
+		ath10k_warn("failed to request irqs: %d\n", ret);
+		goto err_deinit_irq;
+	}
+
 	ret = ath10k_core_register(ar, chip_id);
 	if (ret) {
 		ath10k_err("failed to register driver core: %d\n", ret);
-		goto err_deinit_irq;
+		goto err_free_irq;
 	}
 
 	return 0;
 
+err_free_irq:
+	ath10k_pci_free_irq(ar);
+
 err_deinit_irq:
 	ath10k_pci_deinit_irq(ar);
 
@@ -2654,6 +2565,7 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
 		return;
 
 	ath10k_core_unregister(ar);
+	ath10k_pci_free_irq(ar);
 	ath10k_pci_deinit_irq(ar);
 	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_free_ce(ar);
-- 
1.8.5.3

[PATCH 3/5] ath10k: remove early irq handling

[Michal Kazior]

It's not really necessary to have a dedicated irq
handler just for the sake of catching early fw
crashes. It is now safe to use one handler even
during early stages of device boot up.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 160 ++++++++--------------------------
 1 file changed, 36 insertions(+), 124 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 2272f76..a61dfcf 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -266,46 +266,6 @@ static void ath10k_pci_enable_legacy_irq(struct ath10k *ar)
 				 PCIE_INTR_ENABLE_ADDRESS);
 }
 
-static irqreturn_t ath10k_pci_early_irq_handler(int irq, void *arg)
-{
-	struct ath10k *ar = arg;
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	if (ar_pci->num_msi_intrs == 0) {
-		if (!ath10k_pci_irq_pending(ar))
-			return IRQ_NONE;
-
-		ath10k_pci_disable_and_clear_legacy_irq(ar);
-	}
-
-	tasklet_schedule(&ar_pci->early_irq_tasklet);
-
-	return IRQ_HANDLED;
-}
-
-static int ath10k_pci_request_early_irq(struct ath10k *ar)
-{
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	int ret;
-
-	/* Regardless whether MSI-X/MSI/legacy irqs have been set up the first
-	 * interrupt from irq vector is triggered in all cases for FW
-	 * indication/errors */
-	ret = request_irq(ar_pci->pdev->irq, ath10k_pci_early_irq_handler,
-			  IRQF_SHARED, "ath10k_pci (early)", ar);
-	if (ret) {
-		ath10k_warn("failed to request early irq: %d\n", ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static void ath10k_pci_free_early_irq(struct ath10k *ar)
-{
-	free_irq(ath10k_pci_priv(ar)->pdev->irq, ar);
-}
-
 static inline const char *ath10k_pci_get_irq_method(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -931,7 +891,6 @@ static void ath10k_pci_kill_tasklet(struct ath10k *ar)
 
 	tasklet_kill(&ar_pci->intr_tq);
 	tasklet_kill(&ar_pci->msi_fw_err);
-	tasklet_kill(&ar_pci->early_irq_tasklet);
 
 	for (i = 0; i < CE_COUNT; i++)
 		tasklet_kill(&ar_pci->pipe_info[i].intr);
@@ -1107,20 +1066,10 @@ static int ath10k_pci_post_rx(struct ath10k *ar)
 static int ath10k_pci_hif_start(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	int ret, ret_early;
+	int ret;
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
 
-	ath10k_pci_free_early_irq(ar);
-	ath10k_pci_kill_tasklet(ar);
-
-	ret = ath10k_pci_request_irq(ar);
-	if (ret) {
-		ath10k_warn("failed to post RX buffers for all pipes: %d\n",
-			    ret);
-		goto err_early_irq;
-	}
-
 	ret = ath10k_pci_setup_ce_irq(ar);
 	if (ret) {
 		ath10k_warn("failed to setup CE interrupts: %d\n", ret);
@@ -1140,15 +1089,7 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
 err_stop:
 	ath10k_ce_disable_interrupts(ar);
-	ath10k_pci_free_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
-err_early_irq:
-	/* Though there should be no interrupts (device was reset)
-	 * power_down() expects the early IRQ to be installed as per the
-	 * driver lifecycle. */
-	ret_early = ath10k_pci_request_early_irq(ar);
-	if (ret_early)
-		ath10k_warn("failed to re-enable early irq: %d\n", ret_early);
 
 	return ret;
 }
@@ -1266,13 +1207,8 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
 	if (ret)
 		ath10k_warn("failed to disable CE interrupts: %d\n", ret);
 
-	ath10k_pci_free_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
 
-	ret = ath10k_pci_request_early_irq(ar);
-	if (ret)
-		ath10k_warn("failed to re-enable early irq: %d\n", ret);
-
 	/* At this point, asynchronous threads are stopped, the target should
 	 * not DMA nor interrupt. We process the leftovers and then free
 	 * everything else up. */
@@ -1760,28 +1696,17 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
 	return 0;
 }
 
-static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
+static bool ath10k_pci_fw_crashed(struct ath10k *ar)
 {
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	u32 fw_indicator;
-
-	fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
-
-	if (fw_indicator & FW_IND_EVENT_PENDING) {
-		/* ACK: clear Target-side pending event */
-		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
-				   fw_indicator & ~FW_IND_EVENT_PENDING);
+	return ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
+	       FW_IND_EVENT_PENDING;
+}
 
-		if (ar_pci->started) {
-			ath10k_pci_hif_dump_area(ar);
-		} else {
-			/*
-			 * Probable Target failure before we're prepared
-			 * to handle it.  Generally unexpected.
-			 */
-			ath10k_warn("early firmware event indicated\n");
-		}
-	}
+static void ath10k_pci_fw_crashed_clear(struct ath10k *ar)
+{
+	ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
+			   (ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
+			    ~FW_IND_EVENT_PENDING));
 }
 
 /* this function effectively clears target memory controller assert line */
@@ -1915,34 +1840,26 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
 		goto err;
 	}
 
-	ret = ath10k_pci_request_early_irq(ar);
-	if (ret) {
-		ath10k_err("failed to request early irq: %d\n", ret);
-		goto err_ce;
-	}
-
 	ret = ath10k_pci_wait_for_target_init(ar);
 	if (ret) {
 		ath10k_err("failed to wait for target to init: %d\n", ret);
-		goto err_free_early_irq;
+		goto err_ce;
 	}
 
 	ret = ath10k_pci_init_config(ar);
 	if (ret) {
 		ath10k_err("failed to setup init config: %d\n", ret);
-		goto err_free_early_irq;
+		goto err_ce;
 	}
 
 	ret = ath10k_pci_wake_target_cpu(ar);
 	if (ret) {
 		ath10k_err("could not wake up target CPU: %d\n", ret);
-		goto err_free_early_irq;
+		goto err_ce;
 	}
 
 	return 0;
 
-err_free_early_irq:
-	ath10k_pci_free_early_irq(ar);
 err_ce:
 	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_warm_reset(ar);
@@ -2011,7 +1928,6 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
 {
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif power down\n");
 
-	ath10k_pci_free_early_irq(ar);
 	ath10k_pci_kill_tasklet(ar);
 	ath10k_pci_warm_reset(ar);
 }
@@ -2095,7 +2011,13 @@ static void ath10k_msi_err_tasklet(unsigned long data)
 {
 	struct ath10k *ar = (struct ath10k *)data;
 
-	ath10k_pci_fw_interrupt_handler(ar);
+	if (!ath10k_pci_fw_crashed(ar)) {
+		ath10k_warn("received unsolicited fw crash interrupt\n");
+		return;
+	}
+
+	ath10k_pci_fw_crashed_clear(ar);
+	ath10k_pci_hif_dump_area(ar);
 }
 
 /*
@@ -2156,27 +2078,17 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-static void ath10k_pci_early_irq_tasklet(unsigned long data)
+static void ath10k_pci_tasklet(unsigned long data)
 {
 	struct ath10k *ar = (struct ath10k *)data;
-	u32 fw_ind;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 
-	fw_ind = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
-	if (fw_ind & FW_IND_EVENT_PENDING) {
-		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
-				   fw_ind & ~FW_IND_EVENT_PENDING);
+	if (ath10k_pci_fw_crashed(ar)) {
+		ath10k_pci_fw_crashed_clear(ar);
 		ath10k_pci_hif_dump_area(ar);
+		return;
 	}
 
-	ath10k_pci_enable_legacy_irq(ar);
-}
-
-static void ath10k_pci_tasklet(unsigned long data)
-{
-	struct ath10k *ar = (struct ath10k *)data;
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	ath10k_pci_fw_interrupt_handler(ar); /* FIXME: Handle FW error */
 	ath10k_ce_per_engine_service_any(ar);
 
 	/* Re-enable legacy irq that was disabled in the irq handler */
@@ -2287,8 +2199,6 @@ static void ath10k_pci_init_irq_tasklets(struct ath10k *ar)
 	tasklet_init(&ar_pci->intr_tq, ath10k_pci_tasklet, (unsigned long)ar);
 	tasklet_init(&ar_pci->msi_fw_err, ath10k_msi_err_tasklet,
 		     (unsigned long)ar);
-	tasklet_init(&ar_pci->early_irq_tasklet, ath10k_pci_early_irq_tasklet,
-		     (unsigned long)ar);
 
 	for (i = 0; i < CE_COUNT; i++) {
 		ar_pci->pipe_info[i].ar_pci = ar_pci;
@@ -2412,14 +2322,6 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
 		return -EIO;
 	}
 
-	if (val & FW_IND_EVENT_PENDING) {
-		ath10k_warn("device has crashed during init\n");
-		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
-				   val & ~FW_IND_EVENT_PENDING);
-		ath10k_pci_hif_dump_area(ar);
-		return -ECOMM;
-	}
-
 	if (!(val & FW_IND_INITIALIZED)) {
 		ath10k_err("failed to receive initialized event from target: %08x\n",
 			   val);
@@ -2612,14 +2514,23 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 		    ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
 		    ath10k_pci_irq_mode, ath10k_pci_reset_mode);
 
+	ret = ath10k_pci_request_irq(ar);
+	if (ret) {
+		ath10k_warn("failed to request irqs: %d\n", ret);
+		goto err_deinit_irq;
+	}
+
 	ret = ath10k_core_register(ar, chip_id);
 	if (ret) {
 		ath10k_err("failed to register driver core: %d\n", ret);
-		goto err_deinit_irq;
+		goto err_free_irq;
 	}
 
 	return 0;
 
+err_free_irq:
+	ath10k_pci_free_irq(ar);
+
 err_deinit_irq:
 	ath10k_pci_deinit_irq(ar);
 
@@ -2654,6 +2565,7 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
 		return;
 
 	ath10k_core_unregister(ar);
+	ath10k_pci_free_irq(ar);
 	ath10k_pci_deinit_irq(ar);
 	ath10k_pci_ce_deinit(ar);
 	ath10k_pci_free_ce(ar);
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 4/5] ath10k: split ce irq/handler setup

[Michal Kazior]

It doesn't make much sense to overwrite send_cb
and recv_cb callbacks over and over again whenever
transport starts. Just make sure to unmask copy
engine interrupts when starting.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/ce.c  | 36 +++++++++++-------------------
 drivers/net/wireless/ath/ath10k/ce.h  | 12 ++++------
 drivers/net/wireless/ath/ath10k/pci.c | 41 ++++-------------------------------
 3 files changed, 21 insertions(+), 68 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 5117705..8cbc0ab 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -769,11 +769,11 @@ void ath10k_ce_per_engine_service_any(struct ath10k *ar)
  *
  * Called with ce_lock held.
  */
-static void ath10k_ce_per_engine_handler_adjust(struct ath10k_ce_pipe *ce_state,
-						int disable_copy_compl_intr)
+static void ath10k_ce_per_engine_handler_adjust(struct ath10k_ce_pipe *ce_state)
 {
 	u32 ctrl_addr = ce_state->ctrl_addr;
 	struct ath10k *ar = ce_state->ar;
+	bool disable_copy_compl_intr = ce_state->attr_flags & CE_ATTR_DIS_INTR;
 
 	if ((!disable_copy_compl_intr) &&
 	    (ce_state->send_cb || ce_state->recv_cb))
@@ -799,29 +799,13 @@ int ath10k_ce_disable_interrupts(struct ath10k *ar)
 	return 0;
 }
 
-void ath10k_ce_send_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*send_cb)(struct ath10k_ce_pipe *),
-				int disable_interrupts)
+void ath10k_ce_enable_interrupts(struct ath10k *ar)
 {
-	struct ath10k *ar = ce_state->ar;
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	spin_lock_bh(&ar_pci->ce_lock);
-	ce_state->send_cb = send_cb;
-	ath10k_ce_per_engine_handler_adjust(ce_state, disable_interrupts);
-	spin_unlock_bh(&ar_pci->ce_lock);
-}
-
-void ath10k_ce_recv_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*recv_cb)(struct ath10k_ce_pipe *))
-{
-	struct ath10k *ar = ce_state->ar;
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ce_id;
 
-	spin_lock_bh(&ar_pci->ce_lock);
-	ce_state->recv_cb = recv_cb;
-	ath10k_ce_per_engine_handler_adjust(ce_state, 0);
-	spin_unlock_bh(&ar_pci->ce_lock);
+	for (ce_id = 0; ce_id < CE_COUNT; ce_id++)
+		ath10k_ce_per_engine_handler_adjust(&ar_pci->ce_states[ce_id]);
 }
 
 static int ath10k_ce_init_src_ring(struct ath10k *ar,
@@ -1023,7 +1007,9 @@ ath10k_ce_alloc_dest_ring(struct ath10k *ar, unsigned int ce_id,
  * initialized by software/firmware.
  */
 int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr)
+			const struct ce_attr *attr,
+			void (*send_cb)(struct ath10k_ce_pipe *),
+			void (*recv_cb)(struct ath10k_ce_pipe *))
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 	struct ath10k_ce_pipe *ce_state = &ar_pci->ce_states[ce_id];
@@ -1046,6 +1032,10 @@ int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
 	ce_state->ctrl_addr = ath10k_ce_base_address(ce_id);
 	ce_state->attr_flags = attr->flags;
 	ce_state->src_sz_max = attr->src_sz_max;
+	if (attr->src_nentries)
+		ce_state->send_cb = send_cb;
+	if (attr->dest_nentries)
+		ce_state->recv_cb = recv_cb;
 	spin_unlock_bh(&ar_pci->ce_lock);
 
 	if (attr->src_nentries) {
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index 7a5a36f..d48dbb9 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -162,10 +162,6 @@ int ath10k_ce_send_nolock(struct ath10k_ce_pipe *ce_state,
 
 void __ath10k_ce_send_revert(struct ath10k_ce_pipe *pipe);
 
-void ath10k_ce_send_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*send_cb)(struct ath10k_ce_pipe *),
-				int disable_interrupts);
-
 int ath10k_ce_num_free_src_entries(struct ath10k_ce_pipe *pipe);
 
 /*==================Recv=======================*/
@@ -184,9 +180,6 @@ int ath10k_ce_recv_buf_enqueue(struct ath10k_ce_pipe *ce_state,
 			       void *per_transfer_recv_context,
 			       u32 buffer);
 
-void ath10k_ce_recv_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*recv_cb)(struct ath10k_ce_pipe *));
-
 /* recv flags */
 /* Data is byte-swapped */
 #define CE_RECV_FLAG_SWAPPED	1
@@ -214,7 +207,9 @@ int ath10k_ce_completed_send_next(struct ath10k_ce_pipe *ce_state,
 /*==================CE Engine Initialization=======================*/
 
 int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr);
+			const struct ce_attr *attr,
+			void (*send_cb)(struct ath10k_ce_pipe *),
+			void (*recv_cb)(struct ath10k_ce_pipe *));
 void ath10k_ce_deinit_pipe(struct ath10k *ar, unsigned int ce_id);
 int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
 			  const struct ce_attr *attr);
@@ -245,6 +240,7 @@ int ath10k_ce_cancel_send_next(struct ath10k_ce_pipe *ce_state,
 void ath10k_ce_per_engine_service_any(struct ath10k *ar);
 void ath10k_ce_per_engine_service(struct ath10k *ar, unsigned int ce_id);
 int ath10k_ce_disable_interrupts(struct ath10k *ar);
+void ath10k_ce_enable_interrupts(struct ath10k *ar);
 
 /* ce_attr.flags values */
 /* Use NonSnooping PCIe accesses? */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a61dfcf..c195a11 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -853,37 +853,6 @@ static void ath10k_pci_hif_set_callbacks(struct ath10k *ar,
 	       sizeof(ar_pci->msg_callbacks_current));
 }
 
-static int ath10k_pci_setup_ce_irq(struct ath10k *ar)
-{
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	const struct ce_attr *attr;
-	struct ath10k_pci_pipe *pipe_info;
-	int pipe_num, disable_interrupts;
-
-	for (pipe_num = 0; pipe_num < CE_COUNT; pipe_num++) {
-		pipe_info = &ar_pci->pipe_info[pipe_num];
-
-		/* Handle Diagnostic CE specially */
-		if (pipe_info->ce_hdl == ar_pci->ce_diag)
-			continue;
-
-		attr = &host_ce_config_wlan[pipe_num];
-
-		if (attr->src_nentries) {
-			disable_interrupts = attr->flags & CE_ATTR_DIS_INTR;
-			ath10k_ce_send_cb_register(pipe_info->ce_hdl,
-						   ath10k_pci_ce_send_done,
-						   disable_interrupts);
-		}
-
-		if (attr->dest_nentries)
-			ath10k_ce_recv_cb_register(pipe_info->ce_hdl,
-						   ath10k_pci_ce_recv_data);
-	}
-
-	return 0;
-}
-
 static void ath10k_pci_kill_tasklet(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -1070,11 +1039,7 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
 
-	ret = ath10k_pci_setup_ce_irq(ar);
-	if (ret) {
-		ath10k_warn("failed to setup CE interrupts: %d\n", ret);
-		goto err_stop;
-	}
+	ath10k_ce_enable_interrupts(ar);
 
 	/* Post buffers once to start things off. */
 	ret = ath10k_pci_post_rx(ar);
@@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
 		pipe_info->hif_ce_state = ar;
 		attr = &host_ce_config_wlan[pipe_num];
 
-		ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
+		ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
+					  ath10k_pci_ce_send_done,
+					  ath10k_pci_ce_recv_data);
 		if (ret) {
 			ath10k_err("failed to initialize copy engine pipe %d: %d\n",
 				   pipe_num, ret);
-- 
1.8.5.3

[PATCH 4/5] ath10k: split ce irq/handler setup

[Michal Kazior]

It doesn't make much sense to overwrite send_cb
and recv_cb callbacks over and over again whenever
transport starts. Just make sure to unmask copy
engine interrupts when starting.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/ce.c  | 36 +++++++++++-------------------
 drivers/net/wireless/ath/ath10k/ce.h  | 12 ++++------
 drivers/net/wireless/ath/ath10k/pci.c | 41 ++++-------------------------------
 3 files changed, 21 insertions(+), 68 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 5117705..8cbc0ab 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -769,11 +769,11 @@ void ath10k_ce_per_engine_service_any(struct ath10k *ar)
  *
  * Called with ce_lock held.
  */
-static void ath10k_ce_per_engine_handler_adjust(struct ath10k_ce_pipe *ce_state,
-						int disable_copy_compl_intr)
+static void ath10k_ce_per_engine_handler_adjust(struct ath10k_ce_pipe *ce_state)
 {
 	u32 ctrl_addr = ce_state->ctrl_addr;
 	struct ath10k *ar = ce_state->ar;
+	bool disable_copy_compl_intr = ce_state->attr_flags & CE_ATTR_DIS_INTR;
 
 	if ((!disable_copy_compl_intr) &&
 	    (ce_state->send_cb || ce_state->recv_cb))
@@ -799,29 +799,13 @@ int ath10k_ce_disable_interrupts(struct ath10k *ar)
 	return 0;
 }
 
-void ath10k_ce_send_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*send_cb)(struct ath10k_ce_pipe *),
-				int disable_interrupts)
+void ath10k_ce_enable_interrupts(struct ath10k *ar)
 {
-	struct ath10k *ar = ce_state->ar;
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	spin_lock_bh(&ar_pci->ce_lock);
-	ce_state->send_cb = send_cb;
-	ath10k_ce_per_engine_handler_adjust(ce_state, disable_interrupts);
-	spin_unlock_bh(&ar_pci->ce_lock);
-}
-
-void ath10k_ce_recv_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*recv_cb)(struct ath10k_ce_pipe *))
-{
-	struct ath10k *ar = ce_state->ar;
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ce_id;
 
-	spin_lock_bh(&ar_pci->ce_lock);
-	ce_state->recv_cb = recv_cb;
-	ath10k_ce_per_engine_handler_adjust(ce_state, 0);
-	spin_unlock_bh(&ar_pci->ce_lock);
+	for (ce_id = 0; ce_id < CE_COUNT; ce_id++)
+		ath10k_ce_per_engine_handler_adjust(&ar_pci->ce_states[ce_id]);
 }
 
 static int ath10k_ce_init_src_ring(struct ath10k *ar,
@@ -1023,7 +1007,9 @@ ath10k_ce_alloc_dest_ring(struct ath10k *ar, unsigned int ce_id,
  * initialized by software/firmware.
  */
 int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr)
+			const struct ce_attr *attr,
+			void (*send_cb)(struct ath10k_ce_pipe *),
+			void (*recv_cb)(struct ath10k_ce_pipe *))
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 	struct ath10k_ce_pipe *ce_state = &ar_pci->ce_states[ce_id];
@@ -1046,6 +1032,10 @@ int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
 	ce_state->ctrl_addr = ath10k_ce_base_address(ce_id);
 	ce_state->attr_flags = attr->flags;
 	ce_state->src_sz_max = attr->src_sz_max;
+	if (attr->src_nentries)
+		ce_state->send_cb = send_cb;
+	if (attr->dest_nentries)
+		ce_state->recv_cb = recv_cb;
 	spin_unlock_bh(&ar_pci->ce_lock);
 
 	if (attr->src_nentries) {
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index 7a5a36f..d48dbb9 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -162,10 +162,6 @@ int ath10k_ce_send_nolock(struct ath10k_ce_pipe *ce_state,
 
 void __ath10k_ce_send_revert(struct ath10k_ce_pipe *pipe);
 
-void ath10k_ce_send_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*send_cb)(struct ath10k_ce_pipe *),
-				int disable_interrupts);
-
 int ath10k_ce_num_free_src_entries(struct ath10k_ce_pipe *pipe);
 
 /*==================Recv=======================*/
@@ -184,9 +180,6 @@ int ath10k_ce_recv_buf_enqueue(struct ath10k_ce_pipe *ce_state,
 			       void *per_transfer_recv_context,
 			       u32 buffer);
 
-void ath10k_ce_recv_cb_register(struct ath10k_ce_pipe *ce_state,
-				void (*recv_cb)(struct ath10k_ce_pipe *));
-
 /* recv flags */
 /* Data is byte-swapped */
 #define CE_RECV_FLAG_SWAPPED	1
@@ -214,7 +207,9 @@ int ath10k_ce_completed_send_next(struct ath10k_ce_pipe *ce_state,
 /*==================CE Engine Initialization=======================*/
 
 int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr);
+			const struct ce_attr *attr,
+			void (*send_cb)(struct ath10k_ce_pipe *),
+			void (*recv_cb)(struct ath10k_ce_pipe *));
 void ath10k_ce_deinit_pipe(struct ath10k *ar, unsigned int ce_id);
 int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
 			  const struct ce_attr *attr);
@@ -245,6 +240,7 @@ int ath10k_ce_cancel_send_next(struct ath10k_ce_pipe *ce_state,
 void ath10k_ce_per_engine_service_any(struct ath10k *ar);
 void ath10k_ce_per_engine_service(struct ath10k *ar, unsigned int ce_id);
 int ath10k_ce_disable_interrupts(struct ath10k *ar);
+void ath10k_ce_enable_interrupts(struct ath10k *ar);
 
 /* ce_attr.flags values */
 /* Use NonSnooping PCIe accesses? */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a61dfcf..c195a11 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -853,37 +853,6 @@ static void ath10k_pci_hif_set_callbacks(struct ath10k *ar,
 	       sizeof(ar_pci->msg_callbacks_current));
 }
 
-static int ath10k_pci_setup_ce_irq(struct ath10k *ar)
-{
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	const struct ce_attr *attr;
-	struct ath10k_pci_pipe *pipe_info;
-	int pipe_num, disable_interrupts;
-
-	for (pipe_num = 0; pipe_num < CE_COUNT; pipe_num++) {
-		pipe_info = &ar_pci->pipe_info[pipe_num];
-
-		/* Handle Diagnostic CE specially */
-		if (pipe_info->ce_hdl == ar_pci->ce_diag)
-			continue;
-
-		attr = &host_ce_config_wlan[pipe_num];
-
-		if (attr->src_nentries) {
-			disable_interrupts = attr->flags & CE_ATTR_DIS_INTR;
-			ath10k_ce_send_cb_register(pipe_info->ce_hdl,
-						   ath10k_pci_ce_send_done,
-						   disable_interrupts);
-		}
-
-		if (attr->dest_nentries)
-			ath10k_ce_recv_cb_register(pipe_info->ce_hdl,
-						   ath10k_pci_ce_recv_data);
-	}
-
-	return 0;
-}
-
 static void ath10k_pci_kill_tasklet(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -1070,11 +1039,7 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
 
-	ret = ath10k_pci_setup_ce_irq(ar);
-	if (ret) {
-		ath10k_warn("failed to setup CE interrupts: %d\n", ret);
-		goto err_stop;
-	}
+	ath10k_ce_enable_interrupts(ar);
 
 	/* Post buffers once to start things off. */
 	ret = ath10k_pci_post_rx(ar);
@@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
 		pipe_info->hif_ce_state = ar;
 		attr = &host_ce_config_wlan[pipe_num];
 
-		ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
+		ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
+					  ath10k_pci_ce_send_done,
+					  ath10k_pci_ce_recv_data);
 		if (ret) {
 			ath10k_err("failed to initialize copy engine pipe %d: %d\n",
 				   pipe_num, ret);
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 5/5] ath10k: unmask ce irqs after posting rx buffers

[Michal Kazior]

It doesn't make much sense to enable the
interrupts before posting rx buffers.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index c195a11..c157717 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1039,24 +1039,18 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
 
-	ath10k_ce_enable_interrupts(ar);
-
 	/* Post buffers once to start things off. */
 	ret = ath10k_pci_post_rx(ar);
 	if (ret) {
 		ath10k_warn("failed to post RX buffers for all pipes: %d\n",
 			    ret);
-		goto err_stop;
+		return ret;
 	}
 
+	ath10k_ce_enable_interrupts(ar);
 	ar_pci->started = 1;
-	return 0;
 
-err_stop:
-	ath10k_ce_disable_interrupts(ar);
-	ath10k_pci_kill_tasklet(ar);
-
-	return ret;
+	return 0;
 }
 
 static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
-- 
1.8.5.3

[PATCH 5/5] ath10k: unmask ce irqs after posting rx buffers

[Michal Kazior]

It doesn't make much sense to enable the
interrupts before posting rx buffers.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index c195a11..c157717 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1039,24 +1039,18 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
 
-	ath10k_ce_enable_interrupts(ar);
-
 	/* Post buffers once to start things off. */
 	ret = ath10k_pci_post_rx(ar);
 	if (ret) {
 		ath10k_warn("failed to post RX buffers for all pipes: %d\n",
 			    ret);
-		goto err_stop;
+		return ret;
 	}
 
+	ath10k_ce_enable_interrupts(ar);
 	ar_pci->started = 1;
-	return 0;
 
-err_stop:
-	ath10k_ce_disable_interrupts(ar);
-	ath10k_pci_kill_tasklet(ar);
-
-	return ret;
+	return 0;
 }
 
 static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/5] ath10k: setup irq method in probe

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> It doesn't make sense to re-init irqs completely
> whenever transport is started/stopped. Do it just
> once upon probing/removing.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> @@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
>  		goto err;
>  	}
>  
> -	ret = ath10k_ce_disable_interrupts(ar);
> -	if (ret) {
> -		ath10k_err("failed to disable CE interrupts: %d\n", ret);
> -		goto err_ce;
> -	}
> -
> -	ret = ath10k_pci_init_irq(ar);
> -	if (ret) {
> -		ath10k_err("failed to init irqs: %d\n", ret);
> -		goto err_ce;
> -	}
> -
>  	ret = ath10k_pci_request_early_irq(ar);
>  	if (ret) {
>  		ath10k_err("failed to request early irq: %d\n", ret);
> -		goto err_deinit_irq;
> +		goto err_ce;
>  	}

You add ath10k_pci_ce_init() to probe() and respective
ath10k_pci_ce_deinit() to remove(), and you remove
ath10k_pci_ce_deinit() from hif_power_down(). But why do you leave
ath10k_pci_ce_init() to hif_power_up()? Isn't that unnecessary as we
already do that in probe()?

> +	ath10k_info("pci irq %s (num %d) irq_mode %d reset_mode %d\n",
> +		    ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
> +		    ath10k_pci_irq_mode, ath10k_pci_reset_mode);

"pci irq %s interrupts %d irq_mode %d ..."

-- 
Kalle Valo

Re: [PATCH 2/5] ath10k: setup irq method in probe

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> It doesn't make sense to re-init irqs completely
> whenever transport is started/stopped. Do it just
> once upon probing/removing.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> @@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
>  		goto err;
>  	}
>  
> -	ret = ath10k_ce_disable_interrupts(ar);
> -	if (ret) {
> -		ath10k_err("failed to disable CE interrupts: %d\n", ret);
> -		goto err_ce;
> -	}
> -
> -	ret = ath10k_pci_init_irq(ar);
> -	if (ret) {
> -		ath10k_err("failed to init irqs: %d\n", ret);
> -		goto err_ce;
> -	}
> -
>  	ret = ath10k_pci_request_early_irq(ar);
>  	if (ret) {
>  		ath10k_err("failed to request early irq: %d\n", ret);
> -		goto err_deinit_irq;
> +		goto err_ce;
>  	}

You add ath10k_pci_ce_init() to probe() and respective
ath10k_pci_ce_deinit() to remove(), and you remove
ath10k_pci_ce_deinit() from hif_power_down(). But why do you leave
ath10k_pci_ce_init() to hif_power_up()? Isn't that unnecessary as we
already do that in probe()?

> +	ath10k_info("pci irq %s (num %d) irq_mode %d reset_mode %d\n",
> +		    ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
> +		    ath10k_pci_irq_mode, ath10k_pci_reset_mode);

"pci irq %s interrupts %d irq_mode %d ..."

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/5] ath10k: remove early irq handling

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> It's not really necessary to have a dedicated irq
> handler just for the sake of catching early fw
> crashes. It is now safe to use one handler even
> during early stages of device boot up.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Nice, this really needs to be cleaned up.

>  drivers/net/wireless/ath/ath10k/pci.c | 160 ++++++++--------------------------
>  1 file changed, 36 insertions(+), 124 deletions(-)

Shouldn't you also remove early_irq_tasklet from struct ath10k_pci?

> -static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
> +static bool ath10k_pci_fw_crashed(struct ath10k *ar)
>  {
> -	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
> -	u32 fw_indicator;
> -
> -	fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
> -
> -	if (fw_indicator & FW_IND_EVENT_PENDING) {
> -		/* ACK: clear Target-side pending event */
> -		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
> -				   fw_indicator & ~FW_IND_EVENT_PENDING);
> +	return ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
> +	       FW_IND_EVENT_PENDING;
> +}

Hehe, in Ben's firmware crash dump patches I renamed
ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed(). So we now
have two very similarly named functions doing very different :)

I propose that you rename this function to ath10k_pci_is_fw_crashed() or
ath10k_pci_has_fw_crashed() and I rename ath10k_pci_hif_dump_area() to
ath10k_pci_fw_crashed_dump(). I think that we have pretty consistent
naming, no?

> +static void ath10k_pci_fw_crashed_clear(struct ath10k *ar)
> +{
> +	ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
> +			   (ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
> +			    ~FW_IND_EVENT_PENDING));

Please don't embed calls like this, with a temporary variable you get it
more readable and the resulting code should be the same anyway.

> @@ -2412,14 +2322,6 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
>  		return -EIO;
>  	}
>  
> -	if (val & FW_IND_EVENT_PENDING) {
> -		ath10k_warn("device has crashed during init\n");
> -		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
> -				   val & ~FW_IND_EVENT_PENDING);
> -		ath10k_pci_hif_dump_area(ar);
> -		return -ECOMM;
> -	}

I'm a bit worried about this. Are you relying now that the target will
always trigger an interrupt or what? If yes, how can we sure it will
happen on all possible cases? I would prefer to have some kind of safety
net anyway, even if it's just a simple warning.

-- 
Kalle Valo

Re: [PATCH 3/5] ath10k: remove early irq handling

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> It's not really necessary to have a dedicated irq
> handler just for the sake of catching early fw
> crashes. It is now safe to use one handler even
> during early stages of device boot up.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Nice, this really needs to be cleaned up.

>  drivers/net/wireless/ath/ath10k/pci.c | 160 ++++++++--------------------------
>  1 file changed, 36 insertions(+), 124 deletions(-)

Shouldn't you also remove early_irq_tasklet from struct ath10k_pci?

> -static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
> +static bool ath10k_pci_fw_crashed(struct ath10k *ar)
>  {
> -	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
> -	u32 fw_indicator;
> -
> -	fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
> -
> -	if (fw_indicator & FW_IND_EVENT_PENDING) {
> -		/* ACK: clear Target-side pending event */
> -		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
> -				   fw_indicator & ~FW_IND_EVENT_PENDING);
> +	return ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
> +	       FW_IND_EVENT_PENDING;
> +}

Hehe, in Ben's firmware crash dump patches I renamed
ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed(). So we now
have two very similarly named functions doing very different :)

I propose that you rename this function to ath10k_pci_is_fw_crashed() or
ath10k_pci_has_fw_crashed() and I rename ath10k_pci_hif_dump_area() to
ath10k_pci_fw_crashed_dump(). I think that we have pretty consistent
naming, no?

> +static void ath10k_pci_fw_crashed_clear(struct ath10k *ar)
> +{
> +	ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
> +			   (ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
> +			    ~FW_IND_EVENT_PENDING));

Please don't embed calls like this, with a temporary variable you get it
more readable and the resulting code should be the same anyway.

> @@ -2412,14 +2322,6 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
>  		return -EIO;
>  	}
>  
> -	if (val & FW_IND_EVENT_PENDING) {
> -		ath10k_warn("device has crashed during init\n");
> -		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
> -				   val & ~FW_IND_EVENT_PENDING);
> -		ath10k_pci_hif_dump_area(ar);
> -		return -ECOMM;
> -	}

I'm a bit worried about this. Are you relying now that the target will
always trigger an interrupt or what? If yes, how can we sure it will
happen on all possible cases? I would prefer to have some kind of safety
net anyway, even if it's just a simple warning.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/5] ath10k: split ce irq/handler setup

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> It doesn't make much sense to overwrite send_cb
> and recv_cb callbacks over and over again whenever
> transport starts. Just make sure to unmask copy
> engine interrupts when starting.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

The patch looks, just a followup question for the future:

> @@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
>  		pipe_info->hif_ce_state = ar;
>  		attr = &host_ce_config_wlan[pipe_num];
>  
> -		ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
> +		ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
> +					  ath10k_pci_ce_send_done,
> +					  ath10k_pci_ce_recv_data);

As we call ath10k_ce_init_pipe() only once and seem to have only one set
of functions, why even bother bother with function pointers? What if we
just call the functions directly?

-- 
Kalle Valo

Re: [PATCH 4/5] ath10k: split ce irq/handler setup

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> It doesn't make much sense to overwrite send_cb
> and recv_cb callbacks over and over again whenever
> transport starts. Just make sure to unmask copy
> engine interrupts when starting.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

The patch looks, just a followup question for the future:

> @@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
>  		pipe_info->hif_ce_state = ar;
>  		attr = &host_ce_config_wlan[pipe_num];
>  
> -		ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
> +		ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
> +					  ath10k_pci_ce_send_done,
> +					  ath10k_pci_ce_recv_data);

As we call ath10k_ce_init_pipe() only once and seem to have only one set
of functions, why even bother bother with function pointers? What if we
just call the functions directly?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/5] ath10k: setup irq method in probe

[Michal Kazior]

On 13 August 2014 15:48, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> It doesn't make sense to re-init irqs completely
>> whenever transport is started/stopped. Do it just
>> once upon probing/removing.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> @@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
>>               goto err;
>>       }
>>
>> -     ret = ath10k_ce_disable_interrupts(ar);
>> -     if (ret) {
>> -             ath10k_err("failed to disable CE interrupts: %d\n", ret);
>> -             goto err_ce;
>> -     }
>> -
>> -     ret = ath10k_pci_init_irq(ar);
>> -     if (ret) {
>> -             ath10k_err("failed to init irqs: %d\n", ret);
>> -             goto err_ce;
>> -     }
>> -
>>       ret = ath10k_pci_request_early_irq(ar);
>>       if (ret) {
>>               ath10k_err("failed to request early irq: %d\n", ret);
>> -             goto err_deinit_irq;
>> +             goto err_ce;
>>       }
>
> You add ath10k_pci_ce_init() to probe() and respective
> ath10k_pci_ce_deinit() to remove(), and you remove
> ath10k_pci_ce_deinit() from hif_power_down(). But why do you leave
> ath10k_pci_ce_init() to hif_power_up()? Isn't that unnecessary as we
> already do that in probe()?

Hmm.. I didn't check if copy engine register state (notable ring index
values) is guaranteed to be usable in all cases yet and decided it's
safer to just re-init it on each power up until it is proven it is not
necessary.


>
>> +     ath10k_info("pci irq %s (num %d) irq_mode %d reset_mode %d\n",
>> +                 ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
>> +                 ath10k_pci_irq_mode, ath10k_pci_reset_mode);
>
> "pci irq %s interrupts %d irq_mode %d ..."

Will fix.


Michał

Re: [PATCH 2/5] ath10k: setup irq method in probe

[Michal Kazior]

On 13 August 2014 15:48, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> It doesn't make sense to re-init irqs completely
>> whenever transport is started/stopped. Do it just
>> once upon probing/removing.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> @@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
>>               goto err;
>>       }
>>
>> -     ret = ath10k_ce_disable_interrupts(ar);
>> -     if (ret) {
>> -             ath10k_err("failed to disable CE interrupts: %d\n", ret);
>> -             goto err_ce;
>> -     }
>> -
>> -     ret = ath10k_pci_init_irq(ar);
>> -     if (ret) {
>> -             ath10k_err("failed to init irqs: %d\n", ret);
>> -             goto err_ce;
>> -     }
>> -
>>       ret = ath10k_pci_request_early_irq(ar);
>>       if (ret) {
>>               ath10k_err("failed to request early irq: %d\n", ret);
>> -             goto err_deinit_irq;
>> +             goto err_ce;
>>       }
>
> You add ath10k_pci_ce_init() to probe() and respective
> ath10k_pci_ce_deinit() to remove(), and you remove
> ath10k_pci_ce_deinit() from hif_power_down(). But why do you leave
> ath10k_pci_ce_init() to hif_power_up()? Isn't that unnecessary as we
> already do that in probe()?

Hmm.. I didn't check if copy engine register state (notable ring index
values) is guaranteed to be usable in all cases yet and decided it's
safer to just re-init it on each power up until it is proven it is not
necessary.


>
>> +     ath10k_info("pci irq %s (num %d) irq_mode %d reset_mode %d\n",
>> +                 ath10k_pci_get_irq_method(ar), ar_pci->num_msi_intrs,
>> +                 ath10k_pci_irq_mode, ath10k_pci_reset_mode);
>
> "pci irq %s interrupts %d irq_mode %d ..."

Will fix.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/5] ath10k: remove early irq handling

[Michal Kazior]

On 13 August 2014 16:09, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> It's not really necessary to have a dedicated irq
>> handler just for the sake of catching early fw
>> crashes. It is now safe to use one handler even
>> during early stages of device boot up.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> Nice, this really needs to be cleaned up.
>
>>  drivers/net/wireless/ath/ath10k/pci.c | 160 ++++++++--------------------------
>>  1 file changed, 36 insertions(+), 124 deletions(-)
>
> Shouldn't you also remove early_irq_tasklet from struct ath10k_pci?

Oops. Good catch.


>> -static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
>> +static bool ath10k_pci_fw_crashed(struct ath10k *ar)
>>  {
>> -     struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>> -     u32 fw_indicator;
>> -
>> -     fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
>> -
>> -     if (fw_indicator & FW_IND_EVENT_PENDING) {
>> -             /* ACK: clear Target-side pending event */
>> -             ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> -                                fw_indicator & ~FW_IND_EVENT_PENDING);
>> +     return ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
>> +            FW_IND_EVENT_PENDING;
>> +}
>
> Hehe, in Ben's firmware crash dump patches I renamed
> ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed(). So we now
> have two very similarly named functions doing very different :)
>
> I propose that you rename this function to ath10k_pci_is_fw_crashed() or
> ath10k_pci_has_fw_crashed() and I rename ath10k_pci_hif_dump_area() to
> ath10k_pci_fw_crashed_dump(). I think that we have pretty consistent
> naming, no?

I'm okay with that. WIll fix.


>> +static void ath10k_pci_fw_crashed_clear(struct ath10k *ar)
>> +{
>> +     ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> +                        (ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
>> +                         ~FW_IND_EVENT_PENDING));
>
> Please don't embed calls like this, with a temporary variable you get it
> more readable and the resulting code should be the same anyway.

Will fix.


>
>> @@ -2412,14 +2322,6 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
>>               return -EIO;
>>       }
>>
>> -     if (val & FW_IND_EVENT_PENDING) {
>> -             ath10k_warn("device has crashed during init\n");
>> -             ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> -                                val & ~FW_IND_EVENT_PENDING);
>> -             ath10k_pci_hif_dump_area(ar);
>> -             return -ECOMM;
>> -     }
>
> I'm a bit worried about this. Are you relying now that the target will
> always trigger an interrupt or what? If yes, how can we sure it will
> happen on all possible cases? I would prefer to have some kind of safety
> net anyway, even if it's just a simple warning.

Hmm, I'll keep it. I just remembered it's possible for a device to
fail to fully setup pci link/config and crash without asserting an
interrupt while still providing a trace in the host interest area.


Michał

Re: [PATCH 3/5] ath10k: remove early irq handling

[Michal Kazior]

On 13 August 2014 16:09, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> It's not really necessary to have a dedicated irq
>> handler just for the sake of catching early fw
>> crashes. It is now safe to use one handler even
>> during early stages of device boot up.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> Nice, this really needs to be cleaned up.
>
>>  drivers/net/wireless/ath/ath10k/pci.c | 160 ++++++++--------------------------
>>  1 file changed, 36 insertions(+), 124 deletions(-)
>
> Shouldn't you also remove early_irq_tasklet from struct ath10k_pci?

Oops. Good catch.


>> -static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
>> +static bool ath10k_pci_fw_crashed(struct ath10k *ar)
>>  {
>> -     struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>> -     u32 fw_indicator;
>> -
>> -     fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
>> -
>> -     if (fw_indicator & FW_IND_EVENT_PENDING) {
>> -             /* ACK: clear Target-side pending event */
>> -             ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> -                                fw_indicator & ~FW_IND_EVENT_PENDING);
>> +     return ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
>> +            FW_IND_EVENT_PENDING;
>> +}
>
> Hehe, in Ben's firmware crash dump patches I renamed
> ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed(). So we now
> have two very similarly named functions doing very different :)
>
> I propose that you rename this function to ath10k_pci_is_fw_crashed() or
> ath10k_pci_has_fw_crashed() and I rename ath10k_pci_hif_dump_area() to
> ath10k_pci_fw_crashed_dump(). I think that we have pretty consistent
> naming, no?

I'm okay with that. WIll fix.


>> +static void ath10k_pci_fw_crashed_clear(struct ath10k *ar)
>> +{
>> +     ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> +                        (ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
>> +                         ~FW_IND_EVENT_PENDING));
>
> Please don't embed calls like this, with a temporary variable you get it
> more readable and the resulting code should be the same anyway.

Will fix.


>
>> @@ -2412,14 +2322,6 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
>>               return -EIO;
>>       }
>>
>> -     if (val & FW_IND_EVENT_PENDING) {
>> -             ath10k_warn("device has crashed during init\n");
>> -             ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> -                                val & ~FW_IND_EVENT_PENDING);
>> -             ath10k_pci_hif_dump_area(ar);
>> -             return -ECOMM;
>> -     }
>
> I'm a bit worried about this. Are you relying now that the target will
> always trigger an interrupt or what? If yes, how can we sure it will
> happen on all possible cases? I would prefer to have some kind of safety
> net anyway, even if it's just a simple warning.

Hmm, I'll keep it. I just remembered it's possible for a device to
fail to fully setup pci link/config and crash without asserting an
interrupt while still providing a trace in the host interest area.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/5] ath10k: setup irq method in probe

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

>>> @@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
>>>               goto err;
>>>       }
>>>
>>> -     ret = ath10k_ce_disable_interrupts(ar);
>>> -     if (ret) {
>>> -             ath10k_err("failed to disable CE interrupts: %d\n", ret);
>>> -             goto err_ce;
>>> -     }
>>> -
>>> -     ret = ath10k_pci_init_irq(ar);
>>> -     if (ret) {
>>> -             ath10k_err("failed to init irqs: %d\n", ret);
>>> -             goto err_ce;
>>> -     }
>>> -
>>>       ret = ath10k_pci_request_early_irq(ar);
>>>       if (ret) {
>>>               ath10k_err("failed to request early irq: %d\n", ret);
>>> -             goto err_deinit_irq;
>>> +             goto err_ce;
>>>       }
>>
>> You add ath10k_pci_ce_init() to probe() and respective
>> ath10k_pci_ce_deinit() to remove(), and you remove
>> ath10k_pci_ce_deinit() from hif_power_down(). But why do you leave
>> ath10k_pci_ce_init() to hif_power_up()? Isn't that unnecessary as we
>> already do that in probe()?
>
> Hmm.. I didn't check if copy engine register state (notable ring index
> values) is guaranteed to be usable in all cases yet and decided it's
> safer to just re-init it on each power up until it is proven it is not
> necessary.

So I have been trying to follow this naming scheme for initilisation:

_create() - called during device probe time
_start() - if up / power up
_stop() - if down / power down
_destroy() - called when device is removed

If you call ce_init() both in create() and start() time there has to be
some redundant code and I think the function should be split. This isn't
a concern now, but something for the future.

-- 
Kalle Valo

Re: [PATCH 2/5] ath10k: setup irq method in probe

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

>>> @@ -1905,22 +1915,10 @@ static int __ath10k_pci_hif_power_up(struct ath10k *ar, bool cold_reset)
>>>               goto err;
>>>       }
>>>
>>> -     ret = ath10k_ce_disable_interrupts(ar);
>>> -     if (ret) {
>>> -             ath10k_err("failed to disable CE interrupts: %d\n", ret);
>>> -             goto err_ce;
>>> -     }
>>> -
>>> -     ret = ath10k_pci_init_irq(ar);
>>> -     if (ret) {
>>> -             ath10k_err("failed to init irqs: %d\n", ret);
>>> -             goto err_ce;
>>> -     }
>>> -
>>>       ret = ath10k_pci_request_early_irq(ar);
>>>       if (ret) {
>>>               ath10k_err("failed to request early irq: %d\n", ret);
>>> -             goto err_deinit_irq;
>>> +             goto err_ce;
>>>       }
>>
>> You add ath10k_pci_ce_init() to probe() and respective
>> ath10k_pci_ce_deinit() to remove(), and you remove
>> ath10k_pci_ce_deinit() from hif_power_down(). But why do you leave
>> ath10k_pci_ce_init() to hif_power_up()? Isn't that unnecessary as we
>> already do that in probe()?
>
> Hmm.. I didn't check if copy engine register state (notable ring index
> values) is guaranteed to be usable in all cases yet and decided it's
> safer to just re-init it on each power up until it is proven it is not
> necessary.

So I have been trying to follow this naming scheme for initilisation:

_create() - called during device probe time
_start() - if up / power up
_stop() - if down / power down
_destroy() - called when device is removed

If you call ce_init() both in create() and start() time there has to be
some redundant code and I think the function should be split. This isn't
a concern now, but something for the future.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/5] ath10k: split ce irq/handler setup

[Michal Kazior]

On 14 August 2014 10:40, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> It doesn't make much sense to overwrite send_cb
>> and recv_cb callbacks over and over again whenever
>> transport starts. Just make sure to unmask copy
>> engine interrupts when starting.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> The patch looks, just a followup question for the future:
>
>> @@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
>>               pipe_info->hif_ce_state = ar;
>>               attr = &host_ce_config_wlan[pipe_num];
>>
>> -             ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
>> +             ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
>> +                                       ath10k_pci_ce_send_done,
>> +                                       ath10k_pci_ce_recv_data);
>
> As we call ath10k_ce_init_pipe() only once and seem to have only one set
> of functions, why even bother bother with function pointers? What if we
> just call the functions directly?

Yeah, we can remove this abstraction later. I don't there's anything
depending on it anymore.


Michał

Re: [PATCH 4/5] ath10k: split ce irq/handler setup

[Michal Kazior]

On 14 August 2014 10:40, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> It doesn't make much sense to overwrite send_cb
>> and recv_cb callbacks over and over again whenever
>> transport starts. Just make sure to unmask copy
>> engine interrupts when starting.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> The patch looks, just a followup question for the future:
>
>> @@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
>>               pipe_info->hif_ce_state = ar;
>>               attr = &host_ce_config_wlan[pipe_num];
>>
>> -             ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
>> +             ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
>> +                                       ath10k_pci_ce_send_done,
>> +                                       ath10k_pci_ce_recv_data);
>
> As we call ath10k_ce_init_pipe() only once and seem to have only one set
> of functions, why even bother bother with function pointers? What if we
> just call the functions directly?

Yeah, we can remove this abstraction later. I don't there's anything
depending on it anymore.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/5] ath10k: split ce irq/handler setup

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 14 August 2014 10:40, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> It doesn't make much sense to overwrite send_cb
>>> and recv_cb callbacks over and over again whenever
>>> transport starts. Just make sure to unmask copy
>>> engine interrupts when starting.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> The patch looks, just a followup question for the future:
>>
>>> @@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
>>>               pipe_info->hif_ce_state = ar;
>>>               attr = &host_ce_config_wlan[pipe_num];
>>>
>>> -             ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
>>> +             ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
>>> +                                       ath10k_pci_ce_send_done,
>>> +                                       ath10k_pci_ce_recv_data);
>>
>> As we call ath10k_ce_init_pipe() only once and seem to have only one set
>> of functions, why even bother bother with function pointers? What if we
>> just call the functions directly?
>
> Yeah, we can remove this abstraction later. I don't there's anything
> depending on it anymore.

Good, again something for a rainy day :)

I added it to the TODO list (which we should cleanup as well):

http://wireless.kernel.org/en/users/Drivers/ath10k/todo

-- 
Kalle Valo

Re: [PATCH 4/5] ath10k: split ce irq/handler setup

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 14 August 2014 10:40, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> It doesn't make much sense to overwrite send_cb
>>> and recv_cb callbacks over and over again whenever
>>> transport starts. Just make sure to unmask copy
>>> engine interrupts when starting.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> The patch looks, just a followup question for the future:
>>
>>> @@ -1674,7 +1639,9 @@ static int ath10k_pci_ce_init(struct ath10k *ar)
>>>               pipe_info->hif_ce_state = ar;
>>>               attr = &host_ce_config_wlan[pipe_num];
>>>
>>> -             ret = ath10k_ce_init_pipe(ar, pipe_num, attr);
>>> +             ret = ath10k_ce_init_pipe(ar, pipe_num, attr,
>>> +                                       ath10k_pci_ce_send_done,
>>> +                                       ath10k_pci_ce_recv_data);
>>
>> As we call ath10k_ce_init_pipe() only once and seem to have only one set
>> of functions, why even bother bother with function pointers? What if we
>> just call the functions directly?
>
> Yeah, we can remove this abstraction later. I don't there's anything
> depending on it anymore.

Good, again something for a rainy day :)

I added it to the TODO list (which we should cleanup as well):

http://wireless.kernel.org/en/users/Drivers/ath10k/todo

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k