STK Profile Download makes Nexus S/Galaxy S2' RIL reset

RESOLVED FIXED

Status

defect
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gerard-majax, Assigned: gwang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

STK Profile Download from bug 791934 makes the Nexus S modem rebooting, effectively breaking the main functionnality of a phone.

Commenting out the STK profile download in ril_worker.js worksaround the issue. Attached is the dump from the modem when it crashes and resets.
Component: Hardware → General
Hi Lissy
I just try my latest m-c on Nexus S and it's still crashing even I commented out the profile download code.

my last m-c commit is 

commit 40c9aec14b80a697dd3f2f368a5513d7d1b54a8d
Author: Alex Keybl <akeybl@mozilla.com>
Date:   Mon Nov 19 12:03:27 2012 -0800

    Merging in version bump NO BUG


Can you show me your m-c version so I can give it a try ?

Thanks
Reporter

Comment 2

7 years ago
$ git describe --always master
b373a99

And the change to ril_worker.js that makes me happy:
diff --git a/dom/system/gonk/ril_worker.js b/dom/system/gonk/ril_worker.js
index f14099b..314034b 100644
--- a/dom/system/gonk/ril_worker.js
+++ b/dom/system/gonk/ril_worker.js
@@ -3030,11 +3030,13 @@ let RIL = {
     if (newCardState == GECKO_CARDSTATE_READY) {
       // For type SIM, we need to check EF_phase first.
       // Other types of ICC we can send Terminal_Profile immediately.
+      /*
       if (this.appType == CARD_APPTYPE_SIM) {
         this.getICCPhase();
       } else {
         this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE);
       }
+      */
       this.fetchICCRecords();
       this.reportStkServiceIsRunning();
     }
sorry for my late reply
I tried to reproduce it but I can't.

I just try my latest gecko
commit f29aa5cb0067b628348d53098224f811e8aa7771
Author: Ehsan Akhgari <ehsan@mozilla.com>
Date:   Wed Nov 28 17:13:43 2012 -0500

and gaia
commit 7c3bd603b482676da3214e2a54a697e5089932bc
Merge: 47823b5 2cc7345
Author: Timothy Guan-tin Chien <timdream@gmail.com>
Date:   Wed Nov 28 18:51:37 2012 -0800

and the nexus won't reboot now.

Lissy, can you check again ?
Reporter

Comment 4

7 years ago
I'll try as soon as I get the device back. Do not hesitate to ping me if I forget.
I was experiencing the same issues on SGS2.
Workaround in Comment #2 worked for me.
Summary: STK Profile Download makes Nexus S' RIL reset → STK Profile Download makes Nexus S/Galaxy S2' RIL reset
Reporter

Comment 6

7 years ago
Got my Nexus S back, flashed it with brand new code (completely wiped ccache, out/ and objdir-gecko), still reproducing.

Gecko is at :
$ git describe --always 
acf076f

The workaround "fix" I proposed earlier still works.
Reporter

Comment 7

7 years ago
According to TS 101.127 section 5.2 [http://www.etsi.org/deliver/etsi_ts/101200_101299/101267/08.18.00_60/ts_101267v081800p.pdf], encoding of the profile downloaded to SIM is composed of 19 bytes plus one subsequent byte at zero.

in ril_consts.js, this.STK_SUPPORTED_TERMINAL_PROFILE is defined with 20 bytes, and sendStkTerminalProfile() has the following code:

  sendStkTerminalProfile: function sendStkTerminalProfile(profile) {
    Buf.newParcel(REQUEST_STK_SET_PROFILE);
    Buf.writeUint32(profile.length * 2);
    for (let i = 0; i < profile.length; i++) {
      GsmPDUHelper.writeHexOctet(profile[i]);
    }    
    Buf.writeUint32(0);
    Buf.sendParcel();
  },

So we copy the 20 bytes then add another one. Which makes 21 bytes to me. Commenting the last "Buf.writeUint32(0)" fixes the issue.
Reporter

Comment 8

7 years ago
Looks like "fixed the issue" means in this case "makes no profile being downloaded, therefore worksaround the bug".
Reporter

Comment 9

7 years ago
I did some testing, re-enabling stk profile download and below is the result.

-----------------------------------------------------------------------------
Working diff :
-----------------------------------------------------------------------------
diff --git a/dom/system/gonk/ril_consts.js b/dom/system/gonk/ril_consts.js
index 653c6c0..a2151bc 100644
--- a/dom/system/gonk/ril_consts.js
+++ b/dom/system/gonk/ril_consts.js
@@ -893,6 +893,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // Proactive Commands
   0x00, // Proactive Commands
   0x00, // Softkey support
+  /*
   0x00, // Softkey information
   0x00, // BIP proactive commands
   0x00, // BIP supported bearers
@@ -903,6 +904,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // 18, RFU
   0x00, // 19, RFU
   0x00, // 20, RFU
+  */
 ];
 
 /**
 
 
-----------------------------------------------------------------------------
Does not work :
-----------------------------------------------------------------------------
diff --git a/dom/system/gonk/ril_consts.js b/dom/system/gonk/ril_consts.js
index 653c6c0..99e1ba8 100644
--- a/dom/system/gonk/ril_consts.js
+++ b/dom/system/gonk/ril_consts.js
@@ -894,6 +894,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // Proactive Commands
   0x00, // Softkey support
   0x00, // Softkey information
+  /*
   0x00, // BIP proactive commands
   0x00, // BIP supported bearers
   0x00, // Screen height
@@ -903,6 +904,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // 18, RFU
   0x00, // 19, RFU
   0x00, // 20, RFU
+  */
 ];
 
 /**
Reporter

Comment 10

7 years ago
One will notice that it works well with 10, and that it does not work when going to 11.
Reporter

Comment 11

7 years ago
Some other results of logcat on the radio. I can't decide if in the second case we send the correct profile. But it seems we get the same answer in both case.

Half-length profile (cut in the middle), Buf.writeUint32(profile.length * 2);
E/RIL(s)  (   81): TX: (M)IPC_SAT_CMD  (S)IPC_SAT_PROFILE_DOWNLOAD  (T)IPC_CMD_SET (len:0x1C mseq:0x55 aseq:0x00) [ 01 30 42 45 33 46 46 33 46 31 46 30 30 30 30 30 30 ... ]
E/RIL(s)  (   81): RX: (M)IPC_GEN_CMD  (S)IPC_GEN_PHONE_RES  (T)IPC_CMD_RESP (len:0x0C mseq:0xFF aseq:0x55) [ 0E 01 03 01 80 ]

Full-length profile, Buf.writeUint32(profile.length * 4);
E/RIL(s)  (   81): TX: (M)IPC_SAT_CMD  (S)IPC_SAT_PROFILE_DOWNLOAD  (T)IPC_CMD_SET (len:0x1C mseq:0x52 aseq:0x00) [ 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... ]
E/RIL(s)  (   81): RX: (M)IPC_GEN_CMD  (S)IPC_GEN_PHONE_RES  (T)IPC_CMD_RESP (len:0x0C mseq:0xFF aseq:0x52) [ 0E 01 03 01 80 ]
Reporter

Comment 12

7 years ago
And now a small dump of sent parcel in both cases:

Full-length profile, Buf.writeUint32(profile.length * 4);
I/Gecko   (   77): RIL Worker: New outgoing parcel of type 68
I/Gecko   (   77): RIL Worker: Outgoing parcel: 0,0,0,96,68,0,0,0,44,0,0,0,80,0,0,0,48,0,66,0,69,0,51,0,70,0,70,0,51,0,70,0,49,0,70,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,0,0,0,0

Half-length profile (cut in the middle), Buf.writeUint32(profile.length * 2);
I/Gecko   (   78): RIL Worker: New outgoing parcel of type 68
I/Gecko   (   78): RIL Worker: Outgoing parcel: 0,0,0,56,68,0,0,0,44,0,0,0,20,0,0,0,48,0,66,0,69,0,51,0,70,0,70,0,51,0,70,0,49,0,70,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,0,0,0,0
Reporter

Comment 13

7 years ago
Please note that (profile.length*4) seems to be consistent with bug 819790 from what I can infer in both cases.
(In reply to Alexandre LISSY :gerard-majax from comment #7)
> According to TS 101.127 section 5.2
> [http://www.etsi.org/deliver/etsi_ts/101200_101299/101267/08.18.00_60/
> ts_101267v081800p.pdf], encoding of the profile downloaded to SIM is
> composed of 19 bytes plus one subsequent byte at zero.
> 
> in ril_consts.js, this.STK_SUPPORTED_TERMINAL_PROFILE is defined with 20
> bytes, and sendStkTerminalProfile() has the following code:
> 
>   sendStkTerminalProfile: function sendStkTerminalProfile(profile) {
>     Buf.newParcel(REQUEST_STK_SET_PROFILE);
>     Buf.writeUint32(profile.length * 2);
This line writes 'String length'

>     for (let i = 0; i < profile.length; i++) {
>       GsmPDUHelper.writeHexOctet(profile[i]);
>     }    
>     Buf.writeUint32(0);
This line writes String delimeter.
(In reply to Alexandre LISSY :gerard-majax from comment #13)
> Please note that (profile.length*4) seems to be consistent with bug 819790
> from what I can infer in both cases.

Hi Lissy
I think this bug and bug 819790 are different

From Bug 819790, the length is read from GsmPDUHelper.readHexOctet
so /3 is correct, we don't have to do /2 again (become /6 now) to get the length of octets.

But in STK Profile Download, the length is written for string length.
Each octet will be encoded to two chars, said 0xff -> 'ff'
1 byte -> two chars
so the length should be * 2.
Ok, I was following another path that consisted of sending the command without encoding it with PDU format, and using profile.length as Parecel length. Yoshi saw me that I was wrong cause PDU format is a requierement of ril (as inferred from ril.h). This approach overcomes the crash problem with ril, but I saw that I was receiving this response after sending the Terminal Profile Download command:

I/Gecko   ( 1914): RIL Worker: New incoming parcel of size 12
I/Gecko   ( 1914): RIL Worker: Parcel (size 12): 0,0,0,0,15,0,0,0,2,0,0,0
I/Gecko   ( 1914): RIL Worker: We have at least one complete parcel.
I/Gecko   ( 1914): RIL Worker: Solicited response for request type 68, token 15, error 2

I'm investigating what that response means (error 2?). But I'm receiving the same response when I implement Alexandre approach (using profile.length * 4 with PDU encoding).

Well, at least rild is not crashing anymore, but it looks like the terminal profile message that we are sending is not what Samsung's RIL is expecting.
Reporter

Comment 17

7 years ago
I've also found this: https://android.googlesource.com/device/samsung/crespo/+/0e70776ea20206519aebf48e33483cb53cd452ab%5E!/

The interesting part is:
	[ Problem ]
	 - When we power on Crespo with SKT 3G USIM inserted.
	   Crespo does not recognize USIM and causing infinite sim reset process.

	[ SW Modification ]
	 - We enabled major sim toolkit command like Send SMS, display text, refresh etc.
	   at Terminal Profile which notifing Mobile capability to SIM.

	[ Reason ]
	 - Basically, STK USIM is not following 3GPP 31.101 standard.
	 - When STK USIM receive Terminal Profile which basic sim toolkit command disabled from Mobile,
	   It is not returning any status word 1,2 whcih have the result of current command processed.
	   Because mobile didn't received any sw1, sw2 from SIM, It is trying to recover this invalid situation causing
	   Cold reset to SIM. after that same Terminal Profile fall down to SIM and no sw1, sw2 again.
	   this is repeated.
For the STK_SET_PROFILE request, it seems Samsung RIL doesn't implement this (so the TERMINAL PROFILE is implemented in modem/baseband)

Currently I found some builds of my Nexus-S will crash and keeps rebooting,
but even I remove the those profile download related code, it's still crashing.
So I am thinking maybe it's caused by other stuff.
Reporter

Comment 19

7 years ago
I've updated the code a couple of minutes ago, and once stk profile download commented, no crash.

Comment 20

6 years ago
I'm also getting a hang on boot. This is my logcat:

E/Profiler(  193): BEGIN mozilla_sampler_init
E/Profiler(  193): ----------------- MOZ_PROFILER_NEW not set -----------------
E/Profiler(  193): Registering start signal
E/GeckoConsole(  193): Could not read chrome manifest 'file:///system/b2g/chrome.manifest'.
F/<unknown>(   80): stack corruption detected: aborted
I/Gonk    (   76): OnDisconnect

Note that "stack corruption detected: aborted".

I can confirm the workaround in comment 2 enables my Nexus S to boot and the phone works.
Hi Vincent, 
The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S
(Actually it doesn't work on otoro/unagi either)

Could you add a RIL quirk for this?
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21)
> Hi Vincent, 
> The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S
> (Actually it doesn't work on otoro/unagi either)
> 
> Could you add a RIL quirk for this?

Sure! I will try adding it.
Posted patch WIP V1 (obsolete) — Splinter Review
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21)
> Hi Vincent, 
> The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S
> (Actually it doesn't work on otoro/unagi either)
> 
> Could you add a RIL quirk for this?

Hi Yoshi,

Can you give some feedbacks for the WIP? Thanks!
Attachment #741664 - Flags: feedback?(allstars.chh)
Comment on attachment 741664 [details] [diff] [review]
WIP V1

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

Good!

Thank you :)

::: dom/system/gonk/ril_worker.js
@@ +76,5 @@
>  let RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS = libcutils.property_get("ro.moz.ril.simstate_extra_field", "false") === "true";
>  // Needed for call-waiting on Peak device
>  let RILQUIRKS_EXTRA_UINT32_2ND_CALL = libcutils.property_get("ro.moz.ril.extra_int_2nd_call", "false") == "true";
> +let RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD = libcutils.property_get("ro.moz.ril.send_stk_profile_dl", "false") == "true";
> +// Ril quirk to Send STK Profile Download

Comment on wrong line?
Attachment #741664 - Flags: feedback?(allstars.chh) → feedback+
Posted patch WIP V2 (obsolete) — Splinter Review
(In reply to Alexandre LISSY :gerard-majax from comment #19)
> I've updated the code a couple of minutes ago, and once stk profile download
> commented, no crash.

Can you please add the patch and try again? Thanks
Attachment #741664 - Attachment is obsolete: true
Reporter

Comment 26

6 years ago
(In reply to Vincent Liu[:vliu] from comment #25)
> Created attachment 741670 [details] [diff] [review]
> WIP V2
> 
> (In reply to Alexandre LISSY :gerard-majax from comment #19)
> > I've updated the code a couple of minutes ago, and once stk profile download
> > commented, no crash.
> 
> Can you please add the patch and try again? Thanks

I'm sorry, I don't have the time right now :(
Reporter

Comment 27

6 years ago
(In reply to Vincent Liu[:vliu] from comment #25)
> Created attachment 741670 [details] [diff] [review]
> WIP V2
> 
> (In reply to Alexandre LISSY :gerard-majax from comment #19)
> > I've updated the code a couple of minutes ago, and once stk profile download
> > commented, no crash.
> 
> Can you please add the patch and try again? Thanks

I've retried working on this bug and I can still confirm that changing the length to "(profile.length + 1) * 2" OR removing the leading '0' written fixes the crash. I'm currently in the process of cross checking on Inari, but I don't see why this would not be a proper fix.
Flags: needinfo?(vliu)
Reporter

Comment 28

6 years ago
Please find attached a patch that fixes the issue by setting the correct amount of bytes that we will be sending to the modem. As far as I could test, it does not breaks Inari device.
Assignee: nobody → lissyx+mozillians
Attachment #741670 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #785381 - Flags: review?(allstars.chh)
Hi Alexandre
Can you explain why +1 is needed?

The line 
Buf.writeUint32(profile.length * 2);
is to write the string length.

Each octet 0x12 will be represeneted '12' in hex format string.
And '12' is string of length 2
So that's why *2 is needed.

The following 
Buf.writeUint32(0) is to write string delimiter.
It serves the same purpose as 
Buf.writeStringDelmiter(profile.length * 2)
which is to write 4 octets of zero.

Thanks
Also if you insist +1 could work on Nexus S, 
can you also check GET_STK_PROFILE could return the same profile you set when calling SET_STK_PROFILE?
Reporter

Comment 31

6 years ago
Comment on attachment 741670 [details] [diff] [review]
WIP V2

Actually this is working.
Attachment #741670 - Attachment is obsolete: false
Reporter

Updated

6 years ago
Attachment #785381 - Attachment is obsolete: true
Attachment #785381 - Flags: review?(allstars.chh)
Reporter

Updated

6 years ago
Attachment #741670 - Flags: review?(allstars.chh)
Can vliu or alexandre update the title in the patch?
It seems this patch is malformed I cannot review it entirely on Bugzilla.
Reporter

Comment 33

6 years ago
Same patch, reformated.
Attachment #741670 - Attachment is obsolete: true
Attachment #741670 - Flags: review?(allstars.chh)
Attachment #785710 - Flags: review?(allstars.chh)
Reporter

Updated

6 years ago
Blocks: b2g-nexuss
Comment on attachment 785710 [details] [diff] [review]
Enable STK Profile Download on a per device basis

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

This patch still looks weird to me,
when I do the review,
I only see line 93~95 are adding,
but not the changes in line 3261, 3269, and 10762. 

Also the patch here doesn't contain 8 lines of context.
Reporter

Comment 35

6 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #34)
> Comment on attachment 785710 [details] [diff] [review]
> Enable STK Profile Download on a per device basis
> 
> Review of attachment 785710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch still looks weird to me,
> when I do the review,
> I only see line 93~95 are adding,
> but not the changes in line 3261, 3269, and 10762. 
> 
> Also the patch here doesn't contain 8 lines of context.

I'm getting the same behavior :(

Yet it's just a git format-patch -1
Comment on attachment 788050 [details] [diff] [review]
HG patch

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

Hi, Lissy
can you help to update this HG patch?
And add r=me.

::: dom/system/gonk/ril_worker.js
@@ +89,5 @@
>  let RILQUIRKS_EXTRA_UINT32_2ND_CALL = libcutils.property_get("ro.moz.ril.extra_int_2nd_call", "false") == "true";
>  // On the emulator we support querying the number of lock retries
>  let RILQUIRKS_HAVE_QUERY_ICC_LOCK_RETRY_COUNT = libcutils.property_get("ro.moz.ril.query_icc_count", "false") == "true";
>  
> +// Ril quirk to Send STK Profile Download 

trailing space

@@ +3295,5 @@
>          ICCRecordHelper.fetchICCRecords();
>        } else if (this.appType == CARD_APPTYPE_USIM) {
> +        if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) {
> +          this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE);
> +	}

a Tab here.

@@ +3300,5 @@
>          ICCRecordHelper.fetchICCRecords();
>        } else if (this.appType == CARD_APPTYPE_RUIM) {
> +        if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) {
> +          this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE);
> +	}

here to.
Flags: needinfo?(vliu)
Comment on attachment 785710 [details] [diff] [review]
Enable STK Profile Download on a per device basis

Please try my hg patch.
Attachment #785710 - Flags: review?(allstars.chh)
Duplicate of this bug: 928826
Discussed with Lissy on irc, will assign to Georgia to land this patch.
Assignee: lissyx+mozillians → gwang
Component: General → RIL
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #38)
> Please try my hg patch.

Tried this patch on my nexus-s and call functionality was properly restored. Please check this in!
Assignee

Comment 42

6 years ago
I've also try the patch in comment #38, there's no aborted in log and device works fine. 

Gaia: 9ebfaca574dc70893494ef445880ff897a7cb03b
Gecko: 6a9509a8d7d1fde2f6c77c6b459191683cc23dbe
Assignee

Updated

6 years ago
Attachment #820807 - Flags: review?(allstars.chh)
Assignee

Updated

6 years ago
Attachment #820808 - Flags: review?(allstars.chh)
Attachment #820807 - Flags: review?(allstars.chh) → review+
Attachment #820808 - Flags: review?(allstars.chh) → review+
You need to log in before you can comment on or make changes to this bug.