[PATCH 1/2] ath11k: fix missing srng_access_end in CE

[PATCH 1/2] ath11k: fix missing srng_access_end in CE

[Christian 'Ansuel' Marangi]

When a CE completed send next operation is done, the srng access end is
never called. Correctly end the srng access to make sure we have the
correct values in the srng struct.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
---
 drivers/net/wireless/ath/ath11k/ce.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index c14c51f38709..665205d2322e 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -490,6 +490,8 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe
 	pipe->src_ring->sw_index = sw_index;
 
 err_unlock:
+	ath11k_hal_srng_access_end(ab, srng);
+
 	spin_unlock_bh(&srng->lock);
 
 	spin_unlock_bh(&ab->ce.ce_lock);
-- 
2.36.1


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

[PATCH 1/2] ath11k: fix missing srng_access_end in CE

[Christian 'Ansuel' Marangi]

When a CE completed send next operation is done, the srng access end is
never called. Correctly end the srng access to make sure we have the
correct values in the srng struct.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
---
 drivers/net/wireless/ath/ath11k/ce.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index c14c51f38709..665205d2322e 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -490,6 +490,8 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe
 	pipe->src_ring->sw_index = sw_index;
 
 err_unlock:
+	ath11k_hal_srng_access_end(ab, srng);
+
 	spin_unlock_bh(&srng->lock);
 
 	spin_unlock_bh(&ab->ce.ce_lock);
-- 
2.36.1

[PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Christian 'Ansuel' Marangi]

On htc_tx_completion error the skb is not dropped. This is wrong since
the completion_handler logic expect the skb to be consumed anyway even
when an error is triggerer. Not freeing the skb on error is a memory
leak since the skb won't be freed anywere else. Correctly free the
packet on eid >= ATH11K_HTC_EP_COUNT before returning.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1

Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
---
 drivers/net/wireless/ath/ath11k/htc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/htc.c b/drivers/net/wireless/ath/ath11k/htc.c
index 069c29a4fac7..ca3aedc0252d 100644
--- a/drivers/net/wireless/ath/ath11k/htc.c
+++ b/drivers/net/wireless/ath/ath11k/htc.c
@@ -258,8 +258,10 @@ void ath11k_htc_tx_completion_handler(struct ath11k_base *ab,
 	u8 eid;
 
 	eid = ATH11K_SKB_CB(skb)->eid;
-	if (eid >= ATH11K_HTC_EP_COUNT)
+	if (eid >= ATH11K_HTC_EP_COUNT) {
+		dev_kfree_skb_any(skb);
 		return;
+	}
 
 	ep = &htc->endpoint[eid];
 	spin_lock_bh(&htc->tx_lock);
-- 
2.36.1


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

[PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Christian 'Ansuel' Marangi]

On htc_tx_completion error the skb is not dropped. This is wrong since
the completion_handler logic expect the skb to be consumed anyway even
when an error is triggerer. Not freeing the skb on error is a memory
leak since the skb won't be freed anywere else. Correctly free the
packet on eid >= ATH11K_HTC_EP_COUNT before returning.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1

Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
---
 drivers/net/wireless/ath/ath11k/htc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/htc.c b/drivers/net/wireless/ath/ath11k/htc.c
index 069c29a4fac7..ca3aedc0252d 100644
--- a/drivers/net/wireless/ath/ath11k/htc.c
+++ b/drivers/net/wireless/ath/ath11k/htc.c
@@ -258,8 +258,10 @@ void ath11k_htc_tx_completion_handler(struct ath11k_base *ab,
 	u8 eid;
 
 	eid = ATH11K_SKB_CB(skb)->eid;
-	if (eid >= ATH11K_HTC_EP_COUNT)
+	if (eid >= ATH11K_HTC_EP_COUNT) {
+		dev_kfree_skb_any(skb);
 		return;
+	}
 
 	ep = &htc->endpoint[eid];
 	spin_lock_bh(&htc->tx_lock);
-- 
2.36.1

Re: [PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Jeff Johnson]

On 5/28/2022 7:25 AM, Christian 'Ansuel' Marangi wrote:
> On htc_tx_completion error the skb is not dropped. This is wrong since
> the completion_handler logic expect the skb to be consumed anyway even
> when an error is triggerer. Not freeing the skb on error is a memory

nit: s/triggerer/triggered/

Kalle can fix this when he merges so no need to post a correction

> leak since the skb won't be freed anywere else. Correctly free the
> packet on eid >= ATH11K_HTC_EP_COUNT before returning.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
>   drivers/net/wireless/ath/ath11k/htc.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/htc.c b/drivers/net/wireless/ath/ath11k/htc.c
> index 069c29a4fac7..ca3aedc0252d 100644
> --- a/drivers/net/wireless/ath/ath11k/htc.c
> +++ b/drivers/net/wireless/ath/ath11k/htc.c
> @@ -258,8 +258,10 @@ void ath11k_htc_tx_completion_handler(struct ath11k_base *ab,
>   	u8 eid;
>   
>   	eid = ATH11K_SKB_CB(skb)->eid;
> -	if (eid >= ATH11K_HTC_EP_COUNT)
> +	if (eid >= ATH11K_HTC_EP_COUNT) {
> +		dev_kfree_skb_any(skb);
>   		return;
> +	}
>   
>   	ep = &htc->endpoint[eid];
>   	spin_lock_bh(&htc->tx_lock);


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

Re: [PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Jeff Johnson]

On 5/28/2022 7:25 AM, Christian 'Ansuel' Marangi wrote:
> On htc_tx_completion error the skb is not dropped. This is wrong since
> the completion_handler logic expect the skb to be consumed anyway even
> when an error is triggerer. Not freeing the skb on error is a memory

nit: s/triggerer/triggered/

Kalle can fix this when he merges so no need to post a correction

> leak since the skb won't be freed anywere else. Correctly free the
> packet on eid >= ATH11K_HTC_EP_COUNT before returning.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
>   drivers/net/wireless/ath/ath11k/htc.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/htc.c b/drivers/net/wireless/ath/ath11k/htc.c
> index 069c29a4fac7..ca3aedc0252d 100644
> --- a/drivers/net/wireless/ath/ath11k/htc.c
> +++ b/drivers/net/wireless/ath/ath11k/htc.c
> @@ -258,8 +258,10 @@ void ath11k_htc_tx_completion_handler(struct ath11k_base *ab,
>   	u8 eid;
>   
>   	eid = ATH11K_SKB_CB(skb)->eid;
> -	if (eid >= ATH11K_HTC_EP_COUNT)
> +	if (eid >= ATH11K_HTC_EP_COUNT) {
> +		dev_kfree_skb_any(skb);
>   		return;
> +	}
>   
>   	ep = &htc->endpoint[eid];
>   	spin_lock_bh(&htc->tx_lock);

Re: [PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 5/28/2022 7:25 AM, Christian 'Ansuel' Marangi wrote:
>> On htc_tx_completion error the skb is not dropped. This is wrong since
>> the completion_handler logic expect the skb to be consumed anyway even
>> when an error is triggerer. Not freeing the skb on error is a memory
>
> nit: s/triggerer/triggered/
>
> Kalle can fix this when he merges so no need to post a correction

Yup, fixed now in the pending branch.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

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

Re: [PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 5/28/2022 7:25 AM, Christian 'Ansuel' Marangi wrote:
>> On htc_tx_completion error the skb is not dropped. This is wrong since
>> the completion_handler logic expect the skb to be consumed anyway even
>> when an error is triggerer. Not freeing the skb on error is a memory
>
> nit: s/triggerer/triggered/
>
> Kalle can fix this when he merges so no need to post a correction

Yup, fixed now in the pending branch.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Kalle Valo]

Christian 'Ansuel' Marangi <ansuelsmth@gmail.com> wrote:

> On htc_tx_completion error the skb is not dropped. This is wrong since
> the completion_handler logic expect the skb to be consumed anyway even
> when an error is triggered. Not freeing the skb on error is a memory
> leak since the skb won't be freed anywere else. Correctly free the
> packet on eid >= ATH11K_HTC_EP_COUNT before returning.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

e5646fe3b7ef ath11k: fix missing skb drop on htc_tx_completion error

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220528142516.20819-2-ansuelsmth@gmail.com/

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


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

Re: [PATCH 2/2] ath11k: fix missing skb drop on htc_tx_completion error

[Kalle Valo]

Christian 'Ansuel' Marangi <ansuelsmth@gmail.com> wrote:

> On htc_tx_completion error the skb is not dropped. This is wrong since
> the completion_handler logic expect the skb to be consumed anyway even
> when an error is triggered. Not freeing the skb on error is a memory
> leak since the skb won't be freed anywere else. Correctly free the
> packet on eid >= ATH11K_HTC_EP_COUNT before returning.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

e5646fe3b7ef ath11k: fix missing skb drop on htc_tx_completion error

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220528142516.20819-2-ansuelsmth@gmail.com/

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

Re: [PATCH 1/2] ath11k: fix missing srng_access_end in CE

[Jeff Johnson]

On 5/28/2022 7:25 AM, Christian 'Ansuel' Marangi wrote:
> When a CE completed send next operation is done, the srng access end is
> never called. Correctly end the srng access to make sure we have the
> correct values in the srng struct.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
> ---
>   drivers/net/wireless/ath/ath11k/ce.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
> index c14c51f38709..665205d2322e 100644
> --- a/drivers/net/wireless/ath/ath11k/ce.c
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -490,6 +490,8 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe
>   	pipe->src_ring->sw_index = sw_index;
>   
>   err_unlock:
> +	ath11k_hal_srng_access_end(ab, srng);
> +
>   	spin_unlock_bh(&srng->lock);
>   
>   	spin_unlock_bh(&ab->ce.ce_lock);

NAK -- Not calling ath11k_hal_srng_access_end() is intentional.

CE_SRC ring is a special ring, we perform tx_request and tx_completion 
on the same ring. The completion processing frees SKBs associated to the 
ce_desc, this is what ath11k_ce_completed_send_next() does, it does not 
update the head pointer to hw because the ce_desc at hp does not have 
any new data, it is just available for new messages. ath11k_ce_send() 
retrieves a processed (processed by ath11k_ce_completed_send_next) 
ce_desc and sets it up for the new message. Then it updates the hw about 
the newly available ce_desc by calling ath11k_hal_srng_access_end().

If you want to do anything here, adding a comment based upon the above 
would be good.

/jeff

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

Re: [PATCH 1/2] ath11k: fix missing srng_access_end in CE

[Jeff Johnson]

On 5/28/2022 7:25 AM, Christian 'Ansuel' Marangi wrote:
> When a CE completed send next operation is done, the srng access end is
> never called. Correctly end the srng access to make sure we have the
> correct values in the srng struct.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
> ---
>   drivers/net/wireless/ath/ath11k/ce.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
> index c14c51f38709..665205d2322e 100644
> --- a/drivers/net/wireless/ath/ath11k/ce.c
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -490,6 +490,8 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe
>   	pipe->src_ring->sw_index = sw_index;
>   
>   err_unlock:
> +	ath11k_hal_srng_access_end(ab, srng);
> +
>   	spin_unlock_bh(&srng->lock);
>   
>   	spin_unlock_bh(&ab->ce.ce_lock);

NAK -- Not calling ath11k_hal_srng_access_end() is intentional.

CE_SRC ring is a special ring, we perform tx_request and tx_completion 
on the same ring. The completion processing frees SKBs associated to the 
ce_desc, this is what ath11k_ce_completed_send_next() does, it does not 
update the head pointer to hw because the ce_desc at hp does not have 
any new data, it is just available for new messages. ath11k_ce_send() 
retrieves a processed (processed by ath11k_ce_completed_send_next) 
ce_desc and sets it up for the new message. Then it updates the hw about 
the newly available ce_desc by calling ath11k_hal_srng_access_end().

If you want to do anything here, adding a comment based upon the above 
would be good.

/jeff