| Message ID | 20170317142745.10217-1-matteo.croce@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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.
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
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,
> > 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
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.
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
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?
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) {