963054 - [fugu][DSDS] follow-up for radio control: radio of slot 2 is not on even there's a sim card

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This bug was originally reported on https://bugzilla.mozilla.org/show_bug.cgi?id=943215#c55

My observation:
In _isRadioAbleToEnableAtClient(), we call this._isCardPresentAtClient(clientId) to see if a card is present.  _isCardPresentAtClient() returns true if |cardState !== RIL.GECKO_CARDSTATE_UNDETECTED &&  cardState !== RIL.GECKO_CARDSTATE_UNKNOWN|. 

However, this issue happens when gaia calls setRadioEnabled() before gecko receives the cardstate change event for slot2. So the cardstate cached on gecko/RadioInterface remains _UNKNOWN at that moment.

Since the logic of radio control for fugu depends on cardState determination, seems we need to make sure that gecko receives cardstate message from modem/rild before we do more radio control work.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> This bug was originally reported on
> https://bugzilla.mozilla.org/show_bug.cgi?id=943215#c55
> 
> My observation:
> In _isRadioAbleToEnableAtClient(), we call
> this._isCardPresentAtClient(clientId) to see if a card is present. 
> _isCardPresentAtClient() returns true if |cardState !==
> RIL.GECKO_CARDSTATE_UNDETECTED &&  cardState !==
> RIL.GECKO_CARDSTATE_UNKNOWN|. 
> 
> However, this issue happens when gaia calls setRadioEnabled() before gecko
> receives the cardstate change event for slot2. So the cardstate cached on
> gecko/RadioInterface remains _UNKNOWN at that moment.
> 
> Since the logic of radio control for fugu depends on cardState
> determination, seems we need to make sure that gecko receives cardstate
> message from modem/rild before we do more radio control work.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

After one night... I couldn't see this issue even with the same device, same build... but we believe this is indeed a bug needed fixing.

Hi Sam,
if you have a concrete STR, please share. That helps us verify our solution. Thank you.

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> This bug was originally reported on
> https://bugzilla.mozilla.org/show_bug.cgi?id=943215#c55
> 
> My observation:
> In _isRadioAbleToEnableAtClient(), we call
> this._isCardPresentAtClient(clientId) to see if a card is present. 
> _isCardPresentAtClient() returns true if |cardState !==
> RIL.GECKO_CARDSTATE_UNDETECTED &&  cardState !==
> RIL.GECKO_CARDSTATE_UNKNOWN|. 
> 
> However, this issue happens when gaia calls setRadioEnabled() before gecko
> receives the cardstate change event for slot2. So the cardstate cached on
> gecko/RadioInterface remains _UNKNOWN at that moment.
> 
> Since the logic of radio control for fugu depends on cardState
> determination, seems we need to make sure that gecko receives cardstate
> message from modem/rild before we do more radio control work.

Yes, this is a risk. We should wait until received the first cardstatechange

Comment 4

[sam.hua]

Attachment: logs.zip

Comment 5

[sam.hua]

Sorry,i can't reproduce it in the newest build.
i upload some logs when i reported the issue.I wish them could help it.

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Sam had reproduced this issue. The log shows that we encounter the issue described in Comment 3. We should fix it. The radio control should be only applied after received first update of cardstate.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

nominate because it's a flaw in DSDS radio control.

Comment 8

[Andrew Overholt [:overholt]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #7)
> nominate because it's a flaw in DSDS radio control.

Could you please elaborate on what the user impact is here, Hsin-Yi?  Will the radio control of the second SIM remain in an unknown state and not be able to recover?

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Andrew Overholt [:overholt] from comment #8)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #7)
> > nominate because it's a flaw in DSDS radio control.
> 
> Could you please elaborate on what the user impact is here, Hsin-Yi?  Will
> the radio control of the second SIM remain in an unknown state and not be
> able to recover?

The radio of the second SIM remains in 'disabled' however gaia might think it as 'enabled.' So yeah, in that situation, the radio is unable to recover.

This is a timing issue so not reproducible every time.

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (WIP) Wait card state initialization

Not yet tested. Just showing the picture after fix.

Comment 11

[Preeti Raghunath(:Preeti)]

blocking release 1.3 hence 1.3+

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Wait card state initialization

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8374675 [details] [diff] [review]
Wait card state initialization

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +513,5 @@
>      pendingMessages: [],  // For queueing "RIL:SetRadioEnabled" messages.
>      timer: null,
>      request: null,
>      deactivatingDeferred: {},
> +    cardStateInit: {},

Name it '_initializedCardState' // For queueing card states which have been initialized.

@@ +514,5 @@
>      timer: null,
>      request: null,
>      deactivatingDeferred: {},
> +    cardStateInit: {},
> +    cardStateAllInit: false,

Name it '_allCardStateInitialied'

_ prefix indicates it's a private member.

@@ +522,5 @@
>        this.ril = ril;
>      },
>  
> +    receiveCardState: function(clientId) {
> +      if (!RILQUIRKS_RADIO_OFF_WO_CARD || this.cardStateAllInit) {

Let's split the conditions to two if-clauses because they obviously have distinct meanings. I'd see we have something as below:

if (!RILQUIRKS_RADIO_OFF_WO_CARD) {
  // Don't care card states at all.
  return;
}

if (this.cardStateAllInit) {
  return;
}

@@ +554,5 @@
>          deferred.resolve();
>        }
>      },
>  
> +    _tryStartRunning: function() {

How about _startProcessingPendingMessages() (a little bit long... :P) or _startProcessingPending?

@@ +555,5 @@
>        }
>      },
>  
> +    _tryStartRunning: function() {
> +      if (!this.running) {

s/this.running/this._pendingStarted/ ?

@@ +558,5 @@
> +    _tryStartRunning: function() {
> +      if (!this.running) {
> +        if (DEBUG) debug("RadioControl: start dequeue");
> +        this.running = true;
> +        this._processNextMessage();

Since it's _tryStartRunning setting this.running to true, isn't it clearer that it's he to set the flag back to false?

@@ +570,2 @@
>          return;
>        }

Let's have distinct if-clauses due to distinct implications.

if (this.pendingMessage.length === 0) {
  return;
}

if (RILQUIRKS_RADIO_OFF_WO_CARD && !this.cardStateAllInit) {
  // We care card states and the states are not ready yet.
  return;
}

@@ +583,5 @@
>        }
>        return numCards;
>      },
>  
> +    _isCardStateInited: function() {

Please remove this. The return value doesn't match the function name.

@@ +2188,5 @@
>          this.handleRadioStateChange(message);
>          break;
>        case "cardstatechange":
>          this.rilContext.cardState = message.cardState;
> +        gRadioEnabledController.receiveCardState(this.clientId);

The name looks better :)

::: dom/system/gonk/ril_consts.js
@@ +2431,5 @@
>  this.GECKO_DETAILED_RADIOSTATE_ENABLED    = "enabled";
>  this.GECKO_DETAILED_RADIOSTATE_DISABLING  = "disabling";
>  this.GECKO_DETAILED_RADIOSTATE_DISABLED   = "disabled";
>  
> +this.GECKO_CARDSTATE_NOT_INIT                      = "notInit";

GECKO_CARDSTATE_UNINITIALIZED = "uninitialized"

::: dom/system/gonk/ril_worker.js
@@ +2985,5 @@
>      this.aid = app.aid;
>      this.appType = app.app_type;
>      this.iccInfo.iccType = GECKO_CARD_TYPE[this.appType];
>      // Try to get iccId only when cardState left GECKO_CARDSTATE_UNDETECTED.
> +    if (this.cardState === GECKO_CARDSTATE_NOT_INIT &&

This looks problematic.

Think about a case:
After booting up when a card is present, the cardState changes from 'not_init' to 'present.' Then, turn on airplane mode that causes the cardState changes from 'present' to 'undetected.' After a while, turn off airplane mode, and cardState (reported from modem) should change to 'present' again.

But the line doesn't let us readICCID().

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: #2 Wait card state initialization

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8375365 [details] [diff] [review]
#2 Wait card state initialization

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +513,5 @@
> +  let _timer = null;
> +  let _request = null;
> +  let _deactivatingDeferred = {};
> +  let _initializedCardState = {};
> +  let _allCardStateInitialized = !RILQUIRKS_RADIO_OFF_WO_CARD;

Hide all data members. They cannot be accessed from outside of gRadioEnabledController.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8375365 [details] [diff] [review]
#2 Wait card state initialization

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +513,5 @@
> +  let _timer = null;
> +  let _request = null;
> +  let _deactivatingDeferred = {};
> +  let _initializedCardState = {};
> +  let _allCardStateInitialized = !RILQUIRKS_RADIO_OFF_WO_CARD;

That's truly nice. The code looks more chic. Thanks for making them consistent.

@@ +529,5 @@
> +      if (DEBUG) debug("RadioControl: receive cardState from " + clientId);
> +      _initializedCardState[clientId] = true;
> +      if (Object.keys(_initializedCardState).length == _ril.numRadioInterfaces) {
> +        _allCardStateInitialized = true;
> +        if (_pendingMessages.length !== 0) {

I don't think it's safe to _processNextMessage() without checking|!this.isDeactivatingDataCalls()| before .

Imagine a case: when there's already a message, that said to clientId_0, being processed, and _request would be set as 
  |(function() {
      radioInterface.receiveMessage(msg); //msg.clientId = 0;
   }).bind(this);| in _handleMessage().

Then 2nd message to clientId_1 is coming before _deactivateDataCalls() is resolved. Without the condition, _request will be overwritten and the original msg to clientId_0 will be skipped.

@@ +538,5 @@
> +
> +    receiveMessage: function(msg) {
> +      if (DEBUG) debug("RadioControl: receiveMessage: " + JSON.stringify(msg));
> +      _pendingMessages.push(msg);
> +      this._processNextMessage();

ditto.

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: #3 Wait card state initialization

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8376019 [details] [diff] [review]
#3 Wait card state initialization

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

Ya~ thank you.

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Wait card state initialization. r=hsinyi

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=bbda000139d1

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/85eba307aa43

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/85eba307aa43

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please nominate this patch for approval-mozilla-b2g28 as 1.3+ blocking status does not grant automatic approval for uplift anymore.

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8376082 [details] [diff] [review]
[final] Wait card state initialization. r=hsinyi

NOTE: This flag is now for security issues only. 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 #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 25

[Szu-Yu Chen [:aknow] (inactive)]

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  DSDS feature
User impact if declined:  Sometime radio control would not work after power on. The situation could not recover.
Testing completed:  Tested in local
Risk to taking this patch (and alternatives if risky):  Low
String or UUID changes made by this patch:  None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for b2g28.

Comment 27

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final][b2g28] Wait card state initialization. r=hsinyi

Hi Ryan,

This is the patch for b2g28. Thank you.

Comment 28

[sam.hua]

Hi szu,
do u try the case of SIM2 only?

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #28)
> Hi szu,
> do u try the case of SIM2 only?
Hi Sam,
I've verified the patch with the case of SIM2 only. And it worked well.
Does your problem still stay?

Comment 30

[sam.hua]

yes,i will clean the /out and re-build it again.

Comment 31

[sam.hua]

yes. it turn on the SIM1 radio
D/RILC    (  104): [w] [0015]> RADIO_POWER (1)
D/RIL     (  104): [w] onRequest: RADIO_POWER sState=0
D/RIL     (  104): [w] channel1 state: '0'                        SIM1
D/RIL     (  104): [w] get Channel ID '1'
D/AT      (  104): [w] Channel1: AT> AT+SAUTOATT=0
D/AT      (  104): [w] Channel1: AT< OK
D/AT      (  104): [w] Channel1: AT> AT+SFUN=4

Comment 32

[sam.hua]

Attachment: bug256705_gecko_singlecard.patch

it's the patch i applied

Comment 33

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #32)
> Created attachment 8378131 [details] [diff] [review]
> bug256705_gecko_singlecard.patch
> 
> it's the patch i applied

Sam, did you add back the quirk when you tested this?
Without that quirk, radio_1 will be turned on no matter there's a card or not.

Comment 34

[sam.hua]

add ro.moz.ril.radio_off_wo_card=true in base.mk ?
yes,i did.

Comment 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #34)
> add ro.moz.ril.radio_off_wo_card=true in base.mk ?
> yes,i did.

Sam,
if you still encounter the problem, please attach a log with gecko/ril debugger enabled. Thank you.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/d14ec88b579b

Comment 37

[sam.hua]

Hi hsin,
could u give me the patch for V1.3t?

Comment 38

[sam.hua]

Attachment: sim4_error.txt

the log

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: log -- v1.3 works well

gecko: mozillaorg/v1.3, 7c32fd60a04a2f7e47aef152de6729401a9d4b1e
gaia: mozillaorg/v1.3, a43904d9646109b48836d62f7aa51e499fbf4b2e

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #38)
> Created attachment 8378930 [details]
> sim4_error.txt
> 
> the log

Hi Sam, something went wrong in your log. However, I tested with the latest code again and everything works as expected. I attached the log attachment 8378953 [details]. From that you could see Gecko sends REQUEST_RADIO_POWER (outgoing parcel of type 23) to RILC 3 times, requesting turning off 2 times and requesting turning SIM2 on 1 time. The behaviour is exactly what we expect.

I am still wondering if your base image contains the quirk. Could you please check again, see if there's "ro.moz.ril.radio_off_wo_card=true" when you type "adb shell system/build.prop". Also, please share the exact gecko and gaia version (branch and commit number) you are testing with so that we could make sure we are on the same page.

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #37)
> Hi hsin,
> could u give me the patch for V1.3t?

The patch has already been in v1.3t.

Comment 42

[sam.hua]

Hi hsin,

do u use ./config.sh fugu or sp7710gaplus_gonk4.0? and do u applied sprd patches?

Comment 43

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #42)
> Hi hsin,
> 
> do u use ./config.sh fugu or sp7710gaplus_gonk4.0? and do u applied sprd
> patches?

Dear Sam,

I use the repo "https://github.com/sprd-ffos/b2g-manifest" and configure with "sp7710gaplus_gonk4.0" to get the base image. However, I need to manually add "ro.moz.ril.radio_off_wo_card=true" back to test this patch.

Then, I flashed the latest v1.3 gecko and gaia.

Comment 44

[sam.hua]

i just change the RadioInterfaceLayer.js to

let RILQUIRKS_RADIO_OFF_WO_CARD =1;
//  libcutils.property_get("ro.moz.ril.radio_off_wo_card", "false") == "";

could i use it to test?

Comment 45

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #44)
> i just change the RadioInterfaceLayer.js to
> 
> let RILQUIRKS_RADIO_OFF_WO_CARD =1;
> //  libcutils.property_get("ro.moz.ril.radio_off_wo_card", "false") == "";
> 
> could i use it to test?

Better use "let RILQUIRKS_RADIO_OFF_WO_CARD = true;"

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Sam, is it working for you now?

Comment 47

[sam.hua]

no.it can't find network when sim2 only.

and I follow u:
I use the repo "https://github.com/sprd-ffos/b2g-manifest" and configure with "sp7710gaplus_gonk4.0" to get the base image. 
but i change let RILQUIRKS_RADIO_OFF_WO_CARD = true; in RadioInterfaceLayer.js

Comment 48

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #47)
> no.it can't find network when sim2 only.
> 
> and I follow u:
> I use the repo "https://github.com/sprd-ffos/b2g-manifest" and configure
> with "sp7710gaplus_gonk4.0" to get the base image. 
> but i change let RILQUIRKS_RADIO_OFF_WO_CARD = true; in
> RadioInterfaceLayer.js

Which gecko and gaia version are you using? Do you apply other sprd patches?

Comment 49

[sam.hua]

Ok, i will remove all sprd patches to check it.

my build.
gecko:commit 983294139088ebd3aab14dfeb2a7fcf47f242009
gaia:commit 9d19e6d65c4877461d19a2cf18db1d01232194d6

Comment 50

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #49)
> Ok, i will remove all sprd patches to check it.
> 

That would be great. Please do so that we could get a clear and narrower scope of the issue.

> my build.
> gecko:commit 983294139088ebd3aab14dfeb2a7fcf47f242009
> gaia:commit 9d19e6d65c4877461d19a2cf18db1d01232194d6

Comment 51

[sam.hua]

yes,it is ok now when remove all sprd patches

Comment 52

[sam.hua]

Hi Hsin,

1. SIM2 only.
2. Turn on airplane.
3. reboot
4. Turn off airplane mode

We should only send RADIO_POWRE(1) to SIM2, but i find 
D/AT      (  102): [w] Channel4: AT> AT+SAUTOATT=0
D/AT      (  101): [w] Channel1: AT< OK
D/AT      (  101): [w] Channel1: AT> AT+SFUN=4
D/AT      (  102): [w] Channel4: AT< OK
D/AT      (  102): [w] Channel4: AT> AT+SFUN=4
D/AT      (  101): [w] Channel1: AT< OK
and our modem won't work well now.
but it is ok if it doesn't reboot and just turn on/off airplane.

will u add some control for it?

Comment 53

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #51)
> yes,it is ok now when remove all sprd patches

Glad to hear that :)

(In reply to sam.hua from comment #52)
> Hi Hsin,
> 
> 1. SIM2 only.
> 2. Turn on airplane.
> 3. reboot
> 4. Turn off airplane mode
> 
> We should only send RADIO_POWRE(1) to SIM2, but i find 
> D/AT      (  102): [w] Channel4: AT> AT+SAUTOATT=0
> D/AT      (  101): [w] Channel1: AT< OK
> D/AT      (  101): [w] Channel1: AT> AT+SFUN=4
> D/AT      (  102): [w] Channel4: AT< OK
> D/AT      (  102): [w] Channel4: AT> AT+SFUN=4
> D/AT      (  101): [w] Channel1: AT< OK
> and our modem won't work well now.
> but it is ok if it doesn't reboot and just turn on/off airplane.
> 
> will u add some control for it?

We just checked our device. And we don't see this problem... 
According to your comment 51, I believe the issue originally reported has been resolved. How about we file another bug for the issue on comment 52? And please attach a log with gecko/ril debugger enabled, "adb logcat -b radio -b main", there.

Also, is it fine to add the quirk back to Bug 959920 now?

Comment 54

[sam.hua]

do u open the airplane mode and reboot? it's our modem issue, it can't work well if RIL_WORKER ask modem to open two radio in the case of one SIM card only.

i have added it.

Comment 55

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to sam.hua from comment #54)
> do u open the airplane mode and reboot? it's our modem issue, it can't work
> well if RIL_WORKER ask modem to open two radio in the case of one SIM card
> only.
> 

Yes. We followed your steps on comment 52, aireplane mode on first, then reboot.

> i have added it.

Thank you very much!!!

Comment 56

[:gerard-majax]

I'm wondering if this is not causing bug 976497