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

[Ben Chan]

Hi Dan and Aleksander,

I overlooked one thing when I revised the patches based on Dan's
suggestion,  and also missed that in my testing :-(

MMSimMbim:update_modem_unlock_retries() calls
mm_iface_modem_update_unlock_retries() to report remaining attempts
PIN1.  That won't affect MMBroadbandModemMbimPriv::sim_pin_retries. My
original approach was to handle all MMUnlockRetries updates via
MMIfaceModem.

If we prefer caching PIN1 retries in MMBroadbandModemMbim,
MMBroadbandModemMbim needs to provide a method (e.g.
mm_broadband_modem_mbm_set_sim_pin_retries) for MMSimMbim to propagate
the information.  Would you prefer that or should I go back to my
original approach?

Thanks,
Ben


On Fri, Sep 8, 2017 at 1:31 AM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
> 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