| Message ID | 20231017165306.118779-1-prestwoj@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: ath10k: add support to allow broadcast action from RX | expand |
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))
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.
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
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.
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
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.
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))
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(-)