1111990 - [B2G][RIL] Should only access EF_FDN when FDN service is available

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar] (PTO 2/2 ~ 2/6)]

According to TS 131.102 Clause 4.2.24. EF_FDN is presented only if service n° 4 and/or service n° 89 is available. We should follow this behaviour in ril_worker.

Comment 1

[Edgar Chen [:edgar] (PTO 2/2 ~ 2/6)]

(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> According to TS 131.102 Clause 4.2.24. EF_FDN is presented only if service
> n° 4 and/or service n° 89 is available. We should follow this behaviour in
  ^^^^
  typo, it should be `n° 2`

> ril_worker.

Comment 2

[John Dai[:jdai]]

Attachment: Support access EF_FDN only if FDN service is available

1. Add RUIM FDN ICC Services Table support base on GPP2 C.S0023-D 3.4.18.
2. Check FDN service is available before use.
3. Update the related testcase.

Comment 3

[Edgar Chen [:edgar] (PTO 2/2 ~ 2/6)]

Comment on attachment 8542480 [details] [diff] [review]
Support access EF_FDN only if FDN service is available

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

Please see my comments below. Thank you.

::: dom/system/gonk/ril_consts.js
@@ +1315,5 @@
>      SPDI: 51
>    },
>    // @see 3GPP2 C.S0023-D 3.4.18 (RUIM).
>    ruim: {
> +  	FDN: 3,

nit: indention.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +129,5 @@
>    function do_test(aSimType, aContactType, aExpectedContact, aEnhancedPhoneBook) {
>      ril.appType = aSimType;
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
>      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:

I am wondering why you set 0x22 for the 1st byte of CDMA Service Table.

And I found one interesting thing in |isICCServiceAvailable| of ril_worker, for SIM/RUIM we didn't take the `second bit` into account, I think we need to correct that, maybe in a follow-up bug.

@@ +131,5 @@
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
>      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:
> +                                    [0x22, 0x00, 0x0, 0x0, 0x0];
> +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];

Ditto, and I think we should have two different sst for SIM and USIM.

@@ +242,5 @@
>  
>    function do_test(aSimType, aContactType, aContact, aPin2, aFileType, aHaveIapIndex, aEnhancedPhoneBook) {
>      ril.appType = aSimType;
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]

Ditto

@@ +244,5 @@
>      ril.appType = aSimType;
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]
> +                                                  : [0x22, 0x00, 0x0, 0x0, 0x0];
> +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];

Ditto

Comment 4

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> Comment on attachment 8542480 [details] [diff] [review]
> Support access EF_FDN only if FDN service is available
> 
> Review of attachment 8542480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments below. Thank you.
> 
> ::: dom/system/gonk/ril_consts.js
> @@ +1315,5 @@
> >      SPDI: 51
> >    },
> >    // @see 3GPP2 C.S0023-D 3.4.18 (RUIM).
> >    ruim: {
> > +  	FDN: 3,
> 
> nit: indention.
Will do.
> 
> ::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
> @@ +129,5 @@
> >    function do_test(aSimType, aContactType, aExpectedContact, aEnhancedPhoneBook) {
> >      ril.appType = aSimType;
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> >      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> > +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:
> 
> I am wondering why you set 0x22 for the 1st byte of CDMA Service Table.
> 
It should set 0x20 for the 1st byte. I will modify it.
> And I found one interesting thing in |isICCServiceAvailable| of ril_worker,
> for SIM/RUIM we didn't take the `second bit` into account, I think we need
> to correct that, maybe in a follow-up bug.
I file a follow-up Bug 1116702 to trace this issue.
> 
> @@ +131,5 @@
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> >      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> > +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:
> > +                                    [0x22, 0x00, 0x0, 0x0, 0x0];
> > +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];
> 
> Ditto, and I think we should have two different sst for SIM and USIM.
Will do.
> 
> @@ +242,5 @@
> >  
> >    function do_test(aSimType, aContactType, aContact, aPin2, aFileType, aHaveIapIndex, aEnhancedPhoneBook) {
> >      ril.appType = aSimType;
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> > +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]
> 
> Ditto
Will do.
> 
> @@ +244,5 @@
> >      ril.appType = aSimType;
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> > +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]
> > +                                                  : [0x22, 0x00, 0x0, 0x0, 0x0];
> > +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];
> 
> Ditto
Will do.

Comment 5

[John Dai[:jdai]]

Attachment: Support access EF_FDN only if FDN service is available.(V2)

1. Revise indention.
2. Update the related testcase base on Comment 4.

Comment 6

[Edgar Chen [:edgar] (PTO 2/2 ~ 2/6)]

Comment on attachment 8542876 [details] [diff] [review]
Support access EF_FDN only if FDN service is available.(V2)

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

I guess you uploaded a wrong file. ;)

Comment 7

[John Dai[:jdai]]

Attachment: Support access EF_FDN only if FDN service is available.(V3)

It's my bad. Upload a correct patch.

Comment 8

[Edgar Chen [:edgar] (PTO 2/2 ~ 2/6)]

Comment on attachment 8542904 [details] [diff] [review]
Support access EF_FDN only if FDN service is available.(V3)

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

Looks good, please remember to add r=me in commit message, thank you.

Comment 9

[John Dai[:jdai]]

Attachment: Support access EF_FDN only if FDN service is available.(Final)

Comment 10

[John Dai[:jdai]]

TryServer:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba52d7f2ec24

Comment 11

[Edgar Chen [:edgar] (PTO 2/2 ~ 2/6)]

https://hg.mozilla.org/integration/b2g-inbound/rev/355429828a63

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/355429828a63