Re: ModemManager [PATCH 2/2] Take 3 of: Improvements to SIM PIN handling - Add EnabledFacilityLocks property

[Aleksander Morgado <aleksander lanedo com>]

> > > I guess I don't have a problem with merging them, although strictly
> > > speaking they refer to different things - locks vs. the PINs needed to
> > > unlock those locks.
> > > 
> > Yeah, you're totally right... but at the end the PIN/PUK codes are
> > directly related to facilities. E.g "SIM facility", has "PIN code" and
> > "PUK code". But it really seems broken to use the enum of codes as index
> > for facilities, not the best idea.
> > > 
> > > Two other points to consider regarding lock/PIN handling in the new
> > > API:
> > >       * PIN, PUK, PIN2, and PUK2 are PINs for SIM locks. The rest are
> > >         device locks, so having them belong to the *.Sim interface
> > >         doesn't seem completely right. But 3gpp gloms them all
> > >         together, so maybe we should just live with this.
> > 
> > In the 0.6 API they are not handled in the SIM interface. UnlockRetries
> > and PinRetryCounts are handled in the Modem interface, along with
> > UnlockRequired; and the EnabledFacilityLocks one is handled in the 3GPP
> > interface. Anyway, I'm not very convinced yet, on why we do keep
> > EnabledFacilityLocks in one interface and
> > UnlockRetries/PinRetryCounts/UnlockRequired in another one. Shouldn't we
> > have all in the 3GPP-specific interface? Are there CDMA-only modems with
> > facility locks?
> 
> In Russia and Korea there's RUIMs which are basically SIMs for CDMA and
> aren't 3GPP at all.  They may use the same basic interfaces, but there's
> a lot of Qualcomm-iness in there.  I kept the PIN/Lock stuff on the
> Modem interface so that we could more easily support these types of
> devices in the future.
> 

I see, good to know.

> > >       * Shouldn't we add an argument to EnablePin and ChangePin that
> > >         specifies the lock to which the operation should apply? This
> > >         would mirror the CLCK and CPWD AT commands, which take the
> > >         facility name as an argument. This would correct what seems to
> > >         be an oversight in the current API.
> > 
> > Not an oversight, just that the main use case is to be able to
> > Enable/Change the SIM PIN, and the idea was to keep it simple. But now
> > that we report if given facility locks are enabled or disabled, it could
> > make sense to also allow trying to enable/disable all reported ones. We
> > would need to move Enable() and Change() out from the SIM object and
> > back to some interface in the Modem object.
> > 
> > And now that you talk about EnablePin() and ChangePin(); the same logic
> > could be applied to SendPin() and SendPuk(). SendPin()/SendPuk() should
> > be used only when UnlockRequired says that there is something to unlock;
> > they are actually backed by the same CPIN command. So, UnlockRequired
> > may be saying "ph-sim-pin" (phone-specific PIN required), and we can
> > then use SendPin() to send that specific code to the modem. Therefore,
> > SendPin() is really not related to the SIM, but to UnlockRequired, and
> > so they ought to be in the same interface (in the 0.6 API, SendPin() and
> > SendPuk() are managed in the SIM object).
> 
> That makes some sense.  They were originally added when the world was a
> simpler place :)
> 

If we do move SendPin(), SendPuk, ChangePin(), EnablePin() out of the
SIM object, not sure if it is worth having it around.

> > 
> > And a new brainstorm here... given that we're going to break the DBus
> > API, we can try to rework and consolidate all related properties.
> > 
> > We could assume that each facility has 2 codes (PIN, PUK), and that the
> > PUK code will only be required once all PIN attempts have been used
> > (didn't look at any documentation, but that seems to me always the
> > case).
> 
> That looks to be the case, yes.  The non-call-barring CLCK facilities
> have corresponding lock codes that would be reported by +CPIN?.
> 
> > If we do assume this, we could then setup a "FacilityLocks" dictionary
> > with signature "a{ubuu}" where:
> >  * The uint32 in the first position identifies the facility.
> >  * The boolean in the second position tells whether the facility has a
> > lock.
> >  * The uint32 in the third position has the PIN retry count.
> >  * The uint32 in the fourth position has the PUK retry count.
> > 
> > Then, we could have UnlockRequired be just a uint32 where we identify
> > the 'facility' locked (as opposed to identifying which pin code we
> > need).
> 
> I assume you're suggesting we do an enum for facilities?  (which is
> fine)
> 

We already have that enum, MMModemGsmFacility is called in git master.

> > So, if we get "SIM" facility in UnlockRequired, we could then check the
> > corresponding entry in the FacilityLocks dictionary and see how many
> > retry counts we have for PIN and PUK. If retries for PIN is 0; it means
> > we need the PUK code, unless PUK retries is also 0, which would mean we
> > need to buy a new SIM card. Instead of just the key of the dictionary
> > entry, we could also have the UnlockRequired property be a (ubuu); which
> > would contain the whole entry of the dictionary.
> > 

I'm wondering if it would be a good idea to have a Modem.Lock interface,
with all this stuff (methods to send/enable/change PINs and PUKs,
facility lock enabled flags, pin retry counts, unlock-required value...)

-- 
Aleksander