Re: [PATCH v3 4/8] wifi: ath12k: add WoW net-detect functionality

[Kalle Valo <kvalo@kernel.org>]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

> Implement net-detect feature by setting flag
> WIPHY_WOWLAN_NET_DETECT if firmware supports this
> feature. Driver sets the related PNO configuration
> to firmware before entering WoW and firmware then
> scans periodically and wakes up host if a specific
> SSID is found.
>
> Note that firmware crashes if we enable it for both
> P2P vdev and station vdev simultaneously because
> firmware can only support one vdev at a time. Since
> there is rare scenario for a P2P vdev to do net-detect,
> skip it for P2P vdevs.
>
> Tested-on: WCN7850 hw2.0 PCI
> WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>

[...]

> +struct wmi_wow_nlo_config_cmd {
> +	__le32 tlv_header;
> +	__le32 flags;
> +	__le32 vdev_id;
> +	__le32 fast_scan_max_cycles;
> +	__le32 active_dwell_time;
> +	__le32 passive_dwell_time;
> +	__le32 probe_bundle_size;
> +
> +	/* ART = IRT */
> +	__le32 rest_time;

What's ART and IRT in this context? The comments are supposed to answer
to questions but this is just adding more questions.

> +
> +	/* Max value that can be reached after SBM */
> +	__le32 max_rest_time;

It's good to avoid acronyms so I changed this to:

	/* max value that can be reached after scan_backoff_multiplier */
	__le32 max_rest_time;

> +	/* SBM */
> +	__le32 scan_backoff_multiplier;
> +
> +	/* SCBM */
> +	__le32 fast_scan_period;

These two comments are not really providing any extra information I
removed them.

> +static int
> +ath12k_wow_pno_check_and_convert(struct ath12k *ar, u32 vdev_id,
> +				 const struct cfg80211_sched_scan_request *nd_config,
> +				 struct wmi_pno_scan_req_arg *pno)
> +{
> +	int i, j;
> +	u8 ssid_len;
> +
> +	pno->enable = 1;
> +	pno->vdev_id = vdev_id;
> +	pno->uc_networks_count = nd_config->n_match_sets;
> +
> +	if (!pno->uc_networks_count ||
> +	    pno->uc_networks_count > WMI_PNO_MAX_SUPP_NETWORKS)
> +		return -EINVAL;
> +
> +	if (nd_config->n_channels > WMI_PNO_MAX_NETW_CHANNELS_EX)
> +		return -EINVAL;
> +
> +	/* Filling per profile params */
> +	for (i = 0; i < pno->uc_networks_count; i++) {
> +		ssid_len = nd_config->match_sets[i].ssid.ssid_len;
> +
> +		if (ssid_len == 0 || ssid_len > 32)
> +			return -EINVAL;
> +
> +		pno->a_networks[i].ssid.ssid_len = ssid_len;
> +
> +		memcpy(pno->a_networks[i].ssid.ssid,
> +		       nd_config->match_sets[i].ssid.ssid,
> +		       ssid_len);
> +		pno->a_networks[i].authentication = 0;
> +		pno->a_networks[i].encryption     = 0;
> +		pno->a_networks[i].bcast_nw_type  = 0;
> +
> +		/* Copying list of valid channel into request */
> +		pno->a_networks[i].channel_count = nd_config->n_channels;
> +		pno->a_networks[i].rssi_threshold = nd_config->match_sets[i].rssi_thold;
> +
> +		for (j = 0; j < nd_config->n_channels; j++) {
> +			pno->a_networks[i].channels[j] =
> +					nd_config->channels[j]->center_freq;
> +		}
> +	}
> +
> +	/* set scan to passive if no SSIDs are specified in the request */
> +	if (nd_config->n_ssids == 0)
> +		pno->do_passive_scan = true;
> +	else
> +		pno->do_passive_scan = false;
> +
> +	for (i = 0; i < nd_config->n_ssids; i++) {
> +		j = 0;
> +		while (j < pno->uc_networks_count) {
> +			if (pno->a_networks[j].ssid.ssid_len ==
> +				nd_config->ssids[i].ssid_len &&
> +			    !memcmp(pno->a_networks[j].ssid.ssid,
> +				    nd_config->ssids[i].ssid,
> +				    pno->a_networks[j].ssid.ssid_len)) {
> +				pno->a_networks[j].bcast_nw_type = BCAST_HIDDEN;
> +				break;
> +			}
> +			j++;
> +		}
> +	}

The while loop is just a simple for loop so I changed it to use for statement.

> +
> +	if (nd_config->n_scan_plans == 2) {
> +		pno->fast_scan_period = nd_config->scan_plans[0].interval * MSEC_PER_SEC;
> +		pno->fast_scan_max_cycles = nd_config->scan_plans[0].iterations;
> +		pno->slow_scan_period =
> +			nd_config->scan_plans[1].interval * MSEC_PER_SEC;
> +	} else if (nd_config->n_scan_plans == 1) {
> +		pno->fast_scan_period = nd_config->scan_plans[0].interval * MSEC_PER_SEC;
> +		pno->fast_scan_max_cycles = 1;
> +		pno->slow_scan_period = nd_config->scan_plans[0].interval * MSEC_PER_SEC;
> +	} else {
> +		ath12k_warn(ar->ab, "Invalid number of scan plans %d !!",
> +			    nd_config->n_scan_plans);
> +	}

I cleaned up the error message here.

> +static int ath12k_wow_vdev_clean_nlo(struct ath12k *ar, u32 vdev_id)
> +{
> +	int ret = 0;
> +
> +	if (ar->nlo_enabled) {
> +		struct wmi_pno_scan_req_arg *pno =
> +			kzalloc(sizeof(*pno), GFP_KERNEL);
> +		if (!pno)
> +			return -ENOMEM;
> +
> +		pno->enable = 0;
> +		ar->nlo_enabled = false;
> +		ret = ath12k_wmi_wow_config_pno(ar, vdev_id, pno);
> +		kfree(pno);
> +	}
> +
> +	return ret;
> +}

Avoid initialising ret variables and minimise the indentation. I changed
this to:

static int ath12k_wow_vdev_clean_nlo(struct ath12k *ar, u32 vdev_id)
{
	struct wmi_pno_scan_req_arg *pno;
	int ret;

	if (!ar->nlo_enabled)
		return 0;

	pno = kzalloc(sizeof(*pno), GFP_KERNEL);
	if (!pno)
		return -ENOMEM;

	pno->enable = 0;
	ret = ath12k_wmi_wow_config_pno(ar, vdev_id, pno);
	if (ret) {
		ath12k_warn(ar->ab, "failed to disable PNO: %d", ret);
		goto out;
	}

	ar->nlo_enabled = false;

out:
	kfree(pno);
	return ret;
}

> +static int ath12k_wow_vif_clean_nlo(struct ath12k_vif *arvif)
> +{
> +	struct ath12k *ar = arvif->ar;
> +	int ret = 0;
> +
> +	switch (arvif->vdev_type) {
> +	case WMI_VDEV_TYPE_STA:
> +		ret = ath12k_wow_vdev_clean_nlo(ar, arvif->vdev_id);
> +		break;
> +	default:
> +		break;
> +	}
> +	return ret;
> +}

ret variable is not really needed:

static int ath12k_wow_vif_clean_nlo(struct ath12k_vif *arvif)
{
	struct ath12k *ar = arvif->ar;

	switch (arvif->vdev_type) {
	case WMI_VDEV_TYPE_STA:
		return ath12k_wow_vdev_clean_nlo(ar, arvif->vdev_id);
	default:
		return 0;
	}
}

In the pending branch I did also some minor cosmetic changes to this and
earlier patches, too many to list here.

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

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