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@xxxxxxxxxxxxxx>
> Sent: Thursday, May 7, 2020 11:37 AM
> To: Rakesh Pillai <pillair@xxxxxxxxxxxxxx>
> Cc: ath10k@xxxxxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] ath10k: Remove msdu from idr when management
> pkt send fails
>
> Rakesh Pillai <pillair@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>
> [...]
>
> > --- 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
