don't bring the interface down if WoWLAN is enabled

Message ID 20170317142745.10217-1-matteo.croce@canonical.com
State Changes Requested
Headers show

Commit Message

Matteo Croce March 17, 2017, 2:27 p.m.
---
 src/drivers/driver.h            |  7 +++++++
 src/drivers/driver_nl80211.c    | 38 ++++++++++++++++++++++++++++++++++++++
 wpa_supplicant/wpa_supplicant.c |  7 ++++++-
 3 files changed, 51 insertions(+), 1 deletion(-)

Comments

Johannes Berg March 17, 2017, 2:35 p.m. | #1
Hi,

I think you should solve this problem in network manager - wpa_s should
stay enabled when wowlan is enabled, since you want it to go back to
managing the still-existing connection on resume.

johannes
Dan Williams March 17, 2017, 4:02 p.m. | #2
On Fri, 2017-03-17 at 15:35 +0100, Johannes Berg wrote:
> Hi,
> 
> I think you should solve this problem in network manager - wpa_s
> should
> stay enabled when wowlan is enabled, since you want it to go back to
> managing the still-existing connection on resume.

NM won't clean up WoL (and WoWLAN) enabled interfaces since v1.0
(commit 1218b7e0)  back in 2014.

https://bugzilla.gnome.org/show_bug.cgi?id=712745

Instead it'll postpone doing anything with the supplicant until resume.
 So I wouldn't expect this to be a problem with NM unless there's an NM
bug, which is always possible.

More information needed (like NM version and NM debug logs), but I
think you're right that this patch isn't the right way to address the
problem.

Dan
Matteo Croce March 17, 2017, 4:15 p.m. | #3
My concern was not only suspend/resume, but even wakeup from S5 which
means shutdown.
In that case wpa_s is stopped by the shutdown process and it will
bring the interface down.

Probably I had to document this feature better, sorry.
Dan Williams March 17, 2017, 6:35 p.m. | #4
On Fri, 2017-03-17 at 17:15 +0100, Matteo Croce wrote:
> My concern was not only suspend/resume, but even wakeup from S5 which
> means shutdown.
> In that case wpa_s is stopped by the shutdown process and it will
> bring the interface down.

At least in the systemd case, maybe just not stop the supplicant in S5?
 There's gotta be a way to distinguish "really powering off/restarting"
from "mostly powered off and expected to come back on a moment's
notice", I think.

Dan
Matteo Croce March 23, 2017, 1:49 p.m. | #5
2017-03-17 19:35 GMT+01:00 Dan Williams <dcbw@redhat.com>:
> At least in the systemd case, maybe just not stop the supplicant in S5?
>  There's gotta be a way to distinguish "really powering off/restarting"
> from "mostly powered off and expected to come back on a moment's
> notice", I think.

Hi Dan,

S5 is not "mostly powered off", S5 is power off.
So the only way to wake up from power off is to leave the card powered,
and wpa_s actually prevent this.

Regards,
Johannes Berg March 23, 2017, 3:33 p.m. | #6
> 
> S5 is not "mostly powered off", S5 is power off.
> So the only way to wake up from power off is to leave the card
> powered, and wpa_s actually prevent this.

It's still the wrong thing to make wpa_s unconditionally leave the NIC
turned on, especially if it's no longer managing it.

Frankly, there isn't really a good way to let the NIC stay powered in
S5 anyway because it may wake up for a number of reasons like 4-way-
handshake, and that's not what you want.

johannes
Jouni Malinen March 27, 2017, 1:45 p.m. | #7
On Fri, Mar 17, 2017 at 03:27:45PM +0100, Matteo Croce wrote:
> ---

Would need to have Signed-off-by: line in the commit log as described in
the top level CONTRIBUTIONS file.. That said, based on the discussion,
this did not sound like the best way of handling the issue.

> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> @@ -5386,7 +5386,12 @@ int wpa_supplicant_remove_iface(struct wpa_global *global,
>  		global->p2p_group_formation = NULL;
>  	if (global->p2p_invite_group == wpa_s)
>  		global->p2p_invite_group = NULL;
> -	wpa_supplicant_deinit_iface(wpa_s, 1, terminate);
> +
> +	/* Don't bring the interface down if WoWLAN is enabled */
> +	if (wpa_s->driver->get_wowlan && !wpa_s->driver->get_wowlan(wpa_s->drv_priv))
> +		wpa_supplicant_deinit_iface(wpa_s, 1, terminate);
> +	else
> +		wpa_dbg(wpa_s, MSG_INFO, "Leaving up as WoWLAN is enabled");

And this part looks pretty suspicious to me.. Wouldn't this leak memory
and other resources by skipping all the deinit steps? How is the
wpa_supplicant process supposed to recover from this? By getting killed
and restarted by something external? That would not really be the way to
handle this.. I'm not completely sure I understood what this is trying
to do, but if a change in wpa_supplicant is needed, please describe the
exact state on which the Wi-Fi device is supposed to be left and what
will be the next operation to continue from that state.
Dan Williams March 27, 2017, 4:16 p.m. | #8
On Mon, 2017-03-27 at 16:45 +0300, Jouni Malinen wrote:
> On Fri, Mar 17, 2017 at 03:27:45PM +0100, Matteo Croce wrote:
> > ---
> 
> Would need to have Signed-off-by: line in the commit log as described
> in
> the top level CONTRIBUTIONS file.. That said, based on the
> discussion,
> this did not sound like the best way of handling the issue.
> 
> > diff --git a/wpa_supplicant/wpa_supplicant.c
> > b/wpa_supplicant/wpa_supplicant.c
> > @@ -5386,7 +5386,12 @@ int wpa_supplicant_remove_iface(struct
> > wpa_global *global,
> >  		global->p2p_group_formation = NULL;
> >  	if (global->p2p_invite_group == wpa_s)
> >  		global->p2p_invite_group = NULL;
> > -	wpa_supplicant_deinit_iface(wpa_s, 1, terminate);
> > +
> > +	/* Don't bring the interface down if WoWLAN is enabled */
> > +	if (wpa_s->driver->get_wowlan && !wpa_s->driver-
> > >get_wowlan(wpa_s->drv_priv))
> > +		wpa_supplicant_deinit_iface(wpa_s, 1, terminate);
> > +	else
> > +		wpa_dbg(wpa_s, MSG_INFO, "Leaving up as WoWLAN is
> > enabled");
> 
> And this part looks pretty suspicious to me.. Wouldn't this leak
> memory
> and other resources by skipping all the deinit steps? How is the
> wpa_supplicant process supposed to recover from this? By getting
> killed
> and restarted by something external? That would not really be the way
> to
> handle this.. I'm not completely sure I understood what this is
> trying
> to do, but if a change in wpa_supplicant is needed, please describe
> the
> exact state on which the Wi-Fi device is supposed to be left and what
> will be the next operation to continue from that state.

I believe the issue is that the WiFi interface stays powered while the
system is actually shut down, and the WiFi should wake/start the system
up on magic packets or activity.  But of course, when stopping the
supplicant service during normal system shutdown paths, the supplicant
removes the iface and cleans it up, rendering WoWOL useless.

I'm actually thinking that the supplicant shouldn't be stopped cleanly
in this case at all, and I've got to believe that whatever is managing
system services (systemd? upstart? something else?) can differentiate
between "shut down and keep WOWLAN enabled" and "shut it all down". 
And maybe that thing should be deciding whether or not to stop the
supplicant or to just kill it.

Dan
Matteo Croce March 27, 2017, 8:18 p.m. | #9
2017-03-27 18:16 GMT+02:00 Dan Williams <dcbw@redhat.com>:
> I'm actually thinking that the supplicant shouldn't be stopped cleanly
> in this case at all, and I've got to believe that whatever is managing
> system services (systemd? upstart? something else?) can differentiate
> between "shut down and keep WOWLAN enabled" and "shut it all down".
> And maybe that thing should be deciding whether or not to stop the
> supplicant or to just kill it.

To do this the service manager (systemd, sysvinit, whatever) should
have some logic to query the WoWLAN status (via nl80211, wext,
$proprietaryctl..) or needs to be informed that the system is going to
shutdown but with WoWLAN enabled on almost an interface.
This is a bit tricky as an user can enable it manually with iw or
other interfaces.
Also, imagine to have multiple supplicant instances handling multiple
radios, or even worse, a supplicant managing two radios, only one with
WoWLAN enabled. What should the service manager do? Leave wpa_s
running and leaving the wrong one connected too until the timeout, or
kill it and break WoWLAN?

Patch

diff --git a/src/drivers/driver.h b/src/drivers/driver.h
index fc2593e..4c54987 100644
--- a/src/drivers/driver.h
+++ b/src/drivers/driver.h
@@ -3164,6 +3164,13 @@  struct wpa_driver_ops {
 				unsigned int val);
 
 	/**
+	 * get_wowlan - Get wake-on-wireless status
+	 * @priv: Private driver interface data
+	 * Returns: 1 if enabled, 0 if not
+	 */
+	int (*get_wowlan)(void *priv);
+
+	/**
 	 * set_wowlan - Set wake-on-wireless triggers
 	 * @priv: Private driver interface data
 	 * @triggers: wowlan triggers
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index bceeba2..fafbf2c 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -8613,6 +8613,43 @@  static int nl80211_set_qos_map(void *priv, const u8 *qos_map_set,
 }
 
 
+static int get_wowlan_handler(struct nl_msg *msg, void *arg)
+{
+	struct nlattr *tb[NL80211_ATTR_MAX + 1];
+	struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
+	int *wowlan_enabled = arg;
+
+	nla_parse(tb, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
+		  genlmsg_attrlen(gnlh, 0), NULL);
+
+	*wowlan_enabled = !!tb[NL80211_ATTR_WOWLAN_TRIGGERS];
+
+	return NL_SKIP;
+}
+
+
+static int nl80211_get_wowlan(void *priv)
+{
+	struct i802_bss *bss = priv;
+	struct wpa_driver_nl80211_data *drv = bss->drv;
+	struct nl_msg *msg;
+	int wowlan_enabled;
+
+	wpa_printf(MSG_DEBUG, "nl80211: Querying wowlan setting");
+
+	msg = nl80211_drv_msg(drv, 0, NL80211_CMD_GET_WOWLAN);
+
+	if (!send_and_recv_msgs(drv, msg, get_wowlan_handler, &wowlan_enabled)) {
+		wpa_printf(MSG_DEBUG, "WoWLAN is %s", wowlan_enabled ? "enabled" : "disabled");
+		if (wowlan_enabled)
+			return 1;
+	} else
+		wpa_printf(MSG_DEBUG, "nl80211: wowlan query failed");
+
+	return 0;
+}
+
+
 static int nl80211_set_wowlan(void *priv,
 			      const struct wowlan_triggers *triggers)
 {
@@ -10075,6 +10112,7 @@  const struct wpa_driver_ops wpa_driver_nl80211_ops = {
 #endif /* ANDROID */
 	.vendor_cmd = nl80211_vendor_cmd,
 	.set_qos_map = nl80211_set_qos_map,
+	.get_wowlan = nl80211_get_wowlan,
 	.set_wowlan = nl80211_set_wowlan,
 	.set_mac_addr = nl80211_set_mac_addr,
 #ifdef CONFIG_MESH
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
index e65441d..da06200 100644
--- a/wpa_supplicant/wpa_supplicant.c
+++ b/wpa_supplicant/wpa_supplicant.c
@@ -5386,7 +5386,12 @@  int wpa_supplicant_remove_iface(struct wpa_global *global,
 		global->p2p_group_formation = NULL;
 	if (global->p2p_invite_group == wpa_s)
 		global->p2p_invite_group = NULL;
-	wpa_supplicant_deinit_iface(wpa_s, 1, terminate);
+
+	/* Don't bring the interface down if WoWLAN is enabled */
+	if (wpa_s->driver->get_wowlan && !wpa_s->driver->get_wowlan(wpa_s->drv_priv))
+		wpa_supplicant_deinit_iface(wpa_s, 1, terminate);
+	else
+		wpa_dbg(wpa_s, MSG_INFO, "Leaving up as WoWLAN is enabled");
 
 #ifdef CONFIG_MESH
 	if (mesh_if_created) {