diff mbox series

[6/6] hostapd: Register callback to get/free AID with vendor cmd

Message ID 20181126093457.9723-1-sarada.prasanna.garnayak@intel.com
State Changes Requested
Headers show
Series [1/6] Add a header file defining Intel Lantiq OUI and vendor extensions | expand

Commit Message

Sarada Prasanna Garnayak Nov. 26, 2018, 9:34 a.m. UTC 
Register callbacks to receive a new association ID for a station
from the kernel driver using vendor cmd, which may in turn ask it
from the firmware and that from the hardware. This AID is tied to SID
and will need to be freed eventually during the dissociation phase.

Signed-off-by: Sarada Prasanna Garnayak <sarada.prasanna.garnayak@intel.com>
---
 src/ap/ieee802_11.c               |  5 ++++-
 src/ap/sta_info.c                 | 10 +++++++---
 src/drivers/driver.h              | 24 ++++++++++++++++++++++++
 src/drivers/driver_nl80211.c      | 17 +++++++++--------
 src/drivers/driver_nl80211.h      |  6 ++++++
 src/drivers/driver_nl80211_capa.c | 14 ++++++++++++++
 6 files changed, 64 insertions(+), 12 deletions(-)

Comments

Jouni Malinen Nov. 26, 2018, 11:08 a.m. UTC | #1 
On Mon, Nov 26, 2018 at 03:04:57PM +0530, Sarada Prasanna Garnayak wrote:
> Register callbacks to receive a new association ID for a station
> from the kernel driver using vendor cmd, which may in turn ask it
> from the firmware and that from the hardware. This AID is tied to SID
> and will need to be freed eventually during the dissociation phase.

> diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
> @@ -2130,11 +2130,14 @@ static void handle_auth(struct hostapd_data *hapd,
>  	}
>  }
>  
> -
>  int hostapd_get_aid(struct hostapd_data *hapd, struct sta_info *sta)

Please do not include this type of unrelated whitespace changes (and
hostap.git follows a style where there are two empty lines between
functions, so I would not accept this even as a separate cleanup patch).

> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> @@ -3651,6 +3651,30 @@ struct wpa_driver_ops {

> +   * get_aid - Receive a new association ID for a station
> +   * This function is used to receive a new AID from the kernel driver,
> +   * which may in turn ask it from the FW, and that from the HW.
> +   * This AID is tied to SID and will need to be freed eventually.
> +   */
> +  int (*get_aid)(void *priv, u16 *aid, const u8 *addr);

> +   * free_aid - Release an association ID
> +   * This function is used to release an AID back to the kernel driver,
> +   * which may release it to the FW, and that to the HW.
> +   */
> +  int (*free_aid)(void *priv, u16 *aid);

So the part about defining this type of interface for allowing the
driver to select AID sounds reasonable.. Though, that free_aid() case
should really use u16 aid instead of a pointer to aid (the caller should
be responsible for clearing that internal variable, if appropriate).

However, I don't really like the actual implementation of that driver
op:

> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> @@ -27,6 +27,7 @@
> +#include "common/intel-ltq-vendor.h"

This would bring in all the problematic src/ap/*.h inclusions into
src/drivers/*, so this is not acceptable unless those parts are removed
from intel-ltq-vendor.h as I commented on an earlier patch.

> @@ -10684,6 +10681,10 @@ const struct wpa_driver_ops wpa_driver_nl80211_ops = {

> +#ifdef CONFIG_DRIVER_NL80211_INTEL_LTQ
> +	.get_aid = nl80211_get_aid,
> +	.free_aid = nl80211_free_aid,
> +#endif /* CONFIG_DRIVER_NL80211_INTEL_LTQ */

And these two functions are now coming in as static inlines from
src/common/intel-ltq-vendor.h while they should really be non-inline
functions in src/drivers/driver_nl80211.c.

Furthermore, this AID management functionality by the driver sounds like
a generic design option and as such, it would be much more valuable to
define it as an upstream nl80211 functionality. Could you please
consider such implementation for this instead of this vendor specific
command?
diff mbox series

Patch

diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
index d2d6b1767..da6594757 100644
--- a/src/ap/ieee802_11.c
+++ b/src/ap/ieee802_11.c
@@ -2130,11 +2130,14 @@  static void handle_auth(struct hostapd_data *hapd,
 	}
 }
 
-
 int hostapd_get_aid(struct hostapd_data *hapd, struct sta_info *sta)
 {
 	int i, j = 32, aid;
 
+	if (hapd->driver->get_aid)
+		return (*hapd->driver->get_aid)(hapd->drv_priv,
+						&sta->aid, sta->addr);
+
 	/* get a unique AID */
 	if (sta->aid > 0) {
 		wpa_printf(MSG_DEBUG, "  old AID %d", sta->aid);
diff --git a/src/ap/sta_info.c b/src/ap/sta_info.c
index 179cf43b6..287cf2dfd 100644
--- a/src/ap/sta_info.c
+++ b/src/ap/sta_info.c
@@ -182,9 +182,13 @@  void ap_free_sta(struct hostapd_data *hapd, struct sta_info *sta)
 	ap_sta_hash_del(hapd, sta);
 	ap_sta_list_del(hapd, sta);
 
-	if (sta->aid > 0)
-		hapd->sta_aid[(sta->aid - 1) / 32] &=
-			~BIT((sta->aid - 1) % 32);
+	if (sta->aid > 0) {
+		if (hapd->driver->free_aid)
+			(*hapd->driver->free_aid)(hapd->drv_priv, &sta->aid);
+		else
+			hapd->sta_aid[(sta->aid - 1) / 32] &=
+				~BIT((sta->aid - 1) % 32);
+	}
 
 	hapd->num_sta--;
 	if (sta->nonerp_set) {
diff --git a/src/drivers/driver.h b/src/drivers/driver.h
index d34c6794f..671585e47 100644
--- a/src/drivers/driver.h
+++ b/src/drivers/driver.h
@@ -3651,6 +3651,30 @@  struct wpa_driver_ops {
 	 */
 	int (*status)(void *priv, char *buf, size_t buflen);
 
+  /**
+   * get_aid - Receive a new association ID for a station
+   * @priv: Private driver interface data
+   * @aid: Memory address for storing the received AID
+   * @addr: MAC address of the station
+   * Returns: 0 on success, -1 on failure
+   *
+   * This function is used to receive a new AID from the kernel driver,
+   * which may in turn ask it from the FW, and that from the HW.
+   * This AID is tied to SID and will need to be freed eventually.
+   */
+  int (*get_aid)(void *priv, u16 *aid, const u8 *addr);
+
+  /**
+   * free_aid - Release an association ID
+   * @priv: Private driver interface data
+   * @aid: AID to release
+   * Returns: 0 on success, -1 on failure
+   *
+   * This function is used to release an AID back to the kernel driver,
+   * which may release it to the FW, and that to the HW.
+   */
+  int (*free_aid)(void *priv, u16 *aid);
+
 	/**
 	 * roaming - Set roaming policy for driver-based BSS selection
 	 * @priv: Private driver interface data
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index 771e76696..403c38f6c 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -27,6 +27,7 @@ 
 #include "eloop.h"
 #include "common/qca-vendor.h"
 #include "common/qca-vendor-attr.h"
+#include "common/intel-ltq-vendor.h"
 #include "common/ieee802_11_defs.h"
 #include "common/ieee802_11_common.h"
 #include "common/wpa_common.h"
@@ -105,7 +106,6 @@  static void nl80211_handle_destroy(struct nl_handle *handle)
 }
 #endif /* CONFIG_LIBNL20 */
 
-
 #ifdef ANDROID
 /* system/core/libnl_2 does not include nl_socket_set_nonblocking() */
 #undef nl_socket_set_nonblocking
@@ -6273,7 +6273,6 @@  static int i802_read_sta_data(struct i802_bss *bss,
 	return send_and_recv_msgs(bss->drv, msg, get_sta_handler, data);
 }
 
-
 static int i802_set_tx_queue_params(void *priv, int queue, int aifs,
 				    int cw_min, int cw_max, int burst_time)
 {
@@ -8952,8 +8951,7 @@  static int cmd_reply_handler(struct nl_msg *msg, void *arg)
 }
 #endif /* CONFIG_TESTING_OPTIONS */
 
-
-static int vendor_reply_handler(struct nl_msg *msg, void *arg)
+int vendor_reply_handler(struct nl_msg *msg, void *arg)
 {
 	struct nlattr *tb[NL80211_ATTR_MAX + 1];
 	struct nlattr *nl_vendor_reply, *nl;
@@ -8984,9 +8982,9 @@  static int vendor_reply_handler(struct nl_msg *msg, void *arg)
 }
 
 
-static int nl80211_vendor_cmd(void *priv, unsigned int vendor_id,
-			      unsigned int subcmd, const u8 *data,
-			      size_t data_len, struct wpabuf *buf)
+int nl80211_vendor_cmd(void *priv, unsigned int vendor_id,
+		       unsigned int subcmd, const u8 *data,
+		       size_t data_len, struct wpabuf *buf)
 {
 	struct i802_bss *bss = priv;
 	struct wpa_driver_nl80211_data *drv = bss->drv;
@@ -9029,7 +9027,6 @@  fail:
 	return -ENOBUFS;
 }
 
-
 static int nl80211_set_qos_map(void *priv, const u8 *qos_map_set,
 			       u8 qos_map_set_len)
 {
@@ -10684,6 +10681,10 @@  const struct wpa_driver_ops wpa_driver_nl80211_ops = {
 	.hapd_init = i802_init,
 	.hapd_deinit = i802_deinit,
 	.set_wds_sta = i802_set_wds_sta,
+#ifdef CONFIG_DRIVER_NL80211_INTEL_LTQ
+	.get_aid = nl80211_get_aid,
+	.free_aid = nl80211_free_aid,
+#endif /* CONFIG_DRIVER_NL80211_INTEL_LTQ */
 	.get_seqnum = i802_get_seqnum,
 	.flush = i802_flush,
 	.get_inact_sec = i802_get_inact_sec,
diff --git a/src/drivers/driver_nl80211.h b/src/drivers/driver_nl80211.h
index 5ac0c7dfc..f4cefc54f 100644
--- a/src/drivers/driver_nl80211.h
+++ b/src/drivers/driver_nl80211.h
@@ -165,6 +165,8 @@  struct wpa_driver_nl80211_data {
 	unsigned int he_capab_vendor_cmd_avail:1;
 	unsigned int fetch_bss_trans_status:1;
 	unsigned int roam_vendor_cmd_avail:1;
+	unsigned int get_aid_vendor_cmd_avail:1;
+	unsigned int free_aid_vendor_cmd_avail:1;
 
 	u64 vendor_scan_cookie;
 	u64 remain_on_chan_cookie;
@@ -304,4 +306,8 @@  int wpa_driver_nl80211_vendor_scan(struct i802_bss *bss,
 				   struct wpa_driver_scan_params *params);
 int nl80211_set_default_scan_ies(void *priv, const u8 *ies, size_t ies_len);
 
+int vendor_reply_handler(struct nl_msg *msg, void *arg);
+int nl80211_vendor_cmd(void *priv, unsigned int vendor_id, unsigned int subcmd,
+		       const u8 *data, size_t data_len, struct wpabuf *buf);
+
 #endif /* DRIVER_NL80211_H */
diff --git a/src/drivers/driver_nl80211_capa.c b/src/drivers/driver_nl80211_capa.c
index 7b360d209..59c992368 100644
--- a/src/drivers/driver_nl80211_capa.c
+++ b/src/drivers/driver_nl80211_capa.c
@@ -15,6 +15,7 @@ 
 #include "common/ieee802_11_common.h"
 #include "common/wpa_common.h"
 #include "common/qca-vendor.h"
+#include "common/intel-ltq-vendor.h"
 #include "common/qca-vendor-attr.h"
 #include "driver_nl80211.h"
 
@@ -786,6 +787,19 @@  static int wiphy_info_handler(struct nl_msg *msg, void *arg)
 				}
 			}
 
+			if (vinfo->vendor_id == OUI_INTEL_LTQ) {
+				switch (vinfo->subcmd) {
+#ifdef CONFIG_DRIVER_NL80211_INTEL_LTQ
+				case INTEL_LTQ_NL80211_VENDOR_SUBCMD_GET_AID:
+					drv->get_aid_vendor_cmd_avail = 1;
+					break;
+				case INTEL_LTQ_NL80211_VENDOR_SUBCMD_FREE_AID:
+					drv->free_aid_vendor_cmd_avail = 1;
+					break;
+#endif /* CONFIG_DRIVER_NL80211_INTEL_LTQ */
+				}
+			}
+
 			wpa_printf(MSG_DEBUG, "nl80211: Supported vendor command: vendor_id=0x%x subcmd=%u",
 				   vinfo->vendor_id, vinfo->subcmd);
 		}