[review] https://github.com/cbchan/ModemManager/tree/sim-mbim-unlock-retries

[Ben Chan]

On Thu, Aug 10, 2017 at 2:28 AM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
>
> Hey
>
> Hum... looks like the patched logic is actually way more confusing
> than the original one... See attached logs (the -patched file is after
> your changes).
>

Thanks for taking time to test the patch set.  I should keep calling
the carrier to get a PUK code...

Do you by chance keep the MBIM debug trace during your debugging? That
would be very helpful.

> Several issues in your patches:
>   * When we get SIM-PUK locked after 3 SIM-PIN attempts, we're leaving
> the old "SIM-PIN 1 attempt". We could try to detect that and, as soon
> as we're asked SIM-PUK, set an explicit "SIM-PIN 0 attempts"
> ourselves.
>   * If we then insert SIM-PUK correctly after some bad attempts, we're
> leaving the old "SIM-PUK X attempts". See the "Insert PUK ok and
> recover PIN..." step in the -patched file; it's a bit of a mess, as we
> have there the old "SIM-PIN 1 attempt" and the old "SIM-PUK 9
> attempts", and actually we don't have *any* lock at that moment, they
> both should be at their max number of attempts.
>
> So, we have issues when going from SIM-PIN required -> SIM-PUK
> required and also when successfully entering either SIM-PIN or SIM-PUK
> after some previous failed attempts.
>
> Compared this, to the original log, where the only issue is that if
> the modem has SIM-PIN disabled and we want to enable PIN we don't
> explicitly know how many attempts we have (although we always have the
> MAX attempts in that case anyway)...

Unfortunately, when PIN1 is disabled, it's not necessarily true that
we have the max no. of remaining attempts for PIN1 as a prior enable
PIN1 operation with a wrong PIN could have decreased that. With the
unfortunate limitation imposed by MBIM_CID_PIN query, I don't think we
can always get the correct no. of remaining attempts for PIN1 (maybe
that's by design, who knows). For example, we can't determine the
correct remaining attempts for a disabled PIN1 when MM starts.
However, we may be able to improve the situation a bit when MM is
running by making mm_iface_modem_update_lock_info preserve what we
have learned from a prior MBIM_CID_PIN response.  That's what I'm
trying to address with this patch set.

Now I take another look at the issue, it appears that we only need to
specially handle updating/preserving PIN1 from MM's perspective. There
is no need to query the remaining attempts for PUK1 (or other PIN
types for that matter as MM doesn't use them).

In MMSimMbim::update_modem_unlock_retries(), it seems like we could simply do:

  if (pin_type == MBIM_PIN_TYPE_PIN1)
      unlock_retries = mm_iface_modem_get_unlock_retries
(MM_IFACE_MODEM (modem));
  else
      unlock_retries = mm_unlock_retries_new();
  mm_unlock_retries_set (unlock_retries,
mm_modem_lock_from_mbim_pin_type (pin_type), remaining_attempts);
  mm_iface_modem_update_unlock_retries (MM_IFACE_MODEM (modem), unlock_retries);

>
> Don't know what to say, I think the current logic without your patches
> make more sense right now...
>
>
> --
> Aleksander
> https://aleksander.es