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

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

[Michal Kazior]

Hi,

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

This is related to start/restart sequences.

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


Michal Kazior (5):
  ath10k: rework posting pci rx buffers
  ath10k: dont reset the chip on hif_stop
  ath10k: ignore ar_pci->started in pipe cleanup
  ath10k: remove ar_pci->started
  ath10k: flush hif buffers before recovery

 drivers/net/wireless/ath/ath10k/ce.c   |  71 +++++----
 drivers/net/wireless/ath/ath10k/ce.h   |  16 +-
 drivers/net/wireless/ath/ath10k/core.c |   1 +
 drivers/net/wireless/ath/ath10k/hif.h  |   3 +-
 drivers/net/wireless/ath/ath10k/pci.c  | 279 ++++++++++++++++-----------------
 drivers/net/wireless/ath/ath10k/pci.h  |   4 +-
 6 files changed, 185 insertions(+), 189 deletions(-)

-- 
1.8.5.3

[PATCH 1/5] ath10k: rework posting pci rx buffers

[Michal Kazior]

It was possible on a host system running low on
memory to end up with no rx buffers on pci pipes.

This also cleans up the code a bit and, as a side
effect, makes it possible to call
ath10k_hif_start() more than once.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/ce.c  |  71 +++++++----
 drivers/net/wireless/ath/ath10k/ce.h  |  16 +--
 drivers/net/wireless/ath/ath10k/pci.c | 234 ++++++++++++++++++----------------
 drivers/net/wireless/ath/ath10k/pci.h |   2 +
 4 files changed, 175 insertions(+), 148 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 8cbc0ab..f666816 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -385,44 +385,59 @@ int ath10k_ce_num_free_src_entries(struct ath10k_ce_pipe *pipe)
 	return delta;
 }
 
-int ath10k_ce_recv_buf_enqueue(struct ath10k_ce_pipe *ce_state,
-			       void *per_recv_context,
-			       u32 buffer)
+
+int __ath10k_ce_rx_num_free_bufs(struct ath10k_ce_pipe *pipe)
 {
-	struct ath10k_ce_ring *dest_ring = ce_state->dest_ring;
-	u32 ctrl_addr = ce_state->ctrl_addr;
-	struct ath10k *ar = ce_state->ar;
+	struct ath10k *ar = pipe->ar;
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	struct ath10k_ce_ring *dest_ring = pipe->dest_ring;
 	unsigned int nentries_mask = dest_ring->nentries_mask;
-	unsigned int write_index;
-	unsigned int sw_index;
-	int ret;
+	unsigned int write_index = dest_ring->write_index;
+	unsigned int sw_index = dest_ring->sw_index;
 
-	spin_lock_bh(&ar_pci->ce_lock);
-	write_index = dest_ring->write_index;
-	sw_index = dest_ring->sw_index;
+	lockdep_assert_held(&ar_pci->ce_lock);
 
-	if (CE_RING_DELTA(nentries_mask, write_index, sw_index - 1) > 0) {
-		struct ce_desc *base = dest_ring->base_addr_owner_space;
-		struct ce_desc *desc = CE_DEST_RING_TO_DESC(base, write_index);
+	return CE_RING_DELTA(nentries_mask, write_index, sw_index - 1);
+}
+
+int __ath10k_ce_rx_post_buf(struct ath10k_ce_pipe *pipe, void *ctx, u32 paddr)
+{
+	struct ath10k *ar = pipe->ar;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	struct ath10k_ce_ring *dest_ring = pipe->dest_ring;
+	unsigned int nentries_mask = dest_ring->nentries_mask;
+	unsigned int write_index = dest_ring->write_index;
+	unsigned int sw_index = dest_ring->sw_index;
+	struct ce_desc *base = dest_ring->base_addr_owner_space;
+	struct ce_desc *desc = CE_DEST_RING_TO_DESC(base, write_index);
+	u32 ctrl_addr = pipe->ctrl_addr;
 
-		/* Update destination descriptor */
-		desc->addr    = __cpu_to_le32(buffer);
-		desc->nbytes = 0;
+	lockdep_assert_held(&ar_pci->ce_lock);
 
-		dest_ring->per_transfer_context[write_index] =
-							per_recv_context;
+	if (CE_RING_DELTA(nentries_mask, write_index, sw_index - 1) == 0)
+		return -EIO;
 
-		/* Update Destination Ring Write Index */
-		write_index = CE_RING_IDX_INCR(nentries_mask, write_index);
-		ath10k_ce_dest_ring_write_index_set(ar, ctrl_addr, write_index);
-		dest_ring->write_index = write_index;
-		ret = 0;
-	} else {
-		ret = -EIO;
-	}
+	desc->addr = __cpu_to_le32(paddr);
+	desc->nbytes = 0;
+
+	dest_ring->per_transfer_context[write_index] = ctx;
+	write_index = CE_RING_IDX_INCR(nentries_mask, write_index);
+	ath10k_ce_dest_ring_write_index_set(ar, ctrl_addr, write_index);
+	dest_ring->write_index = write_index;
 
+	return 0;
+}
+
+int ath10k_ce_rx_post_buf(struct ath10k_ce_pipe *pipe, void *ctx, u32 paddr)
+{
+	struct ath10k *ar = pipe->ar;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ret;
+
+	spin_lock_bh(&ar_pci->ce_lock);
+	ret = __ath10k_ce_rx_post_buf(pipe, ctx, paddr);
 	spin_unlock_bh(&ar_pci->ce_lock);
+
 	return ret;
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index d48dbb9..82d1f23 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -166,19 +166,9 @@ int ath10k_ce_num_free_src_entries(struct ath10k_ce_pipe *pipe);
 
 /*==================Recv=======================*/
 
-/*
- * Make a buffer available to receive. The buffer must be at least of a
- * minimal size appropriate for this copy engine (src_sz_max attribute).
- *   ce                    - which copy engine to use
- *   per_transfer_recv_context  - context passed back to caller's recv_cb
- *   buffer                     - address of buffer in CE space
- * Returns 0 on success; otherwise an error status.
- *
- * Implemenation note: Pushes a buffer to Dest ring.
- */
-int ath10k_ce_recv_buf_enqueue(struct ath10k_ce_pipe *ce_state,
-			       void *per_transfer_recv_context,
-			       u32 buffer);
+int __ath10k_ce_rx_num_free_bufs(struct ath10k_ce_pipe *pipe);
+int __ath10k_ce_rx_post_buf(struct ath10k_ce_pipe *pipe, void *ctx, u32 paddr);
+int ath10k_ce_rx_post_buf(struct ath10k_ce_pipe *pipe, void *ctx, u32 paddr);
 
 /* recv flags */
 /* Data is byte-swapped */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index c157717..7efcf8e 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -67,10 +67,7 @@ static DEFINE_PCI_DEVICE_TABLE(ath10k_pci_id_table) = {
 static int ath10k_pci_diag_read_access(struct ath10k *ar, u32 address,
 				       u32 *data);
 
-static int ath10k_pci_post_rx(struct ath10k *ar);
-static int ath10k_pci_post_rx_pipe(struct ath10k_pci_pipe *pipe_info,
-					     int num);
-static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info);
+static void ath10k_pci_buffer_cleanup(struct ath10k *ar);
 static int ath10k_pci_cold_reset(struct ath10k *ar);
 static int ath10k_pci_warm_reset(struct ath10k *ar);
 static int ath10k_pci_wait_for_target_init(struct ath10k *ar);
@@ -278,6 +275,123 @@ static inline const char *ath10k_pci_get_irq_method(struct ath10k *ar)
 		return "legacy";
 }
 
+static int __ath10k_pci_rx_post_buf(struct ath10k_pci_pipe *pipe)
+{
+	struct ath10k *ar = pipe->hif_ce_state;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	struct ath10k_ce_pipe *ce_pipe = pipe->ce_hdl;
+	struct sk_buff *skb;
+	dma_addr_t paddr;
+	int ret;
+
+	lockdep_assert_held(&ar_pci->ce_lock);
+
+	skb = dev_alloc_skb(pipe->buf_sz);
+	if (!skb)
+		return -ENOMEM;
+
+	WARN_ONCE((unsigned long)skb->data & 3, "unaligned skb");
+
+	paddr = dma_map_single(ar->dev, skb->data,
+			       skb->len + skb_tailroom(skb),
+			       DMA_FROM_DEVICE);
+	if (unlikely(dma_mapping_error(ar->dev, paddr))) {
+		ath10k_warn("failed to dma map pci rx buf\n");
+		dev_kfree_skb_any(skb);
+		return -EIO;
+	}
+
+	ATH10K_SKB_CB(skb)->paddr = paddr;
+
+	ret = __ath10k_ce_rx_post_buf(ce_pipe, skb, paddr);
+	if (ret) {
+		ath10k_warn("failed to post pci rx buf: %d\n", ret);
+		dma_unmap_single(ar->dev, paddr, skb->len + skb_tailroom(skb),
+				 DMA_FROM_DEVICE);
+		dev_kfree_skb_any(skb);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __ath10k_pci_rx_post_pipe(struct ath10k_pci_pipe *pipe)
+{
+	struct ath10k *ar = pipe->hif_ce_state;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	struct ath10k_ce_pipe *ce_pipe = pipe->ce_hdl;
+	int ret, num;
+
+	lockdep_assert_held(&ar_pci->ce_lock);
+
+	if (pipe->buf_sz == 0)
+		return 0;
+
+	if (!ce_pipe->dest_ring)
+		return 0;
+
+	num = __ath10k_ce_rx_num_free_bufs(ce_pipe);
+	while (num--) {
+		ret = __ath10k_pci_rx_post_buf(pipe);
+		if (ret) {
+			ath10k_warn("failed to post pci rx buf: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int ath10k_pci_rx_post(struct ath10k *ar)
+{
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	struct ath10k_pci_pipe *pipe;
+	int i, ret;
+
+	spin_lock_bh(&ar_pci->ce_lock);
+	for (i = 0; i < CE_COUNT; i++) {
+		pipe = &ar_pci->pipe_info[i];
+
+		ret = __ath10k_pci_rx_post_pipe(pipe);
+		if (ret) {
+			ath10k_warn("failed to post pci rx pipe %d: %d\n",
+				    i, ret);
+			break;
+		}
+	}
+	spin_unlock_bh(&ar_pci->ce_lock);
+
+	return ret;
+}
+
+static void ath10k_pci_rx_replenish(struct ath10k_pci_pipe *pipe)
+{
+	struct ath10k *ar = pipe->hif_ce_state;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ret;
+
+	spin_lock_bh(&ar_pci->ce_lock);
+	ret = __ath10k_pci_rx_post_pipe(pipe);
+	spin_unlock_bh(&ar_pci->ce_lock);
+
+	if (ret) {
+		ath10k_warn("failed to replenish pci rx pipe %d: %d, arming retry timer\n",
+			    pipe->pipe_num, ret);
+		mod_timer(&ar_pci->rx_replenish_retry,
+			  ATH10K_PCI_RX_REPLENISH_RETRY_MS);
+	}
+}
+
+static void ath10k_pci_rx_replenish_retry(unsigned long ptr)
+{
+	struct ath10k *ar = (void *)ptr;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int i;
+
+	for (i = 0; i < CE_COUNT; i++)
+		ath10k_pci_rx_replenish(&ar_pci->pipe_info[i]);
+}
+
 /*
  * Diagnostic read/write access is provided for startup/config/debug usage.
  * Caller must guarantee proper alignment, when applicable, and single user
@@ -344,7 +458,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
 		nbytes = min_t(unsigned int, remaining_bytes,
 			       DIAG_TRANSFER_LIMIT);
 
-		ret = ath10k_ce_recv_buf_enqueue(ce_diag, NULL, ce_data);
+		ret = ath10k_ce_rx_post_buf(ce_diag, NULL, ce_data);
 		if (ret != 0)
 			goto done;
 
@@ -501,7 +615,7 @@ static int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
 		nbytes = min_t(int, remaining_bytes, DIAG_TRANSFER_LIMIT);
 
 		/* Set up to receive directly into Target(!) address */
-		ret = ath10k_ce_recv_buf_enqueue(ce_diag, NULL, address);
+		ret = ath10k_ce_rx_post_buf(ce_diag, NULL, address);
 		if (ret != 0)
 			goto done;
 
@@ -663,12 +777,10 @@ static void ath10k_pci_ce_recv_data(struct ath10k_ce_pipe *ce_state)
 	unsigned int nbytes, max_nbytes;
 	unsigned int transfer_id;
 	unsigned int flags;
-	int err, num_replenish = 0;
 
 	while (ath10k_ce_completed_recv_next(ce_state, &transfer_context,
 					     &ce_data, &nbytes, &transfer_id,
 					     &flags) == 0) {
-		num_replenish++;
 		skb = transfer_context;
 		max_nbytes = skb->len + skb_tailroom(skb);
 		dma_unmap_single(ar->dev, ATH10K_SKB_CB(skb)->paddr,
@@ -685,12 +797,7 @@ static void ath10k_pci_ce_recv_data(struct ath10k_ce_pipe *ce_state)
 		cb->rx_completion(ar, skb, pipe_info->pipe_num);
 	}
 
-	err = ath10k_pci_post_rx_pipe(pipe_info, num_replenish);
-	if (unlikely(err)) {
-		/* FIXME: retry */
-		ath10k_warn("failed to replenish CE rx ring %d (%d bufs): %d\n",
-			    pipe_info->pipe_num, num_replenish, err);
-	}
+	ath10k_pci_rx_replenish(pipe_info);
 }
 
 static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
@@ -944,94 +1051,6 @@ static void ath10k_pci_hif_get_default_pipe(struct ath10k *ar,
 						 &dl_is_polled);
 }
 
-static int ath10k_pci_post_rx_pipe(struct ath10k_pci_pipe *pipe_info,
-				   int num)
-{
-	struct ath10k *ar = pipe_info->hif_ce_state;
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	struct ath10k_ce_pipe *ce_state = pipe_info->ce_hdl;
-	struct sk_buff *skb;
-	dma_addr_t ce_data;
-	int i, ret = 0;
-
-	if (pipe_info->buf_sz == 0)
-		return 0;
-
-	for (i = 0; i < num; i++) {
-		skb = dev_alloc_skb(pipe_info->buf_sz);
-		if (!skb) {
-			ath10k_warn("failed to allocate skbuff for pipe %d\n",
-				    num);
-			ret = -ENOMEM;
-			goto err;
-		}
-
-		WARN_ONCE((unsigned long)skb->data & 3, "unaligned skb");
-
-		ce_data = dma_map_single(ar->dev, skb->data,
-					 skb->len + skb_tailroom(skb),
-					 DMA_FROM_DEVICE);
-
-		if (unlikely(dma_mapping_error(ar->dev, ce_data))) {
-			ath10k_warn("failed to DMA map sk_buff\n");
-			dev_kfree_skb_any(skb);
-			ret = -EIO;
-			goto err;
-		}
-
-		ATH10K_SKB_CB(skb)->paddr = ce_data;
-
-		pci_dma_sync_single_for_device(ar_pci->pdev, ce_data,
-					       pipe_info->buf_sz,
-					       PCI_DMA_FROMDEVICE);
-
-		ret = ath10k_ce_recv_buf_enqueue(ce_state, (void *)skb,
-						 ce_data);
-		if (ret) {
-			ath10k_warn("failed to enqueue to pipe %d: %d\n",
-				    num, ret);
-			goto err;
-		}
-	}
-
-	return ret;
-
-err:
-	ath10k_pci_rx_pipe_cleanup(pipe_info);
-	return ret;
-}
-
-static int ath10k_pci_post_rx(struct ath10k *ar)
-{
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	struct ath10k_pci_pipe *pipe_info;
-	const struct ce_attr *attr;
-	int pipe_num, ret = 0;
-
-	for (pipe_num = 0; pipe_num < CE_COUNT; pipe_num++) {
-		pipe_info = &ar_pci->pipe_info[pipe_num];
-		attr = &host_ce_config_wlan[pipe_num];
-
-		if (attr->dest_nentries == 0)
-			continue;
-
-		ret = ath10k_pci_post_rx_pipe(pipe_info,
-					      attr->dest_nentries - 1);
-		if (ret) {
-			ath10k_warn("failed to post RX buffer for pipe %d: %d\n",
-				    pipe_num, ret);
-
-			for (; pipe_num >= 0; pipe_num--) {
-				pipe_info = &ar_pci->pipe_info[pipe_num];
-				ath10k_pci_rx_pipe_cleanup(pipe_info);
-			}
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static int ath10k_pci_hif_start(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -1039,11 +1058,10 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
 
-	/* Post buffers once to start things off. */
-	ret = ath10k_pci_post_rx(ar);
+	ret = ath10k_pci_rx_post(ar);
 	if (ret) {
-		ath10k_warn("failed to post RX buffers for all pipes: %d\n",
-			    ret);
+		ath10k_warn("failed to post pci rx buffers: %d\n", ret);
+		ath10k_pci_buffer_cleanup(ar);
 		return ret;
 	}
 
@@ -1232,7 +1250,7 @@ static int ath10k_pci_hif_exchange_bmi_msg(struct ath10k *ar,
 		xfer.wait_for_resp = true;
 		xfer.resp_len = 0;
 
-		ath10k_ce_recv_buf_enqueue(ce_rx, &xfer, resp_paddr);
+		ath10k_ce_rx_post_buf(ce_rx, &xfer, resp_paddr);
 	}
 
 	ret = ath10k_ce_send(ce_tx, &xfer, req_paddr, req_len, -1, 0);
@@ -2427,6 +2445,8 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 	ar_pci->ar = ar;
 
 	spin_lock_init(&ar_pci->ce_lock);
+	setup_timer(&ar_pci->rx_replenish_retry, ath10k_pci_rx_replenish_retry,
+		    (unsigned long)ar);
 
 	ret = ath10k_pci_claim(ar);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 662bca3..68fcb4f 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -185,6 +185,7 @@ struct ath10k_pci {
 
 	/* Map CE id to ce_state */
 	struct ath10k_ce_pipe ce_states[CE_COUNT_MAX];
+	struct timer_list rx_replenish_retry;
 };
 
 static inline struct ath10k_pci *ath10k_pci_priv(struct ath10k *ar)
@@ -192,6 +193,7 @@ static inline struct ath10k_pci *ath10k_pci_priv(struct ath10k *ar)
 	return (struct ath10k_pci *)ar->drv_priv;
 }
 
+#define ATH10K_PCI_RX_REPLENISH_RETRY_MS 50
 #define ATH_PCI_RESET_WAIT_MAX 10 /* ms */
 #define PCIE_WAKE_TIMEOUT 5000	/* 5ms */
 
-- 
1.8.5.3

Re: [PATCH 1/5] ath10k: rework posting pci rx buffers

[Kalle Valo]

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

> It was possible on a host system running low on
> memory to end up with no rx buffers on pci pipes.
>
> This also cleans up the code a bit and, as a side
> effect, makes it possible to call
> ath10k_hif_start() more than once.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

The rework is relatively big so it would be good to have a short
overview how you change it.

> +static void ath10k_pci_rx_replenish(struct ath10k_pci_pipe *pipe)
> +{
> +	struct ath10k *ar = pipe->hif_ce_state;
> +	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
> +	int ret;
> +
> +	spin_lock_bh(&ar_pci->ce_lock);
> +	ret = __ath10k_pci_rx_post_pipe(pipe);
> +	spin_unlock_bh(&ar_pci->ce_lock);
> +
> +	if (ret) {
> +		ath10k_warn("failed to replenish pci rx pipe %d: %d, arming retry timer\n",
> +			    pipe->pipe_num, ret);
> +		mod_timer(&ar_pci->rx_replenish_retry,
> +			  ATH10K_PCI_RX_REPLENISH_RETRY_MS);
> +	}
> +}

Buildbot sent a warning about this one which I think is valid:

drivers/net/wireless/ath/ath10k/pci.c:380 ath10k_pci_rx_replenish() warn: mod_timer() takes an absolute \
time not an offset.

-- 
Kalle Valo

[PATCH 2/5] ath10k: dont reset the chip on hif_stop

[Michal Kazior]

The copy engine structures are never changed until
driver unregisters the device so it is not
necessary to reset the device chip anymore when
stopping.

This reduces driver register time approx. by 200ms.

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

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7efcf8e..6a5ca9e 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1192,13 +1192,6 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
 
 	ath10k_pci_buffer_cleanup(ar);
 
-	/* Make the sure the device won't access any structures on the host by
-	 * resetting it. The device was fed with PCI CE ringbuffer
-	 * configuration during init. If ringbuffers are freed and the device
-	 * were to access them this could lead to memory corruption on the
-	 * host. */
-	ath10k_pci_warm_reset(ar);
-
 	ar_pci->started = 0;
 }
 
-- 
1.8.5.3

Re: [PATCH 2/5] ath10k: dont reset the chip on hif_stop

[Michal Kazior]

On 7 August 2014 11:05, Michal Kazior <michal.kazior@tieto.com> wrote:
> The copy engine structures are never changed until
> driver unregisters the device so it is not
> necessary to reset the device chip anymore when
> stopping.
>
> This reduces driver register time approx. by 200ms.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---
>  drivers/net/wireless/ath/ath10k/pci.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 7efcf8e..6a5ca9e 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -1192,13 +1192,6 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
>
>         ath10k_pci_buffer_cleanup(ar);
>
> -       /* Make the sure the device won't access any structures on the host by
> -        * resetting it. The device was fed with PCI CE ringbuffer
> -        * configuration during init. If ringbuffers are freed and the device
> -        * were to access them this could lead to memory corruption on the
> -        * host. */
> -       ath10k_pci_warm_reset(ar);
> -
>         ar_pci->started = 0;

If firmware crashes it doesn't necessarily mean hardware has crashed
as well. Since htt rx ring dma buffer is fed directly to hardware it
can still be accessed even after firmware has crashed (e.g. receive
beacons in station mode). This means calling hif_stop must guarantee
to stop hardware completely or else it might corrupt host memory
(notably sk_buffs; pretty badly - I've been hitting general protection
faults randomly and was puzzled at first).

Thus this patch has to be dropped (I'll probably replace it with a
patch to update the comment only).


Michał

[PATCH 3/5] ath10k: ignore ar_pci->started in pipe cleanup

[Michal Kazior]

Structures used by these functions are now
guaranteed to remain accessible until driver is
unregistered.

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

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 6a5ca9e..8244658 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1088,10 +1088,6 @@ static void ath10k_pci_rx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
 
 	ar = pipe_info->hif_ce_state;
 	ar_pci = ath10k_pci_priv(ar);
-
-	if (!ar_pci->started)
-		return;
-
 	ce_hdl = pipe_info->ce_hdl;
 
 	while (ath10k_ce_revoke_recv_next(ce_hdl, (void **)&netbuf,
@@ -1122,10 +1118,6 @@ static void ath10k_pci_tx_pipe_cleanup(struct ath10k_pci_pipe *pipe_info)
 
 	ar = pipe_info->hif_ce_state;
 	ar_pci = ath10k_pci_priv(ar);
-
-	if (!ar_pci->started)
-		return;
-
 	ce_hdl = pipe_info->ce_hdl;
 
 	while (ath10k_ce_cancel_send_next(ce_hdl, (void **)&netbuf,
-- 
1.8.5.3

[PATCH 4/5] ath10k: remove ar_pci->started

[Michal Kazior]

There are basically no more uses for
ar_pci->started. It is also perfectly safe to call
hif_stop without hif_start.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/hif.h |  3 +--
 drivers/net/wireless/ath/ath10k/pci.c | 30 ++++++++----------------------
 drivers/net/wireless/ath/ath10k/pci.h |  2 --
 3 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/hif.h b/drivers/net/wireless/ath/ath10k/hif.h
index 2ac7bea..bd97f4a 100644
--- a/drivers/net/wireless/ath/ath10k/hif.h
+++ b/drivers/net/wireless/ath/ath10k/hif.h
@@ -83,8 +83,7 @@ struct ath10k_hif_ops {
 	/* Power up the device and enter BMI transfer mode for FW download */
 	int (*power_up)(struct ath10k *ar);
 
-	/* Power down the device and free up resources. stop() must be called
-	 * before this if start() was called earlier */
+	/* Power down the device and free up resources. */
 	void (*power_down)(struct ath10k *ar);
 
 	int (*suspend)(struct ath10k *ar);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 8244658..4131cc7 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1053,7 +1053,6 @@ static void ath10k_pci_hif_get_default_pipe(struct ath10k *ar,
 
 static int ath10k_pci_hif_start(struct ath10k *ar)
 {
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 	int ret;
 
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif start\n");
@@ -1066,7 +1065,6 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 	}
 
 	ath10k_ce_enable_interrupts(ar);
-	ar_pci->started = 1;
 
 	return 0;
 }
@@ -1162,29 +1160,17 @@ static void ath10k_pci_ce_deinit(struct ath10k *ar)
 		ath10k_ce_deinit_pipe(ar, i);
 }
 
-static void ath10k_pci_hif_stop(struct ath10k *ar)
+static void ath10k_pci_flush(struct ath10k *ar)
 {
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	int ret;
-
-	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif stop\n");
-
-	if (WARN_ON(!ar_pci->started))
-		return;
-
-	ret = ath10k_ce_disable_interrupts(ar);
-	if (ret)
-		ath10k_warn("failed to disable CE interrupts: %d\n", ret);
-
+	ath10k_ce_disable_interrupts(ar);
 	ath10k_pci_kill_tasklet(ar);
-
-	/* At this point, asynchronous threads are stopped, the target should
-	 * not DMA nor interrupt. We process the leftovers and then free
-	 * everything else up. */
-
 	ath10k_pci_buffer_cleanup(ar);
+}
 
-	ar_pci->started = 0;
+static void ath10k_pci_hif_stop(struct ath10k *ar)
+{
+	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif stop\n");
+	ath10k_pci_flush(ar);
 }
 
 static int ath10k_pci_hif_exchange_bmi_msg(struct ath10k *ar,
@@ -1892,7 +1878,7 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar)
 {
 	ath10k_dbg(ATH10K_DBG_BOOT, "boot hif power down\n");
 
-	ath10k_pci_kill_tasklet(ar);
+	ath10k_pci_flush(ar);
 	ath10k_pci_warm_reset(ar);
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 68fcb4f..899e5db 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -171,8 +171,6 @@ struct ath10k_pci {
 	struct tasklet_struct msi_fw_err;
 	struct tasklet_struct early_irq_tasklet;
 
-	int started;
-
 	struct ath10k_pci_pipe pipe_info[CE_COUNT_MAX];
 
 	struct ath10k_hif_cb msg_callbacks_current;
-- 
1.8.5.3

[PATCH 5/5] ath10k: flush hif buffers before recovery

[Michal Kazior]

Transport buffers weren't flushed and processed
before queueing hw recovery request to mac80211.

This could in theory result in an unwanted htt/wmi
rx events being processed while mac80211 recovers
the device and possibly interfere or even crash
the system.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index b5161c8..5c9620b 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -667,6 +667,7 @@ static void ath10k_core_restart(struct work_struct *work)
 		ar->state = ATH10K_STATE_RESTARTING;
 		del_timer_sync(&ar->scan.timeout);
 		ath10k_reset_scan((unsigned long)ar);
+		ath10k_hif_stop(ar);
 		ieee80211_restart_hw(ar->hw);
 		break;
 	case ATH10K_STATE_OFF:
-- 
1.8.5.3

Re: [PATCH 5/5] ath10k: flush hif buffers before recovery

[Kalle Valo]

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

> Transport buffers weren't flushed and processed
> before queueing hw recovery request to mac80211.
>
> This could in theory result in an unwanted htt/wmi
> rx events being processed while mac80211 recovers
> the device and possibly interfere or even crash
> the system.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

This one had a conflict in ath-next-test, I changed the patch to this:

--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -666,6 +666,7 @@ static void ath10k_core_restart(struct work_struct *work)
        case ATH10K_STATE_ON:
                ar->state = ATH10K_STATE_RESTARTING;
                ath10k_scan_finish(ar);
+               ath10k_hif_stop(ar);
                ieee80211_restart_hw(ar->hw);
                break;
        case ATH10K_STATE_OFF:

-- 
Kalle Valo

Re: [PATCH 5/5] ath10k: flush hif buffers before recovery

[Michal Kazior]

On 7 August 2014 11:30, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> Transport buffers weren't flushed and processed
>> before queueing hw recovery request to mac80211.
>>
>> This could in theory result in an unwanted htt/wmi
>> rx events being processed while mac80211 recovers
>> the device and possibly interfere or even crash
>> the system.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> This one had a conflict in ath-next-test, I changed the patch to this:
>
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -666,6 +666,7 @@ static void ath10k_core_restart(struct work_struct *work)
>         case ATH10K_STATE_ON:
>                 ar->state = ATH10K_STATE_RESTARTING;
>                 ath10k_scan_finish(ar);
> +               ath10k_hif_stop(ar);
>                 ieee80211_restart_hw(ar->hw);

Oops. I've accidentally posted an old version of this single patch.
The hif_stop() should be _before_ scan_finish/reset(). I'll wait for
more comments before posting a v2 though.


Michał