Re: [PATCH v3 ath-current] wifi: ath12k: convert scan timeout to wiphy delayed work

[Jeff Johnson]

On 7/16/2026 7:56 PM, Runyu Xiao wrote:
> ath12k_mac_op_stop() is called with the wiphy mutex held and calls
> ath12k_mac_stop(), which cancels ar->scan.timeout. The timeout worker

you are missing a very important detail, namely it is a synchronous cancel

> also takes the wiphy mutex before aborting the scan, so synchronously
> cancelling it from the stop path can deadlock if the worker has started
> and is waiting for the same mutex.
> 
> Do not drop the wiphy mutex inside the mac80211 stop callback. Convert

What does this mean? Where are we currently dropping the wiphy mutex?

You should describe the patch in terms of the current code, not in terms of
any prior versions of your patch.

> ar->scan.timeout to wiphy_delayed_work instead, so the timeout callback
> runs in wiphy work context with the wiphy mutex held. This matches the
> locking model used by the scan vdev cleanup work and lets stop/cancel
> paths use wiphy_delayed_work_cancel() while they already hold the wiphy
> mutex.
> 
> The old scan-finish path could cancel the delayed work directly from WMI

what is "old"? There is the current code and the new proposed code.

> event context. With wiphy_delayed_work that cancellation must happen from
> wiphy context, so keep it in scan.vdev_clean_wk. Mark scans whose cleanup
> work has been queued so a timeout work item that was already queued before
> cleanup runs does not abort a scan that is already finishing.

My AI review agent thinks this logic is unnecessary, but I haven't looked at
this in detail.

"summary": "finish_queued flag is a redundant substitute for
wiphy_delayed_work_cancel: __ath12k_mac_scan_finish is always called under
wiphy lock, making direct cancellation safe",

"failure_scenario": "Every caller of __ath12k_mac_scan_finish holds the wiphy
lock (directly or via wiphy_work dispatch), so wiphy_delayed_work_cancel could
be called there directly — as the old cancel_delayed_work was. The new
finish_queued flag instead defers suppression to the timeout work itself.
Since ath12k_scan_abort already has an explicit ATH12K_SCAN_IDLE bail-out path
(line 5232) with a comment covering the timeout-vs-completion race, the
finish_queued guard in ath12k_scan_timeout_work provides no additional safety.
The cost is 8 reset sites (lines 5337, 5624, 5681, 5775, 10971, 13969, 14012,
15061) where a future error path that forgets the reset leaves
finish_queued=true, silently suppressing the next legitimate timeout abort and
leaving the driver wedged in SCAN_RUNNING with no recovery."
> 
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Suggested-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>
> ---
> Changes in v3:
>   - Convert scan.timeout to wiphy_delayed_work instead of dropping the
>     mac80211-owned wiphy lock in ath12k_mac_op_stop().
>   - Cancel the timeout from scan.vdev_clean_wk to preserve the
>     scan-finish cancellation from wiphy context.
>   - Add scan.finish_queued so an already queued timeout work item does not
>     abort a scan that has already queued its cleanup work.
> 
> Changes in v2:
>   - Rebase on ath.git ath-current.
>   - Use ath-current subject tag as requested.
>   - Move the synchronous scan timeout drain out of the locked stop path
>     instead of adding it in the wrong start-path context.
>   - Update Fixes to the ath12k commit that introduced the stop-time
>     scan.timeout drain.

FWIW I'd suggest looking into using the 'b4' tool for future contributions.
It will help you with patch history, including providing links to the prior
versions

https://b4.docs.kernel.org/en/latest/#for-developers

> 
>  drivers/net/wireless/ath/ath12k/core.c |  2 +-
>  drivers/net/wireless/ath/ath12k/core.h |  3 +-
>  drivers/net/wireless/ath/ath12k/mac.c  | 50 ++++++++++++++++++--------
>  3 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
> index 742d4fd1b598..95673d3be9da 100644
> --- a/drivers/net/wireless/ath/ath12k/core.c
> +++ b/drivers/net/wireless/ath/ath12k/core.c
> @@ -1463,7 +1463,7 @@ void ath12k_core_halt(struct ath12k *ar)
>  
>  	ath12k_mac_scan_finish(ar);
>  	ath12k_mac_peer_cleanup_all(ar);
> -	cancel_delayed_work_sync(&ar->scan.timeout);
> +	wiphy_delayed_work_cancel(ath12k_ar_to_hw(ar)->wiphy, &ar->scan.timeout);
>  	cancel_work_sync(&ar->regd_update_work);
>  	cancel_work_sync(&ar->regd_channel_update_work);
>  	cancel_work_sync(&ab->rfkill_work);
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index fc5127b5c1a3..b6fd5e9dfdb7 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -635,11 +635,12 @@ struct ath12k {
>  		struct completion started;
>  		struct completion completed;
>  		struct completion on_channel;
> -		struct delayed_work timeout;
> +		struct wiphy_delayed_work timeout;
>  		enum ath12k_scan_state state;
>  		bool is_roc;
>  		int roc_freq;
>  		bool roc_notify;
> +		bool finish_queued;
>  		struct wiphy_work vdev_clean_wk;
>  		struct ath12k_link_vif *arvif;
>  	} scan;
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 51c4df32e716..f53df8d4c808 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -5162,7 +5162,7 @@ void __ath12k_mac_scan_finish(struct ath12k *ar)
>  			ieee80211_remain_on_channel_expired(hw);
>  		fallthrough;
>  	case ATH12K_SCAN_STARTING:
> -		cancel_delayed_work(&ar->scan.timeout);
> +		ar->scan.finish_queued = true;
>  		complete_all(&ar->scan.completed);
>  		wiphy_work_queue(ar->ah->hw->wiphy, &ar->scan.vdev_clean_wk);
>  		break;
> @@ -5254,14 +5254,23 @@ static void ath12k_scan_abort(struct ath12k *ar)
>  	spin_unlock_bh(&ar->data_lock);
>  }
>  
> -static void ath12k_scan_timeout_work(struct work_struct *work)
> +static void ath12k_scan_timeout_work(struct wiphy *wiphy,
> +				     struct wiphy_work *work)
>  {
> -	struct ath12k *ar = container_of(work, struct ath12k,
> -					 scan.timeout.work);
> +	struct wiphy_delayed_work *dwork;
> +	struct ath12k *ar;
> +
> +	dwork = container_of(work, struct wiphy_delayed_work, work);
> +	ar = container_of(dwork, struct ath12k, scan.timeout);
> +
> +	spin_lock_bh(&ar->data_lock);
> +	if (ar->scan.finish_queued) {
> +		spin_unlock_bh(&ar->data_lock);
> +		return;
> +	}
> +	spin_unlock_bh(&ar->data_lock);

if the flag is necessary, all of that could become:
	scoped_guard(spinlock_bh, &ar->data_lock)
		if (ar->scan.finish_queued)
			return;

>  
> -	wiphy_lock(ath12k_ar_to_hw(ar)->wiphy);
>  	ath12k_scan_abort(ar);
> -	wiphy_unlock(ath12k_ar_to_hw(ar)->wiphy);
>  }
>  
>  static void ath12k_mac_scan_send_complete(struct ath12k *ar,
> @@ -5292,6 +5301,8 @@ static void ath12k_scan_vdev_clean_work(struct wiphy *wiphy, struct wiphy_work *
>  
>  	arvif = ar->scan.arvif;
>  
> +	wiphy_delayed_work_cancel(wiphy, &ar->scan.timeout);
> +
>  	/* The scan vdev has already been deleted. This can occur when a
>  	 * new scan request is made on the same vif with a different
>  	 * frequency, causing the scan arvif to move from one radio to
> @@ -5323,6 +5334,7 @@ static void ath12k_scan_vdev_clean_work(struct wiphy *wiphy, struct wiphy_work *
>  	}
>  
>  	ar->scan.state = ATH12K_SCAN_IDLE;
> +	ar->scan.finish_queued = false;
>  	ar->scan_channel = NULL;
>  	ar->scan.roc_freq = 0;
>  	spin_unlock_bh(&ar->data_lock);
> @@ -5609,6 +5621,7 @@ static int ath12k_mac_initiate_hw_scan(struct ieee80211_hw *hw,
>  		reinit_completion(&ar->scan.completed);
>  		ar->scan.state = ATH12K_SCAN_STARTING;
>  		ar->scan.is_roc = false;
> +		ar->scan.finish_queued = false;
>  		ar->scan.arvif = arvif;
>  		ret = 0;
>  		break;
> @@ -5665,6 +5678,7 @@ static int ath12k_mac_initiate_hw_scan(struct ieee80211_hw *hw,
>  
>  		spin_lock_bh(&ar->data_lock);
>  		ar->scan.state = ATH12K_SCAN_IDLE;
> +		ar->scan.finish_queued = false;
>  		spin_unlock_bh(&ar->data_lock);
>  		goto exit;
>  	}
> @@ -5672,9 +5686,10 @@ static int ath12k_mac_initiate_hw_scan(struct ieee80211_hw *hw,
>  	ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "mac scan started");
>  
>  	/* Add a margin to account for event/command processing */
> -	ieee80211_queue_delayed_work(ath12k_ar_to_hw(ar), &ar->scan.timeout,
> -				     msecs_to_jiffies(arg->max_scan_time +
> -						      ATH12K_MAC_SCAN_TIMEOUT_MSECS));
> +	wiphy_delayed_work_queue(ath12k_ar_to_hw(ar)->wiphy,
> +				 &ar->scan.timeout,
> +				 msecs_to_jiffies(arg->max_scan_time +
> +						  ATH12K_MAC_SCAN_TIMEOUT_MSECS));
>  
>  exit:
>  	if (arg) {
> @@ -5757,6 +5772,7 @@ int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw,
>  				spin_lock_bh(&ar->data_lock);
>  				ar->scan.arvif = NULL;
>  				ar->scan.state = ATH12K_SCAN_IDLE;
> +				ar->scan.finish_queued = false;
>  				ar->scan_channel = NULL;
>  				ar->scan.roc_freq = 0;
>  				spin_unlock_bh(&ar->data_lock);
> @@ -5792,7 +5808,7 @@ void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw,
>  
>  		ath12k_scan_abort(ar);
>  
> -		cancel_delayed_work_sync(&ar->scan.timeout);
> +		wiphy_delayed_work_cancel(hw->wiphy, &ar->scan.timeout);
>  	}
>  }
>  EXPORT_SYMBOL(ath12k_mac_op_cancel_hw_scan);
> @@ -9935,7 +9951,7 @@ static void ath12k_mac_stop(struct ath12k *ar)
>  
>  	clear_bit(ATH12K_FLAG_CAC_RUNNING, &ar->dev_flags);
>  
> -	cancel_delayed_work_sync(&ar->scan.timeout);
> +	wiphy_delayed_work_cancel(ath12k_ar_to_hw(ar)->wiphy, &ar->scan.timeout);
>  	wiphy_work_cancel(ath12k_ar_to_hw(ar)->wiphy, &ar->scan.vdev_clean_wk);
>  	cancel_work_sync(&ar->regd_channel_update_work);
>  	cancel_work_sync(&ar->regd_update_work);
> @@ -10962,6 +10978,7 @@ void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw,
>  			}
>  
>  			ar->scan.state = ATH12K_SCAN_IDLE;
> +			ar->scan.finish_queued = false;
>  			ar->scan_channel = NULL;
>  			ar->scan.roc_freq = 0;
>  			spin_unlock_bh(&ar->data_lock);
> @@ -13876,7 +13893,7 @@ int ath12k_mac_op_cancel_remain_on_channel(struct ieee80211_hw *hw,
>  
>  	ath12k_scan_abort(ar);
>  
> -	cancel_delayed_work_sync(&ar->scan.timeout);
> +	wiphy_delayed_work_cancel(hw->wiphy, &ar->scan.timeout);
>  	wiphy_work_flush(hw->wiphy, &ar->scan.vdev_clean_wk);
>  
>  	return 0;
> @@ -13956,6 +13973,7 @@ int ath12k_mac_op_remain_on_channel(struct ieee80211_hw *hw,
>  		reinit_completion(&ar->scan.on_channel);
>  		ar->scan.state = ATH12K_SCAN_STARTING;
>  		ar->scan.is_roc = true;
> +		ar->scan.finish_queued = false;
>  		ar->scan.arvif = arvif;
>  		ar->scan.roc_freq = chan->center_freq;
>  		ar->scan.roc_notify = true;
> @@ -13998,6 +14016,7 @@ int ath12k_mac_op_remain_on_channel(struct ieee80211_hw *hw,
>  
>  		spin_lock_bh(&ar->data_lock);
>  		ar->scan.state = ATH12K_SCAN_IDLE;
> +		ar->scan.finish_queued = false;
>  		spin_unlock_bh(&ar->data_lock);
>  		return ret;
>  	}
> @@ -14011,8 +14030,8 @@ int ath12k_mac_op_remain_on_channel(struct ieee80211_hw *hw,
>  		return -ETIMEDOUT;
>  	}
>  
> -	ieee80211_queue_delayed_work(hw, &ar->scan.timeout,
> -				     msecs_to_jiffies(duration));
> +	wiphy_delayed_work_queue(hw->wiphy, &ar->scan.timeout,
> +				 msecs_to_jiffies(duration));
>  
>  	return 0;
>  }
> @@ -15052,6 +15071,7 @@ static void ath12k_mac_setup(struct ath12k *ar)
>  	ar->num_tx_chains = hweight32(pdev->cap.tx_chain_mask);
>  	ar->num_rx_chains = hweight32(pdev->cap.rx_chain_mask);
>  	ar->scan.arvif = NULL;
> +	ar->scan.finish_queued = false;
>  	ar->vdev_id_11d_scan = ATH12K_11D_INVALID_VDEV_ID;
>  
>  	spin_lock_init(&ar->data_lock);
> @@ -15077,7 +15097,7 @@ static void ath12k_mac_setup(struct ath12k *ar)
>  	ar->thermal.temperature = 0;
>  	ar->thermal.hwmon_dev = NULL;
>  
> -	INIT_DELAYED_WORK(&ar->scan.timeout, ath12k_scan_timeout_work);
> +	wiphy_delayed_work_init(&ar->scan.timeout, ath12k_scan_timeout_work);
>  	wiphy_work_init(&ar->scan.vdev_clean_wk, ath12k_scan_vdev_clean_work);
>  	INIT_WORK(&ar->regd_channel_update_work, ath12k_regd_update_chan_list_work);
>  	INIT_LIST_HEAD(&ar->regd_channel_update_queue);