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 <[email protected]>
Closes: https://lore.kernel.org/ath12k/[email protected]/
Signed-off-by: Baochen Qiang <[email protected]>
---
drivers/net/wireless/ath/ath12k/core.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index 9482d5db71e7..e7f628e935e4 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -50,19 +50,16 @@ int ath12k_core_suspend(struct ath12k_base *ab)
if (!ab->hw_params->supports_suspend)
return -EOPNOTSUPP;
- rcu_read_lock();
for (i = 0; i < ab->num_radios; i++) {
- ar = ath12k_mac_get_ar_by_pdev_id(ab, i);
+ ar = ab->pdevs[i].ar;
if (!ar)
continue;
ret = ath12k_mac_wait_tx_complete(ar);
if (ret) {
ath12k_warn(ab, "failed to wait tx complete: %d\n", ret);
- rcu_read_unlock();
return ret;
}
}
- rcu_read_unlock();
/* PM framework skips suspend_late/resume_early callbacks
* if other devices report errors in their suspend callbacks.
base-commit: 1025c616ee13372f3803b158abb1d87ef368ae3d
--
2.25.1
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 <[email protected]>
> Closes: https://lore.kernel.org/ath12k/[email protected]/
> Signed-off-by: Baochen Qiang <[email protected]>
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 <[email protected]>
Baochen Qiang <[email protected]> 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 <[email protected]>
> Closes: https://lore.kernel.org/ath12k/[email protected]/
> Signed-off-by: Baochen Qiang <[email protected]>
> Acked-by: Jeff Johnson <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
33370412eced wifi: ath12k: fix Smatch warnings on ath12k_core_suspend()
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches