1019358 - [dolphin] unable to activate PDP for some specific SIM

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Peipei Cheng (needinfo if you need my action)]

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.

[e-mail] will provide patch

Comment 1

[Peipei Cheng (needinfo if you need my action)]

Attachment: slog

Comment 2

[Ming]

Attachment: 1019358.patch

just check the mncLength

Comment 3

[Ming]

hi pcheng , help to get the right man to review this patch

Comment 4

[Peipei Cheng (needinfo if you need my action)]

Steven, could you please help find someone review this patch?

Comment 5

[Preeti Raghunath(:Preeti)]

What's the user impact here?

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

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 '-'.

Comment 7

[Ming]

Attachment: 1019358.patch

Dear vyang ,plz help to review .

Tks!

Comment 8

[shawn ku [:sku]]

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"}

Comment 9

[Peipei Cheng (needinfo if you need my action)]

(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.

Comment 10

[Ming]

(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);

Comment 11

[Ming]

Attachment: 1019358-v2.patch

dear vyang & sku ,plz help to view

Comment 12

[shawn ku [: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

Comment 13

[Ming]

Attachment: 1019358-v3.patch

Thanks. 
Dear vyang & sku ,plz help to review.

Comment 14

[Vicamo Yang [:vicamo][: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

Comment 15

[shawn ku [:sku]]

clear ni? due to Vicamo already review it.

Comment 16

[Ming]

Attachment: 1019358-v4.patch

Sorry for type error.
Updated. Plz help to review again.
Thanks :)

Comment 17

[Vicamo Yang [:vicamo][: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.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: test cases

Comment 19

[shawn ku [: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.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c6bf6c65c913
https://hg.mozilla.org/integration/b2g-inbound/rev/1edaadcd7e6a

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6bf6c65c913
https://hg.mozilla.org/mozilla-central/rev/1edaadcd7e6a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2e74f337ca70
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/8ee0c4e13afb