Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Brian Norris]

Hi Wen,

You've introduced a regression in 4.20-rc1:

On Thu, Aug 16, 2018 at 02:48:33PM +0800, Wen Gong wrote:
> For WoWLAN support, it expect to support wake up based on discovery of
> one or more known SSIDs. This is the WIPHY_WOWLAN_NET_DETECT feature,
> which shows up as an NL80211 feature flag.
> 
> With an upgrade iw, this shows up in 'iw phy' as:
> WoWLAN support:
> * wake up on network detection, up to 16 match sets
> And it can use command:
> "iw phy0 wowlan enable net-detect interval 5000 delay 30 freqs 2412
> matches ssid foo" to configure the parameters of net detect.
> 
> Firmware will do scan by the configured parameters after suspend and
> wakeup if it found matched SSIDs. Tested with QCA6174 hw3.0 with
> firmware WLAN.RM.4.4.1-00110-QCARMSWPZ-1.
> 
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> ---
> V3:
> -fix the waring of alloc with no test
>  drivers/net/wireless/ath/ath10k/core.h    |   1 +
>  drivers/net/wireless/ath/ath10k/mac.c     |  12 ++
>  drivers/net/wireless/ath/ath10k/wmi-ops.h |  21 +++
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c | 180 +++++++++++++++++++++++-
>  drivers/net/wireless/ath/ath10k/wmi-tlv.h | 226 ++++++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath10k/wmi.h     |  57 ++++++++
>  drivers/net/wireless/ath/ath10k/wow.c     | 174 +++++++++++++++++++++++
>  7 files changed, 670 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 427ee57..7885462 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -904,6 +904,7 @@ struct ath10k {
>  	u32 high_5ghz_chan;
>  	bool ani_enabled;
>  
> +	bool nlo_enabled;
>  	bool p2p;
>  
>  	struct {
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 95243b4..ba9b9af 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -8361,6 +8361,18 @@ int ath10k_mac_register(struct ath10k *ar)
>  	ar->hw->wiphy->max_scan_ssids = WLAN_SCAN_PARAMS_MAX_SSID;
>  	ar->hw->wiphy->max_scan_ie_len = WLAN_SCAN_PARAMS_MAX_IE_LEN;
>  
> +	if (test_bit(WMI_SERVICE_NLO, ar->wmi.svc_map)) {
> +		ar->hw->wiphy->max_sched_scan_reqs = 1;
> +		ar->hw->wiphy->max_sched_scan_ssids = WMI_PNO_MAX_SUPP_NETWORKS;
> +		ar->hw->wiphy->max_match_sets       = WMI_PNO_MAX_SUPP_NETWORKS;
> +		ar->hw->wiphy->max_sched_scan_ie_len = WMI_PNO_MAX_IE_LENGTH;
> +		ar->hw->wiphy->max_sched_scan_plans = WMI_PNO_MAX_SCHED_SCAN_PLANS;
> +		ar->hw->wiphy->max_sched_scan_plan_interval =
> +			WMI_PNO_MAX_SCHED_SCAN_PLAN_INT;
> +		ar->hw->wiphy->max_sched_scan_plan_iterations =
> +			WMI_PNO_MAX_SCHED_SCAN_PLAN_ITRNS;

It seems like youre enabling SCHED_SCAN support? But you're not adding
the NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR feature flag. So it puts
us in a tough place on using randomization -- we either can't trust the
FEATURE flags, or else we can't use both SCHED_SCAN and scan
randomization.

I haven't played with this much at all yet (except to notice that my
tests no longer pass), but maybe you just need to add the FEATURE flag.

Brian

> +	}
> +
>  	ar->hw->vif_data_size = sizeof(struct ath10k_vif);
>  	ar->hw->sta_data_size = sizeof(struct ath10k_sta);
>  	ar->hw->txq_data_size = sizeof(struct ath10k_txq);

...

Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Brian Norris]

Since Wen has once again suggested I use this patch in other forums,
I'll ping here to note:

On Wed, Nov 14, 2018 at 2:59 PM Brian Norris <briannorris@chromium.org> wrote:
> You've introduced a regression in 4.20-rc1:

This regression still survives in the latest tree. Is it fair to just
submit a revert?

Brian

Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> Since Wen has once again suggested I use this patch in other forums,
> I'll ping here to note:
>
> On Wed, Nov 14, 2018 at 2:59 PM Brian Norris <briannorris@chromium.org> wrote:
>> You've introduced a regression in 4.20-rc1:
>
> This regression still survives in the latest tree. Is it fair to just
> submit a revert?

Your description about the problem from an earlier email:

  "It seems like youre enabling SCHED_SCAN support? But you're not
   adding the NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR feature flag.
   So it puts us in a tough place on using randomization -- we either
   can't trust the FEATURE flags, or else we can't use both SCHED_SCAN
   and scan randomization."

So essentially the problem is that with firmwares supporting both
WMI_SERVICE_NLO and WMI_SERVICE_SPOOF_MAC_SUPPORT ath10k enables
NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR, but
NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR is not enabled which is
inconsistent from user space point of view. Is my understanding correct?

Wen, can you enable NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR? Does firmware
support that?

If that's not possible, one workaround might to be to not enable
NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR if firmware supports
WMI_SERVICE_NLO, but of course that would suck big time.

Here's the full context in case someone is interested:

https://patchwork.kernel.org/patch/10567005/

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

Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Brian Norris]

(I realize I replied to the v3, not the v4 which was merged.)

On Wed, Sep 18, 2019 at 7:03 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> Brian Norris <briannorris@chromium.org> writes:
> > Since Wen has once again suggested I use this patch in other forums,
> > I'll ping here to note:
> >
> > On Wed, Nov 14, 2018 at 2:59 PM Brian Norris <briannorris@chromium.org> wrote:
> >> You've introduced a regression in 4.20-rc1:
> >
> > This regression still survives in the latest tree. Is it fair to just
> > submit a revert?
>
> Your description about the problem from an earlier email:
>
>   "It seems like youre enabling SCHED_SCAN support? But you're not
>    adding the NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR feature flag.
>    So it puts us in a tough place on using randomization -- we either
>    can't trust the FEATURE flags, or else we can't use both SCHED_SCAN
>    and scan randomization."

Yeah, maybe I shouldn't have trimmed that context :)

> So essentially the problem is that with firmwares supporting both
> WMI_SERVICE_NLO and WMI_SERVICE_SPOOF_MAC_SUPPORT ath10k enables
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR, but
> NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR is not enabled which is
> inconsistent from user space point of view. Is my understanding correct?

Yes, that sounds about right.

> Wen, can you enable NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR? Does firmware
> support that?

I feel like I've asked him that privately, but asking here might not hurt :D

> If that's not possible, one workaround might to be to not enable
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR if firmware supports
> WMI_SERVICE_NLO, but of course that would suck big time.

Yeah, that would be strictly worse than the current situation I think.
SCAN_RANDOM_MAC_ADDRESS is a product requirement for us. At least
right now, it's possible I could teach a user space to just ignore
SCHED_SCAN if it doesn't support the appropriate randomization
features. (I don't want to have to do that, but at least it's
possible.)

Brian

> Here's the full context in case someone is interested:
>
> https://patchwork.kernel.org/patch/10567005/

RE: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Wen Gong]

> -----Original Message-----
> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Kalle Valo
> Sent: Wednesday, September 18, 2019 10:03 PM
> To: Brian Norris <briannorris@chromium.org>
> Cc: linux-wireless <linux-wireless@vger.kernel.org>; Linux Kernel <linux-
> kernel@vger.kernel.org>; ath10k@lists.infradead.org; Wen Gong
> <wgong@codeaurora.org>
> Subject: [EXT] Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature
> 
> So essentially the problem is that with firmwares supporting both
> WMI_SERVICE_NLO and WMI_SERVICE_SPOOF_MAC_SUPPORT ath10k
> enables
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR, but
> NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR is not enabled
> which is
> inconsistent from user space point of view. Is my understanding correct?
> 
> Wen, can you enable NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR?
> Does firmware
> support that?

Yes, I test again, it is enabled NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR now.

> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Kalle Valo]

Wen Gong <wgong@qti.qualcomm.com> writes:

>> -----Original Message-----
>> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Kalle Valo
>> Sent: Wednesday, September 18, 2019 10:03 PM
>> To: Brian Norris <briannorris@chromium.org>
>> Cc: linux-wireless <linux-wireless@vger.kernel.org>; Linux Kernel <linux-
>> kernel@vger.kernel.org>; ath10k@lists.infradead.org; Wen Gong
>> <wgong@codeaurora.org>
>> Subject: [EXT] Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature
>> 
>> So essentially the problem is that with firmwares supporting both
>> WMI_SERVICE_NLO and WMI_SERVICE_SPOOF_MAC_SUPPORT ath10k
>> enables
>> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR, but
>> NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR is not enabled
>> which is
>> inconsistent from user space point of view. Is my understanding correct?
>> 
>> Wen, can you enable NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR?
>> Does firmware
>> support that?
>
> Yes, I test again, it is enabled NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR now.

Sorry, I'm not quite understanding your reply.

But I mixed up the flags. I meant that can we enable
NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR in ath10k? Does the firmware
releases which have WMI_SERVICE_NLO support
NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR as well?

-- 
Kalle Valo

RE: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Wen Gong]

> -----Original Message-----
> From: Kalle Valo <kvalo@codeaurora.org>
> Sent: Friday, September 20, 2019 3:32 PM
> To: Wen Gong <wgong@qti.qualcomm.com>
> Cc: Brian Norris <briannorris@chromium.org>; linux-wireless <linux-
> wireless@vger.kernel.org>; Linux Kernel <linux-kernel@vger.kernel.org>;
> ath10k@lists.infradead.org; Wen Gong <wgong@codeaurora.org>
> Subject: [EXT] Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature
> 
> >> Wen, can you enable NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR?
> >> Does firmware
> >> support that?
> >
> > Yes, I test again, it is enabled
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR now.
> 
> Sorry, I'm not quite understanding your reply.
> 
> But I mixed up the flags. I meant that can we enable
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR in ath10k? Does the
> firmware
> releases which have WMI_SERVICE_NLO support
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR as well?
Kalle,
I tested with this firmware: https://github.com/kvalo/ath10k-firmware/blob/master/QCA6174/hw3.0/sdio-4.4.1/firmware-sdio-6.bin_WLAN.RMH.4.4.1-00017-QCARMSWPZ-2

In ath10k_mac_register, it has flag WMI_SERVICE_SPOOF_MAC_SUPPORT enabled.
	if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) {
		ar->hw->wiphy->features |=
			NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR;
	}

In ath10k_wow_init, it has flag ATH10K_FW_FEATURE_WOWLAN_SUPPORT, WMI_SERVICE_WOW, WMI_SERVICE_NLO enabled.
int ath10k_wow_init(struct ath10k *ar)
{
	if (!test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
		      ar->running_fw->fw_file.fw_features))
		return 0;

	if (WARN_ON(!test_bit(WMI_SERVICE_WOW, ar->wmi.svc_map)))
		return -EINVAL;

	if (test_bit(WMI_SERVICE_NLO, ar->wmi.svc_map)) {
		ar->wow.wowlan_support.flags |= WIPHY_WOWLAN_NET_DETECT;
		ar->wow.wowlan_support.max_nd_match_sets = WMI_PNO_MAX_SUPP_NETWORKS;
	}
}
> 
> --
> Kalle Valo

Re: [PATCH v3] ath10k: support NET_DETECT WoWLAN feature

[Brian Norris]

Hi Kalle,

Sorry, I failed to follow up on some of this.

On Fri, Sep 20, 2019 at 12:32 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> But I mixed up the flags. I meant that can we enable
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR in ath10k? Does the firmware
> releases which have WMI_SERVICE_NLO support
> NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR as well?

I'm looking at firmware which supports WMI_SERVICE_NLO and
WMI_SERVICE_SPOOF_MAC_SUPPORT. This leads to support for
NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR and
NL80211_WOWLAN_TRIG_NET_DETECT (good!), but it also leads to
NL80211_CMD_START_SCHED_SCAN support and *not*
NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR, which is inconsistent
(bad!).

(I think a few times in here you noted the FEATURE_SCAN variant, when
you probably meant FEATURE_SCHED_SCAN.)

If I understand Wen correctly, he is working on dropping
NL80211_CMD_START_SCHED_SCAN, which would fix the inconsistency.

But I also noticed that ath10k does not support
NL80211_FEATURE_ND_RANDOM_MAC_ADDR, which is again an inconsistency:
we're going to lose randomization when in WoWLAN + NET_DETECT mode. I
don't suspect we (Chrome OS) would ever enable this feature in that
state.

Regards,
Brian