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

[Ben Chan]

On Thu, Aug 10, 2017 at 11:09 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.
>>
>
> 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?

Let me make sure I understand your proposal. Are you suggesting the following?

(1) When MMSimMbim receives a response to a MBIM_CID_PIN set
operation, it always overwrite IfaceModem's MMUnlockRetries as
MBIM_CID_PIN provides the most relevant info at that point, which I
think makes sense (e.g. we really don't care about PIN2 in MM, at
least for now. And we don't care about PIN1 when PUK1 is active)

(2) When MMBroadbandModemMbim later reload unlock retries via a
MBIM_CID_PIN query, we try to preserve PIN1 in IfaceModem's
MMUnlockRetries if MBIM_CID_PIN query reports PIN2, for example.


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