[PATCH v2 4/4] ath10k: introduce basic tdls functionality

[Michal Kazior]

On 25 February 2015 at 08:55, Marek Puzyniak <marek.puzyniak at tieto.com> wrote:
[...]
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 94a8788..14b99c8 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -604,6 +604,7 @@ struct ath10k {
>         /* protected by conf_mutex */
>         int num_peers;
>         int num_stations;
> +       int tdls_peers;

I don't like the idea of having another var in ath10k which isn't on
hotpath and can be derived from other structures/tools we already
have, i.e. ieee80211_iterate_stations_atomic(). You can use that to
implement ath10k_mac_tdls_num_peers(arvif).


>
>         int max_num_peers;
>         int max_num_stations;
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 7378656..21720b8 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -518,6 +518,73 @@ static void ath10k_peer_cleanup_all(struct ath10k *ar)
>         ar->num_stations = 0;
>  }
>
> +static int ath10k_mac_enable_tdls(struct ath10k *ar, u32 vdev_id, bool enable)

With number of tdls peers being derived you won't be able to have this
kind of "recalc" logic anymore. I guess that's a good thing. You'll
need to call ath10k_wmi_update_fw_tdls_state() explicitly in
ath10k_sta_state() depending on context and numbers of peers.


> +static int ath10k_mac_tdls_peer_update(struct ath10k *ar, u32 vdev_id,
> +                                      struct ieee80211_sta *sta,
> +                                      enum wmi_tdls_peer_state state)
> +{
> +       int ret;
> +       struct wmi_tdls_peer_update_cmd_arg arg;

I would `arg = {};` to make sure it's clean. Even if you're positive
you fill all `arg` members here it leaves it open for bugs sneaking in
when you extend the structure later on and forget to init new members
here as well.


> +       struct wmi_tdls_peer_capab_arg cap = {};
> +       struct wmi_channel_arg chan_arg = {};
> +
> +       lockdep_assert_held(&ar->conf_mutex);
> +
> +       arg.vdev_id = vdev_id;
> +       arg.peer_state = state;
> +       ether_addr_copy(arg.addr, sta->addr);
> +
> +       cap.peer_max_sp = sta->max_sp;
> +       cap.peer_uapsd_queues = sta->uapsd_queues;
> +
> +       if (state == WMI_TDLS_PEER_STATE_CONNECTED) {
> +               if (!sta->tdls_initiator)
> +                       cap.is_peer_responder = 1;
> +       }

You can probably make it into a single if() ?


> @@ -4092,9 +4193,30 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
>                         ath10k_warn(ar, "failed to associate station %pM for vdev %i: %i\n",
>                                     sta->addr, arvif->vdev_id, ret);
>         } else if (old_state == IEEE80211_STA_ASSOC &&
> -                  new_state == IEEE80211_STA_AUTH &&
> -                  (vif->type == NL80211_IFTYPE_AP ||
> -                   vif->type == NL80211_IFTYPE_ADHOC)) {
> +                  new_state == IEEE80211_STA_AUTHORIZED &&
> +                  sta->tdls) {
> +               /*
> +                * Tdls station authorized.
> +                */
> +               ath10k_dbg(ar, ATH10K_DBG_MAC, "mac tdls sta %pM authorized\n",
> +                          sta->addr);
> +
> +               ret = ath10k_station_assoc(ar, vif, sta, false);
> +               if (ret) {
> +                       ath10k_warn(ar, "failed to associate station %pM for vdev %i: %i\n",

"failed to associate tdls station...." would be nicer I guess?


> +static u32 ath10k_wmi_tlv_prepare_peer_qos(u8 uapsd_queues, u8 sp)
> +{
> +       u32 peer_qos = 0;
> +
> +       if (uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_VO)
> +               peer_qos |= WMI_TLV_TDLS_PEER_QOS_AC_VO;
> +       if (uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_VI)
> +               peer_qos |= WMI_TLV_TDLS_PEER_QOS_AC_VI;
> +       if (uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_BK)
> +               peer_qos |= WMI_TLV_TDLS_PEER_QOS_AC_BK;
> +       if (uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_BE)
> +               peer_qos |= WMI_TLV_TDLS_PEER_QOS_AC_BE;
> +       peer_qos |= (sp << WMI_TLV_TDLS_PEER_SP_POS);

peer_qos |= (sp << WMI_TLV_TDLS_PEER_SP_SHIFT);

Or even provide _MASK + _SHIFT defines and use SM/MS macros.


> +struct wmi_tdls_peer_update_cmd_arg {
> +       u32 vdev_id;
> +       enum wmi_tdls_peer_state peer_state;
> +       u8 addr[ETH_ALEN];
> +} __packed;

No need to pack local/driver-only structures.


> +struct wmi_tdls_peer_capab_arg {
> +       u8 peer_uapsd_queues;
> +       u8 peer_max_sp;
> +       u32 buff_sta_support;
> +       u32 off_chan_support;
> +       u32 peer_curr_operclass;
> +       u32 self_curr_operclass;
> +       u32 peer_chan_len;
> +       u32 peer_operclass_len;
> +       u8 peer_operclass[WMI_TDLS_MAX_SUPP_OPER_CLASSES];
> +       u32 is_peer_responder;
> +       u32 pref_offchan_num;
> +       u32 pref_offchan_bw;
> +} __packed;

No need to pack local/driver-only structures.


Michał