| Message ID | 1560403912-27385-1-git-send-email-milehu@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hostapd: add HE cross check | expand |
On Wed, Jun 12, 2019 at 10:31:52PM -0700, Miles Hu wrote: > Cross check between firmware and hostapd config capabilities. > diff --git a/src/ap/hw_features.c b/src/ap/hw_features.c > static int ieee80211ax_supported_he_capab(struct hostapd_iface *iface) > + u32 cap_ulmimo = 0, cap_ulofdma = 0; What are those for? They seem to be unused and generate compiler warnings. > +#define HE_CAP_CHECK2(hw_cap, cap, bytes, cap1, bytes1, conf) \ > + do { \ > + if (conf && !(_ieee80211he_cap_check(hw_cap, bytes, cap) || \ > + _ieee80211he_cap_check(hw_cap, bytes1, cap1))) { \ > + wpa_printf(MSG_ERROR, "Driver does not support configured HE" \ > + " capability [%s-%s]", #cap, #cap1); \ > + return 0; \ > + } \ > + } while (0) What exactly is this trying to do? !(cap || cap2) is identical to !cap && !cap2. Is that what this two-capability-version is trying to do? Check that the driver does not support neither of these, i.e., it is fine if either is supported? That error message is quite confusing if that is the case.. > + HE_CAP_CHECK2(hw->phy_cap, HE_PHYCAP_UL_MUMIMO_CAPB, > + HE_PHYCAP_UL_MUMIMO_CAPB_IDX, HE_PHYCAP_UL_MUOFDMA_CAPB, > + HE_PHYCAP_UL_MUOFDMA_CAPB_IDX, conf->he_mu_edca.he_qos_info); I'm not sure how to interpret this since he_qos_info includes information from four undocumented parameters: he_mu_edca_qos_info_param_count, he_mu_edca_qos_info_q_ack, he_mu_edca_qos_info_queue_request, and he_mu_edca_qos_info_txop_request. And it is not obvious how those map to the two PHY capabilities. > + HE_CAP_CHECK2(hw->mac_cap, HE_MACCAP_TWT_REQUESTER, HE_MACCAP_TWT_REQUESTER_IDX, > + HE_MACCAP_TWT_RESPONDER, HE_MACCAP_TWT_RESPONDER_IDX, > + conf->he_op.he_twt_required); What about this one? conf->he_op.he_twt_required is either 0 or 1, but HE_CAP_CHECK2() uses two capability bits. Is this trying to check that the driver support at least TWT requester or TWT responder capability? These would really need comments to document what the macros do and what exactly these HE_CAP_CHECK2 cases are validating. In addition, these checks are breaking mac80211_hwsim test cases he_params, he_40, and he_80_params. That may be expected since mac80211_hwsim does not advertise all HE capabilities. However, I'd like to get a patch that fixes those test cases before the patch to add validation steps are added. > diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h > > /* HE Capabilities Information defines */ > > -#define HE_MACCAP_TWT_RESPONDER ((u8) BIT(3)) That line does not exist in hostap.git, so it cannot be removed either.. I rejected a patch proposing this because the specified value does not seem to match P802.11ax.
On 2021-04-28 15:50, Aloka Dixit wrote: > From: Miles Hu <milehu@codeaurora.org> > > Cross check between firmware and hostapd config capabilities. > > Signed-off-by: Miles Hu <milehu@codeaurora.org> > --- > src/ap/hw_features.c | 53 > ++++++++++++++++++++++++++++++++++++++++++++ > src/common/ieee802_11_defs.h | 12 +++++++++- > 2 files changed, 64 insertions(+), 1 deletion(-) > > diff --git a/src/ap/hw_features.c b/src/ap/hw_features.c > index fec1a78..f1d8418 100644 > --- a/src/ap/hw_features.c > +++ b/src/ap/hw_features.c > +#define HE_PHYCAP_SPR_SR_CAPB_IDX 7 > +#define HE_PHYCAP_SPR_SR_CAPB ((u8) BIT(0)) > + > #define HE_PHYCAP_PPE_THRESHOLD_PRESENT_IDX 6 > #define HE_PHYCAP_PPE_THRESHOLD_PRESENT ((u8) BIT(7)) Please ignore this patch, it was sent by mistake.
diff --git a/src/ap/hw_features.c b/src/ap/hw_features.c index fec1a78..f1d8418 100644 --- a/src/ap/hw_features.c +++ b/src/ap/hw_features.c @@ -657,8 +657,60 @@ static int ieee80211ac_supported_vht_capab(struct hostapd_iface *iface) #ifdef CONFIG_IEEE80211AX +static int _ieee80211he_cap_check(u8 *hw, u32 offset, u8 bits) +{ + if (bits & hw[offset]) + return 1; + + return 0; +} + static int ieee80211ax_supported_he_capab(struct hostapd_iface *iface) { + struct hostapd_hw_modes *mode = iface->current_mode; + struct he_capabilities *hw = &mode->he_capab; + struct hostapd_config *conf = iface->conf; + u32 cap_ulmimo = 0, cap_ulofdma = 0; + +#define HE_CAP_CHECK(hw_cap, cap, bytes, conf) \ + do { \ + if (conf && !_ieee80211he_cap_check(hw_cap, bytes, cap)) { \ + wpa_printf(MSG_ERROR, "Driver does not support configured" \ + " HE capability [%s]", #cap); \ + return 0; \ + } \ + } while (0) + +#define HE_CAP_CHECK2(hw_cap, cap, bytes, cap1, bytes1, conf) \ + do { \ + if (conf && !(_ieee80211he_cap_check(hw_cap, bytes, cap) || \ + _ieee80211he_cap_check(hw_cap, bytes1, cap1))) { \ + wpa_printf(MSG_ERROR, "Driver does not support configured HE" \ + " capability [%s-%s]", #cap, #cap1); \ + return 0; \ + } \ + } while (0) + + HE_CAP_CHECK(hw->phy_cap, HE_PHYCAP_SU_BEAMFORMER_CAPAB, + HE_PHYCAP_SU_BEAMFORMER_CAPAB_IDX, + conf->he_phy_capab.he_su_beamformer); + HE_CAP_CHECK(hw->phy_cap, HE_PHYCAP_SU_BEAMFORMEE_CAPAB, + HE_PHYCAP_SU_BEAMFORMEE_CAPAB_IDX, + conf->he_phy_capab.he_su_beamformee); + HE_CAP_CHECK(hw->phy_cap, HE_PHYCAP_MU_BEAMFORMER_CAPAB, + HE_PHYCAP_MU_BEAMFORMER_CAPAB_IDX, + conf->he_phy_capab.he_mu_beamformer); + HE_CAP_CHECK(hw->phy_cap, HE_PHYCAP_SPR_SR_CAPB, + HE_PHYCAP_SPR_SR_CAPB_IDX, conf->spr.sr_control); + + HE_CAP_CHECK2(hw->phy_cap, HE_PHYCAP_UL_MUMIMO_CAPB, + HE_PHYCAP_UL_MUMIMO_CAPB_IDX, HE_PHYCAP_UL_MUOFDMA_CAPB, + HE_PHYCAP_UL_MUOFDMA_CAPB_IDX, conf->he_mu_edca.he_qos_info); + + HE_CAP_CHECK2(hw->mac_cap, HE_MACCAP_TWT_REQUESTER, HE_MACCAP_TWT_REQUESTER_IDX, + HE_MACCAP_TWT_RESPONDER, HE_MACCAP_TWT_RESPONDER_IDX, + conf->he_op.he_twt_required); + return 1; } #endif /* CONFIG_IEEE80211AX */ @@ -670,6 +722,7 @@ int hostapd_check_ht_capab(struct hostapd_iface *iface) { #ifdef CONFIG_IEEE80211N int ret; + if (!iface->conf->ieee80211n) return 0; diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h index 12c004f..b1f9769 100644 --- a/src/common/ieee802_11_defs.h +++ b/src/common/ieee802_11_defs.h @@ -2139,7 +2139,10 @@ struct ieee80211_spatial_reuse { /* HE Capabilities Information defines */ -#define HE_MACCAP_TWT_RESPONDER ((u8) BIT(3)) +#define HE_MACCAP_TWT_REQUESTER_IDX 0 +#define HE_MACCAP_TWT_REQUESTER ((u8) BIT(1)) +#define HE_MACCAP_TWT_RESPONDER_IDX 0 +#define HE_MACCAP_TWT_RESPONDER ((u8) BIT(2)) #define HE_PHYCAP_CHANNEL_WIDTH_SET_IDX 0 #define HE_PHYCAP_CHANNEL_WIDTH_MASK ((u8) (BIT(1) | BIT(2) | \ @@ -2156,6 +2159,13 @@ struct ieee80211_spatial_reuse { #define HE_PHYCAP_MU_BEAMFORMER_CAPAB_IDX 4 #define HE_PHYCAP_MU_BEAMFORMER_CAPAB ((u8) BIT(1)) +#define HE_PHYCAP_UL_MUMIMO_CAPB_IDX 2 +#define HE_PHYCAP_UL_MUMIMO_CAPB ((u8) BIT(6)) +#define HE_PHYCAP_UL_MUOFDMA_CAPB_IDX 2 +#define HE_PHYCAP_UL_MUOFDMA_CAPB ((u8) BIT(7)) +#define HE_PHYCAP_SPR_SR_CAPB_IDX 7 +#define HE_PHYCAP_SPR_SR_CAPB ((u8) BIT(0)) + #define HE_PHYCAP_PPE_THRESHOLD_PRESENT_IDX 6 #define HE_PHYCAP_PPE_THRESHOLD_PRESENT ((u8) BIT(7))
Cross check between firmware and hostapd config capabilities. Signed-off-by: Miles Hu <milehu@codeaurora.org> --- src/ap/hw_features.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ src/common/ieee802_11_defs.h | 12 +++++++++- 2 files changed, 64 insertions(+), 1 deletion(-)