Re: ModemManager: [PATCH 2/2] Improvements to SIM PIN handling

[Eric Shienbrood <ers google com>]

Why is the new property in the Modem.Gsm.Card interface, and not in the
Modem one (close to UnlockRequired and UnlockRetries). Same for the new
MMModemGsmFacility flags, why not have them in the Modem interface. Some
locks come from the card, but not all, so it's not really card-specific.
Maybe there's some reason to have them split like that?

 

I did that mainly because the SendPin, SendPuk, EnablePin, and ChangePin methods are part of the Modem.Gsm.Card interface. It's true that some of the locks apply to the device rather than the card, but they're all GSM (or more properly 3gpp) specific. I think we'll need to leave it to the new API to get this right - as Dan points out, these should all be part of the 3gpp interface.

For the new API, it would probably be a good idea to have the
UnlockRequired property not return a string, and use new
MMModemLockFacility enum/flags (equivalent to yours MMModemGsmFacility,
not just GSM, or is this a GSM-only thing?). The same enum/flags would
be the one used to identify enabled items in the EnabledFacilityLocks
bitfield.

Sounds like a good idea for the new API.

 

>
> +    <tp:flags name="MM_MODEM_GSM_FACILITY"
> value-prefix="MM_MODEM_GSM_FACILITY" type="u">
> +      <tp:docstring>
> +        A bitfield describing which facilities have a lock enabled,
> i.e.,
> +        requires a pin or unlock code. The facilities include the
> +        personalizations (device locks) described in 3GPP spec TS
> 22.022,
> +        and the PIN and PIN2 locks, which are SIM locks.
> +      </tp:docstring>
> +      <tp:flag suffix="NONE" value="0x0">
> +        <tp:docstring>No facility</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="SIM" value="0x1">
> +        <tp:docstring>SIM lock</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="FIXED_DIALING" value="0x2">
> +        <tp:docstring>Fixed dialing (PIN2) SIM lock</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="PH_SIM" value="0x4">
> +        <tp:docstring>Device is locked to a specific
> SIM</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="PH_FSIM" value="0x8">
> +        <tp:docstring>Device is locked to first SIM
> inserted</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="NET_PERS" value="0x10">
> +        <tp:docstring>Network personalization</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="NET_SUB_PERS" value="0x20">
> +        <tp:docstring>Network subset personalization</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="PROVIDER_PERS" value="0x40">
> +        <tp:docstring>Service provider personalization</tp:docstring>
> +      </tp:flag>
> +      <tp:flag suffix="CORP_PERS" value="0x80">
> +        <tp:docstring>Corporate personalization</tp:docstring>
> +      </tp:flag>


Are these all possibly facility locks that we may find? Or just the ones
currently checked during the modem setup? The list of possible string
replies in "UnlockRequired" has a longer list of values, so it may
happen that we get a string in UnlockRequired without a direct enum
value in MmModemGsmFacility. Is that desired?

The facility locks aren't 1:1 with the UnlockRequired list because the latter includes the PUK versions of each lock. Most of the facility locks have both a PIN and a PUK that can come into play after too many PIN retries.

> diff --git a/src/mm-generic-gsm.c b/src/mm-generic-gsm.c
> index 82dcf00..36490f4 100644
> --- a/src/mm-generic-gsm.c
> +++ b/src/mm-generic-gsm.c
> @@ -129,6 +129,13 @@ typedef struct {
>
>      /* SMS */
>      GHashTable *sms_present;
> +
> +    /* Facility locks */
> +    MMModemGsmFacility supported_facilities;
> +    MMModemGsmFacility enabled_facilities;
> +    MMModemGsmFacility pending_facility;
> +    gboolean pending_facility_enable;
> +    guint facilities_idx;
>  } MMGenericGsmPrivate;
>
>  static void get_registration_status (MMAtSerialPort *port,
> MMCallbackInfo *info);
> @@ -799,6 +806,29 @@ initial_info_check (MMGenericGsm *self)
>      }
>  }
>
> +static void clck_cb (MMAtSerialPort *port, GString *response, GError
> *error, gpointer user_data);
> +
> +static void
> +initial_facility_lock_check (MMGenericGsm *self) {
> +    GError *error = NULL;
> +    MMGenericGsmPrivate *priv;
> +
> +    g_return_if_fail (MM_IS_GENERIC_GSM (self));
> +    priv = MM_GENERIC_GSM_GET_PRIVATE (self);
> +
> +    g_return_if_fail (priv->primary != NULL);
> +
> +    if (mm_serial_port_open (MM_SERIAL_PORT (priv->primary), &error))
> {
> +        mm_at_serial_port_queue_command (priv->primary, "+CLCK=?", 3,
> clck_cb, self);
> +    } else {
> +        g_warning ("%s: failed to open serial port: (%d) %s",
> +                   __func__,
> +                   error ? error->code : -1,
> +                   error && error->message ? error->message :
> "(unknown)");
> +        g_clear_error (&error);
> +    }
> +}
> +
>  static gboolean
>  owns_port (MMModem *modem, const char *subsys, const char *name)
>  {
> @@ -872,6 +902,9 @@ mm_generic_gsm_grab_port (MMGenericGsm *self,
>               */
>              initial_pin_check (self);
>
> +            /* Determine what facility locks are supported */
> +            initial_facility_lock_check (self);
> +
>          } else if (ptype == MM_PORT_TYPE_SECONDARY)
>              priv->secondary = MM_AT_SERIAL_PORT (port);
>      } else if (MM_IS_QCDM_SERIAL_PORT (port)) {
> @@ -1437,6 +1470,91 @@ cusd_enable_cb (MMAtSerialPort *port,
>      MM_GENERIC_GSM_GET_PRIVATE (user_data)->ussd_enabled = TRUE;
>  }
>
> +static void get_facility_lock_state (MMAtSerialPort *port,
> MMGenericGsm *self);
> +
> +static void
> +get_facility_lock_state_done (MMAtSerialPort *port,
> +                              GString *response,
> +                              GError *error,
> +                              gpointer user_data)
> +{
> +    MMGenericGsm *self;
> +    MMGenericGsmPrivate *priv;
> +    gboolean enabled = FALSE;
> +
> +    if (error)
> +        return;
> +
> +    self = MM_GENERIC_GSM (user_data);
> +    priv = MM_GENERIC_GSM_GET_PRIVATE (self);
> +
> +    if (mm_gsm_parse_clck_response (response->str, &enabled)) {
> +        if (enabled)
> +            priv->enabled_facilities |= priv->pending_facility;
> +        else
> +            priv->enabled_facilities &= ~priv->pending_facility;
> +    }
> +    priv->pending_facility = MM_MODEM_GSM_FACILITY_NONE;
> +    ++priv->facilities_idx;
> +    /* Get the state of the next facility */
> +    get_facility_lock_state (port, self);
> +}
> +
> +static void
> +get_facility_lock_state (MMAtSerialPort *port, MMGenericGsm *self)
> +{
> +    MMModemGsmFacility found = MM_MODEM_GSM_FACILITY_NONE;
> +    gchar *cmd;
> +    MMGenericGsmPrivate *priv;
> +    gchar *facility_name;
> +
> +    priv = MM_GENERIC_GSM_GET_PRIVATE (self);
> +    while (priv->facilities_idx < sizeof (MMModemGsmFacility)*8) {
> +        guint32 facility = 1 << priv->facilities_idx;
> +        if (priv->supported_facilities & facility) {
> +            found = facility;
> +            break;
> +        }
> +        ++priv->facilities_idx;
> +    }
> +    if (found != MM_MODEM_GSM_FACILITY_NONE) {
> +        facility_name = mm_gsm_get_facility_name (found);
> +        if (facility_name != NULL) {
> +            priv->pending_facility = found;
> +            cmd = g_strdup_printf ("+CLCK=\"%s\",2", facility_name);
> +            mm_at_serial_port_queue_command (port, cmd, 3,
> get_facility_lock_state_done, self);
> +            g_free (cmd);
> +            return;
> +        }
> +    }
> +    mm_serial_port_close (MM_SERIAL_PORT (port));
> +}
> +
> +static void
> +clck_cb (MMAtSerialPort *port,
> +         GString *response,
> +         GError *error,
> +         gpointer user_data)
> +{
> +    MMGenericGsm *self;
> +    MMGenericGsmPrivate *priv;
> +    MMModemGsmFacility facilities;
> +
> +    if (error)
> +        return;
> +
> +    self = MM_GENERIC_GSM (user_data);
> +    priv = MM_GENERIC_GSM_GET_PRIVATE (self);
> +
> +    if (mm_gsm_parse_clck_test_response (response->str, &facilities))
> {
> +        priv->supported_facilities = facilities;
> +        priv->facilities_idx = 0;
> +        get_facility_lock_state (port, self);
> +    } else {
> +        mm_serial_port_close (MM_SERIAL_PORT (port));
> +    }
> +}

So, first we query the currently supported locks, with AT+CLCK?; and
then we go one by one checking its status. Instead of going one by one,
waiting for a check to end to launch the next one, why not just
port_queue_command() all AT+CLCK checks we need all together, and go
filling-in the priv->enabled_facilities as we get replies of each
command send? That would let us get rid of priv->facilities_idx, which
is only used during this loop check.

Done. It required keeping more state, since the response to AT+CLCK doesn't include the name of the facility that it's reporting on, and we need to know when we can close the port, but it still looks like an improvement over what I had before.

 

>
>  static void
> -enable_pin_done (MMAtSerialPort *port,
> +pin_operation_done (MMAtSerialPort *port,
>                   GString *response,
>                   GError *error,
>                   gpointer user_data)

Some extra whitespaces needed here to align the method members.

Done.

 

>  {
>      MMCallbackInfo *info = (MMCallbackInfo *) user_data;
> +    MMGenericGsmPrivate *priv;
> +    MMModem *modem;
>
>      /* If the modem has already been removed, return without
>       * scheduling callback */
>      if (mm_callback_info_check_modem_removed (info))
>          return;
>
> -    update_pin_puk_status(info, error);
> +    modem = info->modem;
> +    priv = MM_GENERIC_GSM_GET_PRIVATE (modem);
> +    if (!error) {
> +        if (priv->pending_facility != MM_MODEM_GSM_FACILITY_NONE) {
> +            MMModemGsmFacility old = priv->enabled_facilities;
> +            if (priv->pending_facility_enable)
> +                priv->enabled_facilities |= priv->pending_facility;
> +            else
> +                priv->enabled_facilities &= ~priv->pending_facility;
> +            if (priv->enabled_facilities != old)
> +                g_object_notify (G_OBJECT (modem),
> +
> MM_MODEM_GSM_CARD_ENABLED_FACILITY_LOCKS);
> +        }
> +    }
> +    priv->pending_facility = MM_MODEM_GSM_FACILITY_NONE;
> +    update_pin_puk_status (modem, error);
>      if (error)
>          info->error = g_error_copy (error);
>      mm_callback_info_schedule (info);
> @@ -2417,30 +2555,13 @@ enable_pin (MMModemGsmCard *modem,
>
>      info = mm_callback_info_new (MM_MODEM (modem), callback,
> user_data);
>      command = g_strdup_printf ("+CLCK=\"SC\",%d,\"%s\"", enabled ?
> 1 : 0, pin);
> -    mm_at_serial_port_queue_command (priv->primary, command, 3,
> enable_pin_done, info);
> +    priv->pending_facility = MM_MODEM_GSM_FACILITY_SIM;
> +    priv->pending_facility_enable = enabled;
> +    mm_at_serial_port_queue_command (priv->primary, command, 3,
> pin_operation_done, info);
>      g_free (command);
>  }


priv->pending_facility and priv->pending_facility_enabled are just used
here when we need to update the enabled facilities after a user request
to enable/disable pin check. Instead of having them in
MMGenericGsmPrivate, you can better associate both values with
mm_callback_info_set_data() to the info passed in queue_command() and
then use mm_callback_info_get_data() in the callback to get them back.

Thanks, that's much better. I'm still learning my way around the code.

 

> diff --git a/src/mm-modem-gsm-card.h b/src/mm-modem-gsm-card.h
> index 9716bf7..bee9000 100644
> --- a/src/mm-modem-gsm-card.h
> +++ b/src/mm-modem-gsm-card.h
> @@ -19,6 +19,8 @@
>
>  #include <mm-modem.h>
>
> +#define MM_MODEM_GSM_CARD_DBUS_INTERFACE
> "org.freedesktop.ModemManager.Modem.Gsm.Card"
> +


You can use the MM_DBUS_INTERFACE_MODEM_GSM_CARD from ModemManager.h,
instead of defining a new one. I see other files defining custom symbols
for the DBus Interface paths, but that shouldn't be needed as they are
already in the autogenerated ModemManager.h. Not a big deal, will get
this changed in the port to the new API.

OK. All the other code in the vicinity where this is used in mm_generic_gsm_init() is using the custom-defined symbols. So I will leave this as is and let all the uses be changed at one time later.

 

> --
> 1.7.3.1
--
Aleksander