GNOME Bugzilla – Bug 728044
[review] dcbw/wwan-fixes: track modem states and make autoconnect work
Last modified: 2014-05-07 02:58:28 UTC
This branch does a couple things: 1) makes WWAN rfkill handling consistent with WiFi rfkill handling; when airplane mode is on, all devices of the class are disabled. When airplane mode is off, all devices can be enabled. Previously the global WWAN state depended on the sum of the ModemManager 'enabled' states of all modems *and* the rfkill state, such that if rfkill was unblocked, and two modems were found, and one modem could not be used, no modems could be used. This splits that up so that modems can be used independently. For example, if you have an internal modem that is locked to a specific provider and you no longer have a plan with that provider, but you have an external modem that you use instead on a different provider, this now works. Previously the locked modem would sometimes affect the external modem's enable/disable state in confusing ways. 2) Tracks ModemManager modem object state to determine NMDevice states, instead of relying on two ambiguous properties (Enabled & Connected). The modem states let us correctly handle SIM PINs and locked modems, such that even if a modem is locked the NMDeviceModem is in DISCONNECTED state and *could* be activated if some connection has a stored PIN. With states we can provide more information about why the device is unavailable too, because if the modem is in FAILED state we can check the failed reason and tell the user that the SIM is missing or something else more useful than we do now. This also lets WWAN autoconnect work correctly, as long as a connection has a PIN defined. In the future we'll move away from PINs in connections, and to Device Secrets, see below. 3) Read SIM ID and Device ID. We'll use these in the future for Device Secrets, so we can request PIN codes without having to tie them to specific connections. We can also use these to "lock" connections to specific devices+SIM cards, like we do with MAC addresses for other device types.
> rfkill: toggle WWAN rfkill state on user enable/disable - nm_log_warn (LOGD_RFKILL, "(%s): failed to open killswitch device" - "for WiFi radio control", desc); + nm_log_warn (LOGD_RFKILL, "(%s): failed to open killswitch device", desc); > wwan: use modem states instead of enabled/connected properties Do you actually use the fact NM_MODEM_STATE_FAILED < NM_MODEM_STATE_UNKNOWN? If not, I would +typedef enum { /*< underscore_name=nm_modem_state >*/ + NM_MODEM_STATE_UNKNOWN = 0, + NM_MODEM_STATE_FAILED, + NM_MODEM_STATE_INITIALIZING, + NM_MODEM_STATE_LOCKED, + NM_MODEM_STATE_DISABLED, and nm_modem_state_to_string() implement with an static array (as in http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/dhcp-manager/nm-dhcp-client.c#n606) ... oh, now I see. The values correspond to MMModemState -- which simplifies mm_state_to_nm(). Ok, valid. I would do this differently because the states are sorted and compared by numeric values. I would not rely on MM having the same ordering as we want for NM (although it has). anyway: - NM_MODEM_STATE_CONNECTED = 11 + NM_MODEM_STATE_CONNECTED = 11, + /* Pre-empt the state change signal */ + nm_modem_set_state (self, + enabled ? NM_MODEM_STATE_ENABLING : NM_MODEM_STATE_DISABLING, + NULL); could you pass on a reason? also, are we sure that if NM_MODEM_GET_CLASS (self)->set_mm_enabled (self, enabled); fails, that the ENABLING/DISABLING state will be cleared? For example set_mm_enabled_done() does nothing, so I assume the state hangs. (same for modem_disable_ready/modem_enable_ready). In NMModemOld, the string unlock_required is not needed. It could be a boolean instead. Already before the patch set_mm_enabled/set_mm_enabled_done (int NMModemOld) does not ref/unref itself, while NMModemBroadband does it. Maybe it should. + if (nm_modem_get_state (priv->modem) <= NM_MODEM_STATE_INITIALIZING) { + nm_log_dbg (LOGD_MB, "(%s): not available because modem is not ready", + nm_device_get_iface (dev)); + return FALSE; } log the state modem also?^^^ > wwan: disable autoconnect if the given SIM PIN was wrong I would not nm_log_warn for a wrong pin. only <info>. + dbus_g_proxy_begin_call (priv->props_proxy, + "Get", + get_sim_id_done, object, NULL, g_object_ref (object)? Or a weak reference? Make PROP_SIM_ID read-only, or construct-only? Because it gets only updated by the async MM request. Hm, maybe not. Would be nice to have protected setters :) In sim_changed() I would not assert against something that comes via DBUS. At most g_return_if_fail(). get_sim_ready() should g_clear_object(&self->priv->sim) at the beginning. Or is it guaranteed that we don't leak the previous value? The "MMSim *sim;" is not really needed, unless you have later plans with it.
> > 1) makes WWAN rfkill handling consistent with WiFi rfkill handling; when > airplane mode is on, all devices of the class are disabled. When airplane mode > is off, all devices can be enabled. > When airplane mode is on modems are just disabled. Shouldn't we make sure that RF is off in that case as well? i.e disabled + SetPowerState(low)?
(In reply to comment #2) > > > > 1) makes WWAN rfkill handling consistent with WiFi rfkill handling; when > > airplane mode is on, all devices of the class are disabled. When airplane mode > > is off, all devices can be enabled. > > > > When airplane mode is on modems are just disabled. Shouldn't we make sure that > RF is off in that case as well? i.e disabled + SetPowerState(low)? Yes. That would be a future enhancement since NM doesn't currently do that. But it's certainly something that NM should do.
I reworked the branch and pushed fixups for Thomas items, except for the "unlock required" change to boolean. I'd like to keep it a string for now, since in the future we'll distinguish between PIN and PUK locks for auto-unlocking of the modem PIN. I also added a commit for Aleksander's suggestion to move the modem to low power mode when disabling it. THough note that when modems are disabled, NM does set WWAN rfkill to on, so many laptop type modems will drop off the USB bus. But it certainly will do the job for USB devices.
> mobile: make device available whenever it's not rfkilled > This means that autoconnect WWAN connections > will repeatedly attempt to autoconnect even if the modem isn't > registered. If the user doesn't like this, they can obviously > turn off autoconnect, or just hit rfkill. Can't we use nm_settings_connection_set_autoconnect_blocked_reason() ? At any rate, the shell indicator/nm-applet will show that it's repeatedly trying to activate the connection, right? > That all said, however, nm-connection-editor/nm-applet/gnome-shell > and NM's internal connection auto-completion never set autoconnect > by default for WWAN connections, so the only people these issues > would hit are those who've manually twiddled it. but it used to be basically a no-op, right? So people may have twiddled it and forgotten... >+ nm_log_dbg (LOGD_MB, "(%s): not available because WWAN airplane mode is on", >+ nm_device_get_iface (dev)); I'm dubious about logging (even at debug level) from a "get" function... seems like there's a lot of possibility of log spammage. I realize that the code all changes in the next few patches, but at this point in the branch, I don't understand the difference between modem_enabled_cb() and set_enabled(), both of which claim that they are called when the MM enable state changes (in which case, why do we need both?). > wwan: use modem states instead of enabled/connected properties in nm-device-bt.c modem_state_cb: >+ nm_device_state_changed (device, >+ NM_DEVICE_STATE_FAILED, >+ NM_DEVICE_STATE_REASON_USER_REQUESTED); FAILED + USER_REQUESTED ? >+ /* Log initial modem state */ >+ nm_log_info (LOGD_MB, "(%s): modem state '%s'", >+ nm_device_get_iface (device), indentation > is_available (NMDevice *dev) > /* The device is available whenever it's not rfkilled */ no longer true >+ nm_log_info (LOGD_MB, "(%s): modem state changed, '%s' --> '%s' (reason: %s)\n", >+ nm_modem_get_uid (self), indentation (nm-modem.c:nm_modem_set_state()) > wwan: disable autoconnect if the given SIM PIN was wrong > (this doesn't fix auto-connect-with-a-wrong-PIN completely, as > autoconnect is reset when resuming from sleep, but it's a start) If you have nm-policy handle this using nm_settings_connection_set_autoconnect_blocked_reason() (whenever it sees REASON_SIM_PIN_INCORRECT), then it will not get reset on resume. > wwan: read device & SIM identifiers from ModemManager >+ nm_log_warn (LOGD_MB, "(%s) failed to retrieve SIM ojbect: %s", "object"
*** Bug 667488 has been marked as a duplicate of this bug. ***
*** Bug 554538 has been marked as a duplicate of this bug. ***
(In reply to comment #5) > > mobile: make device available whenever it's not rfkilled > > > This means that autoconnect WWAN connections > > will repeatedly attempt to autoconnect even if the modem isn't > > registered. If the user doesn't like this, they can obviously > > turn off autoconnect, or just hit rfkill. > > Can't we use nm_settings_connection_set_autoconnect_blocked_reason() ? Done; see "fixup! wwan: make device available whenever it's not rfkilled" for one approach to that which keeps the device-type-specific logic in the WWAN plugin, instead of putting it into the Policy. > At any rate, the shell indicator/nm-applet will show that it's repeatedly > trying to activate the connection, right? Yes. > > That all said, however, nm-connection-editor/nm-applet/gnome-shell > > and NM's internal connection auto-completion never set autoconnect > > by default for WWAN connections, so the only people these issues > > would hit are those who've manually twiddled it. > > but it used to be basically a no-op, right? So people may have twiddled it and > forgotten... Yeah; though like you suggested above this should be handled now by the autoconnect blocked reason stuff I added. > >+ nm_log_dbg (LOGD_MB, "(%s): not available because WWAN airplane mode is on", > >+ nm_device_get_iface (dev)); > > I'm dubious about logging (even at debug level) from a "get" function... seems > like there's a lot of possibility of log spammage. We talked about this in the review for dcbw/wifi-plugin; OK to leave it here like I did for wifi-plugin? > I realize that the code all changes in the next few patches, but at this point > in the branch, I don't understand the difference between modem_enabled_cb() and > set_enabled(), both of which claim that they are called when the MM enable > state changes (in which case, why do we need both?). I tried to clarify this in the comments for this commit. The previous comments were wrong. modem_enabled_cb() will only be called when something external to NM hits up the MM D-Bus API, and set_enabled() will only be called by NetworkManager internally when somebody does "nmcli r wwan on/off". > > wwan: use modem states instead of enabled/connected properties > > in nm-device-bt.c modem_state_cb: > > >+ nm_device_state_changed (device, > >+ NM_DEVICE_STATE_FAILED, > >+ NM_DEVICE_STATE_REASON_USER_REQUESTED); > > FAILED + USER_REQUESTED ? Fixed to be consistent with nm-device-modem.c (-> DISCONNECTED). > >+ /* Log initial modem state */ > >+ nm_log_info (LOGD_MB, "(%s): modem state '%s'", > >+ nm_device_get_iface (device), > > indentation Fixed. > > is_available (NMDevice *dev) > > /* The device is available whenever it's not rfkilled */ > > no longer true Comment removed. > >+ nm_log_info (LOGD_MB, "(%s): modem state changed, '%s' --> '%s' (reason: %s)\n", > >+ nm_modem_get_uid (self), > > indentation (nm-modem.c:nm_modem_set_state()) Fixed. > > wwan: disable autoconnect if the given SIM PIN was wrong > > > (this doesn't fix auto-connect-with-a-wrong-PIN completely, as > > autoconnect is reset when resuming from sleep, but it's a start) > > If you have nm-policy handle this using > nm_settings_connection_set_autoconnect_blocked_reason() (whenever it sees > REASON_SIM_PIN_INCORRECT), then it will not get reset on resume. Should be fixed by the new autoconnect blocked stuff I think. However, I think autoconnect block *does* get reset on resume? /* Reset retries on all connections so they'll checked on wakeup */ if (sleeping || !enabled) reset_autoconnect_all (policy, NULL); reset_autoconnect_all() clears the block for all connections. > > wwan: read device & SIM identifiers from ModemManager > > >+ nm_log_warn (LOGD_MB, "(%s) failed to retrieve SIM ojbect: %s", > > "object" Fixed. Squashed previous fixups, repushed, please re-review.
Branch looks good to me
basically looks good (In reply to comment #8) > Done; see "fixup! wwan: make device available whenever it's not rfkilled" for > one approach to that which keeps the device-type-specific logic in the WWAN > plugin, instead of putting it into the Policy. Oh, nice. We should probably move some of the other autoconnect-blocked logic into NMDevice too... > > I'm dubious about logging (even at debug level) from a "get" function... seems > > like there's a lot of possibility of log spammage. > > We talked about this in the review for dcbw/wifi-plugin; OK to leave it here > like I did for wifi-plugin? Yes > > I don't understand the difference between modem_enabled_cb() and > > set_enabled(), both of which claim that they are called when the MM enable > > state changes (in which case, why do we need both?). > > I tried to clarify this in the comments for this commit. The previous comments > were wrong. modem_enabled_cb() will only be called when something external to > NM hits up the MM D-Bus API, and set_enabled() will only be called by > NetworkManager internally when somebody does "nmcli r wwan on/off". So which one gets called for hardware rfkill changes? > Should be fixed by the new autoconnect blocked stuff I think. However, I think > autoconnect block *does* get reset on resume? > > /* Reset retries on all connections so they'll checked on wakeup */ > if (sleeping || !enabled) > reset_autoconnect_all (policy, NULL); > > reset_autoconnect_all() clears the block for all connections. Hm... That's the behavior we want for REASON_USER_REQUESTED and REASON_NO_SECRETS. And it's not an awful behavior for REASON_SIM_PIN_INCORRECT I guess.
(In reply to comment #10) > basically looks good > > (In reply to comment #8) > > Done; see "fixup! wwan: make device available whenever it's not rfkilled" for > > one approach to that which keeps the device-type-specific logic in the WWAN > > plugin, instead of putting it into the Policy. > > Oh, nice. We should probably move some of the other autoconnect-blocked logic > into NMDevice too... Yeah, anything that's not generic. For example, if a WiFi connection fails because the supplicant couldn't be started or the supplicant's AddInterface()/GetInterface() call fails, that would be a great candidate for blocking autoconnect to ensure we don't busy-loop trying to start the supplicant (which has happened before). > > > I don't understand the difference between modem_enabled_cb() and > > > set_enabled(), both of which claim that they are called when the MM enable > > > state changes (in which case, why do we need both?). > > > > I tried to clarify this in the comments for this commit. The previous comments > > were wrong. modem_enabled_cb() will only be called when something external to > > NM hits up the MM D-Bus API, and set_enabled() will only be called by > > NetworkManager internally when somebody does "nmcli r wwan on/off". > > So which one gets called for hardware rfkill changes? I tried to clarify that in the comments for the two functions: set_enabled(): + /* Called only by the Manager in response to rfkill switch changes or + * global user WWAN enable/disable preference changes. + */ modem_enabled_cb(): + /* Called when the ModemManager modem enabled state is changed externally + * to NetworkManager (eg something using MM's D-Bus API directly). + */ Does that make things clearer? > > Should be fixed by the new autoconnect blocked stuff I think. However, I think > > autoconnect block *does* get reset on resume? > > > > /* Reset retries on all connections so they'll checked on wakeup */ > > if (sleeping || !enabled) > > reset_autoconnect_all (policy, NULL); > > > > reset_autoconnect_all() clears the block for all connections. > > Hm... That's the behavior we want for REASON_USER_REQUESTED and > REASON_NO_SECRETS. And it's not an awful behavior for REASON_SIM_PIN_INCORRECT > I guess. Yeah, I'd agree for now... though it could mean that after three resumes, your SIM is locked, and you need to enter the PUK code. NetworkManager *never* tries to enter the PUK though, so it's never ever going to completely brick your SIM, at least.
(In reply to comment #11) > I tried to clarify that in the comments for the two functions: ... > Does that make things clearer? Yes. Sorry, I read the comments before and for some reason thought they still left some cases ambiguous.
Squashed and merged to master.