diff mbox series

[v2] ath10k: Remove msdu from idr when management pkt send fails

Message ID 1588667015-25490-1-git-send-email-pillair@codeaurora.org (mailing list archive)
State Accepted
Commit c730c477176ad4af86d9aae4d360a7ad840b073a
Delegated to: Kalle Valo
Headers show
Series [v2] ath10k: Remove msdu from idr when management pkt send fails | expand

Commit Message

Rakesh Pillai May 5, 2020, 8:23 a.m. UTC
Currently when the sending of any management pkt
via wmi command fails, the packet is being unmapped
freed in the error handling. But the idr entry added,
which is used to track these packet is not getting removed.

Hence, during unload, in wmi cleanup, all the entries
in IDR are removed and the corresponding buffer is
attempted to be freed. This can cause a situation where
one packet is attempted to be freed twice.

Fix this error by rmeoving the msdu from the idr
list when the sending of a management packet over
wmi fails.

Tested HW: WCN3990
Tested FW: WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1

Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference support over wmi")
Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
---
Changes from v1:
- Added a helper function in wmi-ops for cleanup_mgmt_tx_send
---
 drivers/net/wireless/ath/ath10k/wmi-ops.h | 15 ++++++++++++++-
 drivers/net/wireless/ath/ath10k/wmi-tlv.c | 15 +++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

Comments

Kalle Valo May 7, 2020, 6:07 a.m. UTC | #1
Rakesh Pillai <pillair@codeaurora.org> writes:

> Currently when the sending of any management pkt
> via wmi command fails, the packet is being unmapped
> freed in the error handling. But the idr entry added,
> which is used to track these packet is not getting removed.
>
> Hence, during unload, in wmi cleanup, all the entries
> in IDR are removed and the corresponding buffer is
> attempted to be freed. This can cause a situation where
> one packet is attempted to be freed twice.
>
> Fix this error by rmeoving the msdu from the idr
> list when the sending of a management packet over
> wmi fails.
>
> Tested HW: WCN3990
> Tested FW: WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
>
> Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference support over wmi")
> Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>

[...]

> --- a/drivers/net/wireless/ath/ath10k/wmi-ops.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h
> @@ -133,6 +133,7 @@ struct wmi_ops {
>  	struct sk_buff *(*gen_mgmt_tx_send)(struct ath10k *ar,
>  					    struct sk_buff *skb,
>  					    dma_addr_t paddr);
> +	int (*cleanup_mgmt_tx_send)(struct ath10k *ar, struct sk_buff *msdu);
>  	struct sk_buff *(*gen_dbglog_cfg)(struct ath10k *ar, u64 module_enable,
>  					  u32 log_level);
>  	struct sk_buff *(*gen_pktlog_enable)(struct ath10k *ar, u32 filter);
> @@ -442,6 +443,15 @@ ath10k_wmi_get_txbf_conf_scheme(struct ath10k *ar)
>  }
>  
>  static inline int
> +ath10k_wmi_cleanup_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu)
> +{
> +	if (!ar->wmi.ops->cleanup_mgmt_tx_send)
> +		return -EOPNOTSUPP;
> +
> +	return ar->wmi.ops->cleanup_mgmt_tx_send(ar, msdu);
> +}
> +
> +static inline int
>  ath10k_wmi_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu,
>  			dma_addr_t paddr)
>  {
> @@ -457,8 +467,11 @@ ath10k_wmi_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu,
>  
>  	ret = ath10k_wmi_cmd_send(ar, skb,
>  				  ar->wmi.cmd->mgmt_tx_send_cmdid);
> -	if (ret)
> +	if (ret) {
> +		/* remove this msdu from idr tracking */
> +		ath10k_wmi_cleanup_mgmt_tx_send(ar, msdu);
>  		return ret;
> +	}

I missed that this call was in wmi-ops.h, but the idea is that file
should be just a dumb wrapper and not have any logic. So I moved this to
mac.c, the functionality should be the same but please do check my
changes:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=master-pending&id=71195d2244ed812c73dc617f7536566400f7ce87
Rakesh Pillai May 7, 2020, 6:15 a.m. UTC | #2
Hi Kalle,
Yes, The change you did is correct and better.
So should I be sending a v3 for this patch ?

Thanks,
Rakesh Pillai.

> -----Original Message-----
> From: Kalle Valo <kvalo@codeaurora.org>
> Sent: Thursday, May 7, 2020 11:37 AM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2] ath10k: Remove msdu from idr when management
> pkt send fails
> 
> Rakesh Pillai <pillair@codeaurora.org> writes:
> 
> > Currently when the sending of any management pkt
> > via wmi command fails, the packet is being unmapped
> > freed in the error handling. But the idr entry added,
> > which is used to track these packet is not getting removed.
> >
> > Hence, during unload, in wmi cleanup, all the entries
> > in IDR are removed and the corresponding buffer is
> > attempted to be freed. This can cause a situation where
> > one packet is attempted to be freed twice.
> >
> > Fix this error by rmeoving the msdu from the idr
> > list when the sending of a management packet over
> > wmi fails.
> >
> > Tested HW: WCN3990
> > Tested FW: WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
> >
> > Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference
> support over wmi")
> > Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
> 
> [...]
> 
> > --- a/drivers/net/wireless/ath/ath10k/wmi-ops.h
> > +++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h
> > @@ -133,6 +133,7 @@ struct wmi_ops {
> >  	struct sk_buff *(*gen_mgmt_tx_send)(struct ath10k *ar,
> >  					    struct sk_buff *skb,
> >  					    dma_addr_t paddr);
> > +	int (*cleanup_mgmt_tx_send)(struct ath10k *ar, struct sk_buff
> *msdu);
> >  	struct sk_buff *(*gen_dbglog_cfg)(struct ath10k *ar, u64
> module_enable,
> >  					  u32 log_level);
> >  	struct sk_buff *(*gen_pktlog_enable)(struct ath10k *ar, u32 filter);
> > @@ -442,6 +443,15 @@ ath10k_wmi_get_txbf_conf_scheme(struct ath10k
> *ar)
> >  }
> >
> >  static inline int
> > +ath10k_wmi_cleanup_mgmt_tx_send(struct ath10k *ar, struct sk_buff
> *msdu)
> > +{
> > +	if (!ar->wmi.ops->cleanup_mgmt_tx_send)
> > +		return -EOPNOTSUPP;
> > +
> > +	return ar->wmi.ops->cleanup_mgmt_tx_send(ar, msdu);
> > +}
> > +
> > +static inline int
> >  ath10k_wmi_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu,
> >  			dma_addr_t paddr)
> >  {
> > @@ -457,8 +467,11 @@ ath10k_wmi_mgmt_tx_send(struct ath10k *ar,
> struct sk_buff *msdu,
> >
> >  	ret = ath10k_wmi_cmd_send(ar, skb,
> >  				  ar->wmi.cmd->mgmt_tx_send_cmdid);
> > -	if (ret)
> > +	if (ret) {
> > +		/* remove this msdu from idr tracking */
> > +		ath10k_wmi_cleanup_mgmt_tx_send(ar, msdu);
> >  		return ret;
> > +	}
> 
> I missed that this call was in wmi-ops.h, but the idea is that file
> should be just a dumb wrapper and not have any logic. So I moved this to
> mac.c, the functionality should be the same but please do check my
> changes:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=m
> aster-pending&id=71195d2244ed812c73dc617f7536566400f7ce87
> 
> --
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingp
> atches
Kalle Valo May 7, 2020, 6:59 a.m. UTC | #3
<pillair@codeaurora.org> writes:

> Yes, The change you did is correct and better.

Thanks for checking.

> So should I be sending a v3 for this patch ?

No need, the patch with this change is now in my pending branch and I'll
apply it to ath-next it in the next few days.
Kalle Valo May 11, 2020, 12:33 p.m. UTC | #4
Rakesh Pillai <pillair@codeaurora.org> wrote:

> Currently when the sending of any management pkt
> via wmi command fails, the packet is being unmapped
> freed in the error handling. But the idr entry added,
> which is used to track these packet is not getting removed.
> 
> Hence, during unload, in wmi cleanup, all the entries
> in IDR are removed and the corresponding buffer is
> attempted to be freed. This can cause a situation where
> one packet is attempted to be freed twice.
> 
> Fix this error by rmeoving the msdu from the idr
> list when the sending of a management packet over
> wmi fails.
> 
> Tested HW: WCN3990
> Tested FW: WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
> 
> Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference support over wmi")
> Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

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

c730c477176a ath10k: Remove msdu from idr when management pkt send fails
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h
index 1491c25..8c3a656 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-ops.h
+++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h
@@ -133,6 +133,7 @@  struct wmi_ops {
 	struct sk_buff *(*gen_mgmt_tx_send)(struct ath10k *ar,
 					    struct sk_buff *skb,
 					    dma_addr_t paddr);
+	int (*cleanup_mgmt_tx_send)(struct ath10k *ar, struct sk_buff *msdu);
 	struct sk_buff *(*gen_dbglog_cfg)(struct ath10k *ar, u64 module_enable,
 					  u32 log_level);
 	struct sk_buff *(*gen_pktlog_enable)(struct ath10k *ar, u32 filter);
@@ -442,6 +443,15 @@  ath10k_wmi_get_txbf_conf_scheme(struct ath10k *ar)
 }
 
 static inline int
+ath10k_wmi_cleanup_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu)
+{
+	if (!ar->wmi.ops->cleanup_mgmt_tx_send)
+		return -EOPNOTSUPP;
+
+	return ar->wmi.ops->cleanup_mgmt_tx_send(ar, msdu);
+}
+
+static inline int
 ath10k_wmi_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu,
 			dma_addr_t paddr)
 {
@@ -457,8 +467,11 @@  ath10k_wmi_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu,
 
 	ret = ath10k_wmi_cmd_send(ar, skb,
 				  ar->wmi.cmd->mgmt_tx_send_cmdid);
-	if (ret)
+	if (ret) {
+		/* remove this msdu from idr tracking */
+		ath10k_wmi_cleanup_mgmt_tx_send(ar, msdu);
 		return ret;
+	}
 
 	return 0;
 }
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index e1ab900f..2a31a42 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -2898,6 +2898,18 @@  ath10k_wmi_tlv_op_gen_request_stats(struct ath10k *ar, u32 stats_mask)
 }
 
 static int
+ath10k_wmi_tlv_op_cleanup_mgmt_tx_send(struct ath10k *ar,
+				       struct sk_buff *msdu)
+{
+	struct ath10k_skb_cb *cb = ATH10K_SKB_CB(msdu);
+	struct ath10k_wmi *wmi = &ar->wmi;
+
+	idr_remove(&wmi->mgmt_pending_tx, cb->msdu_id);
+
+	return 0;
+}
+
+static int
 ath10k_wmi_mgmt_tx_alloc_msdu_id(struct ath10k *ar, struct sk_buff *skb,
 				 dma_addr_t paddr)
 {
@@ -2971,6 +2983,8 @@  ath10k_wmi_tlv_op_gen_mgmt_tx_send(struct ath10k *ar, struct sk_buff *msdu,
 	if (desc_id < 0)
 		goto err_free_skb;
 
+	cb->msdu_id = desc_id;
+
 	ptr = (void *)skb->data;
 	tlv = ptr;
 	tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_MGMT_TX_CMD);
@@ -4419,6 +4433,7 @@  static const struct wmi_ops wmi_tlv_ops = {
 	.gen_force_fw_hang = ath10k_wmi_tlv_op_gen_force_fw_hang,
 	/* .gen_mgmt_tx = not implemented; HTT is used */
 	.gen_mgmt_tx_send = ath10k_wmi_tlv_op_gen_mgmt_tx_send,
+	.cleanup_mgmt_tx_send = ath10k_wmi_tlv_op_cleanup_mgmt_tx_send,
 	.gen_dbglog_cfg = ath10k_wmi_tlv_op_gen_dbglog_cfg,
 	.gen_pktlog_enable = ath10k_wmi_tlv_op_gen_pktlog_enable,
 	.gen_pktlog_disable = ath10k_wmi_tlv_op_gen_pktlog_disable,