Closed Bug 868896 Opened 7 years ago Closed 7 years ago

B2G RIL: support AT command +CPINR

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: vicamo, Assigned: tzimmermann)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 files, 5 obsolete files)

This is a follow-up bug for bug 860660. As the format of the result of REQUST_GET_SIM_STATUS defined in AOSP doesn't contains SIM PIN/PUK/... remaining retry counts, and actually no other places have it, we should provide a device/platform independent solution for retrieving such counts.

In 3GPP TS 27.007 section 8.65 "Remaining PIN retries +CPINR", standard has it optionally for reporting SIM PIN/PUK/PIN2/PUK2 and PH-SIM PIN, PH-FSIM PIN, PH-FSIM PUK remaining retry counts.
Assignee: nobody → tzimmermann
I think, we should reverse the dependencies here. It's cleaner if we first land the new interface and afterwards fix the implementation.
No longer blocks: 875710
Depends on: 875710
I couldn't find a RIL interface for sending +CPINR. It's not implemented afaict.
I tried to run enterICCPIN with a totally bogus pin. My hope was that I'd get the retry count without decreasing the value itself. But when I tried to call RIL_REQUEST_ENTER_SIM_PIN with an empty PIN or no pin at all; and then filter out the retryCount in _processEnterAndChangeICCResponses, I only got '0' (instead of '3').
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> I couldn't find a RIL interface for sending +CPINR. It's not implemented
> afaict.
Thomas, we don't use AT command in FFOS RIL. You should implement it via rild interface.
https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h
(In reply to Ken Chang from comment #4)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> > I couldn't find a RIL interface for sending +CPINR. It's not implemented
> > afaict.
> Thomas, we don't use AT command in FFOS RIL. You should implement it via
> rild interface.
> https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/
> telephony/ril.h

Sorry for being so sloppy with these terms here. I meant that I looked into the rild header file (i.e., the one you linked to), but there is no interface to send a +CPINR command. And I also looked into the implementation of reference-ril, but there is no code for generating +CPINR.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> (In reply to Ken Chang from comment #4)
> > (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> > > I couldn't find a RIL interface for sending +CPINR. It's not implemented
> > > afaict.
> > Thomas, we don't use AT command in FFOS RIL. You should implement it via
> > rild interface.
> > https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/
> > telephony/ril.h
> 
> Sorry for being so sloppy with these terms here. I meant that I looked into
> the rild header file (i.e., the one you linked to), but there is no
> interface to send a +CPINR command. And I also looked into the
> implementation of reference-ril, but there is no code for generating +CPINR.

Yes, but like all the quirks we have in ril_worker to support various vendor specific RIL requests and unsolicited responses, I think, as a reference implementation, we can have some Mozilla's own requests that send CPINR in reference-ril.
That's a proof-of-concept implementation in reference-ril. Maybe this could be useful for supporting CPINR in the emulator.
Attachment #768963 - Flags: feedback?(vyang)
The gecko side for the 'pin' lock type. You'll need the patches from bug 875710 for this.

The code works, but when I run it, I always get 'REQUEST_NOT_SUPPORTED' from rild. That's because I haven't found out how to use libreference-ril on my Unagi without replacing the loader code in rild. It'll generate for the DOM request and the Unlock apps will show their default messages.
Attachment #768965 - Flags: feedback?(vyang)
Comment on attachment 768963 [details]
[01] Bug 868896: Added RIL_REQUEST_GET_UNLOCK_RETRY_COUNT

Please keep the original offset of RIL_REQUEST_ENTER_SIM_PIN  definitions, moving it around doesn't really mean anything, does it?  Besides, I think we would be better begins Mozilla's request id with 900 or something like that, so that we won't conflict with upstream definitions any time soon.
Attachment #768963 - Flags: feedback?(vyang)
Comment on attachment 768965 [details] [diff] [review]
[02] Bug 868896: Use REQUEST_GET_UNLOCK_RETRY_COUNT

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

::: dom/system/gonk/ril_worker.js
@@ +1210,5 @@
>        case "nck":
>        case "cck":
>        case "spck":
>          options.errorMsg = GECKO_ERROR_REQUEST_NOT_SUPPORTED;
>          options.success = false;

If it's not too bothersome, we can still implement everything here.  When we need extra retry counts like puk/pin2/..., we just fix hardware/ril and external/qemu only.

@@ +1227,5 @@
>      // reading the retry count. See bug 868896.
>      options.retryCount = 0;
>      options.success = true;
>      this.sendDOMMessage(options);
> +*/

Please remove dead code.

@@ +1244,5 @@
> +    Buf.writeUint32(RILQUIRKS_V5_LEGACY ? 1 : 2);
> +    Buf.writeString(options.selCode);
> +    if (!RILQUIRKS_V5_LEGACY) {
> +      Buf.writeString(options.aid || this.aid);
> +    }

There is only one string in the newly defined parcel format -- options.selCode.  We don't need aid here.
Attachment #768965 - Flags: feedback?(vyang) → feedback+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9)
> Comment on attachment 768963 [details]
> [01] Bug 868896: Added RIL_REQUEST_GET_UNLOCK_RETRY_COUNT
> 
> Please keep the original offset of RIL_REQUEST_ENTER_SIM_PIN  definitions,
> moving it around doesn't really mean anything, does it?  Besides, I think we
> would be better begins Mozilla's request id with 900 or something like that,
> so that we won't conflict with upstream definitions any time soon.

Oops, RIL_REQUEST_ENTER_SIM_PIN wasn't supposed to be moved around. I'll update the patches according to your comments.

What about merging? Do you think merging the Gecko changes is safe, even though the rild on the phone doesn't support GET_UNLOCK_RETRY_COUNT?
That's a change to the emulator to support 'SIM PUK' in +CPINR. Otherwise, our corresponding marionette test fails.
Attachment #773902 - Flags: review?(vyang)
I set the index of the new request to 150 instead of 900, because rild et al. use the request index for looking up the request handler from an array. I had to insert some empty elements into this array to put GET_UNLOCK_RETRY_COUNT at the correct position. I didn't feel like adding another 750 of them. ;)
Attachment #768963 - Attachment is obsolete: true
Attachment #773904 - Flags: review?(vyang)
This gecko patch should address your comments from the feedback request.
Attachment #768965 - Attachment is obsolete: true
Attachment #773906 - Flags: review?(vyang)
Comment on attachment 773902 [details]
[01] Bug 868896: Support AT+CPINR for PUK locks

Merged on Github: https://github.com/mozilla-b2g/platform_external_qemu/commit/066b7b90c9b504d1f9b181deef3404c7025f5de0
Attachment #773902 - Flags: review?(vyang) → review+
Comment on attachment 773904 [details]
[02] Bug 868896: Added RIL_REQUEST_GET_UNLOCK_RETRY_COUNT

Commented on Github.
Comment on attachment 773906 [details] [diff] [review]
[03] Bug 868896: Use REQUEST_GET_UNLOCK_RETRY_COUNT

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

Thanks! Any plans to have test cases for |getCardLock()|?  We have already some for |setCardLock()| in dom/icc/tests/marionette/test_icc_card_lock.js.

::: dom/system/gonk/ril_consts.js
@@ +129,5 @@
>  this.REQUEST_ISIM_AUTHENTICATION = 105;
>  this.REQUEST_ACKNOWLEDGE_INCOMING_GSM_SMS_WITH_PDU = 106;
>  this.REQUEST_STK_SEND_ENVELOPE_WITH_STATUS = 107;
>  this.REQUEST_VOICE_RADIO_TECH = 108;
> +this.REQUEST_GET_UNLOCK_RETRY_COUNT = 150;

Please move this to a special section and place a comment on it for it's actually a Mozilla specific one.

::: dom/system/gonk/ril_worker.js
@@ +3134,5 @@
>  
>      this.sendDOMMessage(options);
>    },
>  
> +  _processGetUnlockRetryCount: function _processGetUnlockRetryCount(length, options) {

We don't really need an extra function here, right?  Please just move the body into |RIL[REQUEST_GET_UNLOCK_RETRY_COUNT]|.
Attachment #773906 - Flags: review?(vyang) → review+
Hi Vicamo,

Thanks for the review.

I found that rild on the phones does not seem to return an error when it receives REQUEST_GET_UNLOCK_RETRY_COUNT. FTU and System app display incorrect information because of this problem. I need to debug this further, but for now I'd like to know how to enable patch [03] only on the emulator? What's the preferred way to do that?


> Thanks! Any plans to have test cases for |getCardLock()|?  We have already
> some for |setCardLock()| in dom/icc/tests/marionette/test_icc_card_lock.js.

I tested these patches with the test cases I write for bug 875710, and verified that they work on the emulator.

> ::: dom/system/gonk/ril_consts.js
> @@ +129,5 @@
> >  this.REQUEST_ISIM_AUTHENTICATION = 105;
> >  this.REQUEST_ACKNOWLEDGE_INCOMING_GSM_SMS_WITH_PDU = 106;
> >  this.REQUEST_STK_SEND_ENVELOPE_WITH_STATUS = 107;
> >  this.REQUEST_VOICE_RADIO_TECH = 108;
> > +this.REQUEST_GET_UNLOCK_RETRY_COUNT = 150;
> 
> Please move this to a special section and place a comment on it for it's
> actually a Mozilla specific one.

Ok.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +3134,5 @@
> >  
> >      this.sendDOMMessage(options);
> >    },
> >  
> > +  _processGetUnlockRetryCount: function _processGetUnlockRetryCount(length, options) {
> 
> We don't really need an extra function here, right?  Please just move the
> body into |RIL[REQUEST_GET_UNLOCK_RETRY_COUNT]|.

Ok. I was just following the style of other functions here.
Updated according to your review.
Attachment #773904 - Attachment is obsolete: true
Attachment #773904 - Flags: review?(vyang)
Attachment #776360 - Flags: review?(vyang)
I reworked this patch according to your review.

I noticed that the phone's rild don't seem to respond with an error when they get REQUEST_GET_UNLOCK_RETRY_COUNT. So with the new patch, we only send the request on the emulator. On the phones we generate an error directly.
Attachment #773906 - Attachment is obsolete: true
Attachment #776361 - Flags: review?(vyang)
Comment on attachment 776360 [details]
[02] Bug 868896: Added RIL_REQUEST_GET_UNLOCK_RETRY_COUNT (v2)

Merged on Github: https://github.com/mozilla-b2g/platform_hardware_ril/commit/1872da553cec3e8a3e61fa4b0e627e518184fd9d
Attachment #776360 - Flags: review?(vyang) → review+
Comment on attachment 776361 [details] [diff] [review]
[03] Bug 868896: Use REQUEST_GET_UNLOCK_RETRY_COUNT on emulator (v2)

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

::: dom/system/gonk/ril_worker.js
@@ +1225,5 @@
> +      /* Only the emulator supports this request, ... */
> +      options.selCode = selCode[options.lockType];
> +      this.queryICCLockRetryCount(options);
> +    } else {
> +      /* ... while the phones do not. */

At this time we supports device specific behaviours through various quirks.  See http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#81 .  Please create a new quirk RILQUIRKS_HAVE_QUERY_ICC_LOCK_RETRY_COUNT instead of hard-code |property_get(...)| here.
Attachment #776361 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> [03] Bug 868896: Use REQUEST_GET_UNLOCK_RETRY_COUNT on emulator (v2)

We'll also have to update external/qemu, hardware/ril to HEAD to land these test cases.  See bug 869787.
Updated with the RIL quirk.
Attachment #776361 - Attachment is obsolete: true
Attachment #776461 - Flags: review?(vyang)
Maybe a stupid question, but where are those quirks configured? Possibly one of the init.*.rc files? I grep'ed the whole B2G checkout for some of the quirk strings (i.e., ro.moz.ril.*), but couldn't find anything.
Flags: needinfo?(vyang)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> Maybe a stupid question, but where are those quirks configured? Possibly one
> of the init.*.rc files? I grep'ed the whole B2G checkout for some of the
> quirk strings (i.e., ro.moz.ril.*), but couldn't find anything.

Hi, you can find these quirks in Android device makefiles, usually listed in PRODUCT_PROPERTY_OVERRIDES defines.  For example, Inari has this definitions in https://github.com/mozilla-b2g/device-inari/blob/master/full_inari.mk#L13 .
Flags: needinfo?(vyang)
Comment on attachment 776461 [details] [diff] [review]
[03] Bug 868896: Use REQUEST_GET_UNLOCK_RETRY_COUNT on emulator (v3)

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

Thank you!
Attachment #776461 - Flags: review?(vyang) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> I noticed that the phone's rild don't seem to respond with an error when
> they get REQUEST_GET_UNLOCK_RETRY_COUNT. So with the new patch, we only send
> the request on the emulator. On the phones we generate an error directly.

Could you specify which devices have this problem? Might be useful for further works.
Thanks for the review.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> > I noticed that the phone's rild don't seem to respond with an error when
> > they get REQUEST_GET_UNLOCK_RETRY_COUNT. So with the new patch, we only send
> > the request on the emulator. On the phones we generate an error directly.
> 
> Could you specify which devices have this problem? Might be useful for
> further works.

It happens on the Unagi. I haven't tested with other phones.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #26)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> > Maybe a stupid question, but where are those quirks configured? Possibly one
> > of the init.*.rc files? I grep'ed the whole B2G checkout for some of the
> > quirk strings (i.e., ro.moz.ril.*), but couldn't find anything.
> 
> Hi, you can find these quirks in Android device makefiles, usually listed in
> PRODUCT_PROPERTY_OVERRIDES defines.  For example, Inari has this definitions
> in https://github.com/mozilla-b2g/device-inari/blob/master/full_inari.mk#L13
> .

See bug 894884.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #23)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> > [03] Bug 868896: Use REQUEST_GET_UNLOCK_RETRY_COUNT on emulator (v2)
> 
> We'll also have to update external/qemu, hardware/ril to HEAD to land these
> test cases.  See bug 869787.

See bug 894888.
https://hg.mozilla.org/mozilla-central/rev/01169ceebd7a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #26)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #23)
> > We'll also have to update external/qemu, hardware/ril to HEAD to land these
> > test cases.  See bug 869787.
> 
> See bug 894888.

Sorry, from bug 837448 comment 24, I just knew we don't need this anymore.  Thank you!
You need to log in before you can comment on or make changes to this bug.