2013-11-22 13:08:44

by Michal Kazior

Subject: [PATCH 0/8] ath10k: pci fixes 2013-11-22

Hi,

ath10k didn't play well with other devices on a
shared irq line. This patchset fixes support for
shared legacy interrupts.

Some rework was necessary because ath10k was using
disable_irq and CE irq masking (which is not
sufficient for shared interrupts).

Since main irq handlers are now registered after
boot, BMI is now polling for CE updates. I haven't
observed any differences in boot speed.

Also this plugs a leak I spotted during rework and
adds an option for testing different irq modes.


Michal Kazior (8):
ath10k: don't consume other's shared interrupts
ath10k: split up pci irq code
ath10k: fix memory leak on hif_start failpath
ath10k: don't use interrupts for BMI
ath10k: defer irq registration until hif start()
ath10k: extract functions for legacy irq handling
ath10k: re-add support for early fw indication
ath10k: allow explicit MSI/MSI-X disabling

drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/pci.c | 503 ++++++++++++++++++++++------------
drivers/net/wireless/ath/ath10k/pci.h | 1 +
3 files changed, 333 insertions(+), 172 deletions(-)

--
1.8.4.rc3

2013-11-22 13:08:45

by Michal Kazior

Subject: [PATCH 1/8] ath10k: don't consume other's shared interrupts

ath10k assumed all interrupts were directed to it.
This isn't the case for legacy shared interrupts.
ath10k consumed interrupts for other devices.

Check device irq status and return IRQ_NONE when
appropriate.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/pci.c | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 8aeb46d..9535eaa 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -269,6 +269,7 @@ enum ath10k_mcast2ucast_mode {
#define CORE_CTRL_CPU_INTR_MASK 0x00002000
#define CORE_CTRL_ADDRESS 0x0000
#define PCIE_INTR_ENABLE_ADDRESS 0x0008
+#define PCIE_INTR_CAUSE_ADDRESS 0x000c
#define PCIE_INTR_CLR_ADDRESS 0x0014
#define SCRATCH_3_ADDRESS 0x0030

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 2457c8b..ed752d6 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2075,6 +2075,24 @@ static irqreturn_t ath10k_pci_msi_fw_handler(int irq, void *arg)
return IRQ_HANDLED;
}

+static bool ath10k_pci_irq_pending(struct ath10k *ar)
+{
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ u32 cause;
+
+ /* MSIs are always exclusive */
+ if (ar_pci->num_msi_intrs > 0)
+ return true;
+
+ /* Check if the shared legacy irq is for us */
+ cause = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_CAUSE_ADDRESS);
+ if (cause & (PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL))
+ return true;
+
+ return false;
+}
+
/*
* Top-level interrupt handler for all PCI interrupts from a Target.
* When a block of MSI interrupts is allocated, this top-level handler
@@ -2085,6 +2103,9 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
struct ath10k *ar = arg;
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);

+ if (!ath10k_pci_irq_pending(ar))
+ return IRQ_NONE;
+
if (ar_pci->num_msi_intrs == 0) {
/*
* IMPORTANT: INTR_CLR regiser has to be set after
--
1.8.4.rc3

2013-11-22 13:08:43

by Michal Kazior

Subject: [PATCH 2/8] ath10k: split up pci irq code

Hardware waits until host signals whether it has
chosen MSI(-X) or shared legacy interrupts. It is
not required for the driver to register interrupt
handlers immediately.

This patch prepares the pci irq code for more
changes.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 225 ++++++++++++++++++++--------------
1 file changed, 131 insertions(+), 94 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index ed752d6..d4536cb 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -55,8 +55,10 @@ static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info);
static void ath10k_pci_stop_ce(struct ath10k *ar);
static int ath10k_pci_device_reset(struct ath10k *ar);
static int ath10k_pci_wait_for_target_init(struct ath10k *ar);
-static int ath10k_pci_start_intr(struct ath10k *ar);
-static void ath10k_pci_stop_intr(struct ath10k *ar);
+static int ath10k_pci_init_irq(struct ath10k *ar);
+static int ath10k_pci_deinit_irq(struct ath10k *ar);
+static int ath10k_pci_request_irq(struct ath10k *ar);
+static void ath10k_pci_free_irq(struct ath10k *ar);

static const struct ce_attr host_ce_config_wlan[] = {
/* CE0: host->target HTC control and raw streams */
@@ -1341,7 +1343,7 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);

/* Irqs are never explicitly re-enabled. They are implicitly re-enabled
- * by ath10k_pci_start_intr(). */
+ * by upon power_up. */
ath10k_pci_disable_irqs(ar);

ath10k_pci_stop_ce(ar);
@@ -1887,34 +1889,40 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
goto err_ce;
}

- ret = ath10k_pci_start_intr(ar);
+ ret = ath10k_pci_init_irq(ar);
if (ret) {
- ath10k_err("failed to start interrupt handling: %d\n", ret);
+ ath10k_err("failed to init irqs: %d\n", ret);
goto err_ce;
}

+ ret = ath10k_pci_request_irq(ar);
+ if (ret) {
+ ath10k_err("failed to request irqs: %d\n", ret);
+ goto err_deinit_irq;
+ }
+
ret = ath10k_pci_wait_for_target_init(ar);
if (ret) {
ath10k_err("failed to wait for target to init: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ret = ath10k_ce_enable_err_irq(ar);
if (ret) {
ath10k_err("failed to enable CE error irq: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ret = ath10k_pci_init_config(ar);
if (ret) {
ath10k_err("failed to setup init config: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ret = ath10k_pci_wake_target_cpu(ar);
if (ret) {
ath10k_err("could not wake up target CPU: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ath10k_pci_start_bmi(ar);
@@ -1931,11 +1939,12 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)

return 0;

-err_irq:
- ath10k_ce_disable_interrupts(ar);
- ath10k_pci_stop_intr(ar);
+err_free_irq:
+ ath10k_pci_free_irq(ar);
ath10k_pci_kill_tasklet(ar);
ath10k_pci_device_reset(ar);
+err_deinit_irq:
+ ath10k_pci_deinit_irq(ar);
err_ce:
ath10k_pci_ce_deinit(ar);
err_ps:
@@ -1949,7 +1958,8 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);

- ath10k_pci_stop_intr(ar);
+ ath10k_pci_free_irq(ar);
+ ath10k_pci_deinit_irq(ar);
ath10k_pci_device_reset(ar);

ath10k_pci_ce_deinit(ar);
@@ -2157,24 +2167,17 @@ static void ath10k_pci_tasklet(unsigned long data)
}
}

-static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
+static int ath10k_pci_request_irq_msix(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int ret;
- int i;
-
- ret = pci_enable_msi_block(ar_pci->pdev, num);
- if (ret)
- return ret;
+ int ret, i;

ret = request_irq(ar_pci->pdev->irq + MSI_ASSIGN_FW,
ath10k_pci_msi_fw_handler,
IRQF_SHARED, "ath10k_pci", ar);
if (ret) {
- ath10k_warn("request_irq(%d) failed %d\n",
+ ath10k_warn("failed to request MSI-X fw irq %d: %d\n",
ar_pci->pdev->irq + MSI_ASSIGN_FW, ret);
-
- pci_disable_msi(ar_pci->pdev);
return ret;
}

@@ -2183,45 +2186,38 @@ static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
ath10k_pci_per_engine_handler,
IRQF_SHARED, "ath10k_pci", ar);
if (ret) {
- ath10k_warn("request_irq(%d) failed %d\n",
+ ath10k_warn("failed to request MSI-X ce irq %d: %d\n",
ar_pci->pdev->irq + i, ret);

for (i--; i >= MSI_ASSIGN_CE_INITIAL; i--)
free_irq(ar_pci->pdev->irq + i, ar);

free_irq(ar_pci->pdev->irq + MSI_ASSIGN_FW, ar);
- pci_disable_msi(ar_pci->pdev);
return ret;
}
}

- ath10k_dbg(ATH10K_DBG_BOOT,
- "MSI-X interrupt handling (%d intrs)\n", num);
return 0;
}

-static int ath10k_pci_start_intr_msi(struct ath10k *ar)
+static int ath10k_pci_request_irq_msi(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;

- ret = pci_enable_msi(ar_pci->pdev);
- if (ret < 0)
- return ret;
-
ret = request_irq(ar_pci->pdev->irq,
ath10k_pci_interrupt_handler,
IRQF_SHARED, "ath10k_pci", ar);
- if (ret < 0) {
- pci_disable_msi(ar_pci->pdev);
+ if (ret) {
+ ath10k_warn("failed to request MSI irq %d: %d\n",
+ ar_pci->pdev->irq, ret);
return ret;
}

- ath10k_dbg(ATH10K_DBG_BOOT, "MSI interrupt handling\n");
return 0;
}

-static int ath10k_pci_start_intr_legacy(struct ath10k *ar)
+static int ath10k_pci_request_irq_legacy(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;
@@ -2229,99 +2225,140 @@ static int ath10k_pci_start_intr_legacy(struct ath10k *ar)
ret = request_irq(ar_pci->pdev->irq,
ath10k_pci_interrupt_handler,
IRQF_SHARED, "ath10k_pci", ar);
- if (ret < 0)
- return ret;
-
- ret = ath10k_pci_wake(ar);
if (ret) {
- free_irq(ar_pci->pdev->irq, ar);
- ath10k_err("failed to wake up target: %d\n", ret);
+ ath10k_warn("failed to request legacy irq %d: %d\n",
+ ar_pci->pdev->irq, ret);
return ret;
}

- /*
- * A potential race occurs here: The CORE_BASE write
- * depends on target correctly decoding AXI address but
- * host won't know when target writes BAR to CORE_CTRL.
- * This write might get lost if target has NOT written BAR.
- * For now, fix the race by repeating the write in below
- * synchronization checking.
- */
- iowrite32(PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL,
- ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
-
- ath10k_pci_sleep(ar);
- ath10k_dbg(ATH10K_DBG_BOOT, "legacy interrupt handling\n");
return 0;
}

-static int ath10k_pci_start_intr(struct ath10k *ar)
+static int ath10k_pci_request_irq(struct ath10k *ar)
+{
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+
+ switch (ar_pci->num_msi_intrs) {
+ case 0:
+ return ath10k_pci_request_irq_legacy(ar);
+ case 1:
+ return ath10k_pci_request_irq_msi(ar);
+ case MSI_NUM_REQUEST:
+ return ath10k_pci_request_irq_msix(ar);
+ }
+
+ ath10k_warn("unknown irq configuration upon request\n");
+ return -EINVAL;
+}
+
+static void ath10k_pci_free_irq(struct ath10k *ar)
+{
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ int i;
+
+ /* There's at least one interrupt irregardless whether its legacy INTR
+ * or MSI or MSI-X */
+ for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++)
+ free_irq(ar_pci->pdev->irq + i, ar);
+}
+
+static void ath10k_pci_init_irq_tasklets(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int num = MSI_NUM_REQUEST;
- int ret;
int i;

- tasklet_init(&ar_pci->intr_tq, ath10k_pci_tasklet, (unsigned long) 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);
+ (unsigned long)ar);

for (i = 0; i < CE_COUNT; i++) {
ar_pci->pipe_info[i].ar_pci = ar_pci;
- tasklet_init(&ar_pci->pipe_info[i].intr,
- ath10k_pci_ce_tasklet,
+ tasklet_init(&ar_pci->pipe_info[i].intr, ath10k_pci_ce_tasklet,
(unsigned long)&ar_pci->pipe_info[i]);
}
+}
+
+static int ath10k_pci_init_irq(struct ath10k *ar)
+{
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ int ret;
+
+ ath10k_pci_init_irq_tasklets(ar);

if (!test_bit(ATH10K_PCI_FEATURE_MSI_X, ar_pci->features))
- num = 1;
+ goto msi;

- if (num > 1) {
- ret = ath10k_pci_start_intr_msix(ar, num);
- if (ret == 0)
- goto exit;
+ /* Try MSI-X */
+ ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
+ 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);

- ath10k_dbg(ATH10K_DBG_BOOT,
- "MSI-X didn't succeed (%d), trying MSI\n", ret);
- num = 1;
- }
+msi:
+ /* Try MSI */
+ ar_pci->num_msi_intrs = 1;
+ ret = pci_enable_msi(ar_pci->pdev);
+ if (ret == 0)
+ return 0;

- if (num == 1) {
- ret = ath10k_pci_start_intr_msi(ar);
- if (ret == 0)
- goto exit;
+ /* Try legacy irq
+ *
+ * A potential race occurs here: The CORE_BASE write
+ * depends on target correctly decoding AXI address but
+ * host won't know when target writes BAR to CORE_CTRL.
+ * This write might get lost if target has NOT written BAR.
+ * For now, fix the race by repeating the write in below
+ * synchronization checking. */
+ ar_pci->num_msi_intrs = 0;

- ath10k_dbg(ATH10K_DBG_BOOT,
- "MSI didn't succeed (%d), trying legacy INTR\n",
- ret);
- num = 0;
+ ret = ath10k_pci_wake(ar);
+ if (ret) {
+ ath10k_warn("failed to wake target: %d\n", ret);
+ return ret;
}

- ret = ath10k_pci_start_intr_legacy(ar);
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_ENABLE_ADDRESS,
+ PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
+ ath10k_pci_sleep(ar);
+
+ return 0;
+}
+
+static int ath10k_pci_deinit_irq_legacy(struct ath10k *ar)
+{
+ int ret;
+
+ ret = ath10k_pci_wake(ar);
if (ret) {
- ath10k_warn("Failed to start legacy interrupts: %d\n", ret);
+ ath10k_warn("failed to wake target: %d\n", ret);
return ret;
}

-exit:
- ar_pci->num_msi_intrs = num;
- return ret;
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_ENABLE_ADDRESS,
+ 0);
+ ath10k_pci_sleep(ar);
+
+ return 0;
}

-static void ath10k_pci_stop_intr(struct ath10k *ar)
+static int ath10k_pci_deinit_irq(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int i;

- /* There's at least one interrupt irregardless whether its legacy INTR
- * or MSI or MSI-X */
- for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++)
- free_irq(ar_pci->pdev->irq + i, ar);
-
- if (ar_pci->num_msi_intrs > 0)
+ switch (ar_pci->num_msi_intrs) {
+ case 0:
+ return ath10k_pci_deinit_irq_legacy(ar);
+ case 1:
+ /* fall-through */
+ case MSI_NUM_REQUEST:
pci_disable_msi(ar_pci->pdev);
+ return 0;
+ }
+
+ ath10k_warn("unknown irq configuration upon deinit\n");
+ return -EINVAL;
}

static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
--
1.8.4.rc3

2013-11-25 13:10:28

by Michal Kazior

Subject: [PATCH v2 5/8] ath10k: defer irq registration until hif start()

It's impossible to rely on disable_irq() and/or CE
interrupt masking with legacy shared interrupts.
Other devices sharing the same irq line may assert
it while ath10k is doing something that requires
no interrupts.

Irq handlers are now registered after all
preparations are complete so spurious/foreign
interrupts won't do any harm. The handlers are
unregistered when no interrupts are required (i.e.
during driver teardown).

This also removes the ability to receive FW early
indication (since interrupts are not registered
until early boot is complete). This is not mission
critical (it's more of a hint that early boot
failed due to unexpected FW crash) and will be
re-added in a follow up patch.

Signed-off-by: Michal Kazior <[email protected]>
---
v2:
* remove ce_enable_err_irq() - dont enable device
interrupts before irq handlers are registered
* with the earlier pci_start_ce() decoupling
patch, irq handlers are now registered after CE
completions are allocated but before CE
interrupts are enabled via HW registers
(fixes 'irq: nobody cared')

drivers/net/wireless/ath/ath10k/ce.c | 15 --------
drivers/net/wireless/ath/ath10k/ce.h | 1 -
drivers/net/wireless/ath/ath10k/pci.c | 65 +++++++++++++----------------------
3 files changed, 23 insertions(+), 58 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 476928f..d44d618 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -792,21 +792,6 @@ static void ath10k_ce_per_engine_handler_adjust(struct ath10k_ce_pipe *ce_state,
ath10k_pci_sleep(ar);
}

-int ath10k_ce_enable_err_irq(struct ath10k *ar)
-{
- int i, ret;
-
- ret = ath10k_pci_wake(ar);
- if (ret)
- return ret;
-
- for (i = 0; i < CE_COUNT; i++)
- ath10k_ce_error_intr_enable(ar, ath10k_ce_base_address(i));
-
- ath10k_pci_sleep(ar);
- return 0;
-}
-
int ath10k_ce_disable_interrupts(struct ath10k *ar)
{
int ce_id, ret;
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index 8a58bac..67dbde6 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -235,7 +235,6 @@ void ath10k_ce_deinit(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);
-int ath10k_ce_enable_err_irq(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 05e6f8b..bd67311 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -919,13 +919,6 @@ static void ath10k_pci_stop_ce(struct ath10k *ar)
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
struct ath10k_pci_compl *compl;
struct sk_buff *skb;
- int ret;
-
- ret = ath10k_ce_disable_interrupts(ar);
- if (ret)
- ath10k_warn("failed to disable CE interrupts: %d\n", ret);
-
- ath10k_pci_kill_tasklet(ar);

/* Mark pending completions as aborted, so that upper layers free up
* their associated resources */
@@ -1236,10 +1229,17 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
return ret;
}

+ ret = ath10k_pci_request_irq(ar);
+ if (ret) {
+ ath10k_warn("failed to post RX buffers for all pipes: %d\n",
+ ret);
+ goto err_free_compl;
+ }
+
ret = ath10k_pci_setup_ce_irq(ar);
if (ret) {
ath10k_warn("failed to setup CE interrupts: %d\n", ret);
- goto err_free_compl;
+ goto err_stop;
}

/* Post buffers once to start things off. */
@@ -1247,13 +1247,16 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
if (ret) {
ath10k_warn("failed to post RX buffers for all pipes: %d\n",
ret);
- goto err_stop_ce;
+ goto err_stop;
}

ar_pci->started = 1;
return 0;

-err_stop_ce:
+err_stop:
+ ath10k_ce_disable_interrupts(ar);
+ ath10k_pci_free_irq(ar);
+ ath10k_pci_kill_tasklet(ar);
ath10k_pci_stop_ce(ar);
ath10k_pci_process_ce(ar);
err_free_compl:
@@ -1376,25 +1379,19 @@ static void ath10k_pci_ce_deinit(struct ath10k *ar)
}
}

-static void ath10k_pci_disable_irqs(struct ath10k *ar)
-{
- struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int i;
-
- for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++)
- disable_irq(ar_pci->pdev->irq + i);
-}
-
static void ath10k_pci_hif_stop(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ int ret;

ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);

- /* Irqs are never explicitly re-enabled. They are implicitly re-enabled
- * by upon power_up. */
- ath10k_pci_disable_irqs(ar);
+ ret = ath10k_ce_disable_interrupts(ar);
+ if (ret)
+ ath10k_warn("failed to disable CE interrupts: %d\n", ret);

+ ath10k_pci_free_irq(ar);
+ ath10k_pci_kill_tasklet(ar);
ath10k_pci_stop_ce(ar);

/* At this point, asynchronous threads are stopped, the target should
@@ -1945,34 +1942,22 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
goto err_ce;
}

- ret = ath10k_pci_request_irq(ar);
- if (ret) {
- ath10k_err("failed to request irqs: %d\n", ret);
- goto err_deinit_irq;
- }
-
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_irq;
- }
-
- ret = ath10k_ce_enable_err_irq(ar);
- if (ret) {
- ath10k_err("failed to enable CE error irq: %d\n", ret);
- goto err_free_irq;
+ goto err_deinit_irq;
}

ret = ath10k_pci_init_config(ar);
if (ret) {
ath10k_err("failed to setup init config: %d\n", ret);
- goto err_free_irq;
+ goto err_deinit_irq;
}

ret = ath10k_pci_wake_target_cpu(ar);
if (ret) {
ath10k_err("could not wake up target CPU: %d\n", ret);
- goto err_free_irq;
+ goto err_deinit_irq;
}

if (ar_pci->num_msi_intrs > 1)
@@ -1987,14 +1972,11 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)

return 0;

-err_free_irq:
- ath10k_pci_free_irq(ar);
- ath10k_pci_kill_tasklet(ar);
- ath10k_pci_device_reset(ar);
err_deinit_irq:
ath10k_pci_deinit_irq(ar);
err_ce:
ath10k_pci_ce_deinit(ar);
+ ath10k_pci_device_reset(ar);
err_ps:
if (!test_bit(ATH10K_PCI_FEATURE_SOC_POWER_SAVE, ar_pci->features))
ath10k_do_pci_sleep(ar);
@@ -2006,7 +1988,6 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);

- ath10k_pci_free_irq(ar);
ath10k_pci_deinit_irq(ar);
ath10k_pci_device_reset(ar);

--
1.8.4.rc3

2013-11-27 14:48:22

by Kalle Valo

Subject: Re: [PATCH v2 0/8] ath10k: pci fixes 2013-11-22

Michal Kazior <[email protected]> writes:

> Hi,
>
> ath10k didn't play well with other devices on a
> shared irq line. This patchset fixes support for
> shared legacy interrupts.
>
> Some rework was necessary because ath10k was using
> disable_irq and CE irq masking (which is not
> sufficient for shared interrupts).
>
> Since main irq handlers are now registered after
> boot, BMI is now polling for CE updates. I haven't
> observed any differences in boot speed.
>
> Also this plugs a leak I spotted during rework and
> adds an option for testing different irq modes.
>
> v2:
> * simplify ath10k_pci_irq_pending()
> * combine memory leak fix patch with a
> functionality decoupling (it's very closely
> related)
> * fix 'irq: nobody cared'
> * change MSI/MSI-X disabling parameters
> * some minor fixes & code shuffling to avoid
> forward declarations
>
>
> Michal Kazior (8):
> ath10k: don't consume other's shared interrupts
> ath10k: split up pci irq code
> ath10k: don't use interrupts for BMI
> ath10k: decouple ath10k_pci_start_ce()
> ath10k: defer irq registration until hif start()
> ath10k: extract functions for legacy irq handling
> ath10k: re-add support for early fw indication
> ath10k: allow explicit MSI/MSI-X disabling

Thanks, all eight applied.

--
Kalle Valo

2013-11-25 13:10:19

by Michal Kazior

Subject: [PATCH v2 0/8] ath10k: pci fixes 2013-11-22

Hi,

ath10k didn't play well with other devices on a
shared irq line. This patchset fixes support for
shared legacy interrupts.

Some rework was necessary because ath10k was using
disable_irq and CE irq masking (which is not
sufficient for shared interrupts).

Since main irq handlers are now registered after
boot, BMI is now polling for CE updates. I haven't
observed any differences in boot speed.

Also this plugs a leak I spotted during rework and
adds an option for testing different irq modes.

v2:
* simplify ath10k_pci_irq_pending()
* combine memory leak fix patch with a
functionality decoupling (it's very closely
related)
* fix 'irq: nobody cared'
* change MSI/MSI-X disabling parameters
* some minor fixes & code shuffling to avoid
forward declarations


Michal Kazior (8):
ath10k: don't consume other's shared interrupts
ath10k: split up pci irq code
ath10k: don't use interrupts for BMI
ath10k: decouple ath10k_pci_start_ce()
ath10k: defer irq registration until hif start()
ath10k: extract functions for legacy irq handling
ath10k: re-add support for early fw indication
ath10k: allow explicit MSI/MSI-X disabling

drivers/net/wireless/ath/ath10k/ce.c | 15 -
drivers/net/wireless/ath/ath10k/ce.h | 1 -
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/pci.c | 577 ++++++++++++++++++++++------------
drivers/net/wireless/ath/ath10k/pci.h | 1 +
5 files changed, 383 insertions(+), 212 deletions(-)

--
1.8.4.rc3

2013-11-22 13:08:43

by Michal Kazior

Subject: [PATCH 3/8] ath10k: fix memory leak on hif_start failpath

If post_rx failed then completions were not freed.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index d4536cb..4067890 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1205,6 +1205,7 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
if (ret) {
ath10k_warn("failed to post RX buffers for all pipes: %d\n",
ret);
+ ath10k_pci_stop_ce(ar);
return ret;
}

--
1.8.4.rc3

2013-11-25 12:20:34

by Kalle Valo

Subject: Re: [PATCH 7/8] ath10k: re-add support for early fw indication

Michal Kazior <[email protected]> writes:

> It's possible for FW to panic during early boot or
> at driver teardown in some rare cases.
>
> The patch re-introduces support to detect and
> print those crashes.
>
> This introduces an additional irq handler that is
> set for the duration of early boot and shutdown.
> The handler is then overriden with regular
> handlers upon hif start().
>
> Signed-off-by: Michal Kazior <[email protected]>

[...]

> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -59,6 +59,8 @@ static int ath10k_pci_init_irq(struct ath10k *ar);
> static int ath10k_pci_deinit_irq(struct ath10k *ar);
> static int ath10k_pci_request_irq(struct ath10k *ar);
> static void ath10k_pci_free_irq(struct ath10k *ar);
> +static int ath10k_pci_request_early_irq(struct ath10k *ar);
> +static void ath10k_pci_free_early_irq(struct ath10k *ar);

We should always try to avoid using forward declarations. If I
understood correctly these are not needed.

> static int ath10k_pci_bmi_wait(struct ath10k_ce_pipe *tx_pipe,
> struct ath10k_ce_pipe *rx_pipe,
> struct bmi_xfer *xfer);
> @@ -876,6 +878,7 @@ 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);
> @@ -1188,12 +1191,15 @@ 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;
> + int ret, ret2;
> +
> + ath10k_pci_free_early_irq(ar);
> + ath10k_pci_kill_tasklet(ar);

Calling kill_tasklet() looks a bit odd here when you only want to kill
early_irq_tasklet. But I guess that's ok.

> ret = ath10k_pci_start_ce(ar);
> if (ret) {
> ath10k_warn("failed to start CE: %d\n", ret);
> - return ret;
> + goto err_early_irq;
> }
>
> ret = ath10k_pci_request_irq(ar);
> @@ -1219,6 +1225,11 @@ err_free_irq:
> ath10k_pci_kill_tasklet(ar);
> err_stop_ce:
> ath10k_pci_stop_ce(ar);
> +err_early_irq:
> + ret2 = ath10k_pci_request_early_irq(ar);
> + if (ret2)
> + ath10k_warn("failed to re-enable early irq: %d\n", ret2);

ret_early or something like that would be a nicer name.

> @@ -1952,6 +1975,9 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
> {
> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>
> + ath10k_ce_disable_interrupts(ar);
> + ath10k_pci_free_early_irq(ar);
> + ath10k_pci_kill_tasklet(ar);

Should disable_interrupts() and kill_tasklet() be in an earlier patch?

--
Kalle Valo

2013-11-25 13:10:28

by Michal Kazior

Subject: [PATCH v2 8/8] ath10k: allow explicit MSI/MSI-X disabling

This can be useful for testing and debugging.

This introduces new ath10k_pci module parameter
`irq_mode`. By default it is 0, meaning automatic
irq mode (MSI-X as long as both target HW and host
platform supports it). The parameter works on a
best effort basis.

Signed-off-by: Michal Kazior <[email protected]>
---
v2:
* merge 2 parameters into 1
* add info print if the mode is != auto

drivers/net/wireless/ath/ath10k/pci.c | 47 +++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 0a2d1c2..9606b9b 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -33,10 +33,21 @@
#include "ce.h"
#include "pci.h"

+enum ath10k_pci_irq_mode {
+ ATH10K_PCI_IRQ_AUTO = 0,
+ ATH10K_PCI_IRQ_LEGACY = 1,
+ ATH10K_PCI_IRQ_MSI = 2,
+};
+
static unsigned int ath10k_target_ps;
+static unsigned int ath10k_pci_irq_mode = ATH10K_PCI_IRQ_AUTO;
+
module_param(ath10k_target_ps, uint, 0644);
MODULE_PARM_DESC(ath10k_target_ps, "Enable ath10k Target (SoC) PS option");

+module_param_named(irq_mode, ath10k_pci_irq_mode, uint, 0644);
+MODULE_PARM_DESC(irq_mode, "0: ayto, 1: legacy, 2: msi (default: 0)");
+
#define QCA988X_2_0_DEVICE_ID (0x003c)

static DEFINE_PCI_DEVICE_TABLE(ath10k_pci_id_table) = {
@@ -2387,27 +2398,37 @@ static void ath10k_pci_init_irq_tasklets(struct ath10k *ar)
static int ath10k_pci_init_irq(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ bool msix_supported = test_bit(ATH10K_PCI_FEATURE_MSI_X,
+ ar_pci->features);
int ret;

ath10k_pci_init_irq_tasklets(ar);

- if (!test_bit(ATH10K_PCI_FEATURE_MSI_X, ar_pci->features))
- goto msi;
+ if (ath10k_pci_irq_mode != ATH10K_PCI_IRQ_AUTO &&
+ !test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
+ ath10k_info("limiting irq mode to: %d\n", ath10k_pci_irq_mode);

/* Try MSI-X */
- ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
- 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);
+ 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)
+ return 0;
+ if (ret > 0)
+ pci_disable_msi(ar_pci->pdev);
+
+ /* fall-through */
+ }

-msi:
/* Try MSI */
- ar_pci->num_msi_intrs = 1;
- ret = pci_enable_msi(ar_pci->pdev);
- if (ret == 0)
- return 0;
+ if (ath10k_pci_irq_mode != ATH10K_PCI_IRQ_LEGACY) {
+ ar_pci->num_msi_intrs = 1;
+ ret = pci_enable_msi(ar_pci->pdev);
+ if (ret == 0)
+ return 0;
+
+ /* fall-through */
+ }

/* Try legacy irq
*
--
1.8.4.rc3

2013-11-25 13:10:24

by Michal Kazior

Subject: [PATCH v2 4/8] ath10k: decouple ath10k_pci_start_ce()

The function did a couple of things: it allocated
CE completions, registered CE callbacks and
enabled CE interrupts through HW registers.

This cannot be so. Split the function into one
that allocates CE completions and the other one
that starts off CE operation.

This is required for future legacy shared
interrupt handling.

This also fixes possible memory leak if post rx
failed.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 75 +++++++++++++++++++++++++----------
1 file changed, 54 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 327afbc..05e6f8b 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -62,6 +62,7 @@ static void ath10k_pci_free_irq(struct ath10k *ar);
static int ath10k_pci_bmi_wait(struct ath10k_ce_pipe *tx_pipe,
struct ath10k_ce_pipe *rx_pipe,
struct bmi_xfer *xfer);
+static void ath10k_pci_cleanup_ce(struct ath10k *ar);

static const struct ce_attr host_ce_config_wlan[] = {
/* CE0: host->target HTC control and raw streams */
@@ -824,14 +825,13 @@ static void ath10k_pci_hif_set_callbacks(struct ath10k *ar,
sizeof(ar_pci->msg_callbacks_current));
}

-static int ath10k_pci_start_ce(struct ath10k *ar)
+static int ath10k_pci_alloc_compl(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- struct ath10k_ce_pipe *ce_diag = ar_pci->ce_diag;
const struct ce_attr *attr;
struct ath10k_pci_pipe *pipe_info;
struct ath10k_pci_compl *compl;
- int i, pipe_num, completions, disable_interrupts;
+ int i, pipe_num, completions;

spin_lock_init(&ar_pci->compl_lock);
INIT_LIST_HEAD(&ar_pci->compl_process);
@@ -843,34 +843,23 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
INIT_LIST_HEAD(&pipe_info->compl_free);

/* Handle Diagnostic CE specially */
- if (pipe_info->ce_hdl == ce_diag)
+ if (pipe_info->ce_hdl == ar_pci->ce_diag)
continue;

attr = &host_ce_config_wlan[pipe_num];
completions = 0;

- 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->src_nentries)
completions += attr->src_nentries;
- }

- if (attr->dest_nentries) {
- ath10k_ce_recv_cb_register(pipe_info->ce_hdl,
- ath10k_pci_ce_recv_data);
+ if (attr->dest_nentries)
completions += attr->dest_nentries;
- }
-
- if (completions == 0)
- continue;

for (i = 0; i < completions; i++) {
compl = kmalloc(sizeof(*compl), GFP_KERNEL);
if (!compl) {
ath10k_warn("No memory for completion state\n");
- ath10k_pci_stop_ce(ar);
+ ath10k_pci_cleanup_ce(ar);
return -ENOMEM;
}

@@ -882,6 +871,37 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
return 0;
}

+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);
@@ -1210,22 +1230,35 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;

- ret = ath10k_pci_start_ce(ar);
+ ret = ath10k_pci_alloc_compl(ar);
if (ret) {
- ath10k_warn("failed to start CE: %d\n", ret);
+ ath10k_warn("failed to allocate CE completions: %d\n", ret);
return ret;
}

+ ret = ath10k_pci_setup_ce_irq(ar);
+ if (ret) {
+ ath10k_warn("failed to setup CE interrupts: %d\n", ret);
+ goto err_free_compl;
+ }
+
/* 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);
- return ret;
+ goto err_stop_ce;
}

ar_pci->started = 1;
return 0;
+
+err_stop_ce:
+ ath10k_pci_stop_ce(ar);
+ ath10k_pci_process_ce(ar);
+err_free_compl:
+ ath10k_pci_cleanup_ce(ar);
+ return ret;
}

static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
--
1.8.4.rc3

2013-11-25 13:10:28

by Michal Kazior

Subject: [PATCH v2 7/8] ath10k: re-add support for early fw indication

It's possible for FW to panic during early boot.

The patch re-introduces support to detect and
print those crashes.

This introduces an additional irq handler that is
set for the duration of early boot and shutdown.
The handler is then overriden with regular
handlers upon hif start().

Signed-off-by: Michal Kazior <[email protected]>
---
v2:
* move function definitions up to avoid forward
declarations
* forward declarations are gone now
* rename ret2 to ret_early

drivers/net/wireless/ath/ath10k/pci.c | 106 ++++++++++++++++++++++++++++++++--
drivers/net/wireless/ath/ath10k/pci.h | 1 +
2 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 1d2939c..0a2d1c2 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -248,6 +248,46 @@ 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);
+}
+
/*
* Diagnostic read/write access is provided for startup/config/debug usage.
* Caller must guarantee proper alignment, when applicable, and single user
@@ -937,6 +977,7 @@ 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);
@@ -1249,12 +1290,15 @@ 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;
+ int ret, ret_early;
+
+ ath10k_pci_free_early_irq(ar);
+ ath10k_pci_kill_tasklet(ar);

ret = ath10k_pci_alloc_compl(ar);
if (ret) {
ath10k_warn("failed to allocate CE completions: %d\n", ret);
- return ret;
+ goto err_early_irq;
}

ret = ath10k_pci_request_irq(ar);
@@ -1289,6 +1333,14 @@ err_stop:
ath10k_pci_process_ce(ar);
err_free_compl:
ath10k_pci_cleanup_ce(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;
}

@@ -1422,6 +1474,10 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
ath10k_pci_kill_tasklet(ar);
ath10k_pci_stop_ce(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. */
@@ -1970,22 +2026,28 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
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;
+ }
+
ret = ath10k_pci_wait_for_target_init(ar);
if (ret) {
ath10k_err("failed to wait for target to init: %d\n", ret);
- goto err_deinit_irq;
+ goto err_free_early_irq;
}

ret = ath10k_pci_init_config(ar);
if (ret) {
ath10k_err("failed to setup init config: %d\n", ret);
- goto err_deinit_irq;
+ goto err_free_early_irq;
}

ret = ath10k_pci_wake_target_cpu(ar);
if (ret) {
ath10k_err("could not wake up target CPU: %d\n", ret);
- goto err_deinit_irq;
+ goto err_free_early_irq;
}

if (ar_pci->num_msi_intrs > 1)
@@ -2000,6 +2062,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)

return 0;

+err_free_early_irq:
+ ath10k_pci_free_early_irq(ar);
err_deinit_irq:
ath10k_pci_deinit_irq(ar);
err_ce:
@@ -2016,6 +2080,8 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);

+ ath10k_pci_free_early_irq(ar);
+ ath10k_pci_kill_tasklet(ar);
ath10k_pci_deinit_irq(ar);
ath10k_pci_device_reset(ar);

@@ -2164,6 +2230,34 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
return IRQ_HANDLED;
}

+static void ath10k_pci_early_irq_tasklet(unsigned long data)
+{
+ struct ath10k *ar = (struct ath10k *)data;
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ u32 fw_ind;
+ int ret;
+
+ ret = ath10k_pci_wake(ar);
+ if (ret) {
+ ath10k_warn("failed to wake target in early irq tasklet: %d\n",
+ ret);
+ return;
+ }
+
+ fw_ind = ath10k_pci_read32(ar, ar_pci->fw_indicator_address);
+ if (fw_ind & FW_IND_EVENT_PENDING) {
+ ath10k_pci_write32(ar, ar_pci->fw_indicator_address,
+ fw_ind & ~FW_IND_EVENT_PENDING);
+
+ /* Some structures are unavailable during early boot or at
+ * driver teardown so just print that the device has crashed. */
+ ath10k_warn("device crashed - no diagnostics available\n");
+ }
+
+ ath10k_pci_sleep(ar);
+ ath10k_pci_enable_legacy_irq(ar);
+}
+
static void ath10k_pci_tasklet(unsigned long data)
{
struct ath10k *ar = (struct ath10k *)data;
@@ -2280,6 +2374,8 @@ 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;
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 73a3d4e..a4f3203 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -198,6 +198,7 @@ struct ath10k_pci {

struct tasklet_struct intr_tq;
struct tasklet_struct msi_fw_err;
+ struct tasklet_struct early_irq_tasklet;

int started;

--
1.8.4.rc3

2013-11-22 13:08:47

by Michal Kazior

Subject: [PATCH 7/8] ath10k: re-add support for early fw indication

It's possible for FW to panic during early boot or
at driver teardown in some rare cases.

The patch re-introduces support to detect and
print those crashes.

This introduces an additional irq handler that is
set for the duration of early boot and shutdown.
The handler is then overriden with regular
handlers upon hif start().

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 107 ++++++++++++++++++++++++++++++++--
drivers/net/wireless/ath/ath10k/pci.h | 1 +
2 files changed, 102 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a9bc290..47d9b6e 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -59,6 +59,8 @@ static int ath10k_pci_init_irq(struct ath10k *ar);
static int ath10k_pci_deinit_irq(struct ath10k *ar);
static int ath10k_pci_request_irq(struct ath10k *ar);
static void ath10k_pci_free_irq(struct ath10k *ar);
+static int ath10k_pci_request_early_irq(struct ath10k *ar);
+static void ath10k_pci_free_early_irq(struct ath10k *ar);
static int ath10k_pci_bmi_wait(struct ath10k_ce_pipe *tx_pipe,
struct ath10k_ce_pipe *rx_pipe,
struct bmi_xfer *xfer);
@@ -876,6 +878,7 @@ 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);
@@ -1188,12 +1191,15 @@ 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;
+ int ret, ret2;
+
+ ath10k_pci_free_early_irq(ar);
+ ath10k_pci_kill_tasklet(ar);

ret = ath10k_pci_start_ce(ar);
if (ret) {
ath10k_warn("failed to start CE: %d\n", ret);
- return ret;
+ goto err_early_irq;
}

ret = ath10k_pci_request_irq(ar);
@@ -1219,6 +1225,11 @@ err_free_irq:
ath10k_pci_kill_tasklet(ar);
err_stop_ce:
ath10k_pci_stop_ce(ar);
+err_early_irq:
+ ret2 = ath10k_pci_request_early_irq(ar);
+ if (ret2)
+ ath10k_warn("failed to re-enable early irq: %d\n", ret2);
+
return ret;
}

@@ -1352,6 +1363,10 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
ath10k_pci_kill_tasklet(ar);
ath10k_pci_stop_ce(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. */
@@ -1900,28 +1915,34 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
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;
+ }
+
ret = ath10k_pci_wait_for_target_init(ar);
if (ret) {
ath10k_err("failed to wait for target to init: %d\n", ret);
- goto err_deinit_irq;
+ goto err_free_early_irq;
}

ret = ath10k_ce_enable_err_irq(ar);
if (ret) {
ath10k_err("failed to enable CE error irq: %d\n", ret);
- goto err_deinit_irq;
+ goto err_free_early_irq;
}

ret = ath10k_pci_init_config(ar);
if (ret) {
ath10k_err("failed to setup init config: %d\n", ret);
- goto err_deinit_irq;
+ goto err_free_early_irq;
}

ret = ath10k_pci_wake_target_cpu(ar);
if (ret) {
ath10k_err("could not wake up target CPU: %d\n", ret);
- goto err_deinit_irq;
+ goto err_free_early_irq;
}

if (ar_pci->num_msi_intrs > 1)
@@ -1936,6 +1957,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)

return 0;

+err_free_early_irq:
+ ath10k_pci_free_early_irq(ar);
err_deinit_irq:
ath10k_pci_deinit_irq(ar);
err_ce:
@@ -1952,6 +1975,9 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);

+ ath10k_ce_disable_interrupts(ar);
+ ath10k_pci_free_early_irq(ar);
+ ath10k_pci_kill_tasklet(ar);
ath10k_pci_deinit_irq(ar);
ath10k_pci_device_reset(ar);

@@ -2145,6 +2171,50 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
return IRQ_HANDLED;
}

+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 (!ath10k_pci_irq_pending(ar))
+ return IRQ_NONE;
+
+ if (ar_pci->num_msi_intrs == 0)
+ ath10k_pci_disable_and_clear_legacy_irq(ar);
+
+ tasklet_schedule(&ar_pci->early_irq_tasklet);
+
+ return IRQ_HANDLED;
+}
+
+static void ath10k_pci_early_irq_tasklet(unsigned long data)
+{
+ struct ath10k *ar = (struct ath10k *)data;
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ u32 fw_ind;
+ int ret;
+
+ ret = ath10k_pci_wake(ar);
+ if (ret) {
+ ath10k_warn("failed to wake target in early irq tasklet: %d\n",
+ ret);
+ return;
+ }
+
+ fw_ind = ath10k_pci_read32(ar, ar_pci->fw_indicator_address);
+ if (fw_ind & FW_IND_EVENT_PENDING) {
+ ath10k_pci_write32(ar, ar_pci->fw_indicator_address,
+ fw_ind & ~FW_IND_EVENT_PENDING);
+
+ /* Some structures are unavailable during early boot or at
+ * driver teardown so just print that the device has crashed. */
+ ath10k_warn("device crashed - no diagnostics available\n");
+ }
+
+ ath10k_pci_sleep(ar);
+ ath10k_pci_enable_legacy_irq(ar);
+}
+
static void ath10k_pci_tasklet(unsigned long data)
{
struct ath10k *ar = (struct ath10k *)data;
@@ -2253,6 +2323,29 @@ static void ath10k_pci_free_irq(struct ath10k *ar)
free_irq(ar_pci->pdev->irq + i, ar);
}

+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 void ath10k_pci_init_irq_tasklets(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -2261,6 +2354,8 @@ 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;
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 73a3d4e..a4f3203 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -198,6 +198,7 @@ struct ath10k_pci {

struct tasklet_struct intr_tq;
struct tasklet_struct msi_fw_err;
+ struct tasklet_struct early_irq_tasklet;

int started;

--
1.8.4.rc3

2013-11-27 14:47:55

by Kalle Valo

Subject: Re: [PATCH v2 8/8] ath10k: allow explicit MSI/MSI-X disabling

Michal Kazior <[email protected]> writes:

> This can be useful for testing and debugging.
>
> This introduces new ath10k_pci module parameter
> `irq_mode`. By default it is 0, meaning automatic
> irq mode (MSI-X as long as both target HW and host
> platform supports it). The parameter works on a
> best effort basis.
>
> Signed-off-by: Michal Kazior <[email protected]>

[...]

> +MODULE_PARM_DESC(irq_mode, "0: ayto, 1: legacy, 2: msi (default: 0)");

I fixed this typo ("ayto").

--
Kalle Valo

2013-11-25 12:01:58

by Kalle Valo

Subject: Re: [PATCH 8/8] ath10k: allow explicit MSI/MSI-X disabling

Michal Kazior <[email protected]> writes:

> This can be useful for testing and debugging.
>
> Two new ath10k_pci module options are now available:
>
> disable_msix - disable just MSI-X
> disable_msi - disable MSI (along with MSI-X)
>
> Signed-off-by: Michal Kazior <[email protected]>

This is useful but I think it's a bit too much to have two more options,
it would be simpler to have just one. Few quick ideas how to do this a
bit differently:

irq_mode: 0 = automatic, 2 = MSI, 1 = legacy

Here the problem is of course that we can't force use of MSI if the host
only supports legacy interrupts, but IMHO that's a minor nuisance and in
that case we should just enable legacy interrupts.

disable_msi: 0x1 = disable MSI-X, 0x2 = disable MSI

Basically this is just your booleans converted to a bitfield.

I vote for irq_mode because I think that's easier for the user to
understand. But I'm sure there is a better way to do this.

> + if (msix_supported && !ath10k_disable_msix && !ath10k_disable_msi) {
> + ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
> + 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 */
> + }

If we force some other irq mode than the default it would be good to
have an info print for that.

--
Kalle Valo

2013-11-22 13:08:45

by Michal Kazior

Subject: [PATCH 5/8] ath10k: defer irq registration until hif start()

It's impossible to rely on disable_irq() and/or CE
interrupt masking with legacy shared interrupts.
Other devices sharing the same irq line may assert
it while ath10k is doing something that requires
no interrupts.

Irq handlers are now registered after all
preparations are complete so spurious/foreign
interrupts won't do any harm. The handlers are
unregistered when no interrupts are required (i.e.
during driver teardown).

This also removes the ability to receive FW early
indication (since interrupts are not registered
until early boot is complete). This is not mission
critical (it's more of a hint that early boot
failed due to unexpected FW crash) and will be
re-added in a follow up patch.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 62 +++++++++++++++--------------------
1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 1b12db8..fa1162e 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -886,13 +886,6 @@ static void ath10k_pci_stop_ce(struct ath10k *ar)
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
struct ath10k_pci_compl *compl;
struct sk_buff *skb;
- int ret;
-
- ret = ath10k_ce_disable_interrupts(ar);
- if (ret)
- ath10k_warn("failed to disable CE interrupts: %d\n", ret);
-
- ath10k_pci_kill_tasklet(ar);

/* Mark pending completions as aborted, so that upper layers free up
* their associated resources */
@@ -1203,17 +1196,30 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
return ret;
}

+ ret = ath10k_pci_request_irq(ar);
+ if (ret) {
+ ath10k_warn("failed to post RX buffers for all pipes: %d\n",
+ ret);
+ goto err_stop_ce;
+ }
+
/* 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);
- ath10k_pci_stop_ce(ar);
- return ret;
+ goto err_free_irq;
}

ar_pci->started = 1;
return 0;
+
+err_free_irq:
+ ath10k_pci_free_irq(ar);
+ ath10k_pci_kill_tasklet(ar);
+err_stop_ce:
+ ath10k_pci_stop_ce(ar);
+ return ret;
}

static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
@@ -1331,25 +1337,19 @@ static void ath10k_pci_ce_deinit(struct ath10k *ar)
}
}

-static void ath10k_pci_disable_irqs(struct ath10k *ar)
-{
- struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int i;
-
- for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++)
- disable_irq(ar_pci->pdev->irq + i);
-}
-
static void ath10k_pci_hif_stop(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ int ret;

ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);

- /* Irqs are never explicitly re-enabled. They are implicitly re-enabled
- * by upon power_up. */
- ath10k_pci_disable_irqs(ar);
+ ret = ath10k_ce_disable_interrupts(ar);
+ if (ret)
+ ath10k_warn("failed to disable CE interrupts: %d\n", ret);

+ ath10k_pci_free_irq(ar);
+ ath10k_pci_kill_tasklet(ar);
ath10k_pci_stop_ce(ar);

/* At this point, asynchronous threads are stopped, the target should
@@ -1900,34 +1900,28 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
goto err_ce;
}

- ret = ath10k_pci_request_irq(ar);
- if (ret) {
- ath10k_err("failed to request irqs: %d\n", ret);
- goto err_deinit_irq;
- }
-
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_irq;
+ goto err_deinit_irq;
}

ret = ath10k_ce_enable_err_irq(ar);
if (ret) {
ath10k_err("failed to enable CE error irq: %d\n", ret);
- goto err_free_irq;
+ goto err_deinit_irq;
}

ret = ath10k_pci_init_config(ar);
if (ret) {
ath10k_err("failed to setup init config: %d\n", ret);
- goto err_free_irq;
+ goto err_deinit_irq;
}

ret = ath10k_pci_wake_target_cpu(ar);
if (ret) {
ath10k_err("could not wake up target CPU: %d\n", ret);
- goto err_free_irq;
+ goto err_deinit_irq;
}

if (ar_pci->num_msi_intrs > 1)
@@ -1942,14 +1936,11 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)

return 0;

-err_free_irq:
- ath10k_pci_free_irq(ar);
- ath10k_pci_kill_tasklet(ar);
- ath10k_pci_device_reset(ar);
err_deinit_irq:
ath10k_pci_deinit_irq(ar);
err_ce:
ath10k_pci_ce_deinit(ar);
+ ath10k_pci_device_reset(ar);
err_ps:
if (!test_bit(ATH10K_PCI_FEATURE_SOC_POWER_SAVE, ar_pci->features))
ath10k_do_pci_sleep(ar);
@@ -1961,7 +1952,6 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);

- ath10k_pci_free_irq(ar);
ath10k_pci_deinit_irq(ar);
ath10k_pci_device_reset(ar);

--
1.8.4.rc3

2013-11-25 13:10:22

by Michal Kazior

Subject: [PATCH v2 2/8] ath10k: split up pci irq code

Hardware waits until host signals whether it has
chosen MSI(-X) or shared legacy interrupts. It is
not required for the driver to register interrupt
handlers immediately.

This patch prepares the pci irq code for more
changes.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 225 ++++++++++++++++++++--------------
1 file changed, 131 insertions(+), 94 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 12fb12e..957fc59 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -55,8 +55,10 @@ static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info);
static void ath10k_pci_stop_ce(struct ath10k *ar);
static int ath10k_pci_device_reset(struct ath10k *ar);
static int ath10k_pci_wait_for_target_init(struct ath10k *ar);
-static int ath10k_pci_start_intr(struct ath10k *ar);
-static void ath10k_pci_stop_intr(struct ath10k *ar);
+static int ath10k_pci_init_irq(struct ath10k *ar);
+static int ath10k_pci_deinit_irq(struct ath10k *ar);
+static int ath10k_pci_request_irq(struct ath10k *ar);
+static void ath10k_pci_free_irq(struct ath10k *ar);

static const struct ce_attr host_ce_config_wlan[] = {
/* CE0: host->target HTC control and raw streams */
@@ -1354,7 +1356,7 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__);

/* Irqs are never explicitly re-enabled. They are implicitly re-enabled
- * by ath10k_pci_start_intr(). */
+ * by upon power_up. */
ath10k_pci_disable_irqs(ar);

ath10k_pci_stop_ce(ar);
@@ -1900,34 +1902,40 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
goto err_ce;
}

- ret = ath10k_pci_start_intr(ar);
+ ret = ath10k_pci_init_irq(ar);
if (ret) {
- ath10k_err("failed to start interrupt handling: %d\n", ret);
+ ath10k_err("failed to init irqs: %d\n", ret);
goto err_ce;
}

+ ret = ath10k_pci_request_irq(ar);
+ if (ret) {
+ ath10k_err("failed to request irqs: %d\n", ret);
+ goto err_deinit_irq;
+ }
+
ret = ath10k_pci_wait_for_target_init(ar);
if (ret) {
ath10k_err("failed to wait for target to init: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ret = ath10k_ce_enable_err_irq(ar);
if (ret) {
ath10k_err("failed to enable CE error irq: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ret = ath10k_pci_init_config(ar);
if (ret) {
ath10k_err("failed to setup init config: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ret = ath10k_pci_wake_target_cpu(ar);
if (ret) {
ath10k_err("could not wake up target CPU: %d\n", ret);
- goto err_irq;
+ goto err_free_irq;
}

ath10k_pci_start_bmi(ar);
@@ -1944,11 +1952,12 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)

return 0;

-err_irq:
- ath10k_ce_disable_interrupts(ar);
- ath10k_pci_stop_intr(ar);
+err_free_irq:
+ ath10k_pci_free_irq(ar);
ath10k_pci_kill_tasklet(ar);
ath10k_pci_device_reset(ar);
+err_deinit_irq:
+ ath10k_pci_deinit_irq(ar);
err_ce:
ath10k_pci_ce_deinit(ar);
err_ps:
@@ -1962,7 +1971,8 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);

- ath10k_pci_stop_intr(ar);
+ ath10k_pci_free_irq(ar);
+ ath10k_pci_deinit_irq(ar);
ath10k_pci_device_reset(ar);

ath10k_pci_ce_deinit(ar);
@@ -2152,24 +2162,17 @@ static void ath10k_pci_tasklet(unsigned long data)
}
}

-static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
+static int ath10k_pci_request_irq_msix(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int ret;
- int i;
-
- ret = pci_enable_msi_block(ar_pci->pdev, num);
- if (ret)
- return ret;
+ int ret, i;

ret = request_irq(ar_pci->pdev->irq + MSI_ASSIGN_FW,
ath10k_pci_msi_fw_handler,
IRQF_SHARED, "ath10k_pci", ar);
if (ret) {
- ath10k_warn("request_irq(%d) failed %d\n",
+ ath10k_warn("failed to request MSI-X fw irq %d: %d\n",
ar_pci->pdev->irq + MSI_ASSIGN_FW, ret);
-
- pci_disable_msi(ar_pci->pdev);
return ret;
}

@@ -2178,45 +2181,38 @@ static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
ath10k_pci_per_engine_handler,
IRQF_SHARED, "ath10k_pci", ar);
if (ret) {
- ath10k_warn("request_irq(%d) failed %d\n",
+ ath10k_warn("failed to request MSI-X ce irq %d: %d\n",
ar_pci->pdev->irq + i, ret);

for (i--; i >= MSI_ASSIGN_CE_INITIAL; i--)
free_irq(ar_pci->pdev->irq + i, ar);

free_irq(ar_pci->pdev->irq + MSI_ASSIGN_FW, ar);
- pci_disable_msi(ar_pci->pdev);
return ret;
}
}

- ath10k_dbg(ATH10K_DBG_BOOT,
- "MSI-X interrupt handling (%d intrs)\n", num);
return 0;
}

-static int ath10k_pci_start_intr_msi(struct ath10k *ar)
+static int ath10k_pci_request_irq_msi(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;

- ret = pci_enable_msi(ar_pci->pdev);
- if (ret < 0)
- return ret;
-
ret = request_irq(ar_pci->pdev->irq,
ath10k_pci_interrupt_handler,
IRQF_SHARED, "ath10k_pci", ar);
- if (ret < 0) {
- pci_disable_msi(ar_pci->pdev);
+ if (ret) {
+ ath10k_warn("failed to request MSI irq %d: %d\n",
+ ar_pci->pdev->irq, ret);
return ret;
}

- ath10k_dbg(ATH10K_DBG_BOOT, "MSI interrupt handling\n");
return 0;
}

-static int ath10k_pci_start_intr_legacy(struct ath10k *ar)
+static int ath10k_pci_request_irq_legacy(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
int ret;
@@ -2224,99 +2220,140 @@ static int ath10k_pci_start_intr_legacy(struct ath10k *ar)
ret = request_irq(ar_pci->pdev->irq,
ath10k_pci_interrupt_handler,
IRQF_SHARED, "ath10k_pci", ar);
- if (ret < 0)
- return ret;
-
- ret = ath10k_pci_wake(ar);
if (ret) {
- free_irq(ar_pci->pdev->irq, ar);
- ath10k_err("failed to wake up target: %d\n", ret);
+ ath10k_warn("failed to request legacy irq %d: %d\n",
+ ar_pci->pdev->irq, ret);
return ret;
}

- /*
- * A potential race occurs here: The CORE_BASE write
- * depends on target correctly decoding AXI address but
- * host won't know when target writes BAR to CORE_CTRL.
- * This write might get lost if target has NOT written BAR.
- * For now, fix the race by repeating the write in below
- * synchronization checking.
- */
- iowrite32(PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL,
- ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
-
- ath10k_pci_sleep(ar);
- ath10k_dbg(ATH10K_DBG_BOOT, "legacy interrupt handling\n");
return 0;
}

-static int ath10k_pci_start_intr(struct ath10k *ar)
+static int ath10k_pci_request_irq(struct ath10k *ar)
+{
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+
+ switch (ar_pci->num_msi_intrs) {
+ case 0:
+ return ath10k_pci_request_irq_legacy(ar);
+ case 1:
+ return ath10k_pci_request_irq_msi(ar);
+ case MSI_NUM_REQUEST:
+ return ath10k_pci_request_irq_msix(ar);
+ }
+
+ ath10k_warn("unknown irq configuration upon request\n");
+ return -EINVAL;
+}
+
+static void ath10k_pci_free_irq(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int num = MSI_NUM_REQUEST;
- int ret;
int i;

- tasklet_init(&ar_pci->intr_tq, ath10k_pci_tasklet, (unsigned long) ar);
+ /* There's at least one interrupt irregardless whether its legacy INTR
+ * or MSI or MSI-X */
+ for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++)
+ free_irq(ar_pci->pdev->irq + i, ar);
+}
+
+static void ath10k_pci_init_irq_tasklets(struct ath10k *ar)
+{
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ int i;
+
+ 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);
+ (unsigned long)ar);

for (i = 0; i < CE_COUNT; i++) {
ar_pci->pipe_info[i].ar_pci = ar_pci;
- tasklet_init(&ar_pci->pipe_info[i].intr,
- ath10k_pci_ce_tasklet,
+ tasklet_init(&ar_pci->pipe_info[i].intr, ath10k_pci_ce_tasklet,
(unsigned long)&ar_pci->pipe_info[i]);
}
+}
+
+static int ath10k_pci_init_irq(struct ath10k *ar)
+{
+ struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ int ret;
+
+ ath10k_pci_init_irq_tasklets(ar);

if (!test_bit(ATH10K_PCI_FEATURE_MSI_X, ar_pci->features))
- num = 1;
+ goto msi;

- if (num > 1) {
- ret = ath10k_pci_start_intr_msix(ar, num);
- if (ret == 0)
- goto exit;
+ /* Try MSI-X */
+ ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
+ 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);

- ath10k_dbg(ATH10K_DBG_BOOT,
- "MSI-X didn't succeed (%d), trying MSI\n", ret);
- num = 1;
- }
+msi:
+ /* Try MSI */
+ ar_pci->num_msi_intrs = 1;
+ ret = pci_enable_msi(ar_pci->pdev);
+ if (ret == 0)
+ return 0;

- if (num == 1) {
- ret = ath10k_pci_start_intr_msi(ar);
- if (ret == 0)
- goto exit;
+ /* Try legacy irq
+ *
+ * A potential race occurs here: The CORE_BASE write
+ * depends on target correctly decoding AXI address but
+ * host won't know when target writes BAR to CORE_CTRL.
+ * This write might get lost if target has NOT written BAR.
+ * For now, fix the race by repeating the write in below
+ * synchronization checking. */
+ ar_pci->num_msi_intrs = 0;

- ath10k_dbg(ATH10K_DBG_BOOT,
- "MSI didn't succeed (%d), trying legacy INTR\n",
- ret);
- num = 0;
+ ret = ath10k_pci_wake(ar);
+ if (ret) {
+ ath10k_warn("failed to wake target: %d\n", ret);
+ return ret;
}

- ret = ath10k_pci_start_intr_legacy(ar);
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_ENABLE_ADDRESS,
+ PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
+ ath10k_pci_sleep(ar);
+
+ return 0;
+}
+
+static int ath10k_pci_deinit_irq_legacy(struct ath10k *ar)
+{
+ int ret;
+
+ ret = ath10k_pci_wake(ar);
if (ret) {
- ath10k_warn("Failed to start legacy interrupts: %d\n", ret);
+ ath10k_warn("failed to wake target: %d\n", ret);
return ret;
}

-exit:
- ar_pci->num_msi_intrs = num;
- return ret;
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_ENABLE_ADDRESS,
+ 0);
+ ath10k_pci_sleep(ar);
+
+ return 0;
}

-static void ath10k_pci_stop_intr(struct ath10k *ar)
+static int ath10k_pci_deinit_irq(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- int i;

- /* There's at least one interrupt irregardless whether its legacy INTR
- * or MSI or MSI-X */
- for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++)
- free_irq(ar_pci->pdev->irq + i, ar);
-
- if (ar_pci->num_msi_intrs > 0)
+ switch (ar_pci->num_msi_intrs) {
+ case 0:
+ return ath10k_pci_deinit_irq_legacy(ar);
+ case 1:
+ /* fall-through */
+ case MSI_NUM_REQUEST:
pci_disable_msi(ar_pci->pdev);
+ return 0;
+ }
+
+ ath10k_warn("unknown irq configuration upon deinit\n");
+ return -EINVAL;
}

static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
--
1.8.4.rc3

2013-11-25 13:10:26

by Michal Kazior

Subject: [PATCH v2 6/8] ath10k: extract functions for legacy irq handling

Preparation for code re-use. Also use ioread/write
wrappers.

Signed-off-by: Michal Kazior <[email protected]>
---
v2:
* move function definitions up to avoid forward
declarations in subsequent patches

drivers/net/wireless/ath/ath10k/pci.c | 65 +++++++++++++++++------------------
1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index bd67311..1d2939c 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -220,6 +220,34 @@ static bool ath10k_pci_irq_pending(struct ath10k *ar)
return false;
}

+static void ath10k_pci_disable_and_clear_legacy_irq(struct ath10k *ar)
+{
+ /* IMPORTANT: INTR_CLR register has to be set after
+ * INTR_ENABLE is set to 0, otherwise interrupt can not be
+ * really cleared. */
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_ENABLE_ADDRESS,
+ 0);
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_CLR_ADDRESS,
+ PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
+
+ /* IMPORTANT: this extra read transaction is required to
+ * flush the posted write buffer. */
+ (void) ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_ENABLE_ADDRESS);
+}
+
+static void ath10k_pci_enable_legacy_irq(struct ath10k *ar)
+{
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_ENABLE_ADDRESS,
+ PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
+
+ /* IMPORTANT: this extra read transaction is required to
+ * flush the posted write buffer. */
+ (void) ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_ENABLE_ADDRESS);
+}
+
/*
* Diagnostic read/write access is provided for startup/config/debug usage.
* Caller must guarantee proper alignment, when applicable, and single user
@@ -2128,25 +2156,7 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
if (!ath10k_pci_irq_pending(ar))
return IRQ_NONE;

- /*
- * IMPORTANT: INTR_CLR regiser has to be set after
- * INTR_ENABLE is set to 0, otherwise interrupt can not be
- * really cleared.
- */
- iowrite32(0, ar_pci->mem +
- (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
- iowrite32(PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL,
- ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_CLR_ADDRESS));
- /*
- * IMPORTANT: this extra read transaction is required to
- * flush the posted write buffer.
- */
- (void) ioread32(ar_pci->mem +
- (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
+ ath10k_pci_disable_and_clear_legacy_irq(ar);
}

tasklet_schedule(&ar_pci->intr_tq);
@@ -2162,20 +2172,9 @@ static void ath10k_pci_tasklet(unsigned long data)
ath10k_pci_fw_interrupt_handler(ar); /* FIXME: Handle FW error */
ath10k_ce_per_engine_service_any(ar);

- if (ar_pci->num_msi_intrs == 0) {
- /* Enable Legacy PCI line interrupts */
- iowrite32(PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL,
- ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
- /*
- * IMPORTANT: this extra read transaction is required to
- * flush the posted write buffer
- */
- (void) ioread32(ar_pci->mem +
- (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
- }
+ /* Re-enable legacy irq that was disabled in the irq handler */
+ if (ar_pci->num_msi_intrs == 0)
+ ath10k_pci_enable_legacy_irq(ar);
}

static int ath10k_pci_request_irq_msix(struct ath10k *ar)
--
1.8.4.rc3

2013-11-25 12:46:26

by Michal Kazior

Subject: Re: [PATCH 7/8] ath10k: re-add support for early fw indication

On 25 November 2013 13:20, Kalle Valo <[email protected]> wrote:
> Michal Kazior <[email protected]> writes:
>
>> It's possible for FW to panic during early boot or
>> at driver teardown in some rare cases.
>>
>> The patch re-introduces support to detect and
>> print those crashes.
>>
>> This introduces an additional irq handler that is
>> set for the duration of early boot and shutdown.
>> The handler is then overriden with regular
>> handlers upon hif start().
>>
>> Signed-off-by: Michal Kazior <[email protected]>

[...]

>> @@ -1952,6 +1975,9 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
>> {
>> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>>
>> + ath10k_ce_disable_interrupts(ar);
>> + ath10k_pci_free_early_irq(ar);
>> + ath10k_pci_kill_tasklet(ar);
>
> Should disable_interrupts() and kill_tasklet() be in an earlier patch?

No. Before this patch there are no interrupt handlers registered by
power_up, so there are no interrupts to be cleaned up in power_down.

Since this patch introduces early irq handling in power_up, then
power_down must shut everything down. Now that I think about the
ath10k_ce_disable_interrupts() isn't necessary here. The other two
functions are though.


Micha?

2013-11-22 13:08:44

by Michal Kazior

Subject: [PATCH 4/8] ath10k: don't use interrupts for BMI

It's not really necessary for interrupts to be
used for BMI. BMI already assumes there's only one
caller at a time and it works directly with CE.

Make BMI poll for CE completions instead of
waiting for interrupts. This makes disabling
interrupts during early boot possible.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 50 ++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 4067890..1b12db8 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -59,6 +59,9 @@ static int ath10k_pci_init_irq(struct ath10k *ar);
static int ath10k_pci_deinit_irq(struct ath10k *ar);
static int ath10k_pci_request_irq(struct ath10k *ar);
static void ath10k_pci_free_irq(struct ath10k *ar);
+static int ath10k_pci_bmi_wait(struct ath10k_ce_pipe *tx_pipe,
+ struct ath10k_ce_pipe *rx_pipe,
+ struct bmi_xfer *xfer);

static const struct ce_attr host_ce_config_wlan[] = {
/* CE0: host->target HTC control and raw streams */
@@ -1382,6 +1385,8 @@ static int ath10k_pci_hif_exchange_bmi_msg(struct ath10k *ar,
void *treq, *tresp = NULL;
int ret = 0;

+ might_sleep();
+
if (resp && !resp_len)
return -EINVAL;

@@ -1422,14 +1427,12 @@ static int ath10k_pci_hif_exchange_bmi_msg(struct ath10k *ar,
if (ret)
goto err_resp;

- ret = wait_for_completion_timeout(&xfer.done,
- BMI_COMMUNICATION_TIMEOUT_HZ);
- if (ret <= 0) {
+ ret = ath10k_pci_bmi_wait(ce_tx, ce_rx, &xfer);
+ if (ret) {
u32 unused_buffer;
unsigned int unused_nbytes;
unsigned int unused_id;

- ret = -ETIMEDOUT;
ath10k_ce_cancel_send_next(ce_tx, NULL, &unused_buffer,
&unused_nbytes, &unused_id);
} else {
@@ -1497,6 +1500,25 @@ static void ath10k_pci_bmi_recv_data(struct ath10k_ce_pipe *ce_state)
complete(&xfer->done);
}

+static int ath10k_pci_bmi_wait(struct ath10k_ce_pipe *tx_pipe,
+ struct ath10k_ce_pipe *rx_pipe,
+ struct bmi_xfer *xfer)
+{
+ unsigned long timeout = jiffies + BMI_COMMUNICATION_TIMEOUT_HZ;
+
+ while (time_before_eq(jiffies, timeout)) {
+ ath10k_pci_bmi_send_done(tx_pipe);
+ ath10k_pci_bmi_recv_data(rx_pipe);
+
+ if (completion_done(&xfer->done))
+ return 0;
+
+ schedule();
+ }
+
+ return -ETIMEDOUT;
+}
+
/*
* Map from service/endpoint to Copy Engine.
* This table is derived from the CE_PCI TABLE, above.
@@ -1834,24 +1856,6 @@ static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
ath10k_pci_sleep(ar);
}

-static void ath10k_pci_start_bmi(struct ath10k *ar)
-{
- struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- struct ath10k_pci_pipe *pipe;
-
- /*
- * Initially, establish CE completion handlers for use with BMI.
- * These are overwritten with generic handlers after we exit BMI phase.
- */
- pipe = &ar_pci->pipe_info[BMI_CE_NUM_TO_TARG];
- ath10k_ce_send_cb_register(pipe->ce_hdl, ath10k_pci_bmi_send_done, 0);
-
- pipe = &ar_pci->pipe_info[BMI_CE_NUM_TO_HOST];
- ath10k_ce_recv_cb_register(pipe->ce_hdl, ath10k_pci_bmi_recv_data);
-
- ath10k_dbg(ATH10K_DBG_BOOT, "boot start bmi\n");
-}
-
static int ath10k_pci_hif_power_up(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -1926,8 +1930,6 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
goto err_free_irq;
}

- ath10k_pci_start_bmi(ar);
-
if (ar_pci->num_msi_intrs > 1)
irq_mode = "MSI-X";
else if (ar_pci->num_msi_intrs == 1)
--
1.8.4.rc3

2013-11-24 13:59:54

by Kalle Valo

Subject: Re: [PATCH 1/8] ath10k: don't consume other's shared interrupts

Michal Kazior <[email protected]> writes:

> ath10k assumed all interrupts were directed to it.
> This isn't the case for legacy shared interrupts.
> ath10k consumed interrupts for other devices.
>
> Check device irq status and return IRQ_NONE when
> appropriate.
>
> Signed-off-by: Michal Kazior <[email protected]>

[...]

> @@ -2085,6 +2103,9 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
> struct ath10k *ar = arg;
> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>
> + if (!ath10k_pci_irq_pending(ar))
> + return IRQ_NONE;
> +
> if (ar_pci->num_msi_intrs == 0) {
> /*
> * IMPORTANT: INTR_CLR regiser has to be set after

What if you move ath10k_pci_irq_pending() call after
ar_pci->num_msi_intrs == 0 check? That way you could remove the
"ar_pci->num_msi_intrs == 0" check from irq_pending() function.

--
Kalle Valo

2013-11-25 13:10:20

by Michal Kazior

Subject: [PATCH v2 1/8] ath10k: don't consume other's shared interrupts

ath10k assumed all interrupts were directed to it.
This isn't the case for legacy shared interrupts.
ath10k consumed interrupts for other devices.

Check device irq status and return IRQ_NONE when
appropriate.

Signed-off-by: Michal Kazior <[email protected]>
---
v2:
* move function definition up to avoid forward
declaration in subsequent patches
* remove check for legacy interrupts inside the
irq_pending() by guaranteeing it is called
only for legacy interrupts


drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/pci.c | 16 ++++++++++++++++
2 files changed, 17 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 8aeb46d..9535eaa 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -269,6 +269,7 @@ enum ath10k_mcast2ucast_mode {
#define CORE_CTRL_CPU_INTR_MASK 0x00002000
#define CORE_CTRL_ADDRESS 0x0000
#define PCIE_INTR_ENABLE_ADDRESS 0x0008
+#define PCIE_INTR_CAUSE_ADDRESS 0x000c
#define PCIE_INTR_CLR_ADDRESS 0x0014
#define SCRATCH_3_ADDRESS 0x0030

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 2457c8b..12fb12e 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -201,6 +201,19 @@ static const struct ce_pipe_config target_ce_config_wlan[] = {
/* CE7 used only by Host */
};

+static bool ath10k_pci_irq_pending(struct ath10k *ar)
+{
+ u32 cause;
+
+ /* Check if the shared legacy irq is for us */
+ cause = ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_CAUSE_ADDRESS);
+ if (cause & (PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL))
+ return true;
+
+ return false;
+}
+
/*
* Diagnostic read/write access is provided for startup/config/debug usage.
* Caller must guarantee proper alignment, when applicable, and single user
@@ -2086,6 +2099,9 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *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;
+
/*
* IMPORTANT: INTR_CLR regiser has to be set after
* INTR_ENABLE is set to 0, otherwise interrupt can not be
--
1.8.4.rc3

2013-11-22 13:08:47

by Michal Kazior

Subject: [PATCH 8/8] ath10k: allow explicit MSI/MSI-X disabling

This can be useful for testing and debugging.

Two new ath10k_pci module options are now available:

disable_msix - disable just MSI-X
disable_msi - disable MSI (along with MSI-X)

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 43 +++++++++++++++++++++++------------
1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 47d9b6e..b89fd7b 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -34,9 +34,18 @@
#include "pci.h"

static unsigned int ath10k_target_ps;
+static bool ath10k_disable_msix;
+static bool ath10k_disable_msi;
+
module_param(ath10k_target_ps, uint, 0644);
MODULE_PARM_DESC(ath10k_target_ps, "Enable ath10k Target (SoC) PS option");

+module_param_named(disable_msix, ath10k_disable_msix, bool, 0644);
+MODULE_PARM_DESC(disable_msix, "Disable MSI-X support");
+
+module_param_named(disable_msi, ath10k_disable_msi, bool, 0644);
+MODULE_PARM_DESC(disable_msi, "Disable MSI support");
+
#define QCA988X_2_0_DEVICE_ID (0x003c)

static DEFINE_PCI_DEVICE_TABLE(ath10k_pci_id_table) = {
@@ -2367,27 +2376,33 @@ static void ath10k_pci_init_irq_tasklets(struct ath10k *ar)
static int ath10k_pci_init_irq(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+ bool msix_supported = test_bit(ATH10K_PCI_FEATURE_MSI_X,
+ ar_pci->features);
int ret;

ath10k_pci_init_irq_tasklets(ar);

- if (!test_bit(ATH10K_PCI_FEATURE_MSI_X, ar_pci->features))
- goto msi;
-
/* Try MSI-X */
- ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
- 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);
+ if (msix_supported && !ath10k_disable_msix && !ath10k_disable_msi) {
+ ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
+ 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 */
+ }

-msi:
/* Try MSI */
- ar_pci->num_msi_intrs = 1;
- ret = pci_enable_msi(ar_pci->pdev);
- if (ret == 0)
- return 0;
+ if (!ath10k_disable_msi) {
+ ar_pci->num_msi_intrs = 1;
+ ret = pci_enable_msi(ar_pci->pdev);
+ if (ret == 0)
+ return 0;
+
+ /* fall-through */
+ }

/* Try legacy irq
*
--
1.8.4.rc3

2013-11-22 13:08:46

by Michal Kazior

Subject: [PATCH 6/8] ath10k: extract functions for legacy irq handling

Preparation for code re-use. Also use ioread/write
wrappers.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 68 +++++++++++++++++------------------
1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index fa1162e..a9bc290 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2096,6 +2096,34 @@ static bool ath10k_pci_irq_pending(struct ath10k *ar)
return false;
}

+static void ath10k_pci_disable_and_clear_legacy_irq(struct ath10k *ar)
+{
+ /* IMPORTANT: INTR_CLR register has to be set after
+ * INTR_ENABLE is set to 0, otherwise interrupt can not be
+ * really cleared. */
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_ENABLE_ADDRESS,
+ 0);
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_CLR_ADDRESS,
+ PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
+
+ /* IMPORTANT: this extra read transaction is required to
+ * flush the posted write buffer. */
+ (void) ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_ENABLE_ADDRESS);
+}
+
+static void ath10k_pci_enable_legacy_irq(struct ath10k *ar)
+{
+ ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_ENABLE_ADDRESS,
+ PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
+
+ /* IMPORTANT: this extra read transaction is required to
+ * flush the posted write buffer. */
+ (void) ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
+ PCIE_INTR_ENABLE_ADDRESS);
+}
+
/*
* Top-level interrupt handler for all PCI interrupts from a Target.
* When a block of MSI interrupts is allocated, this top-level handler
@@ -2109,27 +2137,8 @@ static irqreturn_t ath10k_pci_interrupt_handler(int irq, void *arg)
if (!ath10k_pci_irq_pending(ar))
return IRQ_NONE;

- if (ar_pci->num_msi_intrs == 0) {
- /*
- * IMPORTANT: INTR_CLR regiser has to be set after
- * INTR_ENABLE is set to 0, otherwise interrupt can not be
- * really cleared.
- */
- iowrite32(0, ar_pci->mem +
- (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
- iowrite32(PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL,
- ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_CLR_ADDRESS));
- /*
- * IMPORTANT: this extra read transaction is required to
- * flush the posted write buffer.
- */
- (void) ioread32(ar_pci->mem +
- (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
- }
+ if (ar_pci->num_msi_intrs == 0)
+ ath10k_pci_disable_and_clear_legacy_irq(ar);

tasklet_schedule(&ar_pci->intr_tq);

@@ -2144,20 +2153,9 @@ static void ath10k_pci_tasklet(unsigned long data)
ath10k_pci_fw_interrupt_handler(ar); /* FIXME: Handle FW error */
ath10k_ce_per_engine_service_any(ar);

- if (ar_pci->num_msi_intrs == 0) {
- /* Enable Legacy PCI line interrupts */
- iowrite32(PCIE_INTR_FIRMWARE_MASK |
- PCIE_INTR_CE_MASK_ALL,
- ar_pci->mem + (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
- /*
- * IMPORTANT: this extra read transaction is required to
- * flush the posted write buffer
- */
- (void) ioread32(ar_pci->mem +
- (SOC_CORE_BASE_ADDRESS |
- PCIE_INTR_ENABLE_ADDRESS));
- }
+ /* Re-enable legacy irq that was disabled in the irq handler */
+ if (ar_pci->num_msi_intrs == 0)
+ ath10k_pci_enable_legacy_irq(ar);
}

static int ath10k_pci_request_irq_msix(struct ath10k *ar)
--
1.8.4.rc3

2013-11-25 13:10:23

by Michal Kazior

Subject: [PATCH v2 3/8] ath10k: don't use interrupts for BMI

It's not really necessary for interrupts to be
used for BMI. BMI already assumes there's only one
caller at a time and it works directly with CE.

Make BMI poll for CE completions instead of
waiting for interrupts. This makes disabling
interrupts during early boot possible.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 50 ++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 957fc59..327afbc 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -59,6 +59,9 @@ static int ath10k_pci_init_irq(struct ath10k *ar);
static int ath10k_pci_deinit_irq(struct ath10k *ar);
static int ath10k_pci_request_irq(struct ath10k *ar);
static void ath10k_pci_free_irq(struct ath10k *ar);
+static int ath10k_pci_bmi_wait(struct ath10k_ce_pipe *tx_pipe,
+ struct ath10k_ce_pipe *rx_pipe,
+ struct bmi_xfer *xfer);

static const struct ce_attr host_ce_config_wlan[] = {
/* CE0: host->target HTC control and raw streams */
@@ -1394,6 +1397,8 @@ static int ath10k_pci_hif_exchange_bmi_msg(struct ath10k *ar,
void *treq, *tresp = NULL;
int ret = 0;

+ might_sleep();
+
if (resp && !resp_len)
return -EINVAL;

@@ -1434,14 +1439,12 @@ static int ath10k_pci_hif_exchange_bmi_msg(struct ath10k *ar,
if (ret)
goto err_resp;

- ret = wait_for_completion_timeout(&xfer.done,
- BMI_COMMUNICATION_TIMEOUT_HZ);
- if (ret <= 0) {
+ ret = ath10k_pci_bmi_wait(ce_tx, ce_rx, &xfer);
+ if (ret) {
u32 unused_buffer;
unsigned int unused_nbytes;
unsigned int unused_id;

- ret = -ETIMEDOUT;
ath10k_ce_cancel_send_next(ce_tx, NULL, &unused_buffer,
&unused_nbytes, &unused_id);
} else {
@@ -1509,6 +1512,25 @@ static void ath10k_pci_bmi_recv_data(struct ath10k_ce_pipe *ce_state)
complete(&xfer->done);
}

+static int ath10k_pci_bmi_wait(struct ath10k_ce_pipe *tx_pipe,
+ struct ath10k_ce_pipe *rx_pipe,
+ struct bmi_xfer *xfer)
+{
+ unsigned long timeout = jiffies + BMI_COMMUNICATION_TIMEOUT_HZ;
+
+ while (time_before_eq(jiffies, timeout)) {
+ ath10k_pci_bmi_send_done(tx_pipe);
+ ath10k_pci_bmi_recv_data(rx_pipe);
+
+ if (completion_done(&xfer->done))
+ return 0;
+
+ schedule();
+ }
+
+ return -ETIMEDOUT;
+}
+
/*
* Map from service/endpoint to Copy Engine.
* This table is derived from the CE_PCI TABLE, above.
@@ -1846,24 +1868,6 @@ static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
ath10k_pci_sleep(ar);
}

-static void ath10k_pci_start_bmi(struct ath10k *ar)
-{
- struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- struct ath10k_pci_pipe *pipe;
-
- /*
- * Initially, establish CE completion handlers for use with BMI.
- * These are overwritten with generic handlers after we exit BMI phase.
- */
- pipe = &ar_pci->pipe_info[BMI_CE_NUM_TO_TARG];
- ath10k_ce_send_cb_register(pipe->ce_hdl, ath10k_pci_bmi_send_done, 0);
-
- pipe = &ar_pci->pipe_info[BMI_CE_NUM_TO_HOST];
- ath10k_ce_recv_cb_register(pipe->ce_hdl, ath10k_pci_bmi_recv_data);
-
- ath10k_dbg(ATH10K_DBG_BOOT, "boot start bmi\n");
-}
-
static int ath10k_pci_hif_power_up(struct ath10k *ar)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -1938,8 +1942,6 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
goto err_free_irq;
}

- ath10k_pci_start_bmi(ar);
-
if (ar_pci->num_msi_intrs > 1)
irq_mode = "MSI-X";
else if (ar_pci->num_msi_intrs == 1)
--
1.8.4.rc3