Closed Bug 1019358 Opened 6 years ago Closed 6 years ago

[dolphin] unable to activate PDP for some specific SIM

Categories

(Firefox OS Graveyard :: RIL, defect)

Other
Other
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox30 wontfix, firefox31 wontfix, firefox32 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: angelc04, Assigned: ming.li)

Details

(Whiteboard: [sprd317137])

Attachments

(3 files, 4 obsolete files)

From spreadtrum developer's analysis:

06-01 11:38:19.025   158   549 D use-Rlog/RLOG-AT: [w] Channel2: AT> AT+CRSM=176,28589,0,0,4,0,"3f007fff"
06-01 11:38:19.115   158   191 D use-Rlog/RLOG-AT: [w] Channel2: AT< +CRSM: 144,0,FFFFFFFF
06-01 11:38:19.115   158   191 D use-Rlog/RLOG-AT: [w] Channel2: AT< OK
06-01 11:38:19.305   106   506 I Gecko   : RIL Worker: [0] AD: 255, 255, 255, 255, 
06-01 11:38:19.305   106   506 I Gecko   : RIL Worker: [0] IMSI: 460077101161943 MCC: 460 MNC: 077101161943

MNC return error 077101161943, and AT+CRSM=176,28589,0,0,4,0,"3f007fff" returns FFFFFFFF

This is because some operator SIM does not have MNC length. Need to handle this in gecko.

ming.li@spreadtrum.com will provide patch
Attached file slog
Attached patch 1019358.patch (obsolete) — Splinter Review
just check the mncLength
hi pcheng , help to get the right man to review this patch
Flags: needinfo?(pcheng)
Steven, could you please help find someone review this patch?
Flags: needinfo?(pcheng) → needinfo?(styang)
Flags: needinfo?(james.zhang)
Whiteboard: [sprd317137]
blocking-b2g: --- → 1.4?
Flags: needinfo?(james.zhang)
Flags: needinfo?(sku)
Attachment #8433111 - Flags: review?(vyang)
Flags: needinfo?(styang)
Flags: needinfo?(sku)
Component: General → RIL
What's the user impact here?
Flags: needinfo?(pcheng)
Comment on attachment 8433111 [details] [diff] [review]
1019358.patch

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

r- because this patch does not contain necessary infos to land.  Please see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for steps.

::: dom/system/gonk/ril_worker.js
@@ +13912,5 @@
>      if (!mncLength) {
> +      mncLength = 0;
> +    }
> +    // if mncLength is not a valid value, use the default value
> +    if (mncLength > (imsi.length-3) || mncLength <= 0) {

nit: space before and after binary operator '-'.
Attachment #8433111 - Flags: review?(vyang) → review-
Attached patch 1019358.patch (obsolete) — Splinter Review
Dear vyang ,plz help to review .

Tks!
Attachment #8433111 - Attachment is obsolete: true
Flags: needinfo?(vyang)
Hi Ming:
  My two cents, it is better to handle this error at [1] since spec. defines only 00000010 or 00000011
are valid values.


[1] - http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#12795

// TS 31.102, clause 4.2.18 EFAD (Administrative Data)
This value codes the number of digits of the MNC in
the IMSI. Only the values '0010' and '0011' are
currently specified, all other values are reserved
for future use.

// Log snippet
05-28 13:44:05.395   134   561 D use-Rlog/RLOG-RILC: [w] [0011]< GET_IMSI {sw1=0x90,sw2=0x0,621C8202412183022FE2A5038001318A01058B032F06018002000A880110} {460077101161943}
05-28 13:44:06.056   134   560 D use-Rlog/RLOG-RILC: [w] [0015]> SIM_IO (cmd=0xB0,efid=0x6FAD,path=3f007fff,0,0,4,(null),pin2=(null),aid=(null))
05-28 13:44:06.086   134   560 D use-Rlog/RLOG-RILC: [w] [0015]< SIM_IO {sw1=0x90,sw2=0x0,FFFFFFFF}

05-28 13:44:06.576   107   535 I Gecko   : RIL Worker: [0] AD: 255, 255, 255, 255, 

05-28 13:44:06.866   107   107 I Gecko   : -*- RadioInterface[0]: Received message from worker: {"iccType":"usim","iccid":"898600*10910","rilMessageType":"iccinfochange","rilMessageClientId":0,"mcc":"460","mnc":"077101161943"}
(In reply to Preeti Raghunath(:Preeti) from comment #5)
> What's the user impact here?

User was unable to use the data connection due to this.
Flags: needinfo?(pcheng)
(In reply to shawn ku [:sku] from comment #8)
> Hi Ming:
>   My two cents, it is better to handle this error at [1] since spec. defines
> only 00000010 or 00000011
> are valid values.
Thanks sku!
So code should be like this?

// TS 31.102, clause 4.2.18 EFAD 
let mncLength = 0;
if (ad  && ad[3]) {
  mncLength = ad[3] & 0x0f;
  if (mncLength != 0x02 || mncLength != 0x03)
  {
    mncLength = 0;
  }
}
// The 4th byte of the response is the length of MNC.
let mccMnc = ICCUtilsHelper.parseMccMncFromImsi(RIL.iccInfoPrivate.imsi,
                                                      mncLength);
Attached patch 1019358-v2.patch (obsolete) — Splinter Review
dear vyang & sku ,plz help to view
Attachment #8433765 - Attachment is obsolete: true
Flags: needinfo?(sku)
Hi Ming:

1. Logic looks okay to me, still need Vicamo's review.
   Besides, [1] also need some revise to make parameter more clear since both CDMA/GSM will share the same API.
2. Please provide hg format patch due this patch is located in gecko side.


[1] - http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#14132

Thanks!!
Shawn
Flags: needinfo?(sku)
Attached patch 1019358-v3.patch (obsolete) — Splinter Review
Thanks. 
Dear vyang & sku ,plz help to review.
Attachment #8433824 - Attachment is obsolete: true
Flags: needinfo?(sku)
Attachment #8433871 - Flags: review?(vyang)
Comment on attachment 8433871 [details] [diff] [review]
1019358-v3.patch

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

r=me. Thank you.

::: dom/system/gonk/ril_worker.js
@@ +13913,4 @@
>     *        The imsi of icc.
>     * @param mncLength [optional]
>     *        The length of mnc.
> +   *        Zero indicates we havn't got a valid mnc length.

nit: haven't
Attachment #8433871 - Flags: review?(vyang) → review+
Assignee: nobody → ming.li
Flags: needinfo?(vyang)
clear ni? due to Vicamo already review it.
Flags: needinfo?(sku)
Attached patch 1019358-v4.patchSplinter Review
Sorry for type error.
Updated. Plz help to review again.
Thanks :)
Attachment #8433871 - Attachment is obsolete: true
Attachment #8434006 - Flags: review?(vyang)
Comment on attachment 8434006 [details] [diff] [review]
1019358-v4.patch

Hi, you don't need another review for r+-ed patches.  Just carry the flag in further revisions unless somehow that ends up with a quite different patch.
Attachment #8434006 - Flags: review?(vyang) → review+
blocking-b2g: 1.4? → 1.4+
Attached patch test casesSplinter Review
Attachment #8434655 - Flags: review?(sku)
Comment on attachment 8434655 [details] [diff] [review]
test cases

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

Thank you to remind us test cases part.
Attachment #8434655 - Flags: review?(sku) → review+
You need to log in before you can comment on or make changes to this bug.