Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 6CE26C43461
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
References: <1608515579-1066-1-git-send-email-miaoqing@codeaurora.org>
        <CA+ASDXP8LotnQZNvXYZqfYH8za6rx1DaZmnH21TsO2NmzX+OZA@mail.gmail.com>
Date:   Mon, 21 Dec 2020 21:53:28 +0200
In-Reply-To: <CA+ASDXP8LotnQZNvXYZqfYH8za6rx1DaZmnH21TsO2NmzX+OZA@mail.gmail.com>
        (Brian Norris's message of "Mon, 21 Dec 2020 11:31:40 -0800")
Message-ID: <87ft3zndfr.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Brian Norris <briannorris@chromium.org> writes:

> Hi,
>
> On Sun, Dec 20, 2020 at 5:53 PM Miaoqing Pan <miaoqing@codeaurora.org> wr=
ote:
>>
>> 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, drop=
ping: -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 =E2=80=98wmi_mgmt_tx_queue=E2=80=99 is pr=
otected 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 *a=
r)
>>  static int ath10k_mac_tx_wmi_mgmt(struct ath10k *ar, struct sk_buff *sk=
b)
>>  {
>>         struct sk_buff_head *q =3D &ar->wmi_mgmt_tx_queue;
>> -       int ret =3D 0;
>> -
>> -       spin_lock_bh(&ar->data_lock);
>>
>>         if (skb_queue_len(q) =3D=3D 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?

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatc=
hes