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

[Ben Chan]

On Tue, Aug 8, 2017 at 12:43 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.

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

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

#1: iface-modem: add mm_iface_modem_get_unlock_retries helper
#2: iface-modem: export update_unlock_retries outside MMIfaceModem
#3: broadband-modem-mbim: preserve unlock retries for multiple lock types
#4: sim-mbim: update unlock retries information after PIN operations

Thanks,
Ben

>
>
> --
> Aleksander
> https://aleksander.es