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.

(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.

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.

(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

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.

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.

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

(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?

(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.

Not yet tested. Just showing the picture after fix.

blocking release 1.3 hence 1.3+

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 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 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 on attachment 8376019 [details] [diff] [review] #3 Wait card state initialization Review of attachment 8376019 [details] [diff] [review]: ----------------------------------------------------------------- Ya~ thank you.

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

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:

[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

This needs rebasing for b2g28.

Hi Ryan, This is the patch for b2g28. Thank you.

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

(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?

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

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

it's the patch i applied

(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.

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

(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.

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

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

the log

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

(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.

(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.

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

(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.

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?

(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;"

Hi Sam, is it working for you now?

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

(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?

Ok, i will remove all sprd patches to check it. my build. gecko:commit 983294139088ebd3aab14dfeb2a7fcf47f242009 gaia:commit 9d19e6d65c4877461d19a2cf18db1d01232194d6

(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

yes,it is ok now when remove all sprd patches

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?

(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?

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.

(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!!!

I'm wondering if this is not causing bug 976497