Re: [PATCH v3 2/2] wifi: ath11k: move update channel list to worker for wait flag

[Kalle Valo <kvalo@kernel.org>]

Kang Yang <quic_kangyang@quicinc.com> writes:

> From: Wen Gong <quic_wgong@quicinc.com>
>
> When wait flag is set for ath11k_reg_update_chan_list(), it will wait
> the completion of 11d/hw scan if 11d/hw scan is running.
>
> With the previous patch "wifi: ath11k: move update channel list from
> update reg worker to reg notifier", ath11k_reg_update_chan_list() will
> be called when reg_work is running. The global lock rtnl_lock will be
> held by reg_work in the meantime. If the wait_for_completion_timeout()
> is called due to 11d/hw scan is running, the occupation time of
> rtnl_lock will increase. This will increase blocking time for other
> threads if they want to use rtnl_lock.
>
> Move update channel list operation in ath11k_reg_update_chan_list() to
> a new worker, then the wait of completion of 11d/hw scan will not
> happen in reg_work and not increase the occupation time of the rtnl_lock.
>
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3
>
> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
> Co-developed-by: Kang Yang <quic_kangyang@quicinc.com>
> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>

Same here, I think the commit message should be more or less rewritten.

> --- a/drivers/net/wireless/ath/ath11k/core.h
> +++ b/drivers/net/wireless/ath/ath11k/core.h
> @@ -743,6 +743,10 @@ struct ath11k {
>  	struct completion bss_survey_done;
>  
>  	struct work_struct regd_update_work;
> +	struct work_struct channel_update_work;
> +	struct list_head channel_update_queue;
> +	/* protects channel_update_queue data */
> +	spinlock_t channel_update_lock;

Do you really need a new lock? Why not use data_lock?

> @@ -6318,6 +6320,15 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw, bool suspend)
>  	}
>  	spin_unlock_bh(&ar->data_lock);
>  
> +	spin_lock_bh(&ar->channel_update_lock);

Empty line here, please.

> +	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
> +						  struct scan_chan_list_params,
> +						  list))) {
> +		list_del(&params->list);
> +		kfree(params);
> +	}

Here also empty line.

> +	spin_unlock_bh(&ar->channel_update_lock);
> +
>  	rcu_assign_pointer(ar->ab->pdevs_active[ar->pdev_idx], NULL);
>  
>  	synchronize_rcu();

[...]

> +void ath11k_regd_update_chan_list_work(struct work_struct *work)
> +{
> +	struct ath11k *ar = container_of(work, struct ath11k,
> +					 channel_update_work);
> +	struct scan_chan_list_params *params;
> +	struct list_head local_update_list;
> +	int left;
> +
> +	INIT_LIST_HEAD(&local_update_list);
> +
> +	spin_lock_bh(&ar->channel_update_lock);
> +	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
> +						  struct scan_chan_list_params,
> +						  list))) {
> +		list_del(&params->list);
> +		list_add_tail(&params->list, &local_update_list);
> +	}
> +	spin_unlock_bh(&ar->channel_update_lock);

What about list_splice_tail_init() or similar?

> +
> +	while ((params = list_first_entry_or_null(&local_update_list,
> +						  struct scan_chan_list_params,
> +						  list))) {
> +		if (ar->state_11d != ATH11K_11D_IDLE) {
> +			left = wait_for_completion_timeout(&ar->completed_11d_scan,
> +							   ATH11K_SCAN_TIMEOUT_HZ);
> +			if (!left) {
> +				ath11k_dbg(ar->ab, ATH11K_DBG_REG,
> +					   "failed to receive 11d scan complete: timed out\n");
> +				ar->state_11d = ATH11K_11D_IDLE;
> +			}

Empty line here.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches