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

[Aleksander Morgado]

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.

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