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

[Ben Chan]

On Tue, Aug 8, 2017 at 11:15 AM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
> Hey,
>
>>> It really is unfortunate that the MBIM_CID_PIN query only reports the
>>> information of the "currently applicable" PIN type, although the truth
>>> is that that's the only useful one.
>>
>> Yes, indeed. It would have been much better if MBIM_CID_PIN query
>> could take a PIN type as argument. I'd expect PIN1 (PUK1) is likely
>> the one being used and thus the most relevant / useful one in typical
>> scenarios.
>>
>> Here's the issue: If PIN1 is currently disabled, some modem reports
>> the info about another PIN, say PIN2, to a MBIM_CID_PIN query. When we
>> try to enable PIN1, we won't know the retries count until after
>> entering a PIN once. Suppose the retries count doesn't go to 0 after a
>> wrong attempt (i.e. not in sim-puk state), we still can't query MM for
>> the retries count of PIN1.
>>
>
> Aha, that was what I was missing, the PIN disabled->enabled transition
> is the problematic one, because with PIN disabled it would report you
> PIN2 attempts. But, if we do the disabled->enabled transition,
> couldn't we reload unlock attempts in that moment (after enabling
> SIM-PIN)? I'm assuming that the "current" PIN would change to PIN1
> right away and we would get reported the number of attempts of PIN1;
> without even trying to send a PIN once.
>

Don't we need to enter the correct PIN to re-enable PIN1? so the
disabled->enabled transition is essentially a MBIM_CID_PIN set
operation. Regardless of whether the operation succeeds or fails, the
response should report the latest remaining attempts for PIN1, which
we want to propagate that to UnlockRetries. That's why patch #4 always
calls mm_iface_modem_update_unlock_retries() if we can parse the
response, regardless of whether the operation succeeds or not.

>>>
>>> This change you're proposing makes the unlock retries updated on the
>>> send PIN completion, e.g. if it fails we get right away the remaining
>>> attempts of the next PIN applicable (either the same one or another
>>> one). I'm not sure the PIN type included in the response is actually
>>> the same one that was sent in the request, I understand it may not
>>> need to be, although didn't test. E.g. I'm assuming if the last
>>> SIM-PIN attempt is sent we may get back in the response SIM-PUK and
>>> the number of attempts for that one (instead of SIM-PIN attempts 0).
>>
>> I haven't tested the SIM-PIN to SIM-PUK transition yet (as it's really
>> a frustrating experience to obtain the PUK code from carriers, which I
>> would like to avoid whenever possible :-)   So I'm not sure if a modem
>> would report the remaining attempts of PIN1 or PUK1 in the response to
>> MBIM_CID_PIN set operation during that transition. But even if the
>> modem reports PUK1 instead of PIN1, it wouldn't be too bad if MM
>> reports something like "sim-pin (1), sim-puk (10)" in UnlockRetries,
>> instead of "sim-pin (0), sim-puk (10)". Anyway we can't no longer deal
>> with PIN1 until a correct PUK1 is entered.  Once the modem receives a
>> correct PUK1, we will likely get the correct retries count for PIN1.
>>
>
> Oh, I have SIM-PINs and PUKs, I will do tests here with a EM7455
> with/without your patches and see what happens.
>
>>>
>>> Before this change, we had the unlock retries updated as part of
>>> update_lock_info() triggered from the IfaceModem, and this happens
>>> after sending PIN and in some other cases as well.
>>>
>>> In either case, we're completely replacing the UnlockRetries exposed
>>> in the IfaceModem interface; and actually, if you replace the
>>> UnlockRetries in the send completion, wouldn't it be anyway
>>> overwritten as part of update_lock_info() just a bit later?
>>
>> Patch #4 modifies MMSimMbim to propagate the retries count observed
>> from the response of a MBIM_CID_PIN set operation to UnlockRetries of
>> IfaceModem.  As you said, a subsequent update_lock_info() could
>> overwrite UnlockRetries of IfaceModem.  For that specific reason,
>> patch #1 introduces mm_iface_modem_get_unlock_retries() for querying
>> existing value of UnlockRetries from IfaceModem, while patch #3 and #4
>> use a read-modify-write approach to update UnlockRetries instead of
>> simply writing a new UnlockRetries. Am I missing something in the
>> patch set?
>>
>
> Ah!
>
> - retries = mm_unlock_retries_new ();
> + retries = mm_iface_modem_get_unlock_retries (self);
>
> That will preserve whatever was before; understood.
>
> --
> Aleksander
> https://aleksander.es