Received-SPF: pass (google.com: domain of linux-wireless+bounces-8288-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <6c8ca575-01b7-41ba-97d9-cfe28f69c1e6@quicinc.com>
Date: Thu, 30 May 2024 14:23:20 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [bug report] wifi: ath11k: add P2P IE in beacon template
To: Dan Carpenter <dan.carpenter@linaro.org>
CC: <linux-wireless@vger.kernel.org>, <ath11k@lists.infradead.org>
References: <2d277abd-5e7b-4da0-80e0-52bd96337f6e@moroto.mountain>
Content-Language: en-US
From: Kang Yang <quic_kangyang@quicinc.com>
In-Reply-To: <2d277abd-5e7b-4da0-80e0-52bd96337f6e@moroto.mountain>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 5/29/2024 10:32 PM, Dan Carpenter wrote:
> Hello Kang Yang,
> 
> Commit 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
> from Feb 28, 2024 (linux-next), leads to the following Smatch static
> checker warning:
> 
> 	drivers/net/wireless/ath/ath11k/wmi.c:1742 ath11k_wmi_p2p_go_bcn_ie()
> 	warn: sleeping in atomic context
> 
> drivers/net/wireless/ath/ath11k/wmi.c
>      1712 int ath11k_wmi_p2p_go_bcn_ie(struct ath11k *ar, u32 vdev_id,
>      1713                              const u8 *p2p_ie)
>      1714 {
>      1715         struct ath11k_pdev_wmi *wmi = ar->wmi;
>      1716         struct wmi_p2p_go_set_beacon_ie_cmd *cmd;
>      1717         size_t p2p_ie_len, aligned_len;
>      1718         struct wmi_tlv *tlv;
>      1719         struct sk_buff *skb;
>      1720         int ret, len;
>      1721
>      1722         p2p_ie_len = p2p_ie[1] + 2;
>      1723         aligned_len = roundup(p2p_ie_len, 4);
>      1724
>      1725         len = sizeof(*cmd) + TLV_HDR_SIZE + aligned_len;
>      1726
>      1727         skb = ath11k_wmi_alloc_skb(wmi->wmi_ab, len);
>      1728         if (!skb)
>      1729                 return -ENOMEM;
>      1730
>      1731         cmd = (struct wmi_p2p_go_set_beacon_ie_cmd *)skb->data;
>      1732         cmd->tlv_header = FIELD_PREP(WMI_TLV_TAG, WMI_TAG_P2P_GO_SET_BEACON_IE) |
>      1733                           FIELD_PREP(WMI_TLV_LEN, sizeof(*cmd) - TLV_HDR_SIZE);
>      1734         cmd->vdev_id = vdev_id;
>      1735         cmd->ie_buf_len = p2p_ie_len;
>      1736
>      1737         tlv = (struct wmi_tlv *)cmd->tlv;
>      1738         tlv->header = FIELD_PREP(WMI_TLV_TAG, WMI_TAG_ARRAY_BYTE) |
>      1739                       FIELD_PREP(WMI_TLV_LEN, aligned_len);
>      1740         memcpy(tlv->value, p2p_ie, p2p_ie_len);
>      1741
> --> 1742         ret = ath11k_wmi_cmd_send(wmi, skb, WMI_P2P_GO_SET_BEACON_IE);
>                         ^^^^^^^^^^^^^^^^^^^
> This is a might_sleep() function.
> 
>      1743         if (ret) {
>      1744                 ath11k_warn(ar->ab, "failed to send WMI_P2P_GO_SET_BEACON_IE\n");
>      1745                 dev_kfree_skb(skb);
>      1746         }
>      1747
>      1748         return ret;
>      1749 }
> 
> The problematic call tree is:
> 
> ath11k_bcn_tx_status_event() <- disables preempt
> -> ath11k_mac_bcn_tx_event()
>     -> ath11k_mac_setup_bcn_tmpl()
>        -> ath11k_mac_setup_bcn_tmpl_ema()
>        -> ath11k_mac_setup_bcn_tmpl_mbssid()
>           -> ath11k_mac_set_vif_params()
>              -> ath11k_mac_setup_bcn_p2p_ie()
>                 -> ath11k_wmi_p2p_go_bcn_ie()
> 
> The ath11k_bcn_tx_status_event() function takes rcu_read_lock() which
> disables preemption.  I don't know the code well enough to say if this
> is a real bug...  If it's a false positive, just ignore it.  These are
> one time emails.


I also found:

ath11k_bcn_tx_status_event() <- disables preempt
-> ath11k_mac_bcn_tx_event()
	-> ath11k_mac_setup_bcn_tmpl()
		-> ath11k_mac_setup_bcn_tmpl_ema()
		-> ath11k_mac_setup_bcn_tmpl_mbssid()
			->ath11k_wmi_bcn_tmpl()
				->ath11k_wmi_cmd_send()


It seems this problem already exist even if without my patch.


Fine, i will find solution for this.

> 
> See my blog for more details.
> https://staticthinking.wordpress.com/2024/05/24/sleeping-in-atomic-warnings/
>  > regards,
> dan carpenter