Closed Bug 849758 Opened 11 years ago Closed 11 years ago

[b2g-ril] SIM PIN requested after booting/rebooting with airplane mode on (mozilla-b2g18 only)

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jaoo, Assigned: mschwart)

References

Details

Attachments

(4 files, 2 obsolete files)

|adb logcat| output RIL debug enabled.
96.59 KB, text/plain
Details
Patch v2.
2.06 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
[b2g18] Patch
2.07 KB, patch
lsblakk
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
[b2g18_v1_0_1] Patch
2.17 KB, patch
allstars.chh
: feedback+
Details | Diff | Splinter Review 
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.
Blocks: b2g-ril
Marking as tef? since the issue is also happening with shipping RIL.
blocking-b2g: --- → tef?
Assignee: nobody → anshulj
blocking-b2g: tef? → tef+
blocking-b2g: tef+ → ---
blocking-b2g: --- → tef+
Attached patch v1 (obsolete) — — Splinter Review 
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.
Attachment #724004 - Flags: feedback?(vyang)
Attachment #724004 - Flags: feedback?(kyle)
Depends on: 822522, 837755
Assignee: anshulj → mschwart
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.
Attachment #724004 - Flags: feedback?(kyle) → feedback+
Comment on attachment 724004 [details] [diff] [review]
v1

Yoshi, could you help this?
Attachment #724004 - Flags: feedback?(vyang) → feedback?(allstars.chh)
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 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.
Attachment #724004 - Flags: feedback?(allstars.chh) → feedback-
Oh, seems patch from Bug 837755 doesn't fix this.
(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.
Hi, Antonio
I wonder why m-c won't have this problem? 
Do you know why?
(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.
I'll also check this and discuss this with Vicamo.
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.
(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.
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).
Attached patch Patch (obsolete) — — Splinter Review 
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 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 :).
Attachment #725300 - Flags: review?(vyang) → review+
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 :)
Attachment #724004 - Attachment is obsolete: true
Have you tested if everything works when landing 822522 and 837755 and this bugs in mozilla-b2g18? Great job here btw. Thanks.
Backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8c5998b7e4

There's some mistake in my patch.
I should haven't returned too early. :(
Attachment #725318 - Flags: review?(vyang) → review+
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 ?
Attached patch [b2g18] Patch — — Splinter Review 
The same patch for b2g18 branch
The b2g18 patch should depend on patches from Bug 822522, and Bug 837755.
ni(m4) for comment 22.
Flags: needinfo?(mschwart)
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.
Flags: needinfo?(mschwart)
(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' ?
https://hg.mozilla.org/mozilla-central/rev/04544e876ce3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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).
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.
Clearing tef+ as we don't need this patch to commercialize v1.0.1
blocking-b2g: tef+ → -
Hi Michael,
Can you see Comment 1 from jaoo as this also happened on partner's RIL?
m4, do we need any gecko/gaia changes to resolve this issue with the latest commercial RIL?
Flags: needinfo?(mschwart)
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.
Target Milestone: mozilla22 → ---
I'd suggest move dicussion of partner's RIL to other bug.
Move Taget Milestone back to Mozilla 22, as I found jaoo seemed to remov it by accendent.
Target Milestone: --- → mozilla22
approval-mozilla-b2g18+ is for v1.1 right?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #38)
> approval-mozilla-b2g18+ is for v1.1 right?

Yes
(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 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.
Attachment #725343 - Flags: approval-mozilla-b2g18?
Comment on attachment 725343 [details] [diff] [review]
[b2g18] Patch

Approving for v1.1 uplift
Attachment #725343 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
(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.
Flags: needinfo?(mschwart)
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?
blocking-b2g: - → tef?
Flags: needinfo?(dcoloma)
Flags: needinfo?(anshulj)
Whiteboard: NO_UPLIFT
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!
Attachment #741714 - Flags: feedback?(allstars.chh)
Attachment #741714 - Flags: feedback?(allstars.chh) → feedback+
blocking-b2g: tef? → tef+
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.
status-b2g18-v1.0.1: wontfix → fixed
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.
Whiteboard: NO_UPLIFT
Also, we already have support for this in commercial RIL on v1.
Flags: needinfo?(anshulj)
Flags: needinfo?(dcoloma)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: