Received-SPF: pass (google.com: domain of linux-wireless+bounces-7645-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <9076663c-cfda-41a4-8580-6c922696b22c@quicinc.com>
Date: Tue, 14 May 2024 07:12:38 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: ath12k: fix Smatch warnings on
 ath12k_core_suspend()
Content-Language: en-US
To: Baochen Qiang <quic_bqiang@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240511095045.9623-1-quic_bqiang@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240511095045.9623-1-quic_bqiang@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 5/11/2024 2:50 AM, Baochen Qiang wrote:
> Smatch is throwing below warning:
> 
> Commit 692921ead832 ("wifi: ath12k: flush all packets before
> suspend") leads to the following Smatch static checker warning:
> 
> 	drivers/net/wireless/ath/ath12k/core.c:58 ath12k_core_suspend()
> 	warn: sleeping in atomic context
> 
> and also gives the reason:
> 
> drivers/net/wireless/ath/ath12k/core.c
>     48         int ret, i;
>     49
>     50         if (!ab->hw_params->supports_suspend)
>     51                 return -EOPNOTSUPP;
>     52
>     53         rcu_read_lock();
>                ^^^^^^^^^^^^^^^
> Disables preemption.
> 
>     54         for (i = 0; i < ab->num_radios; i++) {
>     55                 ar = ath12k_mac_get_ar_by_pdev_id(ab, i);
>     56                 if (!ar)
>     57                         continue;
> --> 58                 ret = ath12k_mac_wait_tx_complete(ar);
>                                         ^^^^^^^
> Sleeping in atomic context.
> 
>     59                 if (ret) {
>     60                         ath12k_warn(ab, "failed to wait tx complete: %d\n", ret);
>     61                         rcu_read_unlock();
>     62                         return ret;
>     63                 }
>     64         }
>     65         rcu_read_unlock();
> 
> But it is weird that no warning on this in run time even with
> CONFIG_DEBUG_ATOMIC_SLEEP=y. With some debug it is found that this is
> because: when system goes to suspend, ath12k_mac_op_stop() gets called
> where then in ath12k_mac_stop() ab->pdevs_active[ar->pdev_idx] is cleared.
> This results in ath12k_mac_get_ar_by_pdev_id() always returning a NULL ar,
> and thereby ath12k_mac_wait_tx_complete() never gets a chance to run.
> 
> Fix it by retrieving ar directly from ab->pdevs[].ar instead of using
> ath12k_mac_get_ar_by_pdev_id(). Since ab->pdevs[].ar is set at boot time
> and won't get cleared when suspend, ath12k_mac_wait_tx_complete() won't
> be skipped. In addition, with ath12k_mac_get_ar_by_pdev_id() removed,
> rcu_read_lock()/unlock() are not needed any more, so remove them. This
> also fixes the warning above.
> 
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
> 
> Fixes: 692921ead832 ("wifi: ath12k: flush all packets before suspend")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/ath12k/7a96ca11-80b5-4751-8cfc-fa637f3aa63a@moroto.mountain/
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>

I still can't get my copy of smatch to find the original issue
[jjohnson:~ 63] smatch --version
v0.5.0-8639-gff1cc4d453ff

But obviously this fixes the issue, so...
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>