From: Kalle Valo <kvalo@codeaurora.org>
To: Brian Norris <briannorris@chromium.org>
Cc: linux-wireless <linux-wireless@vger.kernel.org>,
ath10k <ath10k@lists.infradead.org>,
Miaoqing Pan <miaoqing@codeaurora.org>
Subject: Re: [PATCH] ath10k: fix wmi mgmt tx queue full due to race condition
Date: Mon, 21 Dec 2020 21:53:28 +0200 [thread overview]
Message-ID: <87ft3zndfr.fsf@codeaurora.org> (raw)
In-Reply-To: <CA+ASDXP8LotnQZNvXYZqfYH8za6rx1DaZmnH21TsO2NmzX+OZA@mail.gmail.com> (Brian Norris's message of "Mon, 21 Dec 2020 11:31:40 -0800")
Brian Norris <briannorris@chromium.org> writes:
> Hi,
>
> On Sun, Dec 20, 2020 at 5:53 PM Miaoqing Pan <miaoqing@codeaurora.org> wrote:
>>
>> Failed to transmit wmi management frames:
>>
>> [84977.840894] ath10k_snoc a000000.wifi: wmi mgmt tx queue is full
>> [84977.840913] ath10k_snoc a000000.wifi: failed to transmit packet, dropping: -28
>> [84977.840924] ath10k_snoc a000000.wifi: failed to submit frame: -28
>> [84977.840932] ath10k_snoc a000000.wifi: failed to transmit frame: -28
>>
>> This issue is caused by race condition between skb_dequeue and
>> __skb_queue_tail. The queue of ‘wmi_mgmt_tx_queue’ is protected by a
>> different lock: ar->data_lock vs list->lock, the result is no protection.
>
> Nice catch!
>
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -3763,23 +3763,16 @@ bool ath10k_mac_tx_frm_has_freq(struct ath10k *ar)
>> static int ath10k_mac_tx_wmi_mgmt(struct ath10k *ar, struct sk_buff *skb)
>> {
>> struct sk_buff_head *q = &ar->wmi_mgmt_tx_queue;
>> - int ret = 0;
>> -
>> - spin_lock_bh(&ar->data_lock);
>>
>> if (skb_queue_len(q) == ATH10K_MAX_NUM_MGMT_PENDING) {
>
> I believe you should be switching this to use skb_queue_len_lockless()
> too.
Why lockless? (reads documentation) Ah, is it due to memory
synchronisation now that we don't take the data_lock anymore?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
WARNING: multiple messages have this Message-ID (diff)
From: Kalle Valo <kvalo@codeaurora.org>
To: Brian Norris <briannorris@chromium.org>
Cc: Miaoqing Pan <miaoqing@codeaurora.org>,
linux-wireless <linux-wireless@vger.kernel.org>,
ath10k <ath10k@lists.infradead.org>
Subject: Re: [PATCH] ath10k: fix wmi mgmt tx queue full due to race condition
Date: Mon, 21 Dec 2020 21:53:28 +0200 [thread overview]
Message-ID: <87ft3zndfr.fsf@codeaurora.org> (raw)
In-Reply-To: <CA+ASDXP8LotnQZNvXYZqfYH8za6rx1DaZmnH21TsO2NmzX+OZA@mail.gmail.com> (Brian Norris's message of "Mon, 21 Dec 2020 11:31:40 -0800")
Brian Norris <briannorris@chromium.org> writes:
> Hi,
>
> On Sun, Dec 20, 2020 at 5:53 PM Miaoqing Pan <miaoqing@codeaurora.org> wrote:
>>
>> Failed to transmit wmi management frames:
>>
>> [84977.840894] ath10k_snoc a000000.wifi: wmi mgmt tx queue is full
>> [84977.840913] ath10k_snoc a000000.wifi: failed to transmit packet, dropping: -28
>> [84977.840924] ath10k_snoc a000000.wifi: failed to submit frame: -28
>> [84977.840932] ath10k_snoc a000000.wifi: failed to transmit frame: -28
>>
>> This issue is caused by race condition between skb_dequeue and
>> __skb_queue_tail. The queue of ‘wmi_mgmt_tx_queue’ is protected by a
>> different lock: ar->data_lock vs list->lock, the result is no protection.
>
> Nice catch!
>
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -3763,23 +3763,16 @@ bool ath10k_mac_tx_frm_has_freq(struct ath10k *ar)
>> static int ath10k_mac_tx_wmi_mgmt(struct ath10k *ar, struct sk_buff *skb)
>> {
>> struct sk_buff_head *q = &ar->wmi_mgmt_tx_queue;
>> - int ret = 0;
>> -
>> - spin_lock_bh(&ar->data_lock);
>>
>> if (skb_queue_len(q) == ATH10K_MAX_NUM_MGMT_PENDING) {
>
> I believe you should be switching this to use skb_queue_len_lockless()
> too.
Why lockless? (reads documentation) Ah, is it due to memory
synchronisation now that we don't take the data_lock anymore?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
next prev parent reply other threads:[~2020-12-21 19:53 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-21 1:52 [PATCH] ath10k: fix wmi mgmt tx queue full due to race condition Miaoqing Pan
2020-12-21 1:52 ` Miaoqing Pan
2020-12-21 19:31 ` Brian Norris
2020-12-21 19:31 ` Brian Norris
2020-12-21 19:53 ` Kalle Valo [this message]
2020-12-21 19:53 ` Kalle Valo
2020-12-21 20:20 ` Brian Norris
2020-12-21 20:20 ` Brian Norris
2021-01-18 16:16 ` Kalle Valo
2021-01-18 16:16 ` Kalle Valo
[not found] ` <20210118161650.4B9BAC43461@smtp.codeaurora.org>
2021-01-20 1:33 ` miaoqing
2021-01-20 1:33 ` miaoqing
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ft3zndfr.fsf@codeaurora.org \
--to=kvalo@codeaurora.org \
--cc=ath10k@lists.infradead.org \
--cc=briannorris@chromium.org \
--cc=linux-wireless@vger.kernel.org \
--cc=miaoqing@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.