Last Comment Bug 849758 - [b2g-ril] SIM PIN requested after booting/rebooting with airplane mode on (mozilla-b2g18 only)
: [b2g-ril] SIM PIN requested after booting/rebooting with airplane mode on (mo...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla22
Assigned To: Michael Schwartz [:m4]
:
Mentors:
Depends on: 822522 837755
Blocks: b2g-ril
  Show dependency treegraph
 
Reported: 2013-03-11 02:19 PDT by José Antonio Olivera Ortega [:jaoo]
Modified: 2013-05-03 03:59 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
wontfix
wontfix
fixed
+
fixed
wontfix
fixed


Attachments
|adb logcat| output RIL debug enabled. (96.59 KB, text/plain)
2013-03-11 02:19 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details
v1 (2.50 KB, patch)
2013-03-12 10:09 PDT, José Antonio Olivera Ortega [:jaoo]
kyle: feedback+
Details | Diff | Splinter Review
Patch (2.07 KB, patch)
2013-03-15 00:26 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
vicamo: review+
Details | Diff | Splinter Review
Patch v2. (2.06 KB, patch)
2013-03-15 02:04 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
vicamo: review+
Details | Diff | Splinter Review
[b2g18] Patch (2.07 KB, patch)
2013-03-15 04:19 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
lukasblakk+bugs: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review
[b2g18_v1_0_1] Patch (2.17 KB, patch)
2013-04-25 00:49 PDT, José Antonio Olivera Ortega [:jaoo]
allstars.chh: feedback+
Details | Diff | Splinter Review

Description José Antonio Olivera Ortega [:jaoo] 2013-03-11 02:19:42 PDT
Created attachment 723376 [details]
|adb logcat| output RIL debug enabled.

While trying to uplift bugs 822522 and 837755 to mozilla-b2g18 release repo I've noticed that the phone prompts the user to enter the PIN code even if the phone boots/reboots while airplane mode is on. I'm seeing issue in mozilla-b2g18 release repo.

It seems that something's fishy in the responses for `REQUEST_GET_SIM_STATUS` parcels. Log attached.
Comment 1 José Antonio Olivera Ortega [:jaoo] 2013-03-11 03:07:19 PDT
Marking as tef? since the issue is also happening with shipping RIL.
Comment 2 José Antonio Olivera Ortega [:jaoo] 2013-03-12 10:09:00 PDT
Created attachment 724004 [details] [diff] [review]
v1

After spending lot of time trying to figure out why the behavior between m-c and m-b2g18 is totally different on this issue I've ended up making this patch. I've been trying to find out the patch (or patches) that has been landed on m-b2g18 that makes this issue happens and the patch (or patches) that has been landed on m-c and we must uplift for achieving the same behavior. I had no luck and it seems so difficult to know where the differences between repos are. I request feedback here to you guys to know what to do about this issue.

This patch could be landed on m-c as well as doesn't brake anything and that will make having the same code in the two different repos. This patch depends on both 822522 and 837755 so before landing it on m-b2g18 we must uplift those bugs to m-b2g18 release repo.
Comment 3 Kyle Machulis [:kmachulis] [:qdot] 2013-03-12 14:38:51 PDT
Comment on attachment 724004 [details] [diff] [review]
v1

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

Seems ok to me, but I'm going to defer to vicamo's feedback/review since I'm not up on current RIL dev.
Comment 4 Vicamo Yang [:vicamo][:vyang] 2013-03-12 19:48:32 PDT
Comment on attachment 724004 [details] [diff] [review]
v1

Yoshi, could you help this?
Comment 5 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-12 23:48:36 PDT
Antonio, you should already fixed this in Bug 837755, right?
But the patch in Bug 837755 cannot be applied into b2g18.

I manually fix the conflict, and It works now,
no PIN dialog pop up.
Comment 6 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-12 23:49:29 PDT
Comment on attachment 724004 [details] [diff] [review]
v1

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

I think this bug is already fixed by jaoo himself in Bug 837755.
Comment 7 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-13 00:08:39 PDT
Oh, seems patch from Bug 837755 doesn't fix this.
Comment 8 José Antonio Olivera Ortega [:jaoo] 2013-03-13 00:52:28 PDT
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #7)
> Oh, seems patch from Bug 837755 doesn't fix this.

Yes, that's true. This is because there is an additional patch in this bug. What we should do IMHO is to uplift bugs both 822522 and 837755 and them apply this bug to mozilla-b2g18 release repo. On the other hand this patch could be landed on m-c as well as doesn't brake anything and that will make having the same code in the two different repos.
Comment 9 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-13 02:01:05 PDT
Hi, Antonio
I wonder why m-c won't have this problem? 
Do you know why?
Comment 10 José Antonio Olivera Ortega [:jaoo] 2013-03-13 02:13:37 PDT
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #9)
> Hi, Antonio
> I wonder why m-c won't have this problem? 
> Do you know why?

I've spent lot of time trying to figure out why the behavior between m-c and m-b2g18 is totally different on this issue. I've been trying to find out the patch (or patches) that has been landed on m-b2g18 that makes this issue happens and the patch (or patches) that has been landed on m-c and we must uplift for achieving the same behavior. I had no luck and it seems so difficult to know where the differences between repos are.
Comment 11 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-13 03:24:36 PDT
I'll also check this and discuss this with Vicamo.
Comment 12 Michael Schwartz [:m4] 2013-03-13 18:38:26 PDT
Simply overriding the cardstate at the lower levels will indicate to the UI that the card in not ready when it is.  That will cut the user's ability to interact with card services such as STK, contacts on the card, changing PIN enabled, etc. when it should be OK.  I don't think that would be GCF compliant.

The UI should determine when showing PIN-entry is appropriate and do it then.
Comment 13 José Antonio Olivera Ortega [:jaoo] 2013-03-14 08:17:58 PDT
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #11)
> I'll also check this and discuss this with Vicamo.

Yoshi, any update here? Thanks.
Comment 14 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-14 23:32:23 PDT
Sorry for my late reply,
I also spent a lot of time to figure this out.

I think this is related to some init timing issue.

In b2g18 branch, when ril_worker connects to rild, modem isn't completely initialized yet, so the first 4~5 requests all fail with error 1, RADIO_NOT_AVAILABLE.

Whereas in m-c, there are some changes about SystemWorkerManager and RadioInterfaceLayer, and I compare the timing of socket is connected between m-c and b2g18, m-c is some milliseconds later then b2g18. So on m-c, those first requests returned successfully.

And one of the requests, is to turn off RADIO_POWER.
So RADIO_POWER off request is successed in m-c, but failed in b2g18.

I also check the radio log, on b2g18, ril_worker sends the 1st request to rild immediately.


E/RILC    (  112): isMultiSimEnabled: prop_val = 0 enabled = 0
D/RILC    (  112): RID 0 currentState() -> Radio Unavailable(1)
D/RILC    (  112): RID 0 currentState() -> Radio Unavailable(1)
D/RILC    (  112): getVersion() -> Qualcomm RIL 1.0
D/RILC    (  112): UI --- RIL_REQUEST_RADIO_POWER (23) ---> RIL [RID 0, token id 1, data len 4]
D/RILC    (  112): Ignore RIL_REQUEST_RADIO_POWER, unsupported state.
D/RILC    (  112): UI <--- RIL_REQUEST_RADIO_POWER (23) Complete --- RIL [RID 0, Token 1, Radio Not Available, Len 0 ]


(Actually from your log, the first request is SET_PREFERRED_NETWORK_TYPE, but I disable it for debug purpose, in your log, RADIO_POWER is from token 3).
Comment 15 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 00:26:10 PDT
Created attachment 725300 [details] [diff] [review]
Patch

I think the problem is on b2g18, REQUEST_RADIO_OFF is always failed due to RADIO_UNAVAIABLE, so I move the flag _isInitialRadioState to handler, and only turn it to false only when the request succeed.
Comment 16 Vicamo Yang [:vicamo][:vyang] 2013-03-15 00:45:53 PDT
Comment on attachment 725300 [details] [diff] [review]
Patch

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

After discuss with Yoshi in private, his patch matches Android's behaviour and solves the original problem, too. There is a timing difference between b2g18 and m-c. Althought the latter is immune to this issue right now, I'm not 100% sure it won't happen in the future. I think Yoshi's patch solves the root cause. Thank you both :).
Comment 17 Vicamo Yang [:vicamo][:vyang] 2013-03-15 00:46:57 PDT
Comment on attachment 724004 [details] [diff] [review]
v1

This patch is obsoleted by attachment 725300 [details] [diff] [review]. Thank you for the great job done here :)
Comment 18 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 00:55:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f445e0cbba3b
Comment 19 José Antonio Olivera Ortega [:jaoo] 2013-03-15 01:08:15 PDT
Have you tested if everything works when landing 822522 and 837755 and this bugs in mozilla-b2g18? Great job here btw. Thanks.
Comment 20 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 02:01:35 PDT
Backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8c5998b7e4

There's some mistake in my patch.
I should haven't returned too early. :(
Comment 21 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 02:04:26 PDT
Created attachment 725318 [details] [diff] [review]
Patch v2.
Comment 22 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 04:12:51 PDT
From jaoo's log,

in line 693, radio has reported RADIO_STATE_OFF,
however in line 721, ril_worker has called REQUEST_GET_SIM_STATUS (token 5), in line 743 the response of GET_SIM_STATUS is reporting RIL_CARDSTATE_PRESENT.

That seems a bug from modem side.

Hi, Michael Schwarts, is this a known problem to you ?
Comment 23 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 04:14:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/04544e876ce3
Comment 24 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 04:19:46 PDT
Created attachment 725343 [details] [diff] [review]
[b2g18] Patch

The same patch for b2g18 branch
Comment 25 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 04:20:42 PDT
The b2g18 patch should depend on patches from Bug 822522, and Bug 837755.
Comment 26 Andrew Overholt [:overholt] 2013-03-15 10:30:21 PDT
ni(m4) for comment 22.
Comment 27 Michael Schwartz [:m4] 2013-03-15 16:25:43 PDT
The card is present and it's reporting it as present - I don't see the bug.

jaoo, getting back to the main issue reported, yes Gaia is being notified of the card state transitions.  Gaia needs to be a little smarter and conditionally show PIN entry as needed.

PIN entry shouldn't be required for basic smartphone use - it only locks the subscription information and related files.  Whenever the user tries to access PIN locked functionality, we check the state and conditionally show PIN entry.  During airplane mode, the user doesn't care so we shouldn't show it.
Comment 28 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-15 20:14:38 PDT
(In reply to Michael Schwartz [:m4] from comment #27)
> The card is present and it's reporting it as present - I don't see the bug.
> 

So when we turn off the radio, will card also report 'PRESENT' ?
Comment 29 Phil Ringnalda (:philor) 2013-03-16 15:56:49 PDT
https://hg.mozilla.org/mozilla-central/rev/04544e876ce3
Comment 30 Daniel Coloma:dcoloma 2013-03-18 04:22:11 PDT
Does this needs to be uplifted? If so we need to set-up the tracking-flags accordingly (I assume so as it is a tef+ bug).
Comment 31 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-18 19:52:11 PDT
This bug is already tef+.
And my patch is for Mozilla RIL, not for partner's RIL.
I am not quite sure about the status of them.

Also see my comment 25, there are some other patches should be land first.
Comment 32 Michael Vines [:m1] [:evilmachines] 2013-03-18 19:58:27 PDT
Clearing tef+ as we don't need this patch to commercialize v1.0.1
Comment 33 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-18 23:58:46 PDT
Hi Michael,
Can you see Comment 1 from jaoo as this also happened on partner's RIL?
Comment 34 Michael Vines [:m1] [:evilmachines] 2013-03-19 00:33:08 PDT
m4, do we need any gecko/gaia changes to resolve this issue with the latest commercial RIL?
Comment 35 José Antonio Olivera Ortega [:jaoo] 2013-03-19 00:48:26 PDT
This issue was detected while trying to uplift bugs 822522 and 837755 to mozilla-b2g18 release. Bug 837755 is marked as `approval-mozilla-b2g18+` so this work -and bug 822522- (its dependencies) has to be uplifted as well. This is also happening even with shipping RIL.
Comment 36 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-19 00:50:10 PDT
I'd suggest move dicussion of partner's RIL to other bug.
Comment 37 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2013-03-19 02:07:25 PDT
Move Taget Milestone back to Mozilla 22, as I found jaoo seemed to remov it by accendent.
Comment 38 Michael Vines [:m1] [:evilmachines] 2013-03-19 07:31:59 PDT
approval-mozilla-b2g18+ is for v1.1 right?
Comment 39 Ryan VanderMeulen [:RyanVM] 2013-03-19 08:34:54 PDT
(In reply to Michael Vines [:m1] [:evilmachines] from comment #38)
> approval-mozilla-b2g18+ is for v1.1 right?

Yes
Comment 40 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-19 09:40:04 PDT
(In reply to Michael Vines [:m1] [:evilmachines] from comment #38)
> approval-mozilla-b2g18+ is for v1.1 right?

That's correct for now until we branch v1.1 to its own repo, approval-mozilla-b2g18+ will always mean approval for whatever version is the v1-train.
Comment 41 José Antonio Olivera Ortega [:jaoo] 2013-03-20 10:17:25 PDT
Comment on attachment 725343 [details] [diff] [review]
[b2g18] Patch

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 #): None.
User impact if declined: The phone prompts the user to enter the PIN code even if the phone boots/reboots while airplane mode is on.
Testing completed: Yes.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Comment 42 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-20 12:37:05 PDT
Comment on attachment 725343 [details] [diff] [review]
[b2g18] Patch

Approving for v1.1 uplift
Comment 43 Ryan VanderMeulen [:RyanVM] 2013-03-21 08:51:46 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e5ea983c38a0
Comment 44 Michael Schwartz [:m4] 2013-04-01 11:18:19 PDT
(In reply to Michael Vines [:m1] [:evilmachines] from comment #34)
> m4, do we need any gecko/gaia changes to resolve this issue with the latest
> commercial RIL?

No, a local hack like the one here will work.
Comment 45 José Antonio Olivera Ortega [:jaoo] 2013-04-25 00:45:22 PDT
We need to uplift this bug due the landing of bug 837755 in mozilla-b2g18_v1_0_1 release repo. Requesting tef+ flag here.

Anshul, could you review if this uplifting could bring any problem on shipping RIL?
Comment 46 José Antonio Olivera Ortega [:jaoo] 2013-04-25 00:49:50 PDT
Created attachment 741714 [details] [diff] [review]
[b2g18_v1_0_1] Patch

With Yoshi's permission I'm attaching the patch I've rebased for testing on mozillab2g-18_v1_0_1 release repo. Yoshi, a double check from your side would be great. I've rebase the patch and attached here. Feel free to mark as obsolete if you want to attach your. Thanks Yoshi!
Comment 47 Ryan VanderMeulen [:RyanVM] 2013-04-25 08:15:24 PDT
Ah crap, I confused this with bug 849757 and pushed it to b2g18_v1_0_1.
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e7671882de42

Let me know if I need to backout.
Comment 48 Michael Vines [:m1] [:evilmachines] 2013-04-25 08:19:31 PDT
Doesn't really matter as this patch is unused in commercial devices for both v1.0.1 or v1.1, so it's a NOP.
Comment 49 Anshul 2013-04-25 12:03:16 PDT
Also, we already have support for this in commercial RIL on v1.

Note You need to log in before you can comment on or make changes to this bug.