Closed Bug 766862 Opened 12 years ago Closed 12 years ago

B2G RIL: GET_SIM_STATUS Failed on Otoro

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17
Project Flags:
blocking-basecamp +

People

(Reporter: allstars.chh, Assigned: philikon)

References

Details

Attachments

(1 file)

workaround
2.81 KB, patch
marshall
: review+
Details | Diff | Splinter Review 
06-21 13:56:44.079: I/Gecko(115): RIL Worker: New incoming parcel of size 12
06-21 13:56:44.079: I/Gecko(115): RIL Worker: Parcel (size 12): 0,0,0,0,53,0,0,0,255,255,255,255
06-21 13:56:44.079: I/Gecko(115): RIL Worker: We have at least one complete parcel.
06-21 13:56:44.079: I/Gecko(115): RIL Worker: Solicited response for request type 1, token 53, error -1
06-21 13:56:44.079: I/Gecko(115): RIL Worker: Handling parcel as REQUEST_GET_SIM_STATUS
adb logcat -b radio

D/RILC    (  103): UI <--- RIL_REQUEST_GET_SIM_STATUS (1) Complete --- RIL [RID 0, Token 17, Success, Len 312 ]
E/RILC    (  103): responseSimStatus: A RilCardStatus_v6 or _v5 expected
D/RILC    (  103): Freed AID pointer, app[0] 
D/RILC    (  103): Freed Label pointer, app[0] 
D/RILC    (  103): Freed AID pointer, app[1] 
D/RILC    (  103): Freed Label pointer, app[1] 
D/RILC    (  103): changed_fields = 0x0000000000000480, ut & tz avail = 0, tz avail =0, dst avail =0
CC to ShianYow, Vliu, mvines and philikon

Hi, Michael Vines:
As you pointed out in Bug 763160 comment 14, 
the ril.h is from codeaurora gingerbread-chocolate branch,

From comment 1 the length of the response is 312,
it is larger than the size of expected original RilCardStatus

Did you think it's possible that the ril on otoro is from other version or branch?

Or do you think we should also use the libril.so on device ?

thanks
Using libril.so from device caused other compatibility issue(Bug 763160 comment 19).  Maybe because that libril.so was build with bionic of GB version.

How about solving this by RIL quirk in gecko?
I was responding too quickly.
The size issue happens at libril.so, so we have to either find another version, or create local fork to modify.
(In reply to Shian-Yow Wu from comment #3)
> Using libril.so from device caused other compatibility issue(Bug 763160
> comment 19).

Right. I tried that just to confirm, but I saw the same problem.

(In reply to Shian-Yow Wu from comment #4)
> The size issue happens at libril.so, so we have to either find another
> version, or create local fork to modify.

Correct. Let's hope that we're accidentally using the wrong QC changeset. Hopefully mvines can help out here. Otherwise we can fork.

Wild shot in the dark: it's complaining to be receiving 312 bytes. sizeof(RIL_CardStatus_v6) is 280 bytes, sizeof(RIL_AppStatus) happens to be 32 bytes. Could it be that RIL_CARD_MAX_APPS is 9 instead of 8 for that particular modem?
It would be good to check if the Otoro/ICS build has this same issue (and/or the Akami ICS).

  If this is just an issue the GB RIL then I think we can just WONTFIX.
If this is an issue with Otoro/ICS and not Akami/ICS then I think we can look at getting the Akami fix propagated to Otoro.
(In reply to Philipp von Weitershausen [:philikon] from comment #5)
> Could it be that RIL_CARD_MAX_APPS is 9 instead of 8 for that
> particular modem?

Hi Philikon,
You're right, I try to modify RIL_CARD_MAX_APPS to 9 and it works now,

but libril is cloned from code aurora, any suggestion how should I commit this patch?

thanks
CC mwu:

Sent pull request to mozilla-b2g/platform_hardware_ril 

https://github.com/mozilla-b2g/platform_hardware_ril/pull/1
Yoshi, can you get a phone with the new ICS based build and see if it works? The new ICS based builds pull libril from the base ICS image so I think it should work.
(In reply to Michael Wu [:mwu] from comment #11)
> Yoshi, can you get a phone with the new ICS based build and see if it works?
> The new ICS based builds pull libril from the base ICS image so I think it
> should work.

Okay, I'll verify this on new ICS image, 
thanks for this information.
Thanks for vliu's help, I just got the Otoro on ICS.
GET_SIM_STATUS works on Otoro ICS, though there might be some minor problems,

I/Gecko   (  105): RIL Worker: Handling parcel as REQUEST_GET_SIM_STATUS
I/Gecko   (  105): RIL Worker: iccStatus: {"cardState":1,"universalPINState":0,"gsmUmtsSubscriptionAppIndex":0,"cdmaSubscriptionAppIndex":-1,"imsSubscriptionAppIndex":-1,"apps":[{"app_type":2,"app_state":5,"perso_substate":2,"aid":"a0000000871002f886ff9289050b00ff","app_label":"434854","pin1_replaced":0,"pin1":3,"pin2":1}]}
I/Gecko   (  105): RIL Worker: Parcel handler didn't consume whole parcel, 16 bytes left over

There are extra 16 bytes

Also I found more ICC problems on Otoro ICS
GET_IMSI cannot work, ICC IO can't work either.
will file new bug on this.
file a bug Bug 771440 for the problems I saw in my last comment.
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #13)
> GET_SIM_STATUS works on Otoro ICS, though there might be some minor problems,
...
> There are extra 16 bytes

I'm seeing the opposite:

I/Gecko   (  106): RIL Worker: Parcel (size 292): 0,0,0,0,33,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,255,255,255,255,2,0,0,0,2,0,0,0,5,0,0,0,2,0,0,0,32,0,0,0,97,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,56,0,55,0,49,0,48,0,48,0,50,0,102,0,102,0,102,0,102,0,102,0,102,0,102,0,102,0,56,0,57,0,48,0,51,0,48,0,50,0,48,0,48,0,48,0,48,0,0,0,0,0,8,0,0,0,53,0,53,0,53,0,51,0,52,0,57,0,52,0,100,0,0,0,0,0,0,0,0,0,3,0,0,0,1,0,0,0,3,0,0,0,10,0,0,0,3,0,0,0,10,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,32,0,0,0,97,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,56,0,55,0,49,0,48,0,48,0,52,0,102,0,102,0,102,0,102,0,102,0,102,0,102,0,102,0,56,0,57,0,48,0,51,0,48,0,50,0,48,0,48,0,48,0,48,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
I/Gecko   (  106): RIL Worker: We have at least one complete parcel.
I/Gecko   (  106): RIL Worker: Solicited response for request type 1, token 33, error 0
I/Gecko   (  106): RIL Worker: Handling parcel as REQUEST_GET_SIM_STATUS
I/Gecko   (  106): RIL Worker: Something's wrong, found string delimiter: 48
I/Gecko   (  106): RIL Worker: Parcel handling threw Error: Trying to read data beyond the parcel end!
I/Gecko   (  106): ensureIncomingAvailable@resource://gre/modules/ril_worker.js:183
I/Gecko   (  106): readUint8@resource://gre/modules/ril_worker.js:229
I/Gecko   (  106): readUint16@resource://gre/modules/ril_worker.js:236
I/Gecko   (  106): readString@resource://gre/modules/ril_worker.js:260
I/Gecko   (  106): REQUEST_GET_SIM_STATUS@resource://gre/modules/ril_worker.js:2762
I/Gecko   (  106): handleParcel@resource://gre/modules/ril_worker.js:2731
I/Gecko   (  106): processParcel@resource://gre/modules/ril_worker.js:507
I/Gecko   (  106): processIncoming@resource://gre/modules/ril_worker.js:459
I/Gecko   (  106): onRILMessage@resource://gre/modules/ril_worker.js:4748
Nom'ing for basecamp. We need to know the SIM status so that we can accurately tell the user if there's a SIM card in the device.

Yoshi, can you take a look at this please? Thanks!
Assignee: nobody → allstars.chh
blocking-basecamp: --- → ?
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Nom'ing for basecamp. We need to know the SIM status so that we can
> accurately tell the user if there's a SIM card in the device.
> 
> Yoshi, can you take a look at this please? Thanks!

sure, I'll get latest Otoro on ICS to check this
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Yoshi, can you take a look at this please? Thanks!

Hi philikon
I try this again but it's okay as my comment in comment 13

07-19 18:23:56.242: I/Gecko(105): RIL Worker: New incoming parcel of size 168
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Read 0, but parcel size is 168. Going to sleep.
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Received 168 bytes.
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Already read 0
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Parcel (size 168): ...
07-19 18:23:56.242: I/Gecko(105): RIL Worker: We have at least one complete parcel.
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Solicited response for request type 1, token 68, error 0
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Handling parcel as REQUEST_GET_SIM_STATUS
07-19 18:23:56.242: I/Gecko(105): RIL Worker: iccStatus: {"cardState":1,"universalPINState":0,"gsmUmtsSubscriptionAppIndex":0,"cdmaSubscriptionAppIndex":-1,"imsSubscriptionAppIndex":-1,"apps":[{"app_type":2,"app_state":5,"perso_substate":2,"aid":"a0000000871002f886ff9289050b00ff","app_label":"434854","pin1_replaced":0,"pin1":3,"pin2":1}]}
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Parcel handler didn't consume whole parcel, 16 bytes left over
07-19 18:23:56.242: I/Gecko(105): RIL Worker: Next parcel size unknown, going to sleep. 


Any idea?
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
Also blocks voicemail, per Marshall.
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #18)
> I try this again but it's okay as my comment in comment 13

Well, no, this isn't ok. Something is off. I bet it's SIM dependent. With your SIM, it *happens* to not throw an exception, but we still don't consume the whole parcel. My SIM probably has different data which happens to make it throw.
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #18)
> 07-19 18:23:56.242: I/Gecko(105): RIL Worker: Parcel (size 168): ...

It would be very useful for debugging to include the parcel here instead of replacing it with "..."
blocking-basecamp: + → ?
blocking-kilimanjaro: + → ---
I decoded the parcel that I'm seeing with my AT&T GoPhone SIM card (comment 15):

0,0,0,0,         RESPONSE_TYPE_SOLICITED
33,0,0,0,        token
0,0,0,0,         ERROR_SUCCESS
1,0,0,0,         CARD_STATE_PRESENT
0,0,0,0,         CARD_PINSTATE_UNKNOWN
0,0,0,0,         gsmUmtsSubscriptionAppIndex = 0
255,255,255,255, cdmaSubscriptionAppIndex = -1
255,255,255,255, imsSubScriptionAppIndex = -1
2,0,0,0,         apps.length = 2

  2,0,0,0,         CARD_APPTYPE_USIM
  5,0,0,0,         CARD_APPSTATE_READY
  2,0,0,0,         CARD_PERSOSUBSTATE_READY
  32,0,0,0,        aid.length = 32, aid = "a0000000871002ffffffff8903020000"
  97,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,56,0,55,0,49,0,48,0,48,0,50,0,
  102,0,102,0,102,0,102,0,102,0,102,0,102,0,102,0,56,0,57,0,48,0,51,0,48,0,
  50,0,48,0,48,0,48,0,48,0,0,0,0,0,
  8,0,0,0,         app_label.length = 8, app_label = "5553494d"
  53,0,53,0,53,0,51,0,52,0,57,0,52,0,100,0,0,0,0,0,
  0,0,0,0,         pin1_replaced
  3,0,0,0,         pin1
  1,0,0,0,         pin2
  3,0,0,0,         ???
  10,0,0,0,        ???
  3,0,0,0,         ???
  10,0,0,0,        ???

  0,0,0,0,         CARD_APPTYPE_UNKNOWN
  1,0,0,0,         CARD_APPSTATE_DETECTED
  0,0,0,0,         CARD_PERSOSUBSTATE_UNKNOWN
  32,0,0,0,        aid.length = 32, aid = "..."
  97,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,56,0,55,0,49,0,48,0,48,0,52,0,
  102,0,102,0,102,0,102,0,102,0,102,0,102,0,102,0,56,0,57,0,48,0,51,0,48,0,
  50,0,48,0,48,0,48,0,48,0,0,0,0,0,
  0,0,0,0,         app_label.length = 0
  0,0,0,0,         pin1_replaced
  0,0,0,0,         pin1
  0,0,0,0,         pin2
  0,0,0,0,         ???
  0,0,0,0,         ???
  0,0,0,0,         ???
  0,0,0,0,         ???

0,0,0,0          ???

So, yeah, no clue. I guess we're using libril from the device now, otherwise it would totally barf (because those extra fields aren't in hardware/ril/include/ril.h).
SI'm seeing the same thing as in comment 22 with a T-Mobile (US) SIM card. With a Taiwan Mobile SIM card I can confirm what Yoshi is seeing in comment 18. Specifically:

I/Gecko   (  106): RIL Worker: Parcel (size 172): 0,0,0,0,35,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,255,255,255,255,1,0,0,0,2,0,0,0,5,0,0,0,2,0,0,0,32,0,0,0,97,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,56,0,55,0,49,0,48,0,48,0,50,0,102,0,102,0,52,0,55,0,102,0,48,0,48,0,49,0,56,0,57,0,48,0,48,0,48,0,48,0,48,0,49,0,102,0,102,0,0,0,0,0,8,0,0,0,53,0,53,0,53,0,51,0,52,0,57,0,52,0,100,0,0,0,0,0,0,0,0,0,3,0,0,0,1,0,0,0,3,0,0,0,10,0,0,0,3,0,0,0,10,0,0,0

0,0,0,0,         RESPONSE_TYPE_SOLICITED
35,0,0,0,        token
0,0,0,0,         ERROR_SUCCESS
1,0,0,0,         CARD_STATE_PRESENT
0,0,0,0,         CARD_PINSTATE_UNKNOWN
0,0,0,0,         gsmUmtsSubscriptionAppIndex = 0
255,255,255,255, cdmaSubscriptionAppIndex = -1
255,255,255,255, imsSubScriptionAppIndex = -1
3,0,0,0,         apps.length = 3

  2,0,0,0,         CARD_APPTYPE_USIM
  5,0,0,0,         CARD_APPSTATE_READY
  2,0,0,0,         CARD_PERSOSUBSTATE_READY
  32,0,0,0,        aid.length = 32, aid = "a0000000871002ff47f00189000001ff"
  97,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,56,0,55,0,49,0,48,0,48,0,50,0,102,0,102,0,52,0,55,0,102,0,48,0,48,0,49,0,56,0,57,0,48,0,48,0,48,0,48,0,48,0,49,0,102,0,102,0,0,0,0,0,
  8,0,0,0,         app_label.length = 8, app_label = "5553494d" ("USIM")
  53,0,53,0,53,0,51,0,52,0,57,0,52,0,100,0,0,0,0,0,
  0,0,0,0,         pin1_replaced
  3,0,0,0,         pin1
  1,0,0,0,         pin2
  3,0,0,0,         ???
  10,0,0,0,        ???
  3,0,0,0,         ???
  10,0,0,0         ???

So, Yoshi is seeing the exact same problem as me, only that his SIM card has only one app on the SIM, so it doesn't fail. (Instead if just fails to read the last 16 bytes.)
Attached patch workaround — — Splinter Review 
Reviving the otoro RIL quirk detection to read the 4 extra ints. Not sure they're useful for anything (probably not), but if they are, we can always go back and assign them to anything. For now, this workaround works.
Assignee: allstars.chh → philipp
Attachment #644510 - Flags: review?(marshall)
Comment on attachment 644510 [details] [diff] [review]
workaround

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

The RIL_AppStatus struct in ril.h for otoro contains 4 extra unsigned char fields.

::: dom/system/gonk/ril_worker.js
@@ +727,5 @@
>          break;
>        case "Qualcomm RIL 1.0":
>          let product_model = libcutils.property_get("ro.product.model");
>          if (DEBUG) debug("Detected product model " + product_model);
> +        if (product_model == "otoro1") {

Using ro.product.model == 'otoro' (or 'otoro1') is not ideal as that's not what the real product model is.  can we create a new property like ro.moz.rilquirks or something, with a delimited list of tokens representing the different quirks that exist?
(In reply to Michael Vines [:m1] from comment #25)
> The RIL_AppStatus struct in ril.h for otoro contains 4 extra unsigned char
> fields.

Hey Michael -- I'm currently looking at ril.h from our otoro branch, and I don't see the 4 unsigned char fields in RIL_AppStatus. Which revision are you looking at?

My revision is fe9a3f63922143b57e79ed570bab2328df8c83a5 , and I'm looking at hardware/ril/include/telephony/ril.h
(In reply to Michael Vines [:m1] from comment #25)
> >          break;
> >        case "Qualcomm RIL 1.0":
> >          let product_model = libcutils.property_get("ro.product.model");
> >          if (DEBUG) debug("Detected product model " + product_model);
> > +        if (product_model == "otoro1") {
> 
> Using ro.product.model == 'otoro' (or 'otoro1') is not ideal as that's not
> what the real product model is.  can we create a new property like
> ro.moz.rilquirks or something, with a delimited list of tokens representing
> the different quirks that exist?

Are you proposing to do this at config/build time, e.g. put a setprop call into one of the startup scripts?
(In reply to Marshall Culpepper [:marshall_law] from comment #26)
> (In reply to Michael Vines [:m1] from comment #25)
> > The RIL_AppStatus struct in ril.h for otoro contains 4 extra unsigned char
> > fields.
> 
> Hey Michael -- I'm currently looking at ril.h from our otoro branch, and I
> don't see the 4 unsigned char fields in RIL_AppStatus. Which revision are
> you looking at?
> 
> My revision is fe9a3f63922143b57e79ed570bab2328df8c83a5 , and I'm looking at
> hardware/ril/include/telephony/ril.h

I'm looking at the ril.h from the actual vendor tree.  There have been additional changes to ril.h by the vendor that are not available on the web apparently.


@philikon -- yes, exactly.  Or initialize /system/build.prop appropriately.
(In reply to Michael Vines [:m1] from comment #28)
> (In reply to Marshall Culpepper [:marshall_law] from comment #26)
> > (In reply to Michael Vines [:m1] from comment #25)
> > > The RIL_AppStatus struct in ril.h for otoro contains 4 extra unsigned char
> > > fields.
> > 
> > Hey Michael -- I'm currently looking at ril.h from our otoro branch, and I
> > don't see the 4 unsigned char fields in RIL_AppStatus. Which revision are
> > you looking at?
> > 
> > My revision is fe9a3f63922143b57e79ed570bab2328df8c83a5 , and I'm looking at
> > hardware/ril/include/telephony/ril.h
> 
> I'm looking at the ril.h from the actual vendor tree.  There have been
> additional changes to ril.h by the vendor that are not available on the web
> apparently.

Thanks for confirming!

> @philikon -- yes, exactly.  Or initialize /system/build.prop appropriately.

Ok. I like that solution. I'll do this in a follow-up. Are we ok for the patch to proceed in the meantime?
(In reply to Philipp von Weitershausen [:philikon] from comment #29)
> Are we ok for the patch to proceed in the meantime?

sgtm
Comment on attachment 644510 [details] [diff] [review]
workaround

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

Ship it!
Attachment #644510 - Flags: review?(marshall) → review+
blocking-basecamp: ? → +
https://hg.mozilla.org/mozilla-central/rev/3e8c28be8b62
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: