Received-SPF: pass (google.com: domain of linux-wireless+bounces-1864-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <3771cd9b-f526-7704-75af-76ea8f73a7cc@quicinc.com>
Date: Sat, 13 Jan 2024 07:14:39 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v2 04/10] wifi: ath12k: implement remain on channel for
 P2P mode
Content-Language: en-US
To: Kang Yang <quic_kangyang@quicinc.com>, <ath12k@lists.infradead.org>,
        "Jeff
 Johnson" <quic_jjohnson@quicinc.com>
CC: <linux-wireless@vger.kernel.org>
References: <20240112092824.7664-1-quic_kangyang@quicinc.com>
 <20240112092824.7664-5-quic_kangyang@quicinc.com>
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
In-Reply-To: <20240112092824.7664-5-quic_kangyang@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 1/12/2024 2:58 PM, Kang Yang wrote:
> Implement remain on channel for p2p mode in ath12k_ops:
>          ath12k_mac_op_remain_on_channel
>          ath12k_mac_op_cancel_remain_on_channel
>
> P2P device can trigger ROC scan. Then keep listening or sending management
> frames on particular channels.
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
> ---
>
> v2:
>      1. add Tested-on tag of QCN9274.
>      2. update copyright.
>
> --
>   drivers/net/wireless/ath/ath12k/core.c |   3 +-
>   drivers/net/wireless/ath/ath12k/mac.c  | 113 +++++++++++++++++++++++++
>   drivers/net/wireless/ath/ath12k/wmi.c  |   4 +
>   3 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
> index d73e2d33a41e..d030534a3ef0 100644
> --- a/drivers/net/wireless/ath/ath12k/core.c
> +++ b/drivers/net/wireless/ath/ath12k/core.c
> @@ -1,7 +1,7 @@
>   // SPDX-License-Identifier: BSD-3-Clause-Clear
>   /*
>    * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
> - * Copyright (c) 2021-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>    */
>   
>   #include <linux/module.h>
> @@ -942,6 +942,7 @@ static void ath12k_core_pre_reconfigure_recovery(struct ath12k_base *ab)
>   		ath12k_mac_drain_tx(ar);
>   		complete(&ar->scan.started);
>   		complete(&ar->scan.completed);
> +		complete(&ar->scan.on_channel);
>   		complete(&ar->peer_assoc_done);
>   		complete(&ar->peer_delete_done);
>   		complete(&ar->install_key_done);
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index a4a00a6606b4..92ba88d5e4e9 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -7193,6 +7193,116 @@ static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
>   	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
>   }
>   
> +static int ath12k_mac_op_cancel_remain_on_channel(struct ieee80211_hw *hw,
> +						  struct ieee80211_vif *vif)
> +{
> +	struct ath12k *ar = hw->priv;


Accessing the radio (ar) from hw->priv is modified in the patchset 
"[PATCH v2 2/2] wifi: ath12k: Introduce the container for mac80211 hw".

This change has conflict.

@jeff,

Any comments ?

> +
> +	mutex_lock(&ar->conf_mutex);
> +
> +	spin_lock_bh(&ar->data_lock);
> +	ar->scan.roc_notify = false;
> +	spin_unlock_bh(&ar->data_lock);
> +
> +	ath12k_scan_abort(ar);
> +
> +	mutex_unlock(&ar->conf_mutex);
> +
> +	cancel_delayed_work_sync(&ar->scan.timeout);
> +
> +	return 0;
> +}
> +
> +static int ath12k_mac_op_remain_on_channel(struct ieee80211_hw *hw,
> +					   struct ieee80211_vif *vif,
> +					   struct ieee80211_channel *chan,
> +					   int duration,
> +					   enum ieee80211_roc_type type)
> +{
> +	struct ath12k *ar = hw->priv;


Accessing the radio (ar) from hw->priv is modified in the patchset 
"[PATCH v2 2/2] wifi: ath12k: Introduce the container for mac80211 hw".

This change has conflict.

@jeff,

Any comments ?


> +	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> +	struct ath12k_wmi_scan_req_arg arg;
> +	int ret;
> +	u32 scan_time_msec;
> +
> +	mutex_lock(&ar->conf_mutex);
> +	spin_lock_bh(&ar->data_lock);
> +	switch (ar->scan.state) {
> +	case ATH12K_SCAN_IDLE:
> +		reinit_completion(&ar->scan.started);
> +		reinit_completion(&ar->scan.completed);
> +		reinit_completion(&ar->scan.on_channel);
> +		ar->scan.state = ATH12K_SCAN_STARTING;
> +		ar->scan.is_roc = true;
> +		ar->scan.vdev_id = arvif->vdev_id;
> +		ar->scan.roc_freq = chan->center_freq;
> +		ar->scan.roc_notify = true;
> +		ret = 0;
> +		break;
> +	case ATH12K_SCAN_STARTING:
> +	case ATH12K_SCAN_RUNNING:
> +	case ATH12K_SCAN_ABORTING:
> +		ret = -EBUSY;
> +		break;
> +	}
> +	spin_unlock_bh(&ar->data_lock);
> +
> +	if (ret)
> +		goto exit;
> +
> +	scan_time_msec = ar->hw->wiphy->max_remain_on_channel_duration * 2;
> +
> +	memset(&arg, 0, sizeof(arg));
> +	ath12k_wmi_start_scan_init(ar, &arg);
> +	arg.num_chan = 1;
> +	arg.chan_list = kcalloc(arg.num_chan, sizeof(*arg.chan_list),
> +				GFP_KERNEL);
> +	if (!arg.chan_list) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	arg.vdev_id = arvif->vdev_id;
> +	arg.scan_id = ATH12K_SCAN_ID;
> +	arg.chan_list[0] = chan->center_freq;
> +	arg.dwell_time_active = scan_time_msec;
> +	arg.dwell_time_passive = scan_time_msec;
> +	arg.max_scan_time = scan_time_msec;
> +	arg.scan_flags |= WMI_SCAN_FLAG_PASSIVE;
> +	arg.burst_duration = duration;
> +
> +	ret = ath12k_start_scan(ar, &arg);
> +	if (ret) {
> +		ath12k_warn(ar->ab, "failed to start roc scan: %d\n", ret);
> +
> +		spin_lock_bh(&ar->data_lock);
> +		ar->scan.state = ATH12K_SCAN_IDLE;
> +		spin_unlock_bh(&ar->data_lock);
> +		goto free_chan_list;
> +	}
> +
> +	ret = wait_for_completion_timeout(&ar->scan.on_channel, 3 * HZ);
> +	if (ret == 0) {
> +		ath12k_warn(ar->ab, "failed to switch to channel for roc scan\n");
> +		ret = ath12k_scan_stop(ar);
> +		if (ret)
> +			ath12k_warn(ar->ab, "failed to stop scan: %d\n", ret);
> +		ret = -ETIMEDOUT;
> +		goto free_chan_list;
> +	}
> +
> +	ieee80211_queue_delayed_work(ar->hw, &ar->scan.timeout,
> +				     msecs_to_jiffies(duration));


ath12k_ar_to_hw(ar) is introduced to avoid direct access of hw from 
radio (ie, ar->hw).

This new helper function introduced by the patchset "[PATCH v2 1/2] 
wifi: ath12k: Refactor the mac80211 hw access from link/radio"


@jeff

Any comments ?

> +
> +	ret = 0;
> +
> +free_chan_list:
> +	kfree(arg.chan_list);
> +exit:
> +	mutex_unlock(&ar->conf_mutex);
> +	return ret;
> +}
> +
>   static const struct ieee80211_ops ath12k_ops = {
>   	.tx				= ath12k_mac_op_tx,
>   	.wake_tx_queue			= ieee80211_handle_wake_tx_queue,
> @@ -7227,6 +7337,8 @@ static const struct ieee80211_ops ath12k_ops = {
>   	.get_survey			= ath12k_mac_op_get_survey,
>   	.flush				= ath12k_mac_op_flush,
>   	.sta_statistics			= ath12k_mac_op_sta_statistics,
> +	.remain_on_channel              = ath12k_mac_op_remain_on_channel,
> +	.cancel_remain_on_channel       = ath12k_mac_op_cancel_remain_on_channel,


Both these new mac_op callback need to rework based on the patchset 
"[PATCH v2 0/2] wifi: ath12k: Introduce hw abstraction"


>   };
>   
>   static void ath12k_mac_update_ch_list(struct ath12k *ar,
> @@ -7777,6 +7889,7 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
>   		init_completion(&ar->bss_survey_done);
>   		init_completion(&ar->scan.started);
>   		init_completion(&ar->scan.completed);
> +		init_completion(&ar->scan.on_channel);
>   
>   		INIT_DELAYED_WORK(&ar->scan.timeout, ath12k_scan_timeout_work);
>   		INIT_WORK(&ar->regd_update_work, ath12k_regd_update_work);
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index 861565eaab3d..e10289de84c1 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -5050,6 +5050,8 @@ static void ath12k_wmi_event_scan_started(struct ath12k *ar)
>   		break;
>   	case ATH12K_SCAN_STARTING:
>   		ar->scan.state = ATH12K_SCAN_RUNNING;
> +		if (ar->scan.is_roc)
> +			ieee80211_ready_on_channel(ar->hw);
>   		complete(&ar->scan.started);
>   		break;
>   	}
> @@ -5132,6 +5134,8 @@ static void ath12k_wmi_event_scan_foreign_chan(struct ath12k *ar, u32 freq)
>   	case ATH12K_SCAN_RUNNING:
>   	case ATH12K_SCAN_ABORTING:
>   		ar->scan_channel = ieee80211_get_channel(ar->hw->wiphy, freq);
> +		if (ar->scan.is_roc && ar->scan.roc_freq == freq)
> +			complete(&ar->scan.on_channel);
>   		break;
>   	}
>   }


Thanks,

Karthikeyan