740300 - B2G RIL: improper behavior in RadioStateChange

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

This is a regression from Bug 728886.
This will cause getICCStatus cannot be called

When radio state is SIM_NOT_READY(2),
newState will be GECKO_RADIOSTAE_READY, [1]
and it will set this.radioState to GECKO_RADIOSTATE_READY in [2]

Next when radio state is SIM_READY(4)
newState will again be GECKO_READYSTATE_READY [1]
and the function will return by checking this.radioState == newState [3], 

Therefore getICCStatus() still won't get called even SIM_READY event is passed


[1]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2004
[2]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2037
[2]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2011

Comment 1

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

Attachment: [WIP] RadioState patch

Hi, philikon
I make a temp patch at least for Galaxy S2 now, what I do is mostly revert the code. I am not quite sure why you change the code here for RIL v6.

thanks

Comment 2

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

Attachment: patch to RadioStateChange

I found that we can also call SIM_GET_STATUS when radioState is SIM_NOT_READY in RIL v5, so I remove the check for the RILQUIRKS_V5_LEGACY in the end of RADIO_STATE_CHANGED handler.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #2)
> I found that we can also call SIM_GET_STATUS when radioState is
> SIM_NOT_READY in RIL v5, so I remove the check for the RILQUIRKS_V5_LEGACY
> in the end of RADIO_STATE_CHANGED handler.

Oh, ok! Good fix.

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> (In reply to Yoshi Huang[:yoshi] from comment #2)
> > I found that we can also call SIM_GET_STATUS when radioState is
> > SIM_NOT_READY in RIL v5, so I remove the check for the RILQUIRKS_V5_LEGACY
> > in the end of RADIO_STATE_CHANGED handler.
> 
> Oh, ok! Good fix.

Actually, I wonder why the fix works. Because we should still bail out because of

  if (this.radioState == newState) {
    return;
  }

It seems we may have to relax that early return when RILQUIRKS_V5_LEGACY is enabled. Or am I missing something?

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 611342 [details] [diff] [review]
patch to RadioStateChange

Clearing review flag for now until question in comment 4 is resolved.

Comment 6

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

Hi Philikon:
   Sorry for not explaining this fix.
   I read your original fix for Bug 728886, I wondered how it works for RIL V6 and why you modify this handler, because I check ril.h v6, in RIL_UNSOL_RESPONSE_RADIO_STATE_CHANGED, there seems no difference between v5 and v6.

So I check the code for ril v6 on Android, (reference-ril). I found out RIL_REQUEST_RADIO_POWER and RIL_REQUEST_GET_SIM_STATUS can be invoked in RADIO_STATE_OFF, other commands will return RIL_E_RADIO_NOT_AVAILABLE. So I guess there's the reason you add the RILQUIRKS_V5_LEGACY check in the end, because in older ril_worker.js (ril v5), RIL_REQUEST_GET_SIM_STATUS is invoked in RIL_STATE_SIM_READY.
   But I check the ril v5 code in gonk and ril v4 code before android gingerbread, all versions of RIL_REQUEST_GET_SIM_STATUS can be invoked when RADIO_STATE_OFF, so I think the final check RILQUIRKS_V5_LEGACY is unnecessary, therefore my fix is just to remove that check.
   For your question, yes, when radio event is RADIO_STATE_SIM_READY, it still bail out in the "this.radioState == newState" check, but actually the getICCStatus is already invoked when RADIO_STATE_OFF event comes in, it works as the same way as in RIL v6.

thanks

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #6)
>    I read your original fix for Bug 728886, I wondered how it works for RIL
> V6 and why you modify this handler, because I check ril.h v6, in
> RIL_UNSOL_RESPONSE_RADIO_STATE_CHANGED, there seems no difference between v5
> and v6.

That is not entirely correct. Theh RADIO_STATE_* constants changed and the circumstances in which RIL_UNSOL_RESPONSE_RADIO_STATE_CHANGED is notified also change. In RIL v6, we only have:

  const RADIO_STATE_OFF = 0;
  const RADIO_STATE_UNAVAILABLE = 1;
  const RADIO_STATE_ON = 2;

where as in previous versions, there was

  const RADIO_STATE_SIM_NOT_READY = 2;

etc. My code tried to accomodate for this discrepancy.

>    But I check the ril v5 code in gonk and ril v4 code before android
> gingerbread, all versions of RIL_REQUEST_GET_SIM_STATUS can be invoked when
> RADIO_STATE_OFF, so I think the final check RILQUIRKS_V5_LEGACY is
> unnecessary, therefore my fix is just to remove that check.

Right.

>    For your question, yes, when radio event is RADIO_STATE_SIM_READY, it
> still bail out in the "this.radioState == newState" check, but actually the
> getICCStatus is already invoked when RADIO_STATE_OFF event comes in, it
> works as the same way as in RIL v6.

Sure, but can we read the SIM status reliably when the radio is turned off? IIRC on the Akami, the radio has to be in RADIO_STATE_ON before we can read the ICC status, which is why I had this check in place. Can you verify this?

Comment 8

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

(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> IIRC on the Akami, the radio has to be in RADIO_STATE_ON before we can read
> the ICC status, which is why I had this check in place. Can you verify this?
Sure, I will try to find if I got an Akami here.

But if Akami can only get ICC status when Radio STATE ON, I guess currently on Akami the function getICCStatus doesn't got called as well, like galaxy s2 now.

thanks

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #8)
> Sure, I will try to find if I got an Akami here.

Thanks!

> But if Akami can only get ICC status when Radio STATE ON, I guess currently
> on Akami the function getICCStatus doesn't got called as well, like galaxy
> s2 now.

Yes, because of the `this.radioState == newState` check, right? That's why I suggested that check was wrong.

Comment 10

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

Hi, Philikon
   Sorry I found out I mistype the event name in comment #6

"For your question, yes, when radio event is RADIO_STATE_SIM_READY, it still bail out in the "this.radioState == newState" check, but actually the getICCStatus is already invoked when *RADIO_STATE_SIM_NOT_READY* event comes in."

I put more detail on it.

On Akami, RADIO_STATE_UNAVAILABLE will come first, so on Akami it will bail out in this.radioState == radioState. Galaxy S2 doesn't get RADIO_STATE_UNAVAILABLE event.

Then RADIO_STATE_OFF(both Akami and Galaxy S2), but it should bail out in radioState == RADIO_STATE_UNAVAILABLE || radioState == RADIO_STATE_OFF check.

Then RADIO_STATE_SIM_NOT_READY(both Akami and Galaxy S2), but for RIL v5, it will bail out in the final RILQUIRKS_V5_LEGACY && radioState == RADIO_STATE_SIM_NOT_READY, but for RIL v6, it should bypass this "if" test and call getICCStatus()

Then RADIO_STATE_SIM_READY(but only on Galaxy S2 now, I cannot get SIM_READY event on my akami), but it will bail out in this.radioState == newState check.

The Akami I got here is still RIL v5, but when it called getICCStatus when RADIO_STATE_OFF and RADIO_STATE_SIM_NOT_READY, I got "Subscription application is not present in iccStatus", still not sure about this, also RADIO_STATE_SIM_READY never comes in.

thanks

Comment 11

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

Hi philikon:
   Since my patch purposes that calling getICCStatus in RADIO_STATE_SIM_NOT_READY, is there still some concerns for you? 

thanks

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #11)
> Since my patch purposes that calling getICCStatus in
> RADIO_STATE_SIM_NOT_READY, is there still some concerns for you? 

I will do some tests on my Akami (which is RIL v6) and the Nexus S (which is on ICS) and report back!

Comment 13

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

(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> I will do some tests on my Akami (which is RIL v6) and the Nexus S (which is
> on ICS) and report back!
okay, if there is something I can help to test, please let me know~

great thanks~~

Comment 14

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

Test on the Akami from philikon (RIL v6 , qulacomm ril v2)
With applied patch in Bug 744306 first, 
because we need to set RILQUIRKS_V5_LEGACY to false

I describe the behavior below
1. radioState is RADIO_STATE_UNAVAILABLE 
   it bails out in this.radioState == newState check

2. radioState is RADIO_STATE_OFF
   it bails out in radioState == UNAVAILABLE || radioState == OFF check

3. radioState is 2 (for v5 it's SIM_READY, akami it's RADIO_ON)
   it bypass the RILQUIRKS_V5_LEGACY && SIM_READY check
   and call getICCStatus 

One thing I notice that is on this akami the UNSOLICITED_RESPONSE_SIM_STATUS_CHANGED will come, but on galaxy s2 this event doesn't come

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #14)
> Test on the Akami from philikon (RIL v6 , qulacomm ril v2)

Thanks! I also tested on the Nexus S where this patch works as well.

> One thing I notice that is on this akami the
> UNSOLICITED_RESPONSE_SIM_STATUS_CHANGED will come, but on galaxy s2 this
> event doesn't come

It also doesn't happen on the Nexus S.

Comment 16

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/018d0d6cd9b0

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/018d0d6cd9b0