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

[Dan Williams <dcbw redhat com>]

On Wed, 2011-11-23 at 20:14 +0100, Aleksander Morgado wrote:
> Hey hey,
> 
> > 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.

> >       * 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 :)

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

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

Sounds OK to me, any thoughts Eric?

Dan