Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

[Jeff Johnson]

On 10/17/2023 9:53 AM, James Prestwood wrote:
> Advertise support for multicast frame registration and update the RX
> filter with FIF_MCAST_ACTION to allow broadcast action frames to be
> received. Broadcast action frames are needed for the Device
> Provisioning Protocol (DPP) for Presence and PKEX Exchange requests.
> 
> Signed-off-by: James Prestwood <prestwoj@gmail.com>
> ---
>   drivers/net/wireless/ath/ath10k/mac.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 03e7bc5b6c0b..932ace5b900b 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -1249,7 +1249,7 @@ static bool ath10k_mac_monitor_vdev_is_needed(struct ath10k *ar)
>   	return ar->monitor ||
>   	       (!test_bit(ATH10K_FW_FEATURE_ALLOWS_MESH_BCAST,
>   			  ar->running_fw->fw_file.fw_features) &&
> -		(ar->filter_flags & FIF_OTHER_BSS)) ||
> +		(ar->filter_flags & (FIF_OTHER_BSS | FIF_MCAST_ACTION))) ||
>   	       test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
>   }
>   
> @@ -6020,6 +6020,7 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
>   	FIF_OTHER_BSS |				\
>   	FIF_BCN_PRBRESP_PROMISC |		\
>   	FIF_PROBE_REQ |				\
> +	FIF_MCAST_ACTION |			\
>   	FIF_FCSFAIL)
>   
>   static void ath10k_configure_filter(struct ieee80211_hw *hw,
> @@ -10121,6 +10122,8 @@ int ath10k_mac_register(struct ath10k *ar)
>   	wiphy_ext_feature_set(ar->hw->wiphy,
>   			      NL80211_EXT_FEATURE_SET_SCAN_DWELL);
>   	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_AQL);
> +	wiphy_ext_feature_set(ar->hw->wiphy,
> +			      NL80211_EXT_FEATURE_MULTICAST_REGISTRATIONS);
>   
>   	if (test_bit(WMI_SERVICE_TX_DATA_ACK_RSSI, ar->wmi.svc_map) ||
>   	    test_bit(WMI_SERVICE_HTT_MGMT_TX_COMP_VALID_FLAGS, ar->wmi.svc_map))

+ ath10k@lists.infradead.org


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

Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

[James Prestwood]

Re-sending as its been a few weeks, thanks Jeff for adding the ath10k CC.

On 10/17/23 9:53 AM, James Prestwood wrote:
> Advertise support for multicast frame registration and update the RX
> filter with FIF_MCAST_ACTION to allow broadcast action frames to be
> received. Broadcast action frames are needed for the Device
> Provisioning Protocol (DPP) for Presence and PKEX Exchange requests.
> 
> Signed-off-by: James Prestwood <prestwoj@gmail.com>
> ---
>   drivers/net/wireless/ath/ath10k/mac.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 03e7bc5b6c0b..932ace5b900b 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -1249,7 +1249,7 @@ static bool ath10k_mac_monitor_vdev_is_needed(struct ath10k *ar)
>   	return ar->monitor ||
>   	       (!test_bit(ATH10K_FW_FEATURE_ALLOWS_MESH_BCAST,
>   			  ar->running_fw->fw_file.fw_features) &&
> -		(ar->filter_flags & FIF_OTHER_BSS)) ||
> +		(ar->filter_flags & (FIF_OTHER_BSS | FIF_MCAST_ACTION))) ||
>   	       test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
>   }
>   
> @@ -6020,6 +6020,7 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
>   	FIF_OTHER_BSS |				\
>   	FIF_BCN_PRBRESP_PROMISC |		\
>   	FIF_PROBE_REQ |				\
> +	FIF_MCAST_ACTION |			\
>   	FIF_FCSFAIL)
>   
>   static void ath10k_configure_filter(struct ieee80211_hw *hw,
> @@ -10121,6 +10122,8 @@ int ath10k_mac_register(struct ath10k *ar)
>   	wiphy_ext_feature_set(ar->hw->wiphy,
>   			      NL80211_EXT_FEATURE_SET_SCAN_DWELL);
>   	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_AQL);
> +	wiphy_ext_feature_set(ar->hw->wiphy,
> +			      NL80211_EXT_FEATURE_MULTICAST_REGISTRATIONS);
>   
>   	if (test_bit(WMI_SERVICE_TX_DATA_ACK_RSSI, ar->wmi.svc_map) ||
>   	    test_bit(WMI_SERVICE_HTT_MGMT_TX_COMP_VALID_FLAGS, ar->wmi.svc_map))

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

Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

[Kalle Valo]

James Prestwood <prestwoj@gmail.com> wrote:

> Advertise support for multicast frame registration and update the RX
> filter with FIF_MCAST_ACTION to allow broadcast action frames to be
> received. Broadcast action frames are needed for the Device
> Provisioning Protocol (DPP) for Presence and PKEX Exchange requests.
> 
> Signed-off-by: James Prestwood <prestwoj@gmail.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

On what hardware and firmware did you test with this? As there's so many
different combinations in ath10k we use Tested-on tag to document that.

https://wireless.wiki.kernel.org/en/users/drivers/ath10k/submittingpatches#tested-on_tag

As ath10k hardware and firmware can work very differently from each other I'm
suspicious if this feature really work in all of them.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20231017165306.118779-1-prestwoj@gmail.com/

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


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

Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

[James Prestwood]

Hi Kalle,

On 11/13/23 7:50 AM, Kalle Valo wrote:
> James Prestwood <prestwoj@gmail.com> wrote:
> 
>> Advertise support for multicast frame registration and update the RX
>> filter with FIF_MCAST_ACTION to allow broadcast action frames to be
>> received. Broadcast action frames are needed for the Device
>> Provisioning Protocol (DPP) for Presence and PKEX Exchange requests.
>>
>> Signed-off-by: James Prestwood <prestwoj@gmail.com>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> 
> On what hardware and firmware did you test with this? As there's so many
> different combinations in ath10k we use Tested-on tag to document that.
> 
> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/submittingpatches#tested-on_tag
> 
> As ath10k hardware and firmware can work very differently from each other I'm
> suspicious if this feature really work in all of them.

I only tested on a QCA6174 (and I'll add Tested-on for that). This makes 
sense and maybe enabling unconditionally for all ath10k hardware is the 
wrong way to go about it.

Since I don't have the ability to test every hardware combination 
hopefully someone from atheros can chime in. Is there some 
firmware/driver value that can be queried which tells me if broadcast RX 
is supported? Or if not is checking ar->hw_rev == ATH10K_HW_QCA6174 good 
enough? Or are there sub-variants that may or may not support this?

Thanks
James

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

Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

[Kalle Valo]

James Prestwood <prestwoj@gmail.com> writes:

> On 11/13/23 7:50 AM, Kalle Valo wrote:
>> James Prestwood <prestwoj@gmail.com> wrote:
>> 
>>> Advertise support for multicast frame registration and update the RX
>>> filter with FIF_MCAST_ACTION to allow broadcast action frames to be
>>> received. Broadcast action frames are needed for the Device
>>> Provisioning Protocol (DPP) for Presence and PKEX Exchange requests.
>>>
>>> Signed-off-by: James Prestwood <prestwoj@gmail.com>
>>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> On what hardware and firmware did you test with this? As there's so
>> many
>> different combinations in ath10k we use Tested-on tag to document that.
>> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/submittingpatches#tested-on_tag
>> As ath10k hardware and firmware can work very differently from each
>> other I'm
>> suspicious if this feature really work in all of them.
>
> I only tested on a QCA6174 (and I'll add Tested-on for that). This
> makes sense and maybe enabling unconditionally for all ath10k hardware
> is the wrong way to go about it.
>
> Since I don't have the ability to test every hardware combination
> hopefully someone from atheros can chime in.

Heh, Atheros is long gone. But your comment made me remember the good
old times and smile :)

> Is there some firmware/driver value that can be queried which tells me
> if broadcast RX is supported?

A good question for which I don't have an answer. Does anyone else know?

Do you have a simple test case for this? It would help if people could
test this feature on their ath10k devices and send us results.

> Or if not is checking ar->hw_rev == ATH10K_HW_QCA6174 good enough? 

BTW instead of checking ar->hw_rev our preference is to add a new
boolean to struct ath10k_hw_params. That way it's easier to enable and
disable the feature per hardware version.

> Or are there sub-variants that may or may not support this?

There are several QCA6174 variants and you can check the variants from
ath10k_hw_params_list. For example, hw2.1 or SDIO firmware may very well
behave different from the PCI firmware. To be on the safe side I think it's
best to enable the feature only on the hardware versions we have
verified to work.

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

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

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

Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

[James Prestwood]

On 11/14/23 12:20 AM, Kalle Valo wrote:
> James Prestwood <prestwoj@gmail.com> writes:
> 
>> On 11/13/23 7:50 AM, Kalle Valo wrote:
>>> James Prestwood <prestwoj@gmail.com> wrote:
>>>
>>>> Advertise support for multicast frame registration and update the RX
>>>> filter with FIF_MCAST_ACTION to allow broadcast action frames to be
>>>> received. Broadcast action frames are needed for the Device
>>>> Provisioning Protocol (DPP) for Presence and PKEX Exchange requests.
>>>>
>>>> Signed-off-by: James Prestwood <prestwoj@gmail.com>
>>>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>>> On what hardware and firmware did you test with this? As there's so
>>> many
>>> different combinations in ath10k we use Tested-on tag to document that.
>>> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/submittingpatches#tested-on_tag
>>> As ath10k hardware and firmware can work very differently from each
>>> other I'm
>>> suspicious if this feature really work in all of them.
>>
>> I only tested on a QCA6174 (and I'll add Tested-on for that). This
>> makes sense and maybe enabling unconditionally for all ath10k hardware
>> is the wrong way to go about it.
>>
>> Since I don't have the ability to test every hardware combination
>> hopefully someone from atheros can chime in.
> 
> Heh, Atheros is long gone. But your comment made me remember the good
> old times and smile :)

Oh yeah, QCOM bought Atheros just before I got my first job there. 
Wasn't sure if the remaining devs still thought of themselves "Atheros" 
employees to this day :)

> 
>> Is there some firmware/driver value that can be queried which tells me
>> if broadcast RX is supported?
> 
> A good question for which I don't have an answer. Does anyone else know?
> 
> Do you have a simple test case for this? It would help if people could
> test this feature on their ath10k devices and send us results.

I could try and come up with something. I've been testing with 2 
devices, running the full DPP protocol between... not exactly "simple".

I suppose you could create a station device, register for beacons, just 
sit and see if you see any. I'm not sure though if this is a 1:1 test 
since really its action frame I'm after, and the firmware may treat them 
differently.

> 
>> Or if not is checking ar->hw_rev == ATH10K_HW_QCA6174 good enough?
> 
> BTW instead of checking ar->hw_rev our preference is to add a new
> boolean to struct ath10k_hw_params. That way it's easier to enable and
> disable the feature per hardware version.
> 
>> Or are there sub-variants that may or may not support this?
> 
> There are several QCA6174 variants and you can check the variants from
> ath10k_hw_params_list. For example, hw2.1 or SDIO firmware may very well
> behave different from the PCI firmware. To be on the safe side I think it's
> best to enable the feature only on the hardware versions we have
> verified to work.

Sounds good, I can make it specific to just my hardware and others could 
expand in the future if they need. Out of curiosity is ath9k much more 
limited on unique hardware? I based this patch off one from Jouni for 
ath9k [1] and it unconditionally enables it for the entire driver.

[1] 
https://lore.kernel.org/linux-wireless/20200426084733.7889-1-jouni@codeaurora.org/

Thanks,
James

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

Re: [PATCH] wifi: ath10k: add support to allow broadcast action from RX

[Kalle Valo]

James Prestwood <prestwoj@gmail.com> writes:

>>> Is there some firmware/driver value that can be queried which tells me
>>> if broadcast RX is supported?
>>
>> A good question for which I don't have an answer. Does anyone else
>> know? Do you have a simple test case for this? It would help if
>> people could test this feature on their ath10k devices and send us
>> results.
>
> I could try and come up with something. I've been testing with 2
> devices, running the full DPP protocol between... not exactly
> "simple".

Yeah, that doesn't sound simple.

>>> Or if not is checking ar->hw_rev == ATH10K_HW_QCA6174 good enough?
>>
>> BTW instead of checking ar->hw_rev our preference is to add a new
>> boolean to struct ath10k_hw_params. That way it's easier to enable
>> and disable the feature per hardware version.
>> 
>>> Or are there sub-variants that may or may not support this?
>>
>> There are several QCA6174 variants and you can check the variants
>> from ath10k_hw_params_list. For example, hw2.1 or SDIO firmware may
>> very well behave different from the PCI firmware. To be on the safe
>> side I think it's best to enable the feature only on the hardware
>> versions we have verified to work.
>
> Sounds good, I can make it specific to just my hardware and others
> could expand in the future if they need.

I think that's the best plan.

> Out of curiosity is ath9k much more limited on unique hardware? I
> based this patch off one from Jouni for ath9k [1] and it
> unconditionally enables it for the entire driver.

ath9k doesn't have firmware, or well ath9k PCI devices don't have one,
and that makes things so much simpler. Don't know how thin (or bloated)
ath9k USB firmware is.

Starting from ath10k a firmware was introduced for all 11ac devices, and
not just "the one and only firmware" but N+1 different branches of
firmware. So even if something works with one firmware branch there's no
guarantee how it works in the other N branches. Great fun supporting
that.

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

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

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