| Message ID | 20240511095045.9623-1-quic_bqiang@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 33370412eced2dc7f81f4324e109d69319cafd82 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: fix Smatch warnings on ath12k_core_suspend() | expand |
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>
Baochen Qiang <quic_bqiang@quicinc.com> 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> > Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 33370412eced wifi: ath12k: fix Smatch warnings on ath12k_core_suspend()
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.
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> --- drivers/net/wireless/ath/ath12k/core.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) base-commit: 1025c616ee13372f3803b158abb1d87ef368ae3d