[PATCH v2] offchannel: introduce new offchannel module

[PATCH v2] offchannel: introduce new offchannel module

[James Prestwood]

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

This module provides a convenient wrapper around both
CMD_[CANCEL_]_REMAIN_ON_CHANNEL APIs.

Certain protocols require going offchannel to send frames, and/or
wait for a response. The frame-xchg module somewhat does this but
has some limitations. For example you cannot just go offchannel;
an initial frame must be sent out to start the procedure. In addition
frame-xchg does not work for broadcasts since it expects an ACK.

This module is much simpler and only handles going offchannel for
a duration. During this time frames may be sent or received. After
the duration the caller will get a callback and any included error
if there was one. Any offchannel request can be cancelled prior to
the duration expriring if the offchannel work has finished early.
---
 Makefile.am      |   1 +
 src/offchannel.c | 302 +++++++++++++++++++++++++++++++++++++++++++++++
 src/offchannel.h |  29 +++++
 3 files changed, 332 insertions(+)
 create mode 100644 src/offchannel.c
 create mode 100644 src/offchannel.h

v2:
 * Removed internal timer and intead rely on the kernel to keep track
 * Specially handle the cancel case where the command had been sent to
   the kernel already. For this we need to wait until the genl callback
   to cancel (since we then have the cookie).

diff --git a/Makefile.am b/Makefile.am
index 1e1c230f..d93dd2c9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -247,6 +247,7 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
 					src/ip-pool.h src/ip-pool.c \
 					src/band.h src/band.c \
 					src/sysfs.h src/sysfs.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..4388e398
--- /dev/null
+++ b/src/offchannel.c
@@ -0,0 +1,302 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2021  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
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include <ell/ell.h>
+
+#include "linux/nl80211.h"
+
+#include "src/offchannel.h"
+#include "src/wiphy.h"
+#include "src/nl80211util.h"
+#include "src/iwd.h"
+#include "src/module.h"
+
+struct offchannel_info {
+	uint64_t wdev_id;
+	uint32_t freq;
+	uint32_t duration;
+
+	uint32_t roc_cmd_id;
+	uint64_t roc_cookie;
+
+	offchannel_started_cb_t started;
+	offchannel_destroy_cb_t destroy;
+	void *user_data;
+	int error;
+
+	struct wiphy_radio_work_item work;
+
+	bool needs_cancel : 1;
+};
+
+static struct l_genl_family *nl80211;
+static struct l_queue *offchannel_list;
+
+static bool match_wdev(const void *a, const void *user_data)
+{
+	const struct offchannel_info *info = a;
+	const uint64_t *wdev_id = user_data;
+
+	return info->wdev_id == *wdev_id;
+}
+
+static void offchannel_cancel_roc(struct offchannel_info *info)
+{
+	struct l_genl_msg *msg;
+
+	msg = l_genl_msg_new(NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL);
+
+	l_genl_msg_append_attr(msg, NL80211_ATTR_WDEV, 8, &info->wdev_id);
+	l_genl_msg_append_attr(msg, NL80211_ATTR_COOKIE, 8, &info->roc_cookie);
+
+	/* Nothing much can be done if this fails */
+	if (!l_genl_family_send(nl80211, msg, NULL, NULL, NULL))
+		l_genl_msg_unref(msg);
+}
+
+static void offchannel_roc_cb(struct l_genl_msg *msg, void *user_data)
+{
+	struct offchannel_info *info = user_data;
+
+	info->error = l_genl_msg_get_error(msg);
+	info->roc_cmd_id = 0;
+
+	if (info->error < 0) {
+		l_debug("Error from CMD_REMAIN_ON_CHANNEL (%d)", info->error);
+		goto work_done;
+	}
+
+	info->error = nl80211_parse_attrs(msg, NL80211_ATTR_COOKIE,
+					&info->roc_cookie, NL80211_ATTR_UNSPEC);
+	if (info->error < 0) {
+		l_error("Could not parse ROC cookie");
+		goto work_done;
+	}
+
+	/* This request was cancelled, and ROC needs to be cancelled */
+	if (info->needs_cancel) {
+		offchannel_cancel_roc(info);
+		goto work_done;
+	}
+
+	return;
+
+work_done:
+	wiphy_radio_work_done(wiphy_find_by_wdev(info->wdev_id), info->work.id);
+}
+
+static bool offchannel_work_ready(struct wiphy_radio_work_item *item)
+{
+	struct l_genl_msg *msg;
+	struct offchannel_info *info = l_container_of(item,
+						struct offchannel_info, work);
+
+	msg = l_genl_msg_new(NL80211_CMD_REMAIN_ON_CHANNEL);
+
+	l_genl_msg_append_attr(msg, NL80211_ATTR_WDEV, 8, &info->wdev_id);
+	l_genl_msg_append_attr(msg, NL80211_ATTR_WIPHY_FREQ, 4, &info->freq);
+	l_genl_msg_append_attr(msg, NL80211_ATTR_DURATION, 4, &info->duration);
+
+	info->roc_cmd_id = l_genl_family_send(nl80211, msg, offchannel_roc_cb,
+						info, NULL);
+	if (!info->roc_cmd_id) {
+		info->error = -EIO;
+		l_genl_msg_unref(msg);
+		return true;
+	}
+
+	l_queue_push_head(offchannel_list, info);
+
+	return false;
+}
+
+static void offchannel_work_destroy(struct wiphy_radio_work_item *item)
+{
+	struct offchannel_info *info = l_container_of(item,
+						struct offchannel_info, work);
+
+	if (info->destroy)
+		info->destroy(info->error, info->user_data);
+
+	l_queue_remove(offchannel_list, info);
+	l_free(info);
+}
+
+static const struct wiphy_radio_work_item_ops offchannel_work_ops = {
+	.do_work = offchannel_work_ready,
+	.destroy = offchannel_work_destroy,
+};
+
+uint32_t offchannel_start(uint64_t wdev_id, uint32_t freq, uint32_t duration,
+			offchannel_started_cb_t started, void *user_data,
+			offchannel_destroy_cb_t destroy)
+{
+	struct offchannel_info *info = l_new(struct offchannel_info, 1);
+
+	info->wdev_id = wdev_id;
+	info->freq = freq;
+	info->duration = duration;
+	info->started = started;
+	info->destroy = destroy;
+	info->user_data = user_data;
+	/*
+	 * Set error as cancelled in case this work gets cancelled prior to
+	 * the wiphy work starting.
+	 */
+	info->error = -ECANCELED;
+
+	return wiphy_radio_work_insert(wiphy_find_by_wdev(wdev_id),
+					&info->work, 1, &offchannel_work_ops);
+}
+
+void offchannel_cancel(uint64_t wdev_id, uint32_t id)
+{
+	struct wiphy *wiphy = wiphy_find_by_wdev(wdev_id);
+	struct offchannel_info *info;
+
+	/* Hasn't even started yet */
+	if (!wiphy_radio_work_is_running(wiphy, id))
+		goto work_done;
+
+	info = l_queue_find(offchannel_list, match_wdev, &wdev_id);
+	if (!info)
+		return;
+
+	if (info->roc_cmd_id) {
+		/*
+		 * The command has been sent to the kernel. This means we must
+		 * wait till ROC starts and cancel at that time. Cancelling just
+		 * the ROC command now will have no effect.
+		 */
+		if (l_genl_family_request_sent(nl80211, info->roc_cmd_id))
+			info->needs_cancel = true;
+
+		l_genl_family_cancel(nl80211, info->roc_cmd_id);
+		info->roc_cmd_id = 0;
+
+		if (!info->needs_cancel)
+			goto work_done;
+
+		return;
+	}
+
+	/*
+	 * Something must have happened in the genl callback. Any errors there
+	 * will be handled already
+	 */
+	if (!info->roc_cookie)
+		return;
+
+	/*
+	 * At this point we know ROC has at least been queued (potentially not
+	 * started) and can be cancelled.
+	 */
+	offchannel_cancel_roc(info);
+
+work_done:
+	wiphy_radio_work_done(wiphy, id);
+}
+
+static void offchannel_mlme_notify(struct l_genl_msg *msg, void *user_data)
+{
+	struct offchannel_info *info;
+	uint64_t wdev_id;
+	uint64_t cookie;
+	uint8_t cmd;
+
+	cmd = l_genl_msg_get_command(msg);
+
+	if (cmd != NL80211_CMD_REMAIN_ON_CHANNEL &&
+			cmd != NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL)
+		return;
+
+	if (nl80211_parse_attrs(msg, NL80211_ATTR_WDEV, &wdev_id,
+					NL80211_ATTR_COOKIE, &cookie,
+					NL80211_ATTR_UNSPEC) < 0)
+		return;
+
+	info = l_queue_find(offchannel_list, match_wdev, &wdev_id);
+	if (!info)
+		return;
+
+	/* ROC must have been started elsewhere, not by IWD */
+	if (info->roc_cookie != cookie)
+		return;
+
+	switch (cmd) {
+	case NL80211_CMD_REMAIN_ON_CHANNEL:
+		if (info->started)
+			info->started(info->user_data);
+
+		break;
+	case NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL:
+		info->error = 0;
+
+		wiphy_radio_work_done(wiphy_find_by_wdev(info->wdev_id),
+					info->work.id);
+		break;
+	default:
+		return;
+	}
+}
+
+static int offchannel_init(void)
+{
+	struct l_genl *genl = iwd_get_genl();
+
+	nl80211 = l_genl_family_new(genl, NL80211_GENL_NAME);
+	if (!nl80211) {
+		l_error("Failed to obtain nl80211");
+		return -EIO;
+	}
+
+	if (!l_genl_family_register(nl80211, "mlme", offchannel_mlme_notify,
+								NULL, NULL)) {
+		l_error("Failed to register for MLME");
+		l_genl_family_free(nl80211);
+		nl80211 = NULL;
+
+		return -EIO;
+	}
+
+	offchannel_list = l_queue_new();
+
+	return 0;
+}
+
+static void offchannel_exit(void)
+{
+	l_debug("");
+
+	l_genl_family_free(nl80211);
+	nl80211 = NULL;
+
+	l_queue_destroy(offchannel_list, l_free);
+}
+
+IWD_MODULE(offchannel, offchannel_init, offchannel_exit);
diff --git a/src/offchannel.h b/src/offchannel.h
new file mode 100644
index 00000000..1ffa94f1
--- /dev/null
+++ b/src/offchannel.h
@@ -0,0 +1,29 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2021  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
+ *
+ */
+
+typedef void (*offchannel_started_cb_t)(void *user_data);
+typedef void (*offchannel_destroy_cb_t)(int error, void *user_data);
+
+uint32_t offchannel_start(uint64_t wdev_id, uint32_t freq, uint32_t duration,
+			offchannel_started_cb_t started, void *user_data,
+			offchannel_destroy_cb_t destroy);
+void offchannel_cancel(uint64_t wdev_id, uint32_t id);
-- 
2.31.1

Re: [PATCH v2] offchannel: introduce new offchannel module

[Paul Menzel]

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

Dear James,


Thank you for writing this great commit message, helping me understand, 
what the module does.


Kind regards,

Paul

Re: [PATCH v2] offchannel: introduce new offchannel module

[Denis Kenzior]

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

Hi James,

On 11/30/21 3:49 PM, James Prestwood wrote:
> This module provides a convenient wrapper around both
> CMD_[CANCEL_]_REMAIN_ON_CHANNEL APIs.
> 
> Certain protocols require going offchannel to send frames, and/or
> wait for a response. The frame-xchg module somewhat does this but
> has some limitations. For example you cannot just go offchannel;
> an initial frame must be sent out to start the procedure. In addition
> frame-xchg does not work for broadcasts since it expects an ACK.
> 
> This module is much simpler and only handles going offchannel for
> a duration. During this time frames may be sent or received. After
> the duration the caller will get a callback and any included error
> if there was one. Any offchannel request can be cancelled prior to
> the duration expriring if the offchannel work has finished early.
> ---
>   Makefile.am      |   1 +
>   src/offchannel.c | 302 +++++++++++++++++++++++++++++++++++++++++++++++
>   src/offchannel.h |  29 +++++
>   3 files changed, 332 insertions(+)
>   create mode 100644 src/offchannel.c
>   create mode 100644 src/offchannel.h
> 
> v2:
>   * Removed internal timer and intead rely on the kernel to keep track
>   * Specially handle the cancel case where the command had been sent to
>     the kernel already. For this we need to wait until the genl callback
>     to cancel (since we then have the cookie).
> 

<snip>

> +static void offchannel_cancel_roc(struct offchannel_info *info)
> +{
> +	struct l_genl_msg *msg;
> +
> +	msg = l_genl_msg_new(NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL);
> +
> +	l_genl_msg_append_attr(msg, NL80211_ATTR_WDEV, 8, &info->wdev_id);
> +	l_genl_msg_append_attr(msg, NL80211_ATTR_COOKIE, 8, &info->roc_cookie);
> +
> +	/* Nothing much can be done if this fails */
> +	if (!l_genl_family_send(nl80211, msg, NULL, NULL, NULL))

Would it not be better to wait until the cancel is at least acked and only end 
the wiphy work then?  Might be even better if we waited around until the kernel 
sends the cancel event as well.

> +		l_genl_msg_unref(msg);
> +}
> +
> +static void offchannel_roc_cb(struct l_genl_msg *msg, void *user_data)
> +{
> +	struct offchannel_info *info = user_data;
> +
> +	info->error = l_genl_msg_get_error(msg);
> +	info->roc_cmd_id = 0;
> +
> +	if (info->error < 0) {
> +		l_debug("Error from CMD_REMAIN_ON_CHANNEL (%d)", info->error);
> +		goto work_done;
> +	}
> +
> +	info->error = nl80211_parse_attrs(msg, NL80211_ATTR_COOKIE,
> +					&info->roc_cookie, NL80211_ATTR_UNSPEC);
> +	if (info->error < 0) {
> +		l_error("Could not parse ROC cookie");
> +		goto work_done;
> +	}
> +
> +	/* This request was cancelled, and ROC needs to be cancelled */
> +	if (info->needs_cancel) {
> +		offchannel_cancel_roc(info);
> +		goto work_done;
> +	}
> +
> +	return;
> +
> +work_done:
> +	wiphy_radio_work_done(wiphy_find_by_wdev(info->wdev_id), info->work.id);
> +}
> +

<snip>

> +
> +void offchannel_cancel(uint64_t wdev_id, uint32_t id)
> +{
> +	struct wiphy *wiphy = wiphy_find_by_wdev(wdev_id);
> +	struct offchannel_info *info;
> +

Maybe be paranoid and check wiphy isn't NULL?

Also, shouldn't we check that the work id exists?

> +	/* Hasn't even started yet */
> +	if (!wiphy_radio_work_is_running(wiphy, id))
> +		goto work_done;

Guess we could modify wiphy_radio_work_is_running to return an int and use 0/1 
for true/false and -ENOENT if the work doesn't exist.

> +
> +	info = l_queue_find(offchannel_list, match_wdev, &wdev_id);
> +	if (!info)
> +		return;
> +
> +	if (info->roc_cmd_id) {
> +		/*
> +		 * The command has been sent to the kernel. This means we must
> +		 * wait till ROC starts and cancel at that time. Cancelling just
> +		 * the ROC command now will have no effect.
> +		 */
> +		if (l_genl_family_request_sent(nl80211, info->roc_cmd_id))
> +			info->needs_cancel = true;

Hmm, is a patch missing for ell?

So you set needs_cancel and invoke l_genl_family_cancel, which means that 
offchannel_roc_cb() is never called.  Yet that seems to be the only place you 
check needs_cancel?

> +
> +		l_genl_family_cancel(nl80211, info->roc_cmd_id);
> +		info->roc_cmd_id = 0;
> +
> +		if (!info->needs_cancel)
> +			goto work_done;

If the request hasn't started, then indeed invoking l_genl_family_cancel and 
destroying the work right away should be fine.  If the request has already been 
sent, then we are stuck waiting for the ack.  You could also assume that if 
roc_cmd_id is set, then the command is in flight and setting 
needs_cancel/waiting for the ack is needed.

In either case, we should invoke the destroy callback here and clear out the 
user data/callbacks.

> +
> +		return;
> +	}
> +
> +	/*
> +	 * Something must have happened in the genl callback. Any errors there
> +	 * will be handled already
> +	 */
> +	if (!info->roc_cookie)
> +		return;

I'm guessing this should be an L_WARN_ON.

> +
> +	/*
> +	 * At this point we know ROC has at least been queued (potentially not
> +	 * started) and can be cancelled.
> +	 */
> +	offchannel_cancel_roc(info);
> +
> +work_done:
> +	wiphy_radio_work_done(wiphy, id);
> +}
> +

Regards,
-Denis

Re: [PATCH v2] offchannel: introduce new offchannel module

[James Prestwood]

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

On Wed, 2021-12-01 at 12:06 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 11/30/21 3:49 PM, James Prestwood wrote:
> > This module provides a convenient wrapper around both
> > CMD_[CANCEL_]_REMAIN_ON_CHANNEL APIs.
> > 
> > Certain protocols require going offchannel to send frames, and/or
> > wait for a response. The frame-xchg module somewhat does this but
> > has some limitations. For example you cannot just go offchannel;
> > an initial frame must be sent out to start the procedure. In
> > addition
> > frame-xchg does not work for broadcasts since it expects an ACK.
> > 
> > This module is much simpler and only handles going offchannel for
> > a duration. During this time frames may be sent or received. After
> > the duration the caller will get a callback and any included error
> > if there was one. Any offchannel request can be cancelled prior to
> > the duration expriring if the offchannel work has finished early.
> > ---
> >   Makefile.am      |   1 +
> >   src/offchannel.c | 302
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >   src/offchannel.h |  29 +++++
> >   3 files changed, 332 insertions(+)
> >   create mode 100644 src/offchannel.c
> >   create mode 100644 src/offchannel.h
> > 
> > v2:
> >   * Removed internal timer and intead rely on the kernel to keep
> > track
> >   * Specially handle the cancel case where the command had been
> > sent to
> >     the kernel already. For this we need to wait until the genl
> > callback
> >     to cancel (since we then have the cookie).
> > 
> 
> <snip>
> 
> > +static void offchannel_cancel_roc(struct offchannel_info *info)
> > +{
> > +       struct l_genl_msg *msg;
> > +
> > +       msg = l_genl_msg_new(NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL);
> > +
> > +       l_genl_msg_append_attr(msg, NL80211_ATTR_WDEV, 8, &info-
> > >wdev_id);
> > +       l_genl_msg_append_attr(msg, NL80211_ATTR_COOKIE, 8, &info-
> > >roc_cookie);
> > +
> > +       /* Nothing much can be done if this fails */
> > +       if (!l_genl_family_send(nl80211, msg, NULL, NULL, NULL))
> 
> Would it not be better to wait until the cancel is at least acked and
> only end 
> the wiphy work then?  Might be even better if we waited around until
> the kernel 
> sends the cancel event as well.

Sure I think waiting till the kernel sends cancel ROC is best, that
moves most of the 'done' paths into one spot anyways. I'll change the 
'needs_cancel' check to just return below. 

Even with this though I don't see a point of waiting for the ACK since
the kernel will send cancel ROC. The ACK callback for this would have
no purpose AFAIK.

> 
> > +               l_genl_msg_unref(msg);
> > +}
> > +
> > +static void offchannel_roc_cb(struct l_genl_msg *msg, void
> > *user_data)
> > +{
> > +       struct offchannel_info *info = user_data;
> > +
> > +       info->error = l_genl_msg_get_error(msg);
> > +       info->roc_cmd_id = 0;
> > +
> > +       if (info->error < 0) {
> > +               l_debug("Error from CMD_REMAIN_ON_CHANNEL (%d)",
> > info->error);
> > +               goto work_done;
> > +       }
> > +
> > +       info->error = nl80211_parse_attrs(msg, NL80211_ATTR_COOKIE,
> > +                                       &info->roc_cookie,
> > NL80211_ATTR_UNSPEC);
> > +       if (info->error < 0) {
> > +               l_error("Could not parse ROC cookie");
> > +               goto work_done;
> > +       }
> > +
> > +       /* This request was cancelled, and ROC needs to be
> > cancelled */
> > +       if (info->needs_cancel) {
> > +               offchannel_cancel_roc(info);
> > +               goto work_done;

This can just return, and be 'done' in the ROC cancel event.

> > +       }
> > +
> > +       return;
> > +
> > +work_done:
> > +       wiphy_radio_work_done(wiphy_find_by_wdev(info->wdev_id),
> > info->work.id);
> > +}
> > +
> 
> <snip>
> 
> > +
> > +void offchannel_cancel(uint64_t wdev_id, uint32_t id)
> > +{
> > +       struct wiphy *wiphy = wiphy_find_by_wdev(wdev_id);
> > +       struct offchannel_info *info;
> > +
> 
> Maybe be paranoid and check wiphy isn't NULL?
> 
> Also, shouldn't we check that the work id exists?
> 
> > +       /* Hasn't even started yet */
> > +       if (!wiphy_radio_work_is_running(wiphy, id))
> > +               goto work_done;
> 
> Guess we could modify wiphy_radio_work_is_running to return an int
> and use 0/1 
> for true/false and -ENOENT if the work doesn't exist.

Sure, sounds good.

> 
> > +
> > +       info = l_queue_find(offchannel_list, match_wdev, &wdev_id);
> > +       if (!info)
> > +               return;
> > +
> > +       if (info->roc_cmd_id) {
> > +               /*
> > +                * The command has been sent to the kernel. This
> > means we must
> > +                * wait till ROC starts and cancel at that time.
> > Cancelling just
> > +                * the ROC command now will have no effect.
> > +                */
> > +               if (l_genl_family_request_sent(nl80211, info-
> > >roc_cmd_id))
> > +                       info->needs_cancel = true;
> 
> Hmm, is a patch missing for ell?

Yeah, I'll send this out.

> 
> So you set needs_cancel and invoke l_genl_family_cancel, which means
> that 
> offchannel_roc_cb() is never called.  Yet that seems to be the only
> place you 
> check needs_cancel?
> 
> > +
> > +               l_genl_family_cancel(nl80211, info->roc_cmd_id);
> > +               info->roc_cmd_id = 0;
> > +
> > +               if (!info->needs_cancel)
> > +                       goto work_done;
> 
> If the request hasn't started, then indeed invoking
> l_genl_family_cancel and 
> destroying the work right away should be fine.  If the request has
> already been 
> sent, then we are stuck waiting for the ack.  You could also assume
> that if 
> roc_cmd_id is set, then the command is in flight and setting 
> needs_cancel/waiting for the ack is needed.
> 
> In either case, we should invoke the destroy callback here and clear
> out the 
> user data/callbacks.

Yes, I need to only cancel conditionally. I don't see why we need to
invoke destroy here though? I would rather keep destroy() in one
location, in the work done callback.

> 
> > +
> > +               return;
> > +       }
> > +
> > +       /*
> > +        * Something must have happened in the genl callback. Any
> > errors there
> > +        * will be handled already
> > +        */
> > +       if (!info->roc_cookie)
> > +               return;
> 
> I'm guessing this should be an L_WARN_ON.
> 
> > +
> > +       /*
> > +        * At this point we know ROC has at least been queued
> > (potentially not
> > +        * started) and can be cancelled.
> > +        */
> > +       offchannel_cancel_roc(info);
> > +
> > +work_done:
> > +       wiphy_radio_work_done(wiphy, id);
> > +}
> > +
> 
> Regards,
> -Denis

Re: [PATCH v2] offchannel: introduce new offchannel module

[Denis Kenzior]

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

Hi James,

> Even with this though I don't see a point of waiting for the ACK since
> the kernel will send cancel ROC. The ACK callback for this would have
> no purpose AFAIK.
> 

If you always want to wait for the cancel event, then I would concur.  Question 
is whether the kernel would always guarantee that this is sent.  I guess we'll 
find out :)

> 
> Yes, I need to only cancel conditionally. I don't see why we need to
> invoke destroy here though? I would rather keep destroy() in one
> location, in the work done callback.

Mostly to be nice to the consumer of these methods.  They may want to assume 
that cancel succeeds synchronously and that the destroy method is invoked right 
away.  For example, this may be useful if the destroy method manipulates some 
data hanging off the main object and the main object is about to be destroyed 
(hence the cancel call on the ongoing request).

Regards,
-Denis

Re: [PATCH v2] offchannel: introduce new offchannel module

[James Prestwood]

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

On Mon, 2021-12-06 at 12:48 -0600, Denis Kenzior wrote:
> Hi James,
> 
> > Even with this though I don't see a point of waiting for the ACK
> > since
> > the kernel will send cancel ROC. The ACK callback for this would
> > have
> > no purpose AFAIK.
> > 
> 
> If you always want to wait for the cancel event, then I would
> concur.  Question 
> is whether the kernel would always guarantee that this is sent.  I
> guess we'll 
> find out :)
> 
> > 
> > Yes, I need to only cancel conditionally. I don't see why we need
> > to
> > invoke destroy here though? I would rather keep destroy() in one
> > location, in the work done callback.
> 
> Mostly to be nice to the consumer of these methods.  They may want to
> assume 
> that cancel succeeds synchronously and that the destroy method is
> invoked right 
> away.  For example, this may be useful if the destroy method
> manipulates some 
> data hanging off the main object and the main object is about to be
> destroyed 
> (hence the cancel call on the ongoing request).

Ok makes sense. I guess in this case we can just call then NULL destroy
and all other cases it will still happen in the work done callback.
I've already sent v3 but I'll update this in the next round.

> 
> Regards,
> -Denis