837755 - [b2g-ril] SIM is being reported as 'absent' when airplane mode is on

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Salvador de la Puente González [:salva]]

Attachment: How CC shows when opened during airplane mode

STR:

1 - Long press home and ensure Cost Control application is closed
2 - Enable airplane mode
3 - Open Cost Control application

Expected:
The Cost Control application opens

Actual:
The Cost Control application reports there is no SIM (see attach)

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

It seems reasonable to me that the SIM card state should be different that `absent` while airplane mode is on. Patch's coming.

Comment 2

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

https://www.codeaurora.org/gitweb/quic/qrd-android/?p=platform/hardware/ril.git;a=blob;f=reference-ril/reference-ril.c;h=7b8cf6a46341c90f04278cf17414a4ecc416a0bf;hb=refs/heads/ics_qrd#l2695
https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/reference-ril/reference-ril.c#L1908

Returning SIM_NOT_READY in airplane mode is etched in rild.  Even you can turn it on in Gecko, the rild still answers NO for every operation besides RIL_REQUEST_RADIO_POWER & RIL_REQUEST_GET_SIM_STATUS.

The real problem is in Gaia, not Gecko. Whether do you want to allow user being able to browse cost control settings while in airplane mode is a UI problem.

Comment 3

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #1)
> It seems reasonable to me that the SIM card state should be
> different that `absent` while airplane mode is on. Patch's
> coming.

It's controlled by rild, not Gecko.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #1)
> > It seems reasonable to me that the SIM card state should be
> > different that `absent` while airplane mode is on. Patch's
> > coming.
> 
> It's controlled by rild, not Gecko.

Sure, I'm aware of that and what you told us in comment #2 but the SIM card status in content is `absent` when airplane mode is on so something's fishy here. _processICCStatus() function does some magic, have you taken a look a this function? IMHO the issue is around [1].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2813

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> > (In reply to José Antonio Olivera Ortega [:jaoo] from comment #1)

> Sure, I'm aware of that and what you told us in comment #2 but the SIM card
> status in content is `absent` when airplane mode is on so something's fishy
> here. _processICCStatus() function does some magic, have you taken a look a
> this function? IMHO the issue is around [1].
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#2813

After diving into the code and some debug it seems that even with SIM card present the rild daemon reports cardState `absent` when airplane mode is on. So we cannot do anything to fix that since rild daemon comes from OEMs and its behavior is not under our control. I'll take a look at what happens with the RIL implementation we ship with.

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Carlos Martínez has also figured out that cardstate is `absent` while airplane mode is on in the RIL implementation we ship with.

Comment 7

[Salvador de la Puente González [:salva]]

Changing the component as this is not only affecting Cost Control but Settings too. See bug  828996

Comment 8

[Salvador de la Puente González [:salva]]

I checked I have access to the ICCID during airplane mode but if this can be checked in Gaia, then it can be checked in Gecko.

I don't know if we should return a value distinct from NON-READY but the value must be for sure distinct than ABSENT. Simply because this is not true.

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> https://www.codeaurora.org/gitweb/quic/qrd-android/?p=platform/hardware/ril.
> git;a=blob;f=reference-ril/reference-ril.c;
> h=7b8cf6a46341c90f04278cf17414a4ecc416a0bf;hb=refs/heads/ics_qrd#l2695
> https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/reference-
> ril/reference-ril.c#L1908
> 
> Returning SIM_NOT_READY in airplane mode is etched in rild.

That's not happening at all.

At [1] `iccStatus.cardState` is `CARD_STATE_ABSENT` while airplane mode is on (RADIO_STATE_OFF). That's a rild quirk and we should deal with it and notify content a card state that makes sense. What about notifying `not ready` when radio state is off and card state is `absent`? I don't like at all but it's the best we can do at the moment.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2798

Comment 10

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #9)
> What about notifying `not ready` when radio state is off and
> card state is `absent`?

NO.  In airplane mode, the ICC status is ABSENT.  That's it.  That's a convention in Android rild implementation.

Besides, I don't think NOT_READY means more than ABSENT.  Either NOT_READY or ABSENT, they all mean ICC is not available currently.  However, please read source files more carefully, ril.h[1] clearly points out NOT_READY means "Radio is on, but the SIM interface is not ready", and that certainly doesn't apply to air plane mode (radio off).

Solve UI issue in UI.  That's not a framework issue.

[1]: https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L74

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #9)
> > What about notifying `not ready` when radio state is off and
> > card state is `absent`?
> 
> NO.  In airplane mode, the ICC status is ABSENT.  That's it.  That's a
> convention in Android rild implementation.

You told a different thing earlier, see comment #2. See the code you pointed at [1].

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/reference-ril/reference-ril.c#L1908

> Besides, I don't think NOT_READY means more than ABSENT.  Either NOT_READY
> or ABSENT, they all mean ICC is not available currently.  However, please
> read source files more carefully, ril.h[1] clearly points out NOT_READY
> means "Radio is on, but the SIM interface is not ready", and that certainly
> doesn't apply to air plane mode (radio off).

I agree.

> Solve UI issue in UI.  That's not a framework issue.

I'm not saying this is a framework issue that's something that doesn't make sense. What the UI could do if the SIM cardStatus is `absent` while airplane mode is on when there is actually a SIM card in the device?

Comment 12

[Salvador de la Puente González [:salva]]

Hello Vicamo, here is just my opinion.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #9)
> > What about notifying `not ready` when radio state is off and
> > card state is `absent`?
> 
> NO.  In airplane mode, the ICC status is ABSENT.  That's it.  That's a
> convention in Android rild implementation.

Excuseme Vicamo, can you point where is this said?
I found this:
http://developer.android.com/reference/android/telephony/TelephonyManager.html#SIM_STATE_ABSENT

And this:
http://stackoverflow.com/questions/7849192/what-is-the-value-of-sim-state-when-airplane-mode-is-turned-on

But I can't find any trace about this convention. Furthermore, the docs say:

>  public static final int SIM_STATE_ABSENT
> Added in API level 1
> 
> SIM card state: no SIM card is available in the device 

It clearly say "in the device".

> 
> Besides, I don't think NOT_READY means more than ABSENT.  Either NOT_READY
> or ABSENT, they all mean ICC is not available currently.  However, please
> read source files more carefully, ril.h[1] clearly points out NOT_READY
> means "Radio is on, but the SIM interface is not ready", and that certainly
> doesn't apply to air plane mode (radio off).

I agree too. That enumeration lacks of a category for this situation due to the RADIO_STATE_SIM_LOCKED_OR_ABSENT says: "SIM PIN locked, PUK required, network personalization locked, or SIM absent" and in this case the SIM is not absent but the radio is off.

> 
> Solve UI issue in UI.  That's not a framework issue.

I continue thinking this is a framework issue because we are missing a scenario: RIL off but SIM ready. If SIM were absent it would not make sense to allow access to its interface but it is possible.

Furthermore, if the enumerated being returned in these cases is RADIO_STATE_SIM_LOCKED_OR_ABSENT it should be disambiguated somewhere because the DOM interface allows to distinguish between networkLocked, pinRequired, pukRequired...

So I suggest only to add another disambiguation to take in count RIL is off but there is a SIM in the device.

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Salvador de la Puente González [:salva] from comment #12)
>
> So I suggest only to add another disambiguation to take in count RIL is off
> but there is a SIM in the device.

Can we do that?  I know on some previous chipsets I've worked with, the SIM interface is only attached to the modem, and can't be accessed if the radio is off.

Comment 14

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #11)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> > NO.  In airplane mode, the ICC status is ABSENT.  That's
> > it.  That's a convention in Android rild implementation.
> 
> You told a different thing earlier, see comment #2. See the
> code you pointed at [1].

Sorry, I totally got lost in previous context. It's NOT_READY. Sorry yet again.

> I'm not saying this is a framework issue that's something that
> doesn't make sense. What the UI could do if the SIM cardStatus
> is `absent` while airplane mode is on when there is actually a
> SIM card in the device?

You're right. It should be NOT_READY.

Comment 15

[Salvador de la Puente González [:salva]]

(In reply to Mike Habicher [:mikeh] from comment #13)
> (In reply to Salvador de la Puente González [:salva] from comment #12)
> >
> > So I suggest only to add another disambiguation to take in count RIL is off
> > but there is a SIM in the device.
> 
> Can we do that?  I know on some previous chipsets I've worked with, the SIM
> interface is only attached to the modem, and can't be accessed if the radio
> is off.

I can access ICCID so I suppose it is possible to check if ABSENT, then if it is possible to access ICCID and if so, to knwo we are in airplane mode. :)

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

Kyle, this bug is about re-writing a bit the _processICCStatus() function. The reason is that we were exposing cardState `absent` while airplane mode is on. With this version of the earlier mentioned function we follow the same approach that Android follows (see [1] please). Since Vicamo is on holidays could take a look at it please? Thanks!


[1] https://android.googlesource.com/platform/frameworks/opt/telephony/+/master/src/java/com/android/internal/telephony/uicc/IccCardProxy.java (Line 273).

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 713363 [details] [diff] [review]
v1

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

r=me, though I'm curious about icc error states now.

::: dom/system/gonk/ril_worker.js
@@ +2819,5 @@
> +      this.sendDOMMessage({rilMessageType: "cardstatechange",
> +                           cardState: this.cardState});
> +      return;
> +    }
> +    if (iccStatus.cardState == CARD_STATE_ERROR) {

Hmmm. Kinda curious about this. I had UNKNOWN put in for when we couldn't hit the network due to not being able to register with it. In what cases can we get an icc error? Should it be relayed up the chain better?

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Kyle, this new version has a minor addition. We'll report GECKO_CARDSTATE_UNKNOWN as well while icc application is null. I'll work on the follow-up bug 836257 and will land both patches at the same time for avoiding possible issues. Could take a look at it please? Thx!

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3 r=qDot

Version landed.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e174d6ad4a8

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e174d6ad4a8

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 719020 [details] [diff] [review]
v3 r=qDot

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 836257
User impact if declined: ICC card state is being reported as 'absent' when airplane mode is on.
Testing completed: Yes.
Risk to taking this patch (and alternatives if risky): Low IMHO.
String or UUID changes made by this patch: None.

Comment 24

[Marcelino Veiga Tuimil [:sonmarce]]

This is the remaining work to fix bug 830287, that is already merged in v1-train. Well, there is no dependency between both of them, as no extra work is need in bug 830287, but this bug should be fixed to have the problem described in bug 830287 totally fixed. Sorry, maybe it sounds like a tongue twister, but I could not find a better explanation. :-)

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Low risk and has been on central a few days, approving for v1-train uplift.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2caefd8e38ce

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out due to bustage caused by bug 822522 not landing first.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f086483f9092

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

Bug 836257 can't land on v1-train (where it has tracking+) until this is uplifted to gecko 18.  Is this bug planned to be uplifted to gecko 18?

Comment 29

[José Antonio Olivera Ortega [:jaoo]]

Yes. After rebasing this patch to be uplifted to gecko 18 I've noticed different behaviors between m-c and m-b2g18. I'm trying to figure out what is going on.

Comment 30

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #29)
> Yes. After rebasing this patch to be uplifted to gecko 18 I've noticed
> different behaviors between m-c and m-b2g18. I'm trying to figure out what
> is going on.

The issue is because bug 849758. Do not uplift this work until we figure out what's wrong in bug 849758.

Comment 31

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3 (mozilla-b2g18) r=qDot

Comment 32

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Antonio, will this land on b2g18?

Comment 33

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32)
> Antonio, will this land on b2g18?

Yes, that's the idea. Have you tested if everything works when landing 822522 and 849758 and this bugs in mozilla-b2g18? Great job in bug 849758 btw.

Comment 34

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 724009 [details] [diff] [review]
v3 (mozilla-b2g18) r=qDot

This patch depends on bug 822522 so bug 822522 should be uplifted as well. Ryan, as maybe you remember this work has issues on mozilla-b2g18 release repo. Bug 849758 solves these issue and that bug should be uplifted as well.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

I can't uplift bug 849758 until it gets approval or blocking status.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Bug 822522 also needs approval.

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

bug 849758 and bug 822522 have been approved now.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d806a3770898

Comment 39

[José Antonio Olivera Ortega [:jaoo]]

Noming for tef+ (see https://bugzilla.mozilla.org/show_bug.cgi?id=834103#c22). It blocks a blocker.

This bug depends on bug 822522 which is already tef+ but it has not been landed in mozilla-b2g18_v1_0_1 release branch yet.

Comment 40

[José Antonio Olivera Ortega [:jaoo]]

Anshul, we need you to review if this bug could be uplifted to mozilla-b2g18_v1_0_1 release repo. This bug is blocking other upliftings. Take a look please. Thanks.

Comment 41

[Anshul]

Jose, I explicitly asked for this bug to be also uplifted to v1 as per https://bugzilla.mozilla.org/show_bug.cgi?id=857837#c43 but I was told not to as per https://bugzilla.mozilla.org/show_bug.cgi?id=857837#c44 which didn't make much sense to me. Anyways, seems like the decision has now been made to uplift this to v1.

I will backport the change from v1.1 to v1.0 in commercial RIL today. You may uplift this bug to v1.0.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/fe28f4966397

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Does bug 849758 need to land on v1_0_1 as well?

Comment 44

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> Does bug 849758 need to land on v1_0_1 as well?

This bug was filed because we observed different behaviors between m-c and m-b2g18 repos in the card state reported on boot by the RIL plumbing. I'm not sure about what happens in m-b2g18_v1_0_1 release repo so I'll do a test asap and keep you updated. Thanks Ryan!

Comment 45

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #44)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> > Does bug 849758 need to land on v1_0_1 as well?
> 
> This bug was filed because we observed different behaviors between m-c and
> m-b2g18 repos in the card state reported on boot by the RIL plumbing. I'm
> not sure about what happens in m-b2g18_v1_0_1 release repo so I'll do a test
> asap and keep you updated. Thanks Ryan!

Yes, it seems the uplifting of bug 849758 is also needed. I'll comment it on that bug. Thanks you so much Ryan!