On 6/20/2024 7:10 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> [...]
>
>> +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.
Thanks. All these changes look OK to me.
>
