On Tue, 2022-05-24 at 14:34 +0300, viktor.barna@xxxxxxxxxx wrote:
>
> +static void cl_ops_tx_single(struct cl_hw *cl_hw,
> + struct sk_buff *skb,
> + struct ieee80211_tx_info *tx_info,
> + struct cl_sta *cl_sta,
> + struct ieee80211_sta *sta)
> +{
...
> + if (sta) {
> + u32 sta_vht_cap = sta->vht_cap.cap;
> + struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)skb->data;
> +
> + if (!(sta_vht_cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK))
> + goto out_tx;
> +
> + if (ieee80211_is_assoc_resp(mgmt->frame_control)) {
> + int len = skb->len - (mgmt->u.assoc_resp.variable - skb->data);
> + const u8 *vht_cap_addr = cfg80211_find_ie(WLAN_EID_VHT_CAPABILITY,
> + mgmt->u.assoc_resp.variable,
> + len);
> +
> + if (vht_cap_addr) {
> + struct ieee80211_vht_cap *vht_cap =
> + (struct ieee80211_vht_cap *)(2 + vht_cap_addr);
> +
> + vht_cap->vht_cap_info &=
> + ~(cpu_to_le32(IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK |
> + IEEE80211_VHT_CAP_SHORT_GI_160));
> + }
Huh??
> +int cl_ops_config(struct ieee80211_hw *hw, u32 changed)
> +{
> + /*
> + * Handler for configuration requests. IEEE 802.11 code calls this
> + * function to change hardware configuration, e.g., channel.
> + * This function should never fail but returns a negative error code
> + * if it does. The callback can sleep
> + */
> + int error = 0;
> +
> + if (changed & IEEE80211_CONF_CHANGE_CHANNEL)
> + error = cl_ops_conf_change_channel(hw);
I'm really surprised to see this callback in a modern driver - wouldn't
you want to support some form of multi-channel operation? Even just
using the chanctx callbacks might make some of the DFS things you have
there easier?
>
> +void cl_ops_bss_info_changed(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + struct ieee80211_bss_conf *info,
> + u32 changed)
> +{
...
> + if (beacon) {
> + size_t ies_len = beacon->tail_len;
> + const u8 *ies = beacon->tail;
> + const u8 *cap = NULL;
> + int var_offset = offsetof(struct ieee80211_mgmt, u.beacon.variable);
> + int len = beacon->head_len - var_offset;
> + const u8 *var_pos = beacon->head + var_offset;
> + const u8 *rate_ie = NULL;
> +
> + cl_vif->wmm_enabled = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
> + WLAN_OUI_TYPE_MICROSOFT_WMM,
> + ies,
> + ies_len);
> + cl_dbg_info(cl_hw, "vif=%d wmm_enabled=%d\n",
> + cl_vif->vif_index,
> + cl_vif->wmm_enabled);
> +
> + cap = cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies, ies_len);
> + if (cap && cap[1] >= sizeof(*ht_cap)) {
> + ht_cap = (void *)(cap + 2);
> + sgi_en |= (le16_to_cpu(ht_cap->cap_info) &
> + IEEE80211_HT_CAP_SGI_20) ||
> + (le16_to_cpu(ht_cap->cap_info) &
> + IEEE80211_HT_CAP_SGI_40);
> + }
> +
> + cap = cfg80211_find_ie(WLAN_EID_VHT_CAPABILITY, ies, ies_len);
> + if (cap && cap[1] >= sizeof(*vht_cap)) {
> + vht_cap = (void *)(cap + 2);
> + sgi_en |= (le32_to_cpu(vht_cap->vht_cap_info) &
> + IEEE80211_VHT_CAP_SHORT_GI_80) ||
> + (le32_to_cpu(vht_cap->vht_cap_info) &
> + IEEE80211_VHT_CAP_SHORT_GI_160);
> + }
> +
> + cap = cfg80211_find_ext_ie(WLAN_EID_EXT_HE_CAPABILITY, ies, ies_len);
> + if (cap && cap[1] >= sizeof(*he_cap) + 1)
> + he_cap = (void *)(cap + 3);
> +
> + rate_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, var_pos, len);
> + if (rate_ie) {
> + if (cl_band_is_24g(cl_hw))
> + if (cl_is_valid_g_rates(rate_ie))
> + hw_mode = cl_hw->conf->ci_cck_in_hw_mode ?
> + HW_MODE_BG : HW_MODE_G;
> + else
> + hw_mode = HW_MODE_B;
> + else
> + hw_mode = HW_MODE_A;
> + }
> + } else {
> + cl_dbg_warn(cl_hw, "beacon_data not set!\n");
> + }
> +
This feels ... odd. You really shouldn't have to look into the beacon to
figure out these things?
And SGI etc. are per-STA rate control parameters anyway? Hmm.
> +/* This function is required for PS flow - do not remove */
> +int cl_ops_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set)
> +{
> + return 0;
> +}
You have all this hardware/firmware and you implement this? Interesting
design choice. One that I'm sure you'll revisit for WiFi 7 ;-)
johannes
