[RFC 1/6] frame-xchg: add destroy function to start() APIs

[RFC 1/6] frame-xchg: add destroy function to start() APIs

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4140 bytes --]

This makes things more consistent with other IWD APIs as well as
prepares for unifying frame-xchg and scanning.
---
 src/frame-xchg.c | 36 ++++++++++++++++++++----------------
 src/frame-xchg.h |  6 ++++--
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 7e516d0a..1473e59e 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -106,6 +106,7 @@ struct frame_xchg_data {
 	struct l_timeout *timeout;
 	struct l_queue *rx_watches;
 	frame_xchg_cb_t cb;
+	frame_xchg_destroy_func_t destroy;
 	void *user_data;
 	uint32_t group_id;
 	unsigned int retry_cnt;
@@ -770,26 +771,25 @@ static void frame_xchg_reset(struct frame_xchg_data *fx)
 					frame_xchg_resp_cb, fx);
 }
 
-static void frame_xchg_destroy(struct frame_xchg_data *fx, int err)
+static void frame_xchg_destroy(void *user_data)
 {
-	if (fx->cb)
-		fx->cb(err, fx->user_data);
+	struct frame_xchg_data *fx = user_data;
+
+	if (fx->destroy)
+		fx->destroy(fx->user_data);
 
 	frame_xchg_reset(fx);
 	l_free(fx);
 }
 
-static void frame_xchg_cancel(void *user_data)
-{
-	struct frame_xchg_data *fx = user_data;
-
-	frame_xchg_destroy(fx, -ECANCELED);
-}
-
 static void frame_xchg_done(struct frame_xchg_data *fx, int err)
 {
 	l_queue_remove(frame_xchgs, fx);
-	frame_xchg_destroy(fx, err);
+
+	if (fx->cb)
+		fx->cb(err, fx->user_data);
+
+	frame_xchg_destroy(fx);
 }
 
 static void frame_xchg_timeout_destroy(void *user_data)
@@ -1073,20 +1073,23 @@ static bool frame_xchg_match(const void *a, const void *b)
 void frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
-			frame_xchg_cb_t cb, void *user_data, ...)
+			frame_xchg_cb_t cb, void *user_data,
+			frame_xchg_destroy_func_t destroy, ...)
 {
 	va_list args;
 
-	va_start(args, user_data);
+	va_start(args, destroy);
 	frame_xchg_startv(wdev_id, frame, freq, retry_interval, resp_timeout,
-				retries_on_ack, group_id, cb, user_data, args);
+				retries_on_ack, group_id, cb, user_data,
+				destroy, args);
 	va_end(args);
 }
 
 void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
-			frame_xchg_cb_t cb, void *user_data, va_list resp_args)
+			frame_xchg_cb_t cb, void *user_data,
+			frame_xchg_destroy_func_t destroy, va_list resp_args)
 {
 	struct frame_xchg_data *fx;
 	size_t frame_len;
@@ -1132,6 +1135,7 @@ void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 	fx->resp_timeout = resp_timeout;
 	fx->retries_on_ack = retries_on_ack;
 	fx->cb = cb;
+	fx->destroy = destroy;
 	fx->user_data = user_data;
 	fx->group_id = group_id;
 
@@ -1338,7 +1342,7 @@ static void frame_xchg_exit(void)
 	struct l_queue *xchgs = frame_xchgs;
 
 	frame_xchgs = NULL;
-	l_queue_destroy(xchgs, frame_xchg_cancel);
+	l_queue_destroy(xchgs, frame_xchg_destroy);
 
 	watch_groups = NULL;
 	l_queue_destroy(groups, frame_watch_group_destroy);
diff --git a/src/frame-xchg.h b/src/frame-xchg.h
index 45b0e5fa..55a55f73 100644
--- a/src/frame-xchg.h
+++ b/src/frame-xchg.h
@@ -46,9 +46,11 @@ bool frame_watch_wdev_remove(uint64_t wdev_id);
 void frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
-			frame_xchg_cb_t cb, void *user_data, ...);
+			frame_xchg_cb_t cb, void *user_data,
+			frame_xchg_destroy_func_t destroy, ...);
 void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
-			frame_xchg_cb_t cb, void *user_data, va_list resp_args);
+			frame_xchg_cb_t cb, void *user_data,
+			frame_xchg_destroy_func_t destroy, va_list resp_args);
 void frame_xchg_stop(uint64_t wdev_id);
-- 
2.21.1

[RFC 2/6] offchannel: introduce new offchannel module

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 13411 bytes --]

This module will handle fairness and order in any offchannel work that
needs to be done. Sending offchannel frames (frame-xchg) and scanning
are the two operations which should be queued and done sequentially
rather than simultaneously. The phy itself can only do one at a time,
and the kernel doesn't seem to treat these operations as equals meaning
we need to maintain the order ourselves and only send these types of
requests one at a time.

Both scan and frame-xchg are complex modules (especially scanning)
which is why this module was implemented generic enough where the
changes to both modules will be minimal. Any module that requires
some kind of offchannel work it can push a work item into the offchannel
queue (offchan_work_push) and when the work is ready to be started
offchannel.c will call back into the module. Once the work is completed
(and this may be some time later e.g. in scan results or a frame watch)
the module can signal back to offchannel.c that the work is finished
(offchan_work_done). offchannel will then pop the queue and continue
with the next work item.

A concept of priority was added in order to allow important offchannel
operations (e.g. ANQP) to take priority over other work items. There
are three priorities now: SUSPENDED, NORMAL, and HIGH. NORMAL and HIGH
are as you might expect, HIGH takes precedence and items will be
inserted before any other priorities. SUSPENDED is somewhat different
and exists to support scanning functionally as it is now. A work item
can be inserted as SUSPENDED priority and the item will be initialized
in a suspended state (explained below). Once resumed a SUSPENDED
priority work item will take on a NORMAL priority.

Additionally to support scanning, offchan_work_{suspend,resume} APIs were
added. These behave exactly as you would expect to match the
functionality already in scanning. You can suspend a work item and it
will remain in the queue but not acted on until resumed. The only
difference between this and scan_{suspend,resume} is that offchan_work_
acts on single work items, not the whole queue of work. This is because
the queue will also contain non-scanning items like frame-xchgs which
should still be executed (as they are now, e.g. ANQP).

The offchannel work queue's are created on a per-phy basis, so each
queue is tied to the creation and destruction of wiphys. Because of
this wiphy.c takes care of creating and destroying offchannel queues
using:

offchan_create_from_wiphy()
offchan_destroy()
---
 Makefile.am      |   1 +
 src/offchannel.c | 293 +++++++++++++++++++++++++++++++++++++++++++++++
 src/offchannel.h |  70 +++++++++++
 3 files changed, 364 insertions(+)
 create mode 100644 src/offchannel.c
 create mode 100644 src/offchannel.h

diff --git a/Makefile.am b/Makefile.am
index 57c694df..8c17683e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -229,6 +229,7 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
 					src/frame-xchg.h src/frame-xchg.c \
 					src/eap-wsc.c src/eap-wsc.h \
 					src/wscutil.h src/wscutil.c \
+					src/offchannel.h src/offchannel.c \
 					$(eap_sources) \
 					$(builtin_sources)
 
diff --git a/src/offchannel.c b/src/offchannel.c
new file mode 100644
index 00000000..6ff3d71d
--- /dev/null
+++ b/src/offchannel.c
@@ -0,0 +1,293 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2020  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include <stdbool.h>
+
+#include <ell/ell.h>
+
+#include "src/iwd.h"
+#include "src/module.h"
+#include "src/wiphy.h"
+#include "src/offchannel.h"
+
+struct offchan_work {
+	uint32_t id;
+	offchan_work_func_t do_work;
+	void *user_data;
+	offchan_work_destroy_func_t destroy;
+	enum offchan_work_priority priority;
+	bool suspended : 1;
+};
+
+/*
+ * Per-phy list of work requests
+ */
+struct offchan_phy {
+	struct wiphy *wiphy;
+	struct l_queue *work;
+	bool idle : 1;
+};
+
+static struct l_queue *phys;
+static uint32_t work_ids;
+
+static void destroy_work(void *user_data)
+{
+	struct offchan_work *work = user_data;
+
+	if (work->destroy)
+		work->destroy(work->user_data);
+
+	l_free(work);
+}
+
+static bool match_not_suspended(const void *a, const void *b)
+{
+	const struct offchan_work *work = a;
+
+	return !work->suspended;
+}
+
+static void offchan_work_next(struct offchan_phy *phy)
+{
+	struct offchan_work *work;
+	bool done;
+
+	if (!phy->idle)
+		return;
+
+	/* find first item that isn't suspended */
+	work = l_queue_find(phy->work, match_not_suspended, NULL);
+	if (!work) {
+		phy->idle = true;
+		return;
+	}
+
+	l_debug("Starting offchannel work %u", work->id);
+
+	/* Set idle as false in case the callback starts new work items */
+	phy->idle = false;
+
+	done = work->do_work(work->user_data);
+
+	if (done) {
+		l_debug("Offchannel work %u done", work->id);
+		l_queue_remove(phy->work, work);
+
+		destroy_work(work);
+
+		phy->idle = true;
+
+		offchan_work_next(phy);
+	}
+}
+
+static bool match_wiphy(const void *a, const void *b)
+{
+	const struct offchan_phy *phy = a;
+
+	return phy->wiphy == b;
+}
+
+static struct offchan_phy *find_phy(uint64_t wdev_id)
+{
+	struct wiphy *wiphy = wiphy_find(wdev_id >> 32);
+
+	if (!wiphy)
+		return NULL;
+
+	return l_queue_find(phys, match_wiphy, wiphy);
+}
+
+static int insert_by_priority(const void *a, const void *b, void *user_data)
+{
+	const struct offchan_work *work = b;
+	uint8_t *count = user_data;
+
+	/* This ensures we are not pushing before head, or any priority items */
+	if (*count == 0 || work->priority == OFFCHAN_PRIORITY_HIGH) {
+		l_put_u8(*count + 1, count);
+		return 1;
+	}
+
+	l_debug("Inserting priority work item at index %u", *count);
+
+	return -1;
+}
+
+/*
+ * Push a new work item into the queue for this phy. The wdev_id is used to
+ * get the actual phy its under.
+ */
+uint32_t offchan_work_push(uint64_t wdev_id,
+				enum offchan_work_priority priority,
+				offchan_work_func_t func, void *user_data,
+				offchan_work_destroy_func_t destroy)
+{
+	struct offchan_work *work;
+	struct offchan_phy *phy = find_phy(wdev_id);
+	uint8_t count = 0;
+
+	if (!phy)
+		return 0;
+
+	work = l_new(struct offchan_work, 1);
+	work->do_work = func;
+	work->destroy = destroy;
+	work->user_data = user_data;
+	work->id = ++work_ids;
+	work->priority = priority;
+	work->suspended = priority == OFFCHAN_PRIORITY_SUSPENDED;
+
+	l_debug("Queuing offchannel work %u", work->id);
+
+	if (priority == OFFCHAN_PRIORITY_NORMAL || work->suspended) {
+		l_queue_push_tail(phy->work, work);
+		goto done;
+	}
+
+	/*
+	 * This is a priority work request sent when there are current work
+	 * items in the queue. The head of the queue should always contain a
+	 * started work item, so we need to insert somewhere after that which
+	 * includes any other priority work items that may have been pushed.
+	 */
+	l_queue_insert(phy->work, work, insert_by_priority, &count);
+
+done:
+	/* Start work if we are sitting idle */
+	offchan_work_next(phy);
+
+	return work->id;
+}
+
+static bool match_id(const void *a, const void *b)
+{
+	const struct offchan_work *work = a;
+
+	return work->id == L_PTR_TO_UINT(b);
+}
+
+/*
+ * Signals that the work item is done. This can also be used to cancel any
+ * pending work item in the queue.
+ */
+void offchan_work_done(uint64_t wdev_id, uint32_t id)
+{
+	struct offchan_work *work;
+	struct offchan_phy *phy = find_phy(wdev_id);
+
+	if (!phy)
+		return;
+
+	work = l_queue_remove_if(phy->work, match_id, L_UINT_TO_PTR(id));
+	if (!work)
+		return;
+
+	l_debug("Offchannel work done %u", id);
+
+	destroy_work(work);
+
+	phy->idle = true;
+
+	offchan_work_next(phy);
+}
+
+void offchan_work_suspend(uint64_t wdev_id, uint32_t id)
+{
+	struct offchan_work *work;
+	struct offchan_phy *phy = find_phy(wdev_id);
+
+	if (!phy)
+		return;
+
+	work = l_queue_find(phy->work, match_id, L_UINT_TO_PTR(id));
+	if (!work)
+		return;
+
+
+	work->suspended = true;
+}
+
+void offchan_work_resume(uint64_t wdev_id, uint32_t id)
+{
+	struct offchan_work *work;
+	struct offchan_phy *phy = find_phy(wdev_id);
+
+	if (!phy)
+		return;
+
+	work = l_queue_find(phy->work, match_id, L_UINT_TO_PTR(id));
+	if (!work)
+		return;
+
+	/* If this item was pushed as SUSPENDED, take on NORMAL priority */
+	work->priority = work->priority == OFFCHAN_PRIORITY_SUSPENDED ?
+				OFFCHAN_PRIORITY_NORMAL : work->priority;
+	work->suspended = false;
+
+	offchan_work_next(phy);
+}
+
+void offchan_create_from_wiphy(struct wiphy *wiphy)
+{
+	struct offchan_phy *phy = l_new(struct offchan_phy, 1);
+
+	phy->wiphy = wiphy;
+	phy->work = l_queue_new();
+	phy->idle = true;
+
+	l_queue_push_head(phys, phy);
+}
+
+static void destroy_offchan_phy(void *user_data)
+{
+	struct offchan_phy *phy = user_data;
+
+	l_queue_destroy(phy->work, destroy_work);
+
+	l_free(phy);
+}
+
+void offchan_destroy(struct wiphy *wiphy)
+{
+	struct offchan_phy *phy = l_queue_remove_if(phys, match_wiphy, wiphy);
+
+	if (!phy)
+		return;
+
+	destroy_offchan_phy(phy);
+}
+
+static int offchan_init(void)
+{
+	phys = l_queue_new();
+
+	return 0;
+}
+
+static void offchan_exit(void)
+{
+	l_queue_destroy(phys, destroy_offchan_phy);
+}
+
+IWD_MODULE(offchannel, offchan_init, offchan_exit);
diff --git a/src/offchannel.h b/src/offchannel.h
new file mode 100644
index 00000000..127a0b21
--- /dev/null
+++ b/src/offchannel.h
@@ -0,0 +1,70 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2020  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+/*
+ * Module specific function that does the actual offchannel work, whether this
+ * is scanning, sending offchannel frames, or listening for a message. This
+ * function should return a boolean indicating if the work is done (e.g.
+ * fire-and-forget frame). Most cases the work is not done by the time the
+ * function returns, and generally we need to wait for a frame to be received
+ * or for scan results to come in. In this case offchan_work_done() should be
+ * called asynchronously once the work has completed.
+ */
+#include <stdbool.h>
+#include <stdint.h>
+
+#include <ell/ell.h>
+
+struct wiphy;
+
+/*
+ * The priority value of a work item:
+ *
+ * SUSPENDED - The purpose of this is to allow pushing a work item into the
+ *             queue that is already in a suspended state. This item will not
+ *             be run until it is resumed via offchan_work_resume. Once resumed
+ *             the item will take a NORMAL priority.
+ * NORMAL -    Normal priority. Work item will be executed when it reaches the
+ *             top of the queue.
+ * HIGH -      High priority. Any work item pushed with this priority will be
+ *             inserted before any NORMAL/SUSPENDED priority items, but still
+ *             behind any other HIGH priority work items.
+ */
+enum offchan_work_priority {
+	OFFCHAN_PRIORITY_SUSPENDED,
+	OFFCHAN_PRIORITY_NORMAL,
+	OFFCHAN_PRIORITY_HIGH,
+};
+
+typedef bool (*offchan_work_func_t)(void *user_data);
+typedef void (*offchan_work_destroy_func_t)(void *user_data);
+
+uint32_t offchan_work_push(uint64_t wdev_id,
+				enum offchan_work_priority priority,
+				offchan_work_func_t func, void *user_data,
+				offchan_work_destroy_func_t destroy);
+void offchan_work_done(uint64_t wdev_id, uint32_t id);
+void offchan_work_suspend(uint64_t wdev_id, uint32_t id);
+void offchan_work_resume(uint64_t wdev_id, uint32_t id);
+
+void offchan_create_from_wiphy(struct wiphy *wiphy);
+void offchan_destroy(struct wiphy *wiphy);
-- 
2.21.1

[RFC 3/6] wiphy: integrate offchannel module

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

Tie wiphy creation/destruction to offchannel
---
 src/wiphy.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index aef39549..7e58a402 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -53,6 +53,7 @@
 #include "src/watchlist.h"
 #include "src/nl80211util.h"
 #include "src/nl80211cmd.h"
+#include "src/offchannel.h"
 
 #define EXT_CAP_LEN 10
 
@@ -1235,6 +1236,9 @@ void wiphy_create_complete(struct wiphy *wiphy)
 	wiphy_get_reg_domain(wiphy);
 
 	wiphy_print_basic_info(wiphy);
+
+	if (wiphy->offchannel_tx_ok)
+		offchan_create_from_wiphy(wiphy);
 }
 
 bool wiphy_destroy(struct wiphy *wiphy)
@@ -1247,6 +1251,9 @@ bool wiphy_destroy(struct wiphy *wiphy)
 	if (wiphy->registered)
 		l_dbus_unregister_object(dbus_get_bus(), wiphy_get_path(wiphy));
 
+	if (wiphy->offchannel_tx_ok)
+		offchan_destroy(wiphy);
+
 	wiphy_free(wiphy);
 	return true;
 }
-- 
2.21.1

[RFC 4/6] frame-xchg: refactor to work with offchannel module

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 10171 bytes --]

In order to first integrate frame-xchg some refactoring needed to
be done. First it is useful to allow queueing frames up rather than
requiring the module (p2p, anqp etc) to wait for the last frame to
finish. This can be aided by offchannel but frame-xchg needed some
refactoring as well.

First was getting rid of this fx pointer re-use. It looks like this
was done to save a bit of memory but things get pretty complex
needed to check if the pointer is stale or has been reset. Instead
of this we now just allocate a new pointer each frame-xchg. This
allows for the module to queue multiple requests as well as removes
the complexity of needed to check if the fx pointer is stale.

Next was adding the ability to track frame-xchgs by ID. If a module
can queue up multiple requests it also needs to be able to cancel
them individually vs per-wdev. This comes free with offchannel since
it returns an ID which can be given directly to the caller.

Then offchannel was simply piped in by adding the push/done APIs
as well as some minor modification to internal frame-xchg APIs for
destroy functionality.
---
 src/frame-xchg.c | 97 +++++++++++++++++++++---------------------------
 src/frame-xchg.h |  6 +--
 src/p2p.c        |  9 +++--
 3 files changed, 51 insertions(+), 61 deletions(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 1473e59e..dd60e080 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -41,6 +41,7 @@
 #include "src/nl80211util.h"
 #include "src/netdev.h"
 #include "src/frame-xchg.h"
+#include "src/offchannel.h"
 
 #ifndef SOL_NETLINK
 #define SOL_NETLINK 270
@@ -109,12 +110,11 @@ struct frame_xchg_data {
 	frame_xchg_destroy_func_t destroy;
 	void *user_data;
 	uint32_t group_id;
+	uint32_t offchan_id;
 	unsigned int retry_cnt;
 	unsigned int retry_interval;
 	unsigned int resp_timeout;
 	unsigned int retries_on_ack;
-	bool in_frame_cb : 1;
-	bool stale : 1;
 };
 
 struct frame_xchg_watch_data {
@@ -726,7 +726,7 @@ static bool frame_watch_remove_by_handler(uint64_t wdev_id, uint32_t group_id,
 					&handler_info) > 0;
 }
 
-static void frame_xchg_tx_retry(struct frame_xchg_data *fx);
+static bool frame_xchg_tx_retry(void *user_data);
 static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
 					const void *body, size_t body_len,
 					int rssi, void *user_data);
@@ -753,9 +753,6 @@ static void frame_xchg_wait_cancel(struct frame_xchg_data *fx)
 
 static void frame_xchg_reset(struct frame_xchg_data *fx)
 {
-	fx->in_frame_cb = false;
-	fx->stale = false;
-
 	frame_xchg_wait_cancel(fx);
 
 	if (fx->timeout)
@@ -789,7 +786,7 @@ static void frame_xchg_done(struct frame_xchg_data *fx, int err)
 	if (fx->cb)
 		fx->cb(err, fx->user_data);
 
-	frame_xchg_destroy(fx);
+	offchan_work_done(fx->wdev_id, fx->offchan_id);
 }
 
 static void frame_xchg_timeout_destroy(void *user_data)
@@ -918,12 +915,16 @@ error:
 	frame_xchg_done(fx, error);
 }
 
-static void frame_xchg_tx_retry(struct frame_xchg_data *fx)
+static bool frame_xchg_tx_retry(void *user_data)
 {
+	struct frame_xchg_data *fx = user_data;
 	struct l_genl_msg *msg;
 	uint32_t cmd_id;
 	uint32_t duration = fx->resp_timeout;
 
+	/* push fx now that we are ready to start offchannel work */
+	l_queue_push_tail(frame_xchgs, fx);
+
 	/*
 	 * TODO: in Station, AP, P2P-Client, GO or Ad-Hoc modes if we're
 	 * transmitting the frame on the BSS's operating channel we can skip
@@ -951,13 +952,15 @@ static void frame_xchg_tx_retry(struct frame_xchg_data *fx)
 		l_error("Error sending frame");
 		l_genl_msg_unref(msg);
 		frame_xchg_done(fx, -EIO);
-		return;
+		return true;
 	}
 
 	fx->tx_acked = false;
 	fx->have_cookie = false;
 	fx->early_status = false;
 	fx->retry_cnt++;
+
+	return false;
 }
 
 static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
@@ -999,20 +1002,10 @@ static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
 		if (!fx->tx_acked)
 			goto early_frame;
 
-		fx->in_frame_cb = true;
 		done = watch->cb(mpdu, body, body_len, rssi, fx->user_data);
 
-		/*
-		 * If the callback has started a new frame exchange it will
-		 * have reset and taken over the state variables and we need
-		 * to just exit without touching anything.
-		 */
-		if (!fx->in_frame_cb)
-			return true;
-
-		fx->in_frame_cb = false;
-
-		if (done || fx->stale) {
+		if (done) {
+			/* NULL callback here since the caller is done */
 			fx->cb = NULL;
 			frame_xchg_done(fx, 0);
 			return true;
@@ -1070,22 +1063,24 @@ static bool frame_xchg_match(const void *a, const void *b)
  * @resp_timeout was 0.  @frame is an iovec array terminated by an iovec
  * struct with NULL-iov_base.
  */
-void frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
+uint32_t frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
 			frame_xchg_cb_t cb, void *user_data,
 			frame_xchg_destroy_func_t destroy, ...)
 {
+	uint32_t id;
 	va_list args;
 
 	va_start(args, destroy);
-	frame_xchg_startv(wdev_id, frame, freq, retry_interval, resp_timeout,
+	id = frame_xchg_startv(wdev_id, frame, freq, retry_interval, resp_timeout,
 				retries_on_ack, group_id, cb, user_data,
 				destroy, args);
 	va_end(args);
+	return id;
 }
 
-void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
+uint32_t frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
 			frame_xchg_cb_t cb, void *user_data,
@@ -1103,31 +1098,13 @@ void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 	if (frame_len < sizeof(*mpdu)) {
 		l_error("Frame too short");
 		cb(-EMSGSIZE, user_data);
-		return;
+		return 0;
 	}
 
-	fx = l_queue_find(frame_xchgs, frame_xchg_match, &wdev_id);
+	fx = l_new(struct frame_xchg_data, 1);
 
-	if (fx) {
-		/*
-		 * If a frame callback calls us assume it's the end of
-		 * that earlier frame exchange and the start of a new one.
-		 */
-		if (fx->in_frame_cb)
-			frame_xchg_reset(fx);
-		else {
-			l_error("Frame exchange in progress");
-			cb(-EBUSY, user_data);
-			return;
-		}
-	} else {
-		fx = l_new(struct frame_xchg_data, 1);
-
-		if (!frame_xchgs)
-			frame_xchgs = l_queue_new();
-
-		l_queue_push_tail(frame_xchgs, fx);
-	}
+	if (!frame_xchgs)
+		frame_xchgs = l_queue_new();
 
 	fx->wdev_id = wdev_id;
 	fx->freq = freq;
@@ -1172,22 +1149,34 @@ void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 	}
 
 	fx->retry_cnt = 0;
-	frame_xchg_tx_retry(fx);
+
+	/*
+	 * TODO: Assume any offchannel frames are a high priority. This may
+	 * need to be re-examined in the future if other operations (e.g.
+	 * wait on channel) are introduced.
+	 */
+	fx->offchan_id = offchan_work_push(wdev_id, OFFCHAN_PRIORITY_HIGH,
+						frame_xchg_tx_retry,
+						fx, frame_xchg_destroy);
+	return fx->offchan_id;
 }
 
-void frame_xchg_stop(uint64_t wdev_id)
+static bool frame_xchg_match_id(const void *a, const void *b)
+{
+	const struct frame_xchg_data *fx = a;
+	const uint32_t *id = b;
+
+	return fx->offchan_id == *id;
+}
+
+void frame_xchg_stop(uint64_t wdev_id, uint32_t id)
 {
 	struct frame_xchg_data *fx =
-		l_queue_remove_if(frame_xchgs, frame_xchg_match, &wdev_id);
+		l_queue_remove_if(frame_xchgs, frame_xchg_match_id, &id);
 
 	if (!fx)
 		return;
 
-	if (fx->in_frame_cb) {
-		fx->stale = true;
-		return;
-	}
-
 	frame_xchg_reset(fx);
 	l_free(fx);
 }
diff --git a/src/frame-xchg.h b/src/frame-xchg.h
index 55a55f73..5bcb0ad0 100644
--- a/src/frame-xchg.h
+++ b/src/frame-xchg.h
@@ -43,14 +43,14 @@ bool frame_watch_add(uint64_t wdev_id, uint32_t group, uint16_t frame_type,
 bool frame_watch_group_remove(uint64_t wdev_id, uint32_t group);
 bool frame_watch_wdev_remove(uint64_t wdev_id);
 
-void frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
+uint32_t frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
 			frame_xchg_cb_t cb, void *user_data,
 			frame_xchg_destroy_func_t destroy, ...);
-void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
+uint32_t frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
 			frame_xchg_cb_t cb, void *user_data,
 			frame_xchg_destroy_func_t destroy, va_list resp_args);
-void frame_xchg_stop(uint64_t wdev_id);
+void frame_xchg_stop(uint64_t wdev_id, uint32_t id);
diff --git a/src/p2p.c b/src/p2p.c
index dc78b887..e46abe4f 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -67,6 +67,7 @@ struct p2p_device {
 	uint32_t start_stop_cmd_id;
 	l_dbus_property_complete_cb_t pending_complete;
 	struct l_dbus_message *pending_message;
+	uint32_t xchg_id;
 
 	uint8_t listen_country[3];
 	uint8_t listen_oper_class;
@@ -382,7 +383,7 @@ static void p2p_connection_reset(struct p2p_device *dev)
 	netdev_watch_remove(dev->conn_netdev_watch_id);
 
 	frame_watch_group_remove(dev->wdev_id, FRAME_GROUP_CONNECT);
-	frame_xchg_stop(dev->wdev_id);
+	frame_xchg_stop(dev->wdev_id, dev->xchg_id);
 
 	if (!dev->enabled || (dev->enabled && dev->start_stop_cmd_id)) {
 		/*
@@ -457,9 +458,9 @@ static void p2p_peer_frame_xchg(struct p2p_peer *peer, struct iovec *tx_body,
 		peer->bss->frequency;
 
 	va_start(args, cb);
-	frame_xchg_startv(dev->wdev_id, frame, freq, retry_interval,
-				resp_timeout, retries_on_ack, group_id,
-				cb, dev, args);
+	dev->xchg_id = frame_xchg_startv(dev->wdev_id, frame, freq,
+				retry_interval, resp_timeout, retries_on_ack,
+				group_id, cb, dev, NULL, args);
 	va_end(args);
 
 	l_free(frame);
-- 
2.21.1

[RFC 5/6] anqp: refactor to use frame-xchg

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 17607 bytes --]

The new frame-xchg module now handles a lot of what ANQP used to do. ANQP
now does not need to depend on nl80211/netdev for building and sending
frames. It also no longer needs any of the request lookups, frame watches
or to maintain a queue of requests because frame-xchg filters this for us.

From an API perspective:
 - anqp_request() was changed to take the wdev_id rather than ifindex.
 - anqp_cancel() was added so that station can properly clean up ANQP
   requests if the device disappears.

During testing a bug was also fixed in station on the timeout path
where the request queue would get popped twice.
---
 src/anqp.c    | 443 +++++++++++---------------------------------------
 src/anqp.h    |   3 +-
 src/station.c |  20 +--
 3 files changed, 104 insertions(+), 362 deletions(-)

diff --git a/src/anqp.c b/src/anqp.c
index 6627bd81..36d5a04b 100644
--- a/src/anqp.c
+++ b/src/anqp.c
@@ -31,105 +31,61 @@
 #include "src/module.h"
 #include "src/anqp.h"
 #include "src/util.h"
-#include "src/eap-private.h"
 #include "src/ie.h"
-#include "src/nl80211util.h"
-#include "src/nl80211cmd.h"
 #include "src/scan.h"
-#include "src/netdev.h"
 #include "src/iwd.h"
 #include "src/mpdu.h"
-#include "src/wiphy.h"
+#include "src/frame-xchg.h"
 
 #include "linux/nl80211.h"
 
+#define ANQP_GROUP	0
+
 struct anqp_request {
-	uint32_t ifindex;
+	uint64_t wdev_id;
 	anqp_response_func_t anqp_cb;
 	anqp_destroy_func_t anqp_destroy;
 	void *anqp_data;
-	uint64_t anqp_cookie;
 	uint8_t anqp_token;
+	uint32_t frequency;
+	uint8_t *frame;
+	size_t frame_len;
+	uint32_t id;
 };
 
-static struct l_genl_family *nl80211 = NULL;
-
-static struct l_queue *anqp_requests;
 static uint8_t anqp_token = 0;
 
-static uint32_t netdev_watch;
-static uint32_t unicast_watch;
-
 static void anqp_destroy(void *user_data)
 {
 	struct anqp_request *request = user_data;
 
 	if (request->anqp_destroy)
 		request->anqp_destroy(request->anqp_data);
-}
-
-static void netdev_gas_request_cb(struct l_genl_msg *msg, void *user_data)
-{
-	struct anqp_request *request = user_data;
-
-	if (l_genl_msg_get_error(msg) != 0)
-		goto error;
-
-	if (nl80211_parse_attrs(msg, NL80211_ATTR_COOKIE, &request->anqp_cookie,
-					NL80211_ATTR_UNSPEC) < 0)
-		goto error;
-
-	return;
-
-error:
-	l_debug("Error sending CMD_FRAME (%d)", l_genl_msg_get_error(msg));
-
-	if (request->anqp_cb)
-		request->anqp_cb(ANQP_FAILED, NULL, 0, request->anqp_data);
-
-	if (request->anqp_destroy)
-		request->anqp_destroy(request->anqp_data);
 
+	l_free(request->frame);
 	l_free(request);
 }
 
-static bool match_token(const void *a, const void *b)
-{
-	const struct anqp_request *request = a;
-	const struct token_match {
-		uint32_t ifindex;
-		uint8_t token;
-
-	} *match = b;
-
-	if (request->ifindex != match->ifindex)
-		return false;
-
-	if (request->anqp_token != match->token)
-		return false;
-
-	return true;
-}
-
-static void anqp_response_frame_event(uint32_t ifindex,
-					const struct mmpdu_header *hdr,
-					const void *body, size_t body_len)
+/*
+ * By using frame-xchg we should get called back here for any frame matching our
+ * prefix until the duration expires. If frame-xchg is never signalled 'done'
+ * (returning true) we should get a timeout in anqp_frame_timeout. This is
+ * why we drop any improperly formatted frames without cleaning up the request.
+ */
+static bool anqp_response_frame_event(const struct mmpdu_header *hdr,
+					const void *body, size_t body_len,
+					int rssi, void *user_data)
 {
-	struct anqp_request *request;
+	struct anqp_request *request = user_data;
 	const uint8_t *ptr = body;
 	uint16_t status_code;
 	uint16_t delay;
 	uint16_t qrlen;
 	uint8_t adv_proto_len;
 	uint8_t token;
-	struct token_match {
-		uint32_t ifindex;
-		uint8_t token;
-
-	} match;
 
 	if (body_len < 9)
-		return;
+		return false;
 
 	/* Skip past category/action since this frame was prefixed matched */
 	ptr += 2;
@@ -138,12 +94,8 @@ static void anqp_response_frame_event(uint32_t ifindex,
 	/* dialog token */
 	token = *ptr++;
 
-	match.ifindex = ifindex;
-	match.token = token;
-
-	request = l_queue_find(anqp_requests, match_token, &match);
-	if (!request)
-		return;
+	if (request->anqp_token != token)
+		return false;
 
 	status_code = l_get_le16(ptr);
 	ptr += 2;
@@ -151,7 +103,7 @@ static void anqp_response_frame_event(uint32_t ifindex,
 
 	if (status_code != 0) {
 		l_error("Bad status code on GAS response %u", status_code);
-		return;
+		return false;
 	}
 
 	delay = l_get_le16(ptr);
@@ -166,12 +118,12 @@ static void anqp_response_frame_event(uint32_t ifindex,
 	 */
 	if (delay != 0) {
 		l_error("GAS comeback delay was not zero");
-		return;
+		return false;
 	}
 
 	if (*ptr != IE_TYPE_ADVERTISEMENT_PROTOCOL) {
 		l_error("GAS request not advertisement protocol");
-		return;
+		return false;
 	}
 
 	ptr++;
@@ -181,339 +133,128 @@ static void anqp_response_frame_event(uint32_t ifindex,
 	body_len--;
 
 	if (body_len < adv_proto_len)
-		return;
+		return false;
 
 	ptr += adv_proto_len;
 	body_len -= adv_proto_len;
 
 	if (body_len < 2)
-		return;
+		return false;
 
 	qrlen = l_get_le16(ptr);
 	ptr += 2;
 
 	if (body_len < qrlen)
-		return;
-
-	l_queue_remove(anqp_requests, request);
+		return false;
 
 	l_debug("ANQP response received from "MAC, MAC_STR(hdr->address_2));
 
 	if (request->anqp_cb)
-		request->anqp_cb(ANQP_SUCCESS, ptr, qrlen,
-					request->anqp_data);
-
-	if (request->anqp_destroy)
-		request->anqp_destroy(request->anqp_data);
+		request->anqp_cb(ANQP_SUCCESS, ptr, qrlen, request->anqp_data);
 
-	l_free(request);
+	anqp_destroy(request);
 
-	return;
+	return true;
 }
 
-static void netdev_gas_timeout_cb(void *user_data)
+static const struct frame_xchg_prefix anqp_frame_prefix = {
+	.data = (uint8_t []) {
+		0x04, 0x0b,
+	},
+	.len = 2,
+};
+
+static void anqp_frame_timeout(int error, void *user_data)
 {
 	struct anqp_request *request = user_data;
+	enum anqp_result result = ANQP_TIMEOUT;
 
-	l_debug("GAS request timed out");
+	if (error < 0) {
+		result = ANQP_FAILED;
+		l_error("Sending ANQP request failed: %s (%i)",
+			strerror(-error), -error);
+	}
 
 	if (request->anqp_cb)
-		request->anqp_cb(ANQP_TIMEOUT, NULL, 0,
-					request->anqp_data);
+		request->anqp_cb(result, NULL, 0, request->anqp_data);
 
-	/* allows anqp_request to be re-entrant */
 	if (request->anqp_destroy)
 		request->anqp_destroy(request->anqp_data);
 
-	l_queue_remove(anqp_requests, request);
-	l_free(request);
+	anqp_destroy(request);
 }
 
-static bool match_cookie(const void *a, const void *b)
+static uint8_t *anqp_build_frame(const uint8_t *addr, struct scan_bss *bss,
+					const uint8_t *anqp, size_t len,
+					size_t *len_out)
 {
-	const struct anqp_request *request = a;
-	const struct cookie_match {
-		uint64_t cookie;
-		uint32_t ifindex;
-	} *match = b;
+	uint8_t *frame = l_malloc(len + 33);
+	uint8_t *ptr;
 
-	if (match->ifindex != request->ifindex)
-		return false;
+	memset(frame, 0, len + 33);
 
-	if (match->cookie != request->anqp_cookie)
-		return false;
+	l_put_le16(0x00d0, frame + 0);
+	memcpy(frame + 4, bss->addr, 6);
+	memcpy(frame + 10, addr, 6);
+	memcpy(frame + 16, bss->addr, 6);
 
-	return true;
-}
+	ptr = frame + 24;
 
-static void anqp_frame_wait_cancel_event(struct l_genl_msg *msg,
-						uint32_t ifindex)
-{
-	uint64_t cookie;
-	struct anqp_request *request;
-	struct cookie_match {
-		uint64_t cookie;
-		uint32_t ifindex;
-	} match;
+	*ptr++ = 0x04;			/* Category: Public */
+	*ptr++ = 0x0a;			/* Action: GAS initial Request */
+	*ptr++ = anqp_token++;		/* Dialog Token */
+	*ptr++ = IE_TYPE_ADVERTISEMENT_PROTOCOL;
+	*ptr++ = 2;
 
-	l_debug("");
-
-	if (nl80211_parse_attrs(msg, NL80211_ATTR_COOKIE, &cookie,
-					NL80211_ATTR_UNSPEC) < 0)
-		return;
-
-	match.cookie = cookie;
-	match.ifindex = ifindex;
+	*ptr++ = 0x7f;
+	*ptr++ = IE_ADVERTISEMENT_ANQP;
+	l_put_le16(len, ptr);
+	ptr += 2;
 
-	request = l_queue_find(anqp_requests, match_cookie, &match);
-	if (!request)
-		return;
+	memcpy(ptr, anqp, len);
+	ptr += len;
 
-	if (cookie != request->anqp_cookie)
-		return;
+	*len_out = ptr - frame;
 
-	netdev_gas_timeout_cb(request);
+	return frame;
 }
 
-uint32_t anqp_request(uint32_t ifindex, const uint8_t *addr,
-				struct scan_bss *bss, const uint8_t *anqp,
-				size_t len, anqp_response_func_t cb,
-				void *user_data, anqp_destroy_func_t destroy)
+uint32_t anqp_request(uint64_t wdev_id, const uint8_t *addr,
+			struct scan_bss *bss, const uint8_t *anqp,
+			size_t len, anqp_response_func_t cb,
+			void *user_data, anqp_destroy_func_t destroy)
 {
 	struct anqp_request *request;
-	uint8_t frame[512];
-	struct l_genl_msg *msg;
 	struct iovec iov[2];
-	uint32_t id;
-	uint32_t duration = 300;
-	struct netdev *netdev = netdev_find(ifindex);
-
-	if (!netdev)
-		return 0;
-
-	/*
-	 * TODO: Netdev dependencies will eventually be removed so we need
-	 * another way to figure out wiphy capabilities.
-	 */
-	if (!wiphy_can_offchannel_tx(netdev_get_wiphy(netdev))) {
-		l_error("ANQP failed, driver does not support offchannel TX");
-		return 0;
-	}
-
-	frame[0] = 0x04;		/* Category: Public */
-	frame[1] = 0x0a;		/* Action: GAS initial Request */
-	frame[2] = anqp_token;		/* Dialog Token */
-	frame[3] = IE_TYPE_ADVERTISEMENT_PROTOCOL;
-	frame[4] = 2;
-	frame[5] = 0x7f;
-	frame[6] = IE_ADVERTISEMENT_ANQP;
-	l_put_le16(len, frame + 7);
-
-	iov[0].iov_base = frame;
-	iov[0].iov_len = 9;
-	iov[1].iov_base = (void *)anqp;
-	iov[1].iov_len = len;
 
 	request = l_new(struct anqp_request, 1);
 
-	request->ifindex = ifindex;
+	request->wdev_id = wdev_id;
+	request->frequency = bss->frequency;
 	request->anqp_cb = cb;
 	request->anqp_destroy = destroy;
-	request->anqp_token = anqp_token++;
+	request->anqp_token = anqp_token;
 	request->anqp_data = user_data;
 
-	msg = nl80211_build_cmd_frame(ifindex, addr, bss->addr,
-					bss->frequency, iov, 2);
-
-	l_genl_msg_append_attr(msg, NL80211_ATTR_OFFCHANNEL_TX_OK, 0, "");
-	l_genl_msg_append_attr(msg, NL80211_ATTR_DURATION, 4, &duration);
-
-	id = l_genl_family_send(nl80211, msg, netdev_gas_request_cb,
-					request, NULL);
-
-	if (!id) {
-		l_debug("Failed to send ANQP request");
-		l_genl_msg_unref(msg);
-		l_free(request);
-		return 0;
-	}
-
-	l_debug("ANQP request sent to "MAC, MAC_STR(bss->addr));
-
-	l_queue_push_head(anqp_requests, request);
-
-	return id;
-}
-
-static void netdev_frame_cb(struct l_genl_msg *msg, void *user_data)
-{
-	if (l_genl_msg_get_error(msg) < 0)
-		l_error("Could not register frame watch type %04x: %i",
-			L_PTR_TO_UINT(user_data), l_genl_msg_get_error(msg));
-}
-
-static void anqp_register_frame(uint32_t ifindex)
-{
-	struct l_genl_msg *msg;
-	uint16_t frame_type = 0x00d0;
-	uint8_t prefix[] = { 0x04, 0x0b };
-
-	msg = l_genl_msg_new_sized(NL80211_CMD_REGISTER_FRAME, 34);
-
-	l_genl_msg_append_attr(msg, NL80211_ATTR_IFINDEX, 4, &ifindex);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_FRAME_TYPE, 2, &frame_type);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_FRAME_MATCH,
-					sizeof(prefix), prefix);
-
-	l_genl_family_send(nl80211, msg, netdev_frame_cb,
-			L_UINT_TO_PTR(frame_type), NULL);
-}
-
-static void anqp_netdev_watch(struct netdev *netdev,
-				enum netdev_watch_event event, void *user_data)
-{
-	switch (event) {
-	case NETDEV_WATCH_EVENT_NEW:
-		if (netdev_get_iftype(netdev) == NETDEV_IFTYPE_STATION)
-			anqp_register_frame(netdev_get_ifindex(netdev));
-
-		return;
-	default:
-		break;
-	}
-}
-
-static void anqp_unicast_notify(struct l_genl_msg *msg, void *user_data)
-{
-	const struct mmpdu_header *mpdu = NULL;
-	const uint8_t *body;
-	struct l_genl_attr attr;
-	uint16_t type, len;
-	uint16_t frame_len = 0;
-	const void *data;
-	uint8_t cmd;
-	uint32_t ifindex = 0;
-
-	if (l_queue_isempty(anqp_requests))
-		return;
-
-	cmd = l_genl_msg_get_command(msg);
-	if (!cmd)
-		return;
-
-	if (!l_genl_attr_init(&attr, msg))
-		return;
-
-	while (l_genl_attr_next(&attr, &type, &len, &data)) {
-		switch (type) {
-		case NL80211_ATTR_IFINDEX:
-			if (len != sizeof(uint32_t)) {
-				l_warn("Invalid interface index attribute");
-				return;
-			}
-
-			ifindex = *((uint32_t *) data);
-
-			break;
-		case NL80211_ATTR_FRAME:
-			if (mpdu)
-				return;
-
-			mpdu = mpdu_validate(data, len);
-			if (!mpdu)
-				l_error("Frame didn't validate as MMPDU");
-
-			frame_len = len;
-			break;
-		}
-	}
-
-	if (!ifindex || !mpdu)
-		return;
-
-	body = mmpdu_body(mpdu);
-
-	anqp_response_frame_event(ifindex, mpdu, body,
-				(const uint8_t *) mpdu + frame_len - body);
-}
-
-static void anqp_mlme_notify(struct l_genl_msg *msg, void *user_data)
-{
-	struct l_genl_attr attr;
-	uint16_t type, len;
-	const void *data;
-	uint8_t cmd;
-	uint32_t ifindex = 0;
-
-	if (l_queue_isempty(anqp_requests))
-		return;
-
-	cmd = l_genl_msg_get_command(msg);
-
-	l_debug("MLME notification %s(%u)", nl80211cmd_to_string(cmd), cmd);
-
-	if (!l_genl_attr_init(&attr, msg))
-		return;
-
-	while (l_genl_attr_next(&attr, &type, &len, &data)) {
-		switch (type) {
-		case NL80211_ATTR_IFINDEX:
-			if (len != sizeof(uint32_t)) {
-				l_warn("Invalid interface index attribute");
-				return;
-			}
-
-			ifindex = *((uint32_t *) data);
-			break;
-		}
-	}
+	request->frame = anqp_build_frame(addr, bss, anqp, len,
+						&request->frame_len);
 
-	if (!ifindex) {
-		l_warn("MLME notification is missing ifindex attribute");
-		return;
-	}
-
-	switch (cmd) {
-	case NL80211_CMD_FRAME_WAIT_CANCEL:
-		anqp_frame_wait_cancel_event(msg, ifindex);
-		break;
-	}
-}
-
-static int anqp_init(void)
-{
-	struct l_genl *genl = iwd_get_genl();
-
-	nl80211 = l_genl_family_new(genl, NL80211_GENL_NAME);
-
-	anqp_requests = l_queue_new();
+	iov[0].iov_base = request->frame;
+	iov[0].iov_len = request->frame_len;
+	iov[1].iov_base = NULL;
 
-	netdev_watch =  netdev_watch_add(anqp_netdev_watch, NULL, NULL);
+	l_debug("Sending ANQP request");
 
-	unicast_watch = l_genl_add_unicast_watch(genl, NL80211_GENL_NAME,
-						anqp_unicast_notify,
-						NULL, NULL);
+	request->id = frame_xchg_start(request->wdev_id, iov,
+				request->frequency, 0, 300, 0,
+				ANQP_GROUP, anqp_frame_timeout, request, NULL,
+				&anqp_frame_prefix, anqp_response_frame_event,
+				NULL);
 
-	if (!l_genl_family_register(nl80211, "mlme", anqp_mlme_notify,
-								NULL, NULL))
-		l_error("Registering for MLME notification failed");
-
-	return 0;
+	return true;
 }
 
-static void anqp_exit(void)
+void anqp_cancel(uint64_t wdev_id, uint32_t id)
 {
-	struct l_genl *genl = iwd_get_genl();
-
-	l_genl_family_free(nl80211);
-	nl80211 = NULL;
-
-	l_queue_destroy(anqp_requests, anqp_destroy);
-
-	netdev_watch_remove(netdev_watch);
-
-	l_genl_remove_unicast_watch(genl, unicast_watch);
+	frame_xchg_stop(wdev_id, id);
 }
-
-IWD_MODULE(anqp, anqp_init, anqp_exit);
-IWD_MODULE_DEPENDS(anqp, netdev);
diff --git a/src/anqp.h b/src/anqp.h
index 998277dd..6b97efef 100644
--- a/src/anqp.h
+++ b/src/anqp.h
@@ -34,7 +34,8 @@ typedef void (*anqp_response_func_t)(enum anqp_result result,
 					const void *anqp, size_t len,
 					void *user_data);
 
-uint32_t anqp_request(uint32_t ifindex, const uint8_t *addr,
+uint32_t anqp_request(uint64_t wdev_id, const uint8_t *addr,
 			struct scan_bss *bss, const uint8_t *anqp, size_t len,
 			anqp_response_func_t cb, void *user_data,
 			anqp_destroy_func_t destroy);
+void anqp_cancel(uint64_t wdev_id, uint32_t id);
diff --git a/src/station.c b/src/station.c
index 80b6f07e..8b252b53 100644
--- a/src/station.c
+++ b/src/station.c
@@ -445,6 +445,10 @@ static bool match_pending(const void *a, const void *b)
 static void remove_anqp(void *data)
 {
 	struct anqp_entry *entry = data;
+	uint64_t wdev_id = netdev_get_wdev_id(entry->station->netdev);
+
+	if (entry->pending)
+		anqp_cancel(wdev_id, entry->pending);
 
 	l_free(entry);
 }
@@ -475,12 +479,9 @@ static void station_anqp_response_cb(enum anqp_result result,
 	char **realms = NULL;
 	struct nai_search search;
 
-	entry->pending = 0;
-
 	l_debug("");
 
-	if (result == ANQP_TIMEOUT) {
-		l_queue_remove(station->anqp_pending, entry);
+	if (result != ANQP_SUCCESS) {
 		/* TODO: try next BSS */
 		goto request_done;
 	}
@@ -582,11 +583,10 @@ static bool station_start_anqp(struct station *station, struct network *network,
 	 * these are checked in hs20_find_settings_file.
 	 */
 
-	entry->pending = anqp_request(netdev_get_ifindex(station->netdev),
-					netdev_get_address(station->netdev),
-					bss, anqp, ptr - anqp,
-					station_anqp_response_cb,
-					entry, l_free);
+	entry->pending = anqp_request(netdev_get_wdev_id(station->netdev),
+				netdev_get_address(station->netdev), bss, anqp,
+				ptr - anqp, station_anqp_response_cb,
+				entry, l_free);
 	if (!entry->pending) {
 		l_free(entry);
 		return false;
@@ -3215,7 +3215,7 @@ static void station_free(struct station *station)
 
 	watchlist_destroy(&station->state_watches);
 
-	l_queue_destroy(station->anqp_pending, l_free);
+	l_queue_destroy(station->anqp_pending, remove_anqp);
 
 	l_free(station);
 }
-- 
2.21.1

[RFC 6/6] scan: refactor to use offchannel module

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 7960 bytes --]

To use offchannel scanning mostly remained the same.
start_next_scan_request was modified to be used as the offchannel work
callback, as well as not start the next scan if the current one was
done (since this is taken care of by offchannel now). All calls to
start_next_scan_request were removed, and more or less replaced with
offchan_work_done.

Since scan_suspend is per wdev we must iterate all the requests
and suspend each using offchan_work_suspend. And the opposite is
done for resuming. A small feature was added to scan_suspend/resume
(ref counting) to prevent a single call to scan_resume from resuming
if other modules had previously suspended. This way scanning doesnt
get resumed out from under a module from somewhere else.

At the moment nothing else besides ANQP suspends scanning, but this may
change. Station may want to suspend scanning while connecting because
it could cause delays connection. A carefully timed scan could end up
being started after CMD_CONNECT but before eapol. This could be handled
by simply returning Busy, but because the scan could take place after
connecting we might as well honor the users request, just delayed until
after connecting.
---
 src/scan.c | 114 +++++++++++++++++++++++++++++------------------------
 1 file changed, 63 insertions(+), 51 deletions(-)

diff --git a/src/scan.c b/src/scan.c
index 83804e57..d13d4223 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -49,6 +49,7 @@
 #include "src/p2putil.h"
 #include "src/mpdu.h"
 #include "src/scan.h"
+#include "src/offchannel.h"
 
 #define SCAN_MAX_INTERVAL 320
 #define SCAN_INIT_INTERVAL 10
@@ -56,7 +57,8 @@
 static struct l_queue *scan_contexts;
 
 static struct l_genl_family *nl80211;
-static uint32_t next_scan_request_id;
+
+struct scan_context;
 
 struct scan_periodic {
 	struct l_timeout *timeout;
@@ -70,6 +72,7 @@ struct scan_periodic {
 };
 
 struct scan_request {
+	struct scan_context *sc;
 	uint32_t id;
 	scan_trigger_func_t trigger;
 	scan_notify_func_t callback;
@@ -106,7 +109,7 @@ struct scan_context {
 	bool triggered:1;
 	/* Whether any commands from current request's queue have started */
 	bool started:1;
-	bool suspended:1;
+	int32_t suspended;
 	struct wiphy *wiphy;
 };
 
@@ -118,7 +121,7 @@ struct scan_results {
 	struct scan_request *sr;
 };
 
-static bool start_next_scan_request(struct scan_context *sc);
+static bool start_next_scan_request(void *user_data);
 static void scan_periodic_rearm(struct scan_context *sc);
 
 static bool scan_context_match(const void *a, const void *b)
@@ -159,7 +162,7 @@ static void scan_request_failed(struct scan_context *sc,
 	else if (sr->callback)
 		sr->callback(err, NULL, sr->userdata);
 
-	scan_request_free(sr);
+	offchan_work_done(sc->wdev_id, sr->id);
 }
 
 static struct scan_context *scan_context_new(uint64_t wdev_id)
@@ -180,11 +183,18 @@ static struct scan_context *scan_context_new(uint64_t wdev_id)
 	return sc;
 }
 
+static void scan_request_cancel(void *data)
+{
+	struct scan_request *sr = data;
+
+	offchan_work_done(sr->sc->wdev_id, sr->id);
+}
+
 static void scan_context_free(struct scan_context *sc)
 {
 	l_debug("sc: %p", sc);
 
-	l_queue_destroy(sc->requests, scan_request_free);
+	l_queue_destroy(sc->requests, scan_request_cancel);
 
 	if (sc->sp.timeout)
 		l_timeout_remove(sc->sp.timeout);
@@ -215,7 +225,6 @@ static void scan_request_triggered(struct l_genl_msg *msg, void *userdata)
 		}
 
 		l_queue_remove(sc->requests, sr);
-		start_next_scan_request(sc);
 
 		scan_request_failed(sc, sr, err);
 
@@ -532,35 +541,24 @@ static uint32_t scan_common(uint64_t wdev_id, bool passive,
 		return 0;
 
 	sr = l_new(struct scan_request, 1);
+	sr->sc = sc;
 	sr->trigger = trigger;
 	sr->callback = notify;
 	sr->userdata = userdata;
 	sr->destroy = destroy;
 	sr->passive = passive;
-	sr->id = ++next_scan_request_id;
 	sr->cmds = l_queue_new();
 
 	scan_cmds_add(sr->cmds, sc, passive, params);
 
-	/* Queue empty implies !sc->triggered && !sc->start_cmd_id */
-	if (!l_queue_isempty(sc->requests))
-		goto done;
-
-	if (sc->suspended)
-		goto done;
-
-	if (sc->state != SCAN_STATE_NOT_RUNNING)
-		goto done;
-
-	if (!scan_request_send_trigger(sc, sr))
-		goto done;
-
-	sr->destroy = NULL;	/* Don't call destroy when returning error */
-	scan_request_free(sr);
-	return 0;
-done:
 	l_queue_push_tail(sc->requests, sr);
 
+	sr->id = offchan_work_push(wdev_id, sc->suspended > 0 ?
+					OFFCHAN_PRIORITY_SUSPENDED :
+					OFFCHAN_PRIORITY_NORMAL,
+					start_next_scan_request, sr,
+					scan_request_free);
+
 	return sr->id;
 }
 
@@ -633,6 +631,8 @@ bool scan_cancel(uint64_t wdev_id, uint32_t id)
 			sr->destroy = NULL;
 		}
 
+		offchan_work_done(wdev_id, sr->id);
+
 		return true;
 	}
 
@@ -649,11 +649,11 @@ bool scan_cancel(uint64_t wdev_id, uint32_t id)
 		sc->start_cmd_id = 0;
 		l_queue_remove(sc->requests, sr);
 		sc->started = false;
-		start_next_scan_request(sc);
 	} else
 		l_queue_remove(sc->requests, sr);
 
-	scan_request_free(sr);
+	offchan_work_done(wdev_id, sr->id);
+
 	return true;
 }
 
@@ -832,34 +832,22 @@ static void scan_periodic_rearm(struct scan_context *sc)
 						scan_periodic_timeout_destroy);
 }
 
-static bool start_next_scan_request(struct scan_context *sc)
+static bool start_next_scan_request(void *user_data)
 {
-	struct scan_request *sr = l_queue_peek_head(sc->requests);
+	struct scan_request *sr = user_data;
+	struct scan_context *sc = sr->sc;
 
-	if (sc->suspended)
-		return true;
-
-	if (sc->state != SCAN_STATE_NOT_RUNNING)
-		return true;
-
-	if (sc->start_cmd_id || sc->get_scan_cmd_id)
-		return true;
-
-	while (sr) {
-		if (!scan_request_send_trigger(sc, sr))
-			return true;
-
-		scan_request_failed(sc, sr, -EIO);
+	if (!scan_request_send_trigger(sc, sr))
+		return false;
 
-		sr = l_queue_peek_head(sc->requests);
-	}
+	scan_request_failed(sc, sr, -EIO);
 
 	if (sc->sp.retry) {
 		sc->sp.retry = false;
 		scan_periodic_queue(sc);
 	}
 
-	return false;
+	return true;
 }
 
 static bool scan_parse_vendor_specific(struct scan_bss *bss, const void *data,
@@ -1511,9 +1499,7 @@ static void scan_finished(struct scan_context *sc,
 		 * taken care of sending the next command for a new or ongoing
 		 * scan, or scheduling the next periodic scan.
 		 */
-		start_next_scan_request(sc);
-
-		scan_request_free(sr);
+		offchan_work_done(sc->wdev_id, sr->id);
 	} else if (sc->sp.callback)
 		new_owner = sc->sp.callback(err, bss_list, sc->sp.userdata);
 
@@ -2133,6 +2119,26 @@ bool scan_wdev_remove(uint64_t wdev_id)
 	return true;
 }
 
+static void scan_request_suspend(void *data, void *user_data)
+{
+	struct scan_request *sr = data;
+	uint64_t *wdev_id = user_data;
+
+	offchan_work_suspend(*wdev_id, sr->id);
+}
+
+static void scan_request_resume(void *data, void *user_data)
+{
+	struct scan_request *sr = data;
+	uint64_t *wdev_id = user_data;
+
+	offchan_work_resume(*wdev_id, sr->id);
+}
+
+/*
+ * This will suspend any work items in offchannel, but leave the request
+ * queue untouched so all pending scans can be resumed later.
+ */
 bool scan_suspend(uint64_t wdev_id)
 {
 	struct scan_context *sc;
@@ -2141,7 +2147,11 @@ bool scan_suspend(uint64_t wdev_id)
 	if (!sc)
 		return false;
 
-	sc->suspended = true;
+	/* only suspend requests once */
+	if (sc->suspended == 0)
+		l_queue_foreach(sc->requests, scan_request_suspend, &wdev_id);
+
+	sc->suspended++;
 
 	return true;
 }
@@ -2154,9 +2164,11 @@ void scan_resume(uint64_t wdev_id)
 	if (!sc)
 		return;
 
-	sc->suspended = false;
+	if (sc->suspended > 0)
+		sc->suspended--;
 
-	start_next_scan_request(sc);
+	if (sc->suspended == 0)
+		l_queue_foreach(sc->requests, scan_request_resume, &wdev_id);
 }
 
 static int scan_init(void)
-- 
2.21.1

Re: [RFC 3/6] wiphy: integrate offchannel module

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

Hi James,

On 6/17/20 12:56 PM, James Prestwood wrote:
> Tie wiphy creation/destruction to offchannel
> ---
>   src/wiphy.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/src/wiphy.c b/src/wiphy.c
> index aef39549..7e58a402 100644
> --- a/src/wiphy.c
> +++ b/src/wiphy.c
> @@ -53,6 +53,7 @@
>   #include "src/watchlist.h"
>   #include "src/nl80211util.h"
>   #include "src/nl80211cmd.h"
> +#include "src/offchannel.h"
>   
>   #define EXT_CAP_LEN 10
>   
> @@ -1235,6 +1236,9 @@ void wiphy_create_complete(struct wiphy *wiphy)
>   	wiphy_get_reg_domain(wiphy);
>   
>   	wiphy_print_basic_info(wiphy);
> +
> +	if (wiphy->offchannel_tx_ok)
> +		offchan_create_from_wiphy(wiphy);

Hmm, why? Isn't scanning always supported and hence you always want 
offchan_create to be called?

>   }
>   
>   bool wiphy_destroy(struct wiphy *wiphy)

Regards,
-Denis

Re: [RFC 4/6] frame-xchg: refactor to work with offchannel module

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

Hi James,

On 6/17/20 12:56 PM, James Prestwood wrote:
> In order to first integrate frame-xchg some refactoring needed to
> be done. First it is useful to allow queueing frames up rather than
> requiring the module (p2p, anqp etc) to wait for the last frame to
> finish. This can be aided by offchannel but frame-xchg needed some
> refactoring as well.
> 
> First was getting rid of this fx pointer re-use. It looks like this
> was done to save a bit of memory but things get pretty complex
> needed to check if the pointer is stale or has been reset. Instead
> of this we now just allocate a new pointer each frame-xchg. This
> allows for the module to queue multiple requests as well as removes
> the complexity of needed to check if the fx pointer is stale.
> 
> Next was adding the ability to track frame-xchgs by ID. If a module
> can queue up multiple requests it also needs to be able to cancel
> them individually vs per-wdev. This comes free with offchannel since
> it returns an ID which can be given directly to the caller.
> 
> Then offchannel was simply piped in by adding the push/done APIs
> as well as some minor modification to internal frame-xchg APIs for
> destroy functionality.
> ---
>   src/frame-xchg.c | 97 +++++++++++++++++++++---------------------------
>   src/frame-xchg.h |  6 +--
>   src/p2p.c        |  9 +++--
>   3 files changed, 51 insertions(+), 61 deletions(-)
> 

<snip>

> -void frame_xchg_stop(uint64_t wdev_id);
> +void frame_xchg_stop(uint64_t wdev_id, uint32_t id);

Do we even need the wdev_id here now? Also, should this be _cancel?

Regards,
-Denis

Re: [RFC 3/6] wiphy: integrate offchannel module

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On Mon, 2020-06-22 at 14:44 -0500, Denis Kenzior wrote:
> Hi James,
> 
> On 6/17/20 12:56 PM, James Prestwood wrote:
> > Tie wiphy creation/destruction to offchannel
> > ---
> >   src/wiphy.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/wiphy.c b/src/wiphy.c
> > index aef39549..7e58a402 100644
> > --- a/src/wiphy.c
> > +++ b/src/wiphy.c
> > @@ -53,6 +53,7 @@
> >   #include "src/watchlist.h"
> >   #include "src/nl80211util.h"
> >   #include "src/nl80211cmd.h"
> > +#include "src/offchannel.h"
> >   
> >   #define EXT_CAP_LEN 10
> >   
> > @@ -1235,6 +1236,9 @@ void wiphy_create_complete(struct wiphy
> > *wiphy)
> >   	wiphy_get_reg_domain(wiphy);
> >   
> >   	wiphy_print_basic_info(wiphy);
> > +
> > +	if (wiphy->offchannel_tx_ok)
> > +		offchan_create_from_wiphy(wiphy);
> 
> Hmm, why? Isn't scanning always supported and hence you always want 
> offchan_create to be called?

Yeah, that was an oversight.

> 
> >   }
> >   
> >   bool wiphy_destroy(struct wiphy *wiphy)
> 
> Regards,
> -Denis

Re: [RFC 4/6] frame-xchg: refactor to work with offchannel module

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]

On Mon, 2020-06-22 at 14:55 -0500, Denis Kenzior wrote:
> Hi James,
> 
> On 6/17/20 12:56 PM, James Prestwood wrote:
> > In order to first integrate frame-xchg some refactoring needed to
> > be done. First it is useful to allow queueing frames up rather than
> > requiring the module (p2p, anqp etc) to wait for the last frame to
> > finish. This can be aided by offchannel but frame-xchg needed some
> > refactoring as well.
> > 
> > First was getting rid of this fx pointer re-use. It looks like this
> > was done to save a bit of memory but things get pretty complex
> > needed to check if the pointer is stale or has been reset. Instead
> > of this we now just allocate a new pointer each frame-xchg. This
> > allows for the module to queue multiple requests as well as removes
> > the complexity of needed to check if the fx pointer is stale.
> > 
> > Next was adding the ability to track frame-xchgs by ID. If a module
> > can queue up multiple requests it also needs to be able to cancel
> > them individually vs per-wdev. This comes free with offchannel
> > since
> > it returns an ID which can be given directly to the caller.
> > 
> > Then offchannel was simply piped in by adding the push/done APIs
> > as well as some minor modification to internal frame-xchg APIs for
> > destroy functionality.
> > ---
> >   src/frame-xchg.c | 97 +++++++++++++++++++++--------------------
> > -------
> >   src/frame-xchg.h |  6 +--
> >   src/p2p.c        |  9 +++--
> >   3 files changed, 51 insertions(+), 61 deletions(-)
> > 
> 
> <snip>
> 
> > -void frame_xchg_stop(uint64_t wdev_id);
> > +void frame_xchg_stop(uint64_t wdev_id, uint32_t id);
> 
> Do we even need the wdev_id here now? Also, should this be _cancel?

It just made the lookup easier, but yeah we could iterate each queue in
the phy list. And yeah, I could change that to _cancel.

> 
> Regards,
> -Denis

Re: [RFC 4/6] frame-xchg: refactor to work with offchannel module

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

Hi James,

>>> -void frame_xchg_stop(uint64_t wdev_id);
>>> +void frame_xchg_stop(uint64_t wdev_id, uint32_t id);
>>
>> Do we even need the wdev_id here now? Also, should this be _cancel?
> 
> It just made the lookup easier, but yeah we could iterate each queue in
> the phy list. And yeah, I could change that to _cancel.
> 

Hmm, how so? I don't even see wdev_id being used now.

Regards,
-Denis

Re: [RFC 1/6] frame-xchg: add destroy function to start() APIs

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

Hi James,

On 6/17/20 12:56 PM, James Prestwood wrote:
> This makes things more consistent with other IWD APIs as well as
> prepares for unifying frame-xchg and scanning.
> ---
>   src/frame-xchg.c | 36 ++++++++++++++++++++----------------
>   src/frame-xchg.h |  6 ++++--
>   2 files changed, 24 insertions(+), 18 deletions(-)
> 

Can we just get this commit in right away?  I think it makes sense on its own, 
but results in some compiler errors.

Regards,
-Denis

Re: [RFC 4/6] frame-xchg: refactor to work with offchannel module

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Mon, 2020-06-22 at 15:03 -0500, Denis Kenzior wrote:
> Hi James,
> 
> > > > -void frame_xchg_stop(uint64_t wdev_id);
> > > > +void frame_xchg_stop(uint64_t wdev_id, uint32_t id);
> > > 
> > > Do we even need the wdev_id here now? Also, should this be
> > > _cancel?
> > 
> > It just made the lookup easier, but yeah we could iterate each
> > queue in
> > the phy list. And yeah, I could change that to _cancel.
> > 
> 
> Hmm, how so? I don't even see wdev_id being used now.

Ah yeah not in frame-xchg now. I moved that into offchannel.c. So it
just makes *that* lookup easier :)

But when I think about it we will generally only ever have a couple
phy's (or just one), and barely any requests per-phy anyways so a full
lookup isnt too expensive.

> 
> Regards,
> -Denis
> 

Re: [RFC 1/6] frame-xchg: add destroy function to start() APIs

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

On Mon, 2020-06-22 at 15:05 -0500, Denis Kenzior wrote:
> Hi James,
> 
> On 6/17/20 12:56 PM, James Prestwood wrote:
> > This makes things more consistent with other IWD APIs as well as
> > prepares for unifying frame-xchg and scanning.
> > ---
> >   src/frame-xchg.c | 36 ++++++++++++++++++++----------------
> >   src/frame-xchg.h |  6 ++++--
> >   2 files changed, 24 insertions(+), 18 deletions(-)
> > 
> 
> Can we just get this commit in right away?  I think it makes sense on
> its own, 
> but results in some compiler errors.

Sure, I'll get it building on its own. I thought it was but apparently
not.
> 
> Regards,
> -Denis
>