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

[Kang Yang]

On 12/12/2024 10:07 PM, Kalle Valo wrote:
> Kang Yang <quic_kangyang at quicinc.com> writes:
> 
>> From: Wen Gong <quic_wgong at 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 at quicinc.com>
>> Co-developed-by: Kang Yang <quic_kangyang at quicinc.com>
>> Signed-off-by: Kang Yang <quic_kangyang at 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?

Seems data_lock is OK, will change in next version.

> 
>> @@ -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?

Seems list_splice_tail_init() is better. The time complexity is O(1).👍

> 
>> +
>> +	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.
>