linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] ath10k: pci fixes 2014-05-15
@ 2014-05-15 12:48 Michal Kazior
  2014-05-15 12:48 ` [PATCH 1/3] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Michal Kazior @ 2014-05-15 12:48 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, apenwarr, Michal Kazior

Hi,

Patches 1 and 2 were made as a result of analysing
`ath10k firmware crash after 4 hours of heavy TCP
traffic` bug reported by Avery.

Patch 3 fixes a bug reported by Avery that was
apparently related to missing memory barriers on
his ARM test system. @Avery, can you take a look
or test it, please? I was never able to reproduce
this but it just seems like the right thing to do.
Or you can simply post your patch and I'll drop
mine then.

Note: this is based on ath-next-test branch.


Michal Kazior (3):
  ath10k: protect src_ring state with ce_lock in tx_sg()
  ath10k: revert incomplete scatter-gather pci tx
  ath10k: add explicit memory barrier for ring index update

 drivers/net/wireless/ath/ath10k/ce.c  | 29 +++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/ce.h  |  2 ++
 drivers/net/wireless/ath/ath10k/pci.c | 27 ++++++++++++++++++---------
 3 files changed, 49 insertions(+), 9 deletions(-)

-- 
1.8.5.3


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] ath10k: protect src_ring state with ce_lock in tx_sg()
  2014-05-15 12:48 [PATCH 0/3] ath10k: pci fixes 2014-05-15 Michal Kazior
@ 2014-05-15 12:48 ` Michal Kazior
  2014-05-15 12:48 ` [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx Michal Kazior
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Michal Kazior @ 2014-05-15 12:48 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, apenwarr, Michal Kazior

It was possible to read invalid state of CE ring
buffer indexes. This could lead to scatter-gather
transfer failure in mid-way and crash firmware
later by leaving garbage data on the ring.

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

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 91d6076..04d8cbf 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -762,13 +762,17 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 	struct ath10k_pci_pipe *pci_pipe = &ar_pci->pipe_info[pipe_id];
 	struct ath10k_ce_pipe *ce_pipe = pci_pipe->ce_hdl;
 	struct ath10k_ce_ring *src_ring = ce_pipe->src_ring;
-	unsigned int nentries_mask = src_ring->nentries_mask;
-	unsigned int sw_index = src_ring->sw_index;
-	unsigned int write_index = src_ring->write_index;
+	unsigned int nentries_mask;
+	unsigned int sw_index;
+	unsigned int write_index;
 	int err, i;
 
 	spin_lock_bh(&ar_pci->ce_lock);
 
+	nentries_mask = src_ring->nentries_mask;
+	sw_index = src_ring->sw_index;
+	write_index = src_ring->write_index;
+
 	if (unlikely(CE_RING_DELTA(nentries_mask,
 				   write_index, sw_index - 1) < n_items)) {
 		err = -ENOBUFS;
-- 
1.8.5.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx
  2014-05-15 12:48 [PATCH 0/3] ath10k: pci fixes 2014-05-15 Michal Kazior
  2014-05-15 12:48 ` [PATCH 1/3] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
@ 2014-05-15 12:48 ` Michal Kazior
  2014-05-25  7:53   ` Kalle Valo
  2014-05-15 12:48 ` [PATCH 3/3] ath10k: add explicit memory barrier for ring index update Michal Kazior
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Michal Kazior @ 2014-05-15 12:48 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, apenwarr, Michal Kazior

This prevents leaving incomplete scatter-gather
transfer on CE rings which can lead firmware to
crash.

Reported-By: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/ce.c  | 27 +++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/ce.h  |  2 ++
 drivers/net/wireless/ath/ath10k/pci.c | 17 +++++++++++------
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 1e4cad8..966f26e 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -329,6 +329,33 @@ exit:
 	return ret;
 }
 
+void __ath10k_ce_send_revert(struct ath10k_ce_pipe *pipe)
+{
+	struct ath10k *ar = pipe->ar;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	struct ath10k_ce_ring *src_ring = pipe->src_ring;
+	u32 ctrl_addr = pipe->ctrl_addr;
+
+	lockdep_assert_held(&ar_pci->ce_lock);
+
+	/*
+	 * This function must be called only if there is an incomplete
+	 * scatter-gather transfer (before index register is updated)
+	 * that needs to be cleaned up.
+	 */
+	if (WARN_ON(src_ring->write_index == src_ring->sw_index))
+		return;
+
+	if (WARN_ON(src_ring->write_index ==
+		    ath10k_ce_src_ring_write_index_get(ar, ctrl_addr)))
+		return;
+
+	src_ring->write_index--;
+	src_ring->write_index &= src_ring->nentries_mask;
+
+	src_ring->per_transfer_context[src_ring->write_index] = NULL;
+}
+
 int ath10k_ce_send(struct ath10k_ce_pipe *ce_state,
 		   void *per_transfer_context,
 		   u32 buffer,
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index fd0bc35..7a5a36f 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -160,6 +160,8 @@ int ath10k_ce_send_nolock(struct ath10k_ce_pipe *ce_state,
 			  unsigned int transfer_id,
 			  unsigned int flags);
 
+void __ath10k_ce_send_revert(struct ath10k_ce_pipe *pipe);
+
 void ath10k_ce_send_cb_register(struct ath10k_ce_pipe *ce_state,
 				void (*send_cb)(struct ath10k_ce_pipe *),
 				int disable_interrupts);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 04d8cbf..059add4 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -765,7 +765,7 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 	unsigned int nentries_mask;
 	unsigned int sw_index;
 	unsigned int write_index;
-	int err, i;
+	int err, i = 0;
 
 	spin_lock_bh(&ar_pci->ce_lock);
 
@@ -776,7 +776,7 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 	if (unlikely(CE_RING_DELTA(nentries_mask,
 				   write_index, sw_index - 1) < n_items)) {
 		err = -ENOBUFS;
-		goto unlock;
+		goto err;
 	}
 
 	for (i = 0; i < n_items - 1; i++) {
@@ -793,7 +793,7 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 					    items[i].transfer_id,
 					    CE_SEND_FLAG_GATHER);
 		if (err)
-			goto unlock;
+			goto err;
 	}
 
 	/* `i` is equal to `n_items -1` after for() */
@@ -811,10 +811,15 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 				    items[i].transfer_id,
 				    0);
 	if (err)
-		goto unlock;
+		goto err;
+
+	spin_unlock_bh(&ar_pci->ce_lock);
+	return 0;
+
+err:
+	for (; i > 0; i--)
+		__ath10k_ce_send_revert(ce_pipe);
 
-	err = 0;
-unlock:
 	spin_unlock_bh(&ar_pci->ce_lock);
 	return err;
 }
-- 
1.8.5.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] ath10k: add explicit memory barrier for ring index update
  2014-05-15 12:48 [PATCH 0/3] ath10k: pci fixes 2014-05-15 Michal Kazior
  2014-05-15 12:48 ` [PATCH 1/3] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
  2014-05-15 12:48 ` [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx Michal Kazior
@ 2014-05-15 12:48 ` Michal Kazior
  2014-05-16 12:32   ` Kalle Valo
  2014-05-25  7:44   ` Kalle Valo
  2014-05-16 12:34 ` [PATCH 0/3] ath10k: pci fixes 2014-05-15 Kalle Valo
  2014-05-26 10:02 ` [PATCH v2 0/2] " Michal Kazior
  4 siblings, 2 replies; 14+ messages in thread
From: Michal Kazior @ 2014-05-15 12:48 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, apenwarr, Michal Kazior

Avery reported he had some issues related to
instructions being re-ordered on his ARM test
system resulting in firmware crashes.

This makes sure that data is in place before CE
ring index is updated telling firmware it can
fetch the data.

Reported-By: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/ce.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 966f26e..e66159e 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -63,6 +63,7 @@ static inline void ath10k_ce_dest_ring_write_index_set(struct ath10k *ar,
 						       u32 ce_ctrl_addr,
 						       unsigned int n)
 {
+	mb();
 	ath10k_pci_write32(ar, ce_ctrl_addr + DST_WR_INDEX_ADDRESS, n);
 }
 
@@ -76,6 +77,7 @@ static inline void ath10k_ce_src_ring_write_index_set(struct ath10k *ar,
 						      u32 ce_ctrl_addr,
 						      unsigned int n)
 {
+	mb();
 	ath10k_pci_write32(ar, ce_ctrl_addr + SR_WR_INDEX_ADDRESS, n);
 }
 
-- 
1.8.5.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ath10k: add explicit memory barrier for ring index update
  2014-05-15 12:48 ` [PATCH 3/3] ath10k: add explicit memory barrier for ring index update Michal Kazior
@ 2014-05-16 12:32   ` Kalle Valo
  2014-05-25  7:44   ` Kalle Valo
  1 sibling, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2014-05-16 12:32 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless, apenwarr

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

> Avery reported he had some issues related to
> instructions being re-ordered on his ARM test
> system resulting in firmware crashes.
>
> This makes sure that data is in place before CE
> ring index is updated telling firmware it can
> fetch the data.
>
> Reported-By: Avery Pennarun <apenwarr@gmail.com>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

[...]

> --- a/drivers/net/wireless/ath/ath10k/ce.c
> +++ b/drivers/net/wireless/ath/ath10k/ce.c
> @@ -63,6 +63,7 @@ static inline void ath10k_ce_dest_ring_write_index_set(struct ath10k *ar,
>  						       u32 ce_ctrl_addr,
>  						       unsigned int n)
>  {
> +	mb();
>  	ath10k_pci_write32(ar, ce_ctrl_addr + DST_WR_INDEX_ADDRESS, n);
>  }
>  
> @@ -76,6 +77,7 @@ static inline void ath10k_ce_src_ring_write_index_set(struct ath10k *ar,
>  						      u32 ce_ctrl_addr,
>  						      unsigned int n)
>  {
> +	mb();
>  	ath10k_pci_write32(ar, ce_ctrl_addr + SR_WR_INDEX_ADDRESS, n);
>  }

I see two new checkpatch warnings:

drivers/net/wireless/ath/ath10k/ce.c:66: CHECK: memory barrier without comment
drivers/net/wireless/ath/ath10k/ce.c:80: CHECK: memory barrier without comment

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] ath10k: pci fixes 2014-05-15
  2014-05-15 12:48 [PATCH 0/3] ath10k: pci fixes 2014-05-15 Michal Kazior
                   ` (2 preceding siblings ...)
  2014-05-15 12:48 ` [PATCH 3/3] ath10k: add explicit memory barrier for ring index update Michal Kazior
@ 2014-05-16 12:34 ` Kalle Valo
  2014-05-26 10:02 ` [PATCH v2 0/2] " Michal Kazior
  4 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2014-05-16 12:34 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless, apenwarr

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

> Hi,
>
> Patches 1 and 2 were made as a result of analysing
> `ath10k firmware crash after 4 hours of heavy TCP
> traffic` bug reported by Avery.
>
> Patch 3 fixes a bug reported by Avery that was
> apparently related to missing memory barriers on
> his ARM test system. @Avery, can you take a look
> or test it, please? I was never able to reproduce
> this but it just seems like the right thing to do.
> Or you can simply post your patch and I'll drop
> mine then.

I think it would be good still to wait a while with patch 3 as
apparently Avery is still seeing some odd firmware crashes. Once we are
confident that the memory barrier issues are handled we come up with a
final patch.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] ath10k: add explicit memory barrier for ring index update
  2014-05-15 12:48 ` [PATCH 3/3] ath10k: add explicit memory barrier for ring index update Michal Kazior
  2014-05-16 12:32   ` Kalle Valo
@ 2014-05-25  7:44   ` Kalle Valo
  1 sibling, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2014-05-25  7:44 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless, apenwarr

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

> Avery reported he had some issues related to
> instructions being re-ordered on his ARM test
> system resulting in firmware crashes.
>
> This makes sure that data is in place before CE
> ring index is updated telling firmware it can
> fetch the data.
>
> Reported-By: Avery Pennarun <apenwarr@gmail.com>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

I have dropped this patch for now. Like I said before, let's first be
sure that all memory barrier issues are resolved and only then apply
this.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx
  2014-05-15 12:48 ` [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx Michal Kazior
@ 2014-05-25  7:53   ` Kalle Valo
  2014-05-26  5:37     ` Michal Kazior
  0 siblings, 1 reply; 14+ messages in thread
From: Kalle Valo @ 2014-05-25  7:53 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless, apenwarr

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

> This prevents leaving incomplete scatter-gather
> transfer on CE rings which can lead firmware to
> crash.
>
> Reported-By: Avery Pennarun <apenwarr@gmail.com>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

The title is a bit misleading as usually the commit log with the word
revert means that the commit is reverting another git commit. Maybe
something like this is better:

ath10k: drop incomplete scatter-gather pci tx transfers

> +	if (WARN_ON(src_ring->write_index == src_ring->sw_index))
> +		return;
> +
> +	if (WARN_ON(src_ring->write_index ==
> +		    ath10k_ce_src_ring_write_index_get(ar, ctrl_addr)))
> +		return;

WARN_ON() on data path is dangerous. WARN_ON_ONCE() or ath10k_warn() is
better.

> +err:
> +	for (; i > 0; i--)

Isn't this just a fancy way to say 'while (i-- > 0)'?

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx
  2014-05-25  7:53   ` Kalle Valo
@ 2014-05-26  5:37     ` Michal Kazior
  2014-05-26  9:19       ` Kalle Valo
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Kazior @ 2014-05-26  5:37 UTC (permalink / raw)
  To: Kalle Valo; +Cc: ath10k, linux-wireless, Avery Pennarun

On 25 May 2014 09:53, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This prevents leaving incomplete scatter-gather
>> transfer on CE rings which can lead firmware to
>> crash.
>>
>> Reported-By: Avery Pennarun <apenwarr@gmail.com>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> The title is a bit misleading as usually the commit log with the word
> revert means that the commit is reverting another git commit. Maybe
> something like this is better:
>
> ath10k: drop incomplete scatter-gather pci tx transfers

Good point. I was actually thinking 'abort' .. 'properly'.


>> +     if (WARN_ON(src_ring->write_index == src_ring->sw_index))
>> +             return;
>> +
>> +     if (WARN_ON(src_ring->write_index ==
>> +                 ath10k_ce_src_ring_write_index_get(ar, ctrl_addr)))
>> +             return;
>
> WARN_ON() on data path is dangerous. WARN_ON_ONCE() or ath10k_warn() is
> better.

Good point!


>> +err:
>> +     for (; i > 0; i--)
>
> Isn't this just a fancy way to say 'while (i-- > 0)'?

Not really. More like do { .. } while (--i > 0), no? First iteration
must use unmodified `i`.


Michał

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx
  2014-05-26  5:37     ` Michal Kazior
@ 2014-05-26  9:19       ` Kalle Valo
  0 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2014-05-26  9:19 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless, Avery Pennarun

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

> On 25 May 2014 09:53, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> This prevents leaving incomplete scatter-gather
>>> transfer on CE rings which can lead firmware to
>>> crash.
>>>
>>> Reported-By: Avery Pennarun <apenwarr@gmail.com>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> The title is a bit misleading as usually the commit log with the word
>> revert means that the commit is reverting another git commit. Maybe
>> something like this is better:
>>
>> ath10k: drop incomplete scatter-gather pci tx transfers
>
> Good point. I was actually thinking 'abort' .. 'properly'.

Sounds good to me.

>>> +err:
>>> +     for (; i > 0; i--)
>>
>> Isn't this just a fancy way to say 'while (i-- > 0)'?
>
> Not really. More like do { .. } while (--i > 0), no? First iteration
> must use unmodified `i`.

Ok, I missed that. Then the for loop is good here.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 0/2] ath10k: pci fixes 2014-05-15
  2014-05-15 12:48 [PATCH 0/3] ath10k: pci fixes 2014-05-15 Michal Kazior
                   ` (3 preceding siblings ...)
  2014-05-16 12:34 ` [PATCH 0/3] ath10k: pci fixes 2014-05-15 Kalle Valo
@ 2014-05-26 10:02 ` Michal Kazior
  2014-05-26 10:02   ` [PATCH v2 1/2] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
                     ` (2 more replies)
  4 siblings, 3 replies; 14+ messages in thread
From: Michal Kazior @ 2014-05-26 10:02 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, apenwarr, Michal Kazior

Hi,

Patches are a result of analysing `ath10k firmware
crash after 4 hours of heavy TCP traffic` bug
reported by Avery.

v2:
 * dropped patch 3/3 (memory barrier addition) [Kalle]

Based on github.com/kvalo/ath/master


Michal Kazior (2):
  ath10k: protect src_ring state with ce_lock in tx_sg()
  ath10k: abort incomplete scatter-gather pci tx properly

 drivers/net/wireless/ath/ath10k/ce.c  | 27 +++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/ce.h  |  2 ++
 drivers/net/wireless/ath/ath10k/pci.c | 27 ++++++++++++++++++---------
 3 files changed, 47 insertions(+), 9 deletions(-)

-- 
1.8.5.3


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/2] ath10k: protect src_ring state with ce_lock in tx_sg()
  2014-05-26 10:02 ` [PATCH v2 0/2] " Michal Kazior
@ 2014-05-26 10:02   ` Michal Kazior
  2014-05-26 10:02   ` [PATCH v2 2/2] ath10k: abort incomplete scatter-gather pci tx properly Michal Kazior
  2014-05-27  9:32   ` [PATCH v2 0/2] ath10k: pci fixes 2014-05-15 Kalle Valo
  2 siblings, 0 replies; 14+ messages in thread
From: Michal Kazior @ 2014-05-26 10:02 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, apenwarr, Michal Kazior, Kalle Valo

It was possible to read invalid state of CE ring
buffer indexes. This could lead to scatter-gather
transfer failure in mid-way and crash firmware
later by leaving garbage data on the ring.

Reported-By: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 71ab110..b1eb915 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -762,13 +762,17 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 	struct ath10k_pci_pipe *pci_pipe = &ar_pci->pipe_info[pipe_id];
 	struct ath10k_ce_pipe *ce_pipe = pci_pipe->ce_hdl;
 	struct ath10k_ce_ring *src_ring = ce_pipe->src_ring;
-	unsigned int nentries_mask = src_ring->nentries_mask;
-	unsigned int sw_index = src_ring->sw_index;
-	unsigned int write_index = src_ring->write_index;
+	unsigned int nentries_mask;
+	unsigned int sw_index;
+	unsigned int write_index;
 	int err, i;
 
 	spin_lock_bh(&ar_pci->ce_lock);
 
+	nentries_mask = src_ring->nentries_mask;
+	sw_index = src_ring->sw_index;
+	write_index = src_ring->write_index;
+
 	if (unlikely(CE_RING_DELTA(nentries_mask,
 				   write_index, sw_index - 1) < n_items)) {
 		err = -ENOBUFS;
-- 
1.8.5.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/2] ath10k: abort incomplete scatter-gather pci tx properly
  2014-05-26 10:02 ` [PATCH v2 0/2] " Michal Kazior
  2014-05-26 10:02   ` [PATCH v2 1/2] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
@ 2014-05-26 10:02   ` Michal Kazior
  2014-05-27  9:32   ` [PATCH v2 0/2] ath10k: pci fixes 2014-05-15 Kalle Valo
  2 siblings, 0 replies; 14+ messages in thread
From: Michal Kazior @ 2014-05-26 10:02 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, apenwarr, Michal Kazior, Kalle Valo

This prevents leaving incomplete scatter-gather
transfer on CE rings which can lead firmware to
crash.

Reported-By: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---

Notes:
    v2:
     * send_revert(): s/WARN_ON/WARN_ON_ONCE/ [Kalle]
     * commit subject: s/revert/abort/, s/$/properly/ [Kalle]

 drivers/net/wireless/ath/ath10k/ce.c  | 27 +++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/ce.h  |  2 ++
 drivers/net/wireless/ath/ath10k/pci.c | 17 +++++++++++------
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 1e4cad8..d185dc0 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -329,6 +329,33 @@ exit:
 	return ret;
 }
 
+void __ath10k_ce_send_revert(struct ath10k_ce_pipe *pipe)
+{
+	struct ath10k *ar = pipe->ar;
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	struct ath10k_ce_ring *src_ring = pipe->src_ring;
+	u32 ctrl_addr = pipe->ctrl_addr;
+
+	lockdep_assert_held(&ar_pci->ce_lock);
+
+	/*
+	 * This function must be called only if there is an incomplete
+	 * scatter-gather transfer (before index register is updated)
+	 * that needs to be cleaned up.
+	 */
+	if (WARN_ON_ONCE(src_ring->write_index == src_ring->sw_index))
+		return;
+
+	if (WARN_ON_ONCE(src_ring->write_index ==
+			 ath10k_ce_src_ring_write_index_get(ar, ctrl_addr)))
+		return;
+
+	src_ring->write_index--;
+	src_ring->write_index &= src_ring->nentries_mask;
+
+	src_ring->per_transfer_context[src_ring->write_index] = NULL;
+}
+
 int ath10k_ce_send(struct ath10k_ce_pipe *ce_state,
 		   void *per_transfer_context,
 		   u32 buffer,
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index fd0bc35..7a5a36f 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -160,6 +160,8 @@ int ath10k_ce_send_nolock(struct ath10k_ce_pipe *ce_state,
 			  unsigned int transfer_id,
 			  unsigned int flags);
 
+void __ath10k_ce_send_revert(struct ath10k_ce_pipe *pipe);
+
 void ath10k_ce_send_cb_register(struct ath10k_ce_pipe *ce_state,
 				void (*send_cb)(struct ath10k_ce_pipe *),
 				int disable_interrupts);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index b1eb915..d0004d5 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -765,7 +765,7 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 	unsigned int nentries_mask;
 	unsigned int sw_index;
 	unsigned int write_index;
-	int err, i;
+	int err, i = 0;
 
 	spin_lock_bh(&ar_pci->ce_lock);
 
@@ -776,7 +776,7 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 	if (unlikely(CE_RING_DELTA(nentries_mask,
 				   write_index, sw_index - 1) < n_items)) {
 		err = -ENOBUFS;
-		goto unlock;
+		goto err;
 	}
 
 	for (i = 0; i < n_items - 1; i++) {
@@ -793,7 +793,7 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 					    items[i].transfer_id,
 					    CE_SEND_FLAG_GATHER);
 		if (err)
-			goto unlock;
+			goto err;
 	}
 
 	/* `i` is equal to `n_items -1` after for() */
@@ -811,10 +811,15 @@ static int ath10k_pci_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
 				    items[i].transfer_id,
 				    0);
 	if (err)
-		goto unlock;
+		goto err;
+
+	spin_unlock_bh(&ar_pci->ce_lock);
+	return 0;
+
+err:
+	for (; i > 0; i--)
+		__ath10k_ce_send_revert(ce_pipe);
 
-	err = 0;
-unlock:
 	spin_unlock_bh(&ar_pci->ce_lock);
 	return err;
 }
-- 
1.8.5.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 0/2] ath10k: pci fixes 2014-05-15
  2014-05-26 10:02 ` [PATCH v2 0/2] " Michal Kazior
  2014-05-26 10:02   ` [PATCH v2 1/2] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
  2014-05-26 10:02   ` [PATCH v2 2/2] ath10k: abort incomplete scatter-gather pci tx properly Michal Kazior
@ 2014-05-27  9:32   ` Kalle Valo
  2 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2014-05-27  9:32 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless, apenwarr

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

> Hi,
>
> Patches are a result of analysing `ath10k firmware
> crash after 4 hours of heavy TCP traffic` bug
> reported by Avery.
>
> v2:
>  * dropped patch 3/3 (memory barrier addition) [Kalle]
>
> Based on github.com/kvalo/ath/master
>
>
> Michal Kazior (2):
>   ath10k: protect src_ring state with ce_lock in tx_sg()
>   ath10k: abort incomplete scatter-gather pci tx properly

Thanks, both patches applied.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2014-05-27  9:33 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-15 12:48 [PATCH 0/3] ath10k: pci fixes 2014-05-15 Michal Kazior
2014-05-15 12:48 ` [PATCH 1/3] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
2014-05-15 12:48 ` [PATCH 2/3] ath10k: revert incomplete scatter-gather pci tx Michal Kazior
2014-05-25  7:53   ` Kalle Valo
2014-05-26  5:37     ` Michal Kazior
2014-05-26  9:19       ` Kalle Valo
2014-05-15 12:48 ` [PATCH 3/3] ath10k: add explicit memory barrier for ring index update Michal Kazior
2014-05-16 12:32   ` Kalle Valo
2014-05-25  7:44   ` Kalle Valo
2014-05-16 12:34 ` [PATCH 0/3] ath10k: pci fixes 2014-05-15 Kalle Valo
2014-05-26 10:02 ` [PATCH v2 0/2] " Michal Kazior
2014-05-26 10:02   ` [PATCH v2 1/2] ath10k: protect src_ring state with ce_lock in tx_sg() Michal Kazior
2014-05-26 10:02   ` [PATCH v2 2/2] ath10k: abort incomplete scatter-gather pci tx properly Michal Kazior
2014-05-27  9:32   ` [PATCH v2 0/2] ath10k: pci fixes 2014-05-15 Kalle Valo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).