[PATCH 1/2] ath11k/hal: Fix few bugs in ath11k_hal_srng_dst_num_free()

[PATCH 1/2] ath11k/hal: Fix few bugs in ath11k_hal_srng_dst_num_free()

[Vasanthakumar Thiagarajan]

The logic to compute the number of available buffers in destination
ring is wrong. It should be just the different between head and
tail pointers in terms of the entry size. This functions currently
unused, this is fixed to make use of this function in follow-up
patches. Also make destination ring head pointer volatile because
it is independently updated by HW.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
---
 drivers/net/wireless/ath/ath11k/hal.c | 7 +++----
 drivers/net/wireless/ath/ath11k/hal.h | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c
index 7da42a1..9eac311 100644
--- a/drivers/net/wireless/ath/ath11k/hal.c
+++ b/drivers/net/wireless/ath/ath11k/hal.c
@@ -745,9 +745,9 @@ int ath11k_hal_srng_dst_num_free(struct ath11k_base *ab, struct hal_srng *srng,
 	}
 
 	if (hp >= tp)
-		return ((hp - tp) / srng->entry_size) - 1;
+		return (hp - tp) / srng->entry_size;
 	else
-		return ((srng->ring_size - tp + hp) / srng->entry_size) - 1;
+		return (srng->ring_size - tp + hp) / srng->entry_size;
 }
 
 /* Returns number of available entries in src ring */
@@ -862,8 +862,7 @@ void ath11k_hal_srng_access_begin(struct ath11k_base *ab, struct hal_srng *srng)
 		srng->u.src_ring.cached_tp =
 			*(volatile u32 *)srng->u.src_ring.tp_addr;
 	else
-		srng->u.dst_ring.cached_hp =
-			*(volatile u32 *)srng->u.dst_ring.hp_addr;
+		srng->u.dst_ring.cached_hp = *srng->u.dst_ring.hp_addr;
 }
 
 /* Update cached ring head/tail pointers to HW. ath11k_hal_srng_access_begin()
diff --git a/drivers/net/wireless/ath/ath11k/hal.h b/drivers/net/wireless/ath/ath11k/hal.h
index a1e917e..d580acf 100644
--- a/drivers/net/wireless/ath/ath11k/hal.h
+++ b/drivers/net/wireless/ath/ath11k/hal.h
@@ -538,7 +538,7 @@ struct hal_srng {
 			u32 tp;
 
 			/* Shadow head pointer location to be updated by HW */
-			u32 *hp_addr;
+			volatile u32 *hp_addr;
 
 			/* Cached head pointer */
 			u32 cached_hp;
-- 
1.9.1


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

[PATCH 2/2] ath11k/dp_rx: Fix possible REO ring desc overwrite

[Vasanthakumar Thiagarajan]

When we find there is not desc available in REO ring based
on the cached hp (head pointer), we again try to read the REO
ting with the latest hp that hw might have just updated. In this
case we update the tp (tail pointer) before the corresponding
desc is completely processed by the driver. This would lead to
a scenario where hw will be overwriting the desc which is still
not completely prcoessed. Make sure tp is not updated to hw before
the residing desc is completely processed.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
---
 drivers/net/wireless/ath/ath11k/dp_rx.c | 45 +++++++++++++--------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/dp_rx.c b/drivers/net/wireless/ath/ath11k/dp_rx.c
index 686b140..63b7275 100644
--- a/drivers/net/wireless/ath/ath11k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath11k/dp_rx.c
@@ -2104,32 +2104,6 @@ static void ath11k_dp_rx_pre_deliver_amsdu(struct ath11k *ar,
 	}
 }
 
-static u32 *ath11k_dp_rx_get_reo_desc(struct ath11k_base *ab,
-				      struct hal_srng *srng)
-{
-	u32 *rx_desc;
-
-	lockdep_assert_held(&srng->lock);
-
-	rx_desc = ath11k_hal_srng_dst_get_next_entry(ab, srng);
-
-	/* Hw might have updated the head pointer after we cached it.
-	 * In this case, even though there are entries in the ring we'll
-	 * get rx_desc NULL. Give the read another try with updated cached
-	 * head pointer so that we can reap complete MPDU in the current
-	 * rx processing.
-	 */
-	if (!rx_desc) {
-		ath11k_hal_srng_access_begin(ab, srng);
-		rx_desc = ath11k_hal_srng_dst_get_next_entry(ab, srng);
-		if (!rx_desc)
-			return NULL;
-		ath11k_hal_srng_access_end(ab, srng);
-	}
-
-	return rx_desc;
-}
-
 static void ath11k_dp_rx_process_pending_packets(struct ath11k_base *ab,
 						 struct napi_struct *napi,
 						 struct sk_buff_head *pending_q,
@@ -2178,6 +2152,7 @@ int ath11k_dp_process_rx(struct ath11k_base *ab, int mac_id,
 	int num_buffs_reaped = 0;
 	int quota = budget;
 	int ret;
+	bool done = false;
 
 	/* Process any pending packets from the previous napi poll.
 	 * Note: All msdu's in this pending_q corresponds to the same mac id
@@ -2201,7 +2176,8 @@ int ath11k_dp_process_rx(struct ath11k_base *ab, int mac_id,
 
 	ath11k_hal_srng_access_begin(ab, srng);
 
-	while ((rx_desc = ath11k_dp_rx_get_reo_desc(ab, srng))) {
+try_again:
+	while ((rx_desc = ath11k_hal_srng_dst_get_next_entry(ab, srng))) {
 		memset(&meta_info, 0, sizeof(meta_info));
 		ath11k_hal_rx_parse_dst_ring_desc(ab, rx_desc, &meta_info);
 
@@ -2250,8 +2226,21 @@ int ath11k_dp_process_rx(struct ath11k_base *ab, int mac_id,
 		 * and how use of budget instead of remaining quota affects it.
 		 */
 		if (num_buffs_reaped >= quota && rxcb->is_last_msdu &&
-		    !rxcb->is_continuation)
+		    !rxcb->is_continuation) {
+			done = true;
 			break;
+		}
+	}
+
+	/* Hw might have updated the head pointer after we cached it.
+	 * In this case, even though there are entries in the ring we'll
+	 * get rx_desc NULL. Give the read another try with updated cached
+	 * head pointer so that we can reap complete MPDU in the current
+	 * rx processing.
+	 */
+	if (!done && ath11k_hal_srng_dst_num_free(ab, srng, true)) {
+		ath11k_hal_srng_access_end(ab, srng);
+		goto try_again;
 	}
 
 	ath11k_hal_srng_access_end(ab, srng);
-- 
1.9.1


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

Re: [PATCH 1/2] ath11k/hal: Fix few bugs in ath11k_hal_srng_dst_num_free()

[Kalle Valo]

Vasanthakumar Thiagarajan <vthiagar@codeaurora.org> writes:

> The logic to compute the number of available buffers in destination
> ring is wrong. It should be just the different between head and
> tail pointers in terms of the entry size. This functions currently
> unused, this is fixed to make use of this function in follow-up
> patches. Also make destination ring head pointer volatile because
> it is independently updated by HW.
>
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>

[...]

> --- a/drivers/net/wireless/ath/ath11k/hal.h
> +++ b/drivers/net/wireless/ath/ath11k/hal.h
> @@ -538,7 +538,7 @@ struct hal_srng {
>  			u32 tp;
>  
>  			/* Shadow head pointer location to be updated by HW */
> -			u32 *hp_addr;
> +			volatile u32 *hp_addr;

What about tp_addr, shouldn't we make that also volatile to remove the
ugly cast? Can someone send another patch to fix that, please?

-- 
Kalle Valo

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

Re: [PATCH 1/2] ath11k/hal: Fix few bugs in ath11k_hal_srng_dst_num_free()

[Vasanthakumar Thiagarajan]

On 2019-07-18 17:07, Kalle Valo wrote:
> Vasanthakumar Thiagarajan <vthiagar@codeaurora.org> writes:
> 
>> The logic to compute the number of available buffers in destination
>> ring is wrong. It should be just the different between head and
>> tail pointers in terms of the entry size. This functions currently
>> unused, this is fixed to make use of this function in follow-up
>> patches. Also make destination ring head pointer volatile because
>> it is independently updated by HW.
>> 
>> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
> 
> [...]
> 
>> --- a/drivers/net/wireless/ath/ath11k/hal.h
>> +++ b/drivers/net/wireless/ath/ath11k/hal.h
>> @@ -538,7 +538,7 @@ struct hal_srng {
>>  			u32 tp;
>> 
>>  			/* Shadow head pointer location to be updated by HW */
>> -			u32 *hp_addr;
>> +			volatile u32 *hp_addr;
> 
> What about tp_addr, shouldn't we make that also volatile to remove the
> ugly cast? Can someone send another patch to fix that, please?

As mentioned, i'll address the other case separately.

Vasanth

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

Re: [PATCH 1/2] ath11k/hal: Fix few bugs in ath11k_hal_srng_dst_num_free()

[Kalle Valo]

Vasanthakumar Thiagarajan <vthiagar@codeaurora.org> wrote:

> The logic to compute the number of available buffers in destination
> ring is wrong. It should be just the different between head and
> tail pointers in terms of the entry size. This functions currently
> unused, this is fixed to make use of this function in follow-up
> patches. Also make destination ring head pointer volatile because
> it is independently updated by HW.
> 
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

2 patches applied to ath11k-bringup branch of ath.git, thanks.

df3ae4c41c13 ath11k/hal: Fix few bugs in ath11k_hal_srng_dst_num_free()
b5ca4711131d ath11k/dp_rx: Fix possible REO ring desc overwrite

-- 
https://patchwork.kernel.org/patch/11048495/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


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