Closed Bug 772357 Opened 7 years ago Closed 7 years ago

B2G RIL: Support network depersonalization

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: jaoo, Assigned: jaoo)

References

Details

(Keywords: feature, Whiteboard: [LOE:S][WebAPI:P0])

Attachments

(4 files, 5 obsolete files)

This capability is about allowing the user to deactivate that the
phone is personalised to a particular network (carrier). The
network personalisation feature is controlled by a Network
Control Key, (NCK) which has to be entered into the mobile
equipment (ME) in order to network de-personalise it.

For more information please take a look at 3GPP TS 22.022
document (section 5) [1] and [2].

[1] http://www.3gpp.org/ftp/Specs/html-info/22022.htm
[2] http://en.wikipedia.org/wiki/SIM_lock
Assignee: nobody → josea.olivera
Blocks: b2g-ril
Attached patch WIP v1 (obsolete) — Splinter Review
Here is a WIP for this capability. The implementation is pretty easy and looks
similar -for example- to the SIM PIN lock one. I just ask for feedback
especially from philikon and mvines. The tricky part here is how to test it
because we need a phone with that network personalisation enabled.
Attachment #640530 - Flags: feedback?(philipp)
Attachment #640530 - Flags: feedback?(mvines)
Comment on attachment 640530 [details] [diff] [review]
WIP v1

Review of attachment 640530 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/ril_worker.js
@@ +902,5 @@
> +   *
> +   * @param requestId
> +   *        Request Id from RadioInterfaceLayer.
> +   */
> +  getICCNetworkControlKey: function getICCNetworkControlKey(options) {

Not sure I like the naming of this method. AFAICT, this fetches the *state* of the network control key, not the actual network control key, right?

@@ +2851,5 @@
> +  this.sendDOMMessage({type: "iccunlockcardlock",
> +                       lockType: "nck",
> +                       result: options.rilRequestError == 0 ? true : false,
> +                       retryCount: length ? Buf.readUint32List()[0] : -1,
> +                       requestId: options.requestId});

Please reuse the `options` object, that way you don't have to copy over stuff like `requestId` and `lockType`.

Also, I suggest renaming s/result/success/. Speaking of the success flag, I realize that error handling for REQUEST_QUERY_FACILITY_LOCK is pretty much non-existent. We should deal with that in a follow-up. Just silently dropping errors is not a good idea, we should dispatch the error event on the DOMRequest.
Attachment #640530 - Flags: feedback?(philipp) → feedback+
(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> > +  getICCNetworkControlKey: function getICCNetworkControlKey(options) {
> 
> Not sure I like the naming of this method. AFAICT, this fetches the *state*
> of the network control key, not the actual network control key, right?

Yes, you are right. The reason of that name is because for fetching the state of the SIM PIN lock the naming of that function is similar 'getICCPinLock'. It does not retrieve the value of that PIN but the *state*. Should I change the name of the function to something like 'getICCNetworkControlKeyState'?
 
> @@ +2851,5 @@
> > +  this.sendDOMMessage({type: "iccunlockcardlock",
> > +                       lockType: "nck",
> > +                       result: options.rilRequestError == 0 ? true : false,
> > +                       retryCount: length ? Buf.readUint32List()[0] : -1,
> > +                       requestId: options.requestId});
> 
> Please reuse the `options` object, that way you don't have to copy over
> stuff like `requestId` and `lockType`.

I did the same here that what is done in functions REQUEST_ENTER_* for maintaining the same way of handling the response. Is that what you suggest?

+RIL[REQUEST_ENTER_NETWORK_DEPERSONALIZATION] = 
+  function REQUEST_ENTER_NETWORK_DEPERSONALIZATION(length, options) {
+  options.type = "iccunlockcardlock";
+  options.result = options.rilRequestError == 0 ? true : false;
+  options.retryCount = length ? Buf.readUint32List()[0] : -1;
+  this.sendDOMMessage(options);
+};
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #3)
> (In reply to Philipp von Weitershausen [:philikon] from comment #2)
> > > +  getICCNetworkControlKey: function getICCNetworkControlKey(options) {
> > 
> > Not sure I like the naming of this method. AFAICT, this fetches the *state*
> > of the network control key, not the actual network control key, right?
> 
> Yes, you are right. The reason of that name is because for fetching the
> state of the SIM PIN lock the naming of that function is similar
> 'getICCPinLock'. It does not retrieve the value of that PIN but the *state*.
> Should I change the name of the function to something like
> 'getICCNetworkControlKeyState'?

Hmm, I see. It's probably fine. I would almost want to rename `getICCPinLock` to `getICCPinLockState`, but it's probably not worth the trouble.

> I did the same here that what is done in functions REQUEST_ENTER_* for
> maintaining the same way of handling the response.

Sure. They should be fixed, too, then. (In a follow-up bug. Probably the one I was talking about in comment 2 regarding error handling. Can you file that please?)

> Is that what you suggest?
> 
> +RIL[REQUEST_ENTER_NETWORK_DEPERSONALIZATION] = 
> +  function REQUEST_ENTER_NETWORK_DEPERSONALIZATION(length, options) {
> +  options.type = "iccunlockcardlock";
> +  options.result = options.rilRequestError == 0 ? true : false;
> +  options.retryCount = length ? Buf.readUint32List()[0] : -1;
> +  this.sendDOMMessage(options);
> +};

Yes, although s/result/success/ and error handling, please!
Depends on: 774114
Attached patch WIP v2 (obsolete) — Splinter Review
This patch depends on bug 774114. This is still a WIP patch. It needs to be tested on a phone. Just need some feedback before continuing.
Attachment #640530 - Attachment is obsolete: true
Attachment #640530 - Flags: feedback?(mvines)
Confirmed with jaoo. 
This one should be a MUST have for v1.
(In reply to khu from comment #6)
> Confirmed with jaoo. 
> This one should be a MUST have for v1.

Nom'ing for basecamp then.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
I'll spend some time on this bug and bug 774114 during the rest of the week.
Attached patch WIP v3 (obsolete) — Splinter Review
Yoshi, Michael: need some feedback from you. After testing this patch on otoro I have noticed that It does not work at all. I believe the logic implemented here for fetching the state of the network control key and for disabling the SIM-lock capability built into a phone is right but it does not work since that capability is not built into the otoro by its manufacturer. Is that possible? Every time I test I get the same error, generic failure.
Attachment #642422 - Attachment is obsolete: true
Attachment #650581 - Flags: feedback?(mvines)
Attachment #650581 - Flags: feedback?(allstars.chh)
Comment on attachment 650581 [details] [diff] [review]
WIP v3

Review of attachment 650581 [details] [diff] [review]:
-----------------------------------------------------------------

Hi jaoo,
I think your parcel is wrong here.
Could you check that in ril.h again?

Also you can take the reference code from codeaurora [1]. 

[1]: http://bit.ly/Nr4h7c

Thanks

::: dom/system/gonk/ril_worker.js
@@ +857,5 @@
> +   * Requests a network personalization be deactivated.
> +   *
> +   * @param depersonalizationType
> +   *        Integer indicating the network personalization be deactivated.
> +   * @param code

code or pin?

@@ +864,5 @@
> +  enterICCDepersonalization: function enterICCDepersonalization(options) {
> +    Buf.newParcel(REQUEST_ENTER_NETWORK_DEPERSONALIZATION, options);
> +    Buf.writeUint32(RILQUIRKS_V5_LEGACY ? 1 : 2);
> +    Buf.writeUint32(options.depersonalizationType);
> +    Buf.writeString(options.pin);

Your parcel is completely wrong, I think.

Could you check the ril.h for otoro ?
you should take a look at RIL_Depersonalization.
it's a string and an int.

Also even you're writing for galaxy-s2 or ril v5, the data is a **char,
it is also wrong here.
So you don't have to supply depersonalizationType either.

@@ +866,5 @@
> +    Buf.writeUint32(RILQUIRKS_V5_LEGACY ? 1 : 2);
> +    Buf.writeUint32(options.depersonalizationType);
> +    Buf.writeString(options.pin);
> +    if (!RILQUIRKS_V5_LEGACY) {
> +      Buf.writeString(options.aid ? options.aid : this.aid);

DEPERSONALIZATION doesn't have aid. You don't have to provide it here.
Attachment #650581 - Flags: feedback?(allstars.chh) → feedback-
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #10)
> Comment on attachment 650581 [details] [diff] [review]
> WIP v3
> 
> Review of attachment 650581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi jaoo,
> I think your parcel is wrong here.

Thanks for comments, I'll poke around how to send the parcel properly. Actually that part of code hasn't been tested yet. I have started my test fetching the state of the network control key and in that test is where I get the generic failure error. So the earlier question that was "The logic does not work since that capability is not built into the otoro by its manufacturer. Is that possible?" is still not clear to me. Thanks in advance if you have further comments.
Comment on attachment 650581 [details] [diff] [review]
WIP v3

Review of attachment 650581 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, jaoo
Sorry I didn't read your comment carefully.

I've applied your patch and I got the same result with you.
still don't know why.

from 'adb logcat -b radio' I also see 
"received invalid paramets in RIL_REQUEST_QUERY_FACILITY_LOCK"

BTW, recently the message passing between RadioInterfaceLayer and ril_worker has been changed,
so you need to change to 'type' to 'rilMessageType' when sending those msgs.

thanks

::: dom/system/gonk/ril_worker.js
@@ +998,5 @@
>      options.facility = ICC_CB_FACILITY_SIM;
>      options.password = ""; // For query no need to provide pin.
>      options.serviceClass = ICC_SERVICE_CLASS_VOICE |
>                             ICC_SERVICE_CLASS_DATA  |
>                             ICC_SERVICE_CLASS_FAX,

Can you help to correct this for me? 
It should end with semicolon.
Sorry for my typo

@@ +1014,5 @@
> +    options.facility = ICC_CB_FACILITY_NETWORK_PERSONALIZATION;
> +    options.password = ""; // For query no need to provide pin.
> +    options.serviceClass = ICC_SERVICE_CLASS_VOICE |
> +                           ICC_SERVICE_CLASS_DATA  |
> +                           ICC_SERVICE_CLASS_FAX,

also here
Attachment #650581 - Flags: feedback- → feedback+
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #12)
> Comment on attachment 650581 [details] [diff] [review]
> WIP v3
> 
> Review of attachment 650581 [details] [diff] [review]:
> -----------------------------------------------------------------
> Can you help to correct this for me? 
> It should end with semicolon.
> Sorry for my typo
> 
> @@ +1014,5 @@
> > +    options.facility = ICC_CB_FACILITY_NETWORK_PERSONALIZATION;
> > +    options.password = ""; // For query no need to provide pin.
> > +    options.serviceClass = ICC_SERVICE_CLASS_VOICE |
> > +                           ICC_SERVICE_CLASS_DATA  |
> > +                           ICC_SERVICE_CLASS_FAX,
> 
> also here

Sure, done in bug 774114 since that bug is about refactoring and such stuff.
Attached patch WIP v4 (obsolete) — Splinter Review
I believe the logic implemented here is right but tests report "Generic Failure" every time I try to fetch the state of the lock. I have requested a sample to my local contact for testing on a otoro with the SIM-lock capability enabled on the device. I'll keep you updated.
Attachment #650581 - Attachment is obsolete: true
Attachment #650581 - Flags: feedback?(mvines)
Whiteboard: [LOE:S]
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P0]
Keywords: feature
Just received an otoro with a network personalization enabled. Here is a logcat radio output from a SIM-Locked phone (otoro) running Android. It could give us some hints about the behavior of the phone in such circumstances and could be useful for testing and finishing this work.
New WIP. If the phone is personalised to a particular network (carrier) and the user inserts an ICC card from another carrier `nsIDOMMozMobileConnection.cardState` indicates that the state of the device's ICC card is `networkLocked`. This patch allows the user to supply the depersonalization code through nsIDOMMozMobileConnection by using the `unlockCardLock` function.
Attachment #651341 - Attachment is obsolete: true
This patch updates the nsIDOMMozMobileConnection specs with some useful information about how to supply the code for disabling the lock from a phone that is personalised to a particular network (carrier).
Attachment #662591 - Attachment description: WIP v5 - Part 1: RIL implementation. → Part 1: RIL implementation. - WIP v5.
Do the patches here need reviewers?  Will this work make the feature freeze?
I need to test these patches on a phone or on the emulator before requesting review. We have a problem here because we need a SIM-locked phone for testing and this capability is not built into the otoro by the phone manufacturer. In that situation is difficult to test current patches, request review and resolve this bug. On the other hand AFAIK the emulator lacks of this support :(. 

A few days ago I received a SIM-locked GB-based otoro phone. Tomorrow I'll try to build B2G for that device and test the patches but since we are now ICS I have to recover the old GB-based build environment for B2G and I could run into build problems and won't be able to test them. Let me do that tomorrow and I'll update you with my progress.
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #19)
> I need to test these patches on a phone or on the emulator before requesting
> review. We have a problem here because we need a SIM-locked phone for
> testing and this capability is not built into the otoro by the phone
> manufacturer. In that situation is difficult to test current patches,
> request review and resolve this bug. On the other hand AFAIK the emulator
> lacks of this support :(. 

How about we land what you have now and deal with any bugfixes later? So long as the feature doesn't introduce any risk (I would say it doesn't introduce much risk), there should be no reason not to land this IMHO.

(I do wonder if we're going to ship with a SIM lock enabled, btw. We said this is a must-have for v1, but is it really? Just want to double-check!)

> A few days ago I received a SIM-locked GB-based otoro phone. Tomorrow I'll
> try to build B2G for that device and test the patches but since we are now
> ICS I have to recover the old GB-based build environment for B2G and I could
> run into build problems and won't be able to test them. Let me do that
> tomorrow and I'll update you with my progress.

Is it an Otoro with GB android or just the GB modem firmware? The current build system works just fine with the GB modem firmware, you just need to swap out your backup-otoro directory for one with the GB-based blobs and reconfigure. Please ping me if you need any help!
(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> (I do wonder if we're going to ship with a SIM lock enabled, btw. We said
> this is a must-have for v1, but is it really? Just want to double-check!)
> 
The activation of this feature will depend on the country and operator we want to launch. ie. Brasil Vivo do not ask for SIM lock while any other country in LATAM with Telefonica will ask for it. We should prepare the platform for its support.
Attachment #662591 - Attachment is obsolete: true
Attachment #665405 - Flags: review?(philipp)
Attachment #662593 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> How about we land what you have now and deal with any bugfixes later? So
> long as the feature doesn't introduce any risk (I would say it doesn't
> introduce much risk), there should be no reason not to land this IMHO.

SGTM, I'm afraid we only have that solution for this issue. You're right this code doesn't introduce any risk. Doesn't broke the RIL at all and just reports `ERROR_GENERIC_FAILURE` if we try to unlock the lock.

> Is it an Otoro with GB android or just the GB modem firmware? The current
> build system works just fine with the GB modem firmware, you just need to
> swap out your backup-otoro directory for one with the GB-based blobs and
> reconfigure. Please ping me if you need any help!

Thanks for your help!

It's an Otoro with GB Android (SIM-locked for -a Spanish Carrier- called YOIGO). The current build system works but B2G doesn't boot properly and it causes I cannot test. It's a pain to waste more time on this and IMHO it doesn't maks sense to test it for GB.

I've requested an ICS-based SIM-locked phone to my local contact from otoro but It'll take some time. Landing what I have now and deal with any bugfixes later sounds good to me too.
Current build system works for GB but B2G doesn't boot properly and it causes I cannot test. Please, find attached the logcat output if you guys are curious.
Attachment #665405 - Flags: review?(philipp) → review+
Comment on attachment 662593 [details] [diff] [review]
Part 2: Update nsIDOMMozMobileConnection specs. - v1.

Review of attachment 662593 [details] [diff] [review]:
-----------------------------------------------------------------

Probably want to collapse this with the other patch.
Attachment #662593 - Flags: review?(philipp) → review+
(In reply to Philipp von Weitershausen [:philikon] from comment #25)
> Comment on attachment 662593 [details] [diff] [review]
> Part 2: Update nsIDOMMozMobileConnection specs. - v1.
> 
> Review of attachment 662593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Probably want to collapse this with the other patch.

Sure, done.
https://hg.mozilla.org/mozilla-central/rev/c3d7c4fc731f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.