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

[Aleksander Morgado]

On Fri, Aug 11, 2017 at 1:02 AM, Ben Chan <benchan at chromium.org> wrote:
> On Thu, Aug 10, 2017 at 3:34 PM, Aleksander Morgado
> <aleksander at aleksander.es> wrote:
>>>>> 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.
>>>
>>
>> I was actually suggesting we leave this as it is now (i.e. query pin
>> lock and retries, and just use that, without merging)...
>>
>> From a user point of view, we have the main features we need:
>>   * If locked, we know the lock enabled and the amount of unlock retries left.
>>   * We know if a lock unlock operation succeeds or fails, and if it
>> fails we know the amount of unlock retries left.
>>   * We know if a lock enable/disable operation succeeds or fails.
>>
>> What we don't know is the amount of retries left if the lock
>> enable/disable operation fails. We could try to hack something in MM
>> to keep some of that logic, but then, if the device/system is rebooted
>> between e.g. failed lock enable attempts, we're back without knowing
>> anything until we let the user retry again. There's no perfect
>> solution either way, not sure.
>
> I totally agree that we don't have a perfect solution here. The
> workaround I propose here is a bit of a hack. The part that I'm not
> very comfortable with the current situation is that an UI can't
> indicate the remaining attempts for PIN1 after a user tries to enable
> the SIM lock with the wrong PIN (even though we've got the infor about
> PIN1, which is overwritten by info about PIN2).
>
> What makes the situation worse from a user perspective is how
> counter-intuitive the SIM lock mechanism is. Some users may actually
> think that they should enter a new PIN when enabling the SIM lock.
> They may not know that a SIM may come with a default PIN. Or they may
> believe that when they previously disabled the SIM lock, the PIN was
> clear.  So knowing the remaining attempts for PIN1 when they try to
> enable the SIM lock would be quite helpful as they may stop entering a
> new PIN after the first failed attempt.
>
> The SIM lock mechanism is dated and it seems overlooked that MBIM
> provides no way to explicitly query the remaining attempts for PIN1.
> Unfortunately, we will live with these issues until the SIM locking
> mechanism is deprecated.
>
> Anyway, I'd try one more time :)  I've modified the patch to address
> the issues with PIN-PUK transition and tried it on a SIM (finally I
> got the PUK code :p).  Here's the mmcli output.  Thanks again for
> reviewing. Let me know how you think.
>

These got merged to git master.

-- 
Aleksander
https://aleksander.es