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

[Aleksander Morgado]

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.
>

Nope, sorry, next time I'll keep it.

>> 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.

Ah, yes, that is true. This limitation is imposed by the MBIM protocol
itself, and there is no fix ever possible in ModemManager to solve
that... unless we provide plugin-specific AT fallbacks in case there's
an AT port around... :/

> 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.
>

Yes, I agree, keeping the information returned in a response is
useful, but why not fully overwrite the list of unlock attempts
instead of merging as you were trying?

> 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);
>

I believe I understand your reasoning; we only want to merge the
attempts with what we had before when a query gives us e.g. PIN2
attempts but we failed a couple of PIN1 enable operations with an
invalid PIN1 (so PIN1 was decreased but we're still good to go as PIN1
is disabled and we get reported PIN2 when we query). But I was also
sure before that the logic was right until I played with it myself :)

-- 
Aleksander
https://aleksander.es