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

[Kang Yang <quic_kangyang@xxxxxxxxxxx>]

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