952374 - [Fugu] data connectivity lost after left idle

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

Attachment: 20131219_fugu_data_lost.log

After left idle, data call (with cid '1') is somehow disconnected by remote/network. When this happens, gecko will retry to setup the data call and receives a new data call with cid '2'; this data call is ignored by gecko cause the cid is not recognized, leaving the phone with no connectivity.

We are seeing two problems here:

1. gecko does not clear cid when data call state changes to disconnected or when reconnecting.

2. modem should clean up disconnected data calls, or it will end up with no slots for data calls eventually.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: 20140107_fugu_data_lost_with_fix.log

I have tested overnight with the proposed patch, however I found that when data call was lost and RIL retried to setup the data call, the data call's cid returned was still 1, and not incrementing like in the previous log.

Have sprd fixed this or it happens randomly? Thank you.

Comment 3

[sam.hua]

pls look 943180

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to sam.hua from comment #3)
> pls look 943180

Thank you. I see that you have found the issue.
So did you fix it in libril_sp.so to avoid this issue? From attachment 8356939 [details], it seems that cid is the same when reconnecting.

Comment 5

[sam.hua]

The root cause of this issue should be in gecko.

1 we have pdp[3] to save active pdp link in RILC for one sim card. 

2 when ril_work send setupdatacall to rilc, we find one available pdp index and set the cid = index+1

3 when ril_work send deactivateDataConnection with cid parameters, RILC will clear the pdp[index] with the cid.

4 when detach the GRPS,we will clear all pdp[i]

so, when gecko reconnect the pdp, it should send deactivateDataConnection first and then setupdatacall again.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to sam.hua from comment #5)
> The root cause of this issue should be in gecko.
> 
> 1 we have pdp[3] to save active pdp link in RILC for one sim card. 
> 
> 2 when ril_work send setupdatacall to rilc, we find one available pdp index
> and set the cid = index+1
> 
> 3 when ril_work send deactivateDataConnection with cid parameters, RILC will
> clear the pdp[index] with the cid.
> 
> 4 when detach the GRPS,we will clear all pdp[i]
> 
> so, when gecko reconnect the pdp, it should send deactivateDataConnection
> first and then setupdatacall again.

We would clear the cid when we found that the data call is disconnected, but I don't think we should deactivate a 'already disconnected' data call. See the latest code in Android: onDataStateChanged() in DcController.java [1], it will only cleanup those disconnected data calls with permanent fail cause. You will also meet this issue when you upgrade to newer version of Android.

Back to my question in Comment 4: I can't reproduce the original issue reported here, from attachment 8356939 [details], it seems that 'cid is the same' when reconnecting. Did sprd change anything?

Thanks.

[1] https://android.googlesource.com/platform/frameworks/opt/telephony/+/master/src/java/com/android/internal/telephony/dataconnection/DcController.java

Comment 7

[sam.hua]

Hi jessica,

Before RILC sending onDataCallListChanged to ril_worker, RILC will check the pdp link is deactived by gecko or not.
if it isn't,RILC will clear the cid, and following logs will be logged.
    RILLOGD("put pdp[%d]", cid);
    RILLOGD("pdp[0].state = %d, pdp[1].state = %d,pdp[2].state = %d", pdp[0].state, pdp[1].state, pdp[2].state);

it is added into RILC in Nov26,2013.

Comment 8

[sam.hua]

AT< ^CEND is the at command to report the changes of data calls

Comment 9

[Jessica Jong [:jessica] (inactive)]

Thank you sam, it seems that my second test includes the sprd fix mentioned in Comment 7.

Hsinyi, IMHO, I think we should still land the proposed patch in spite of the sprd's fix. What do you think?

Comment 10

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #9)
> Thank you sam, it seems that my second test includes the sprd fix mentioned
> in Comment 7.
> 
> Hsinyi, IMHO, I think we should still land the proposed patch in spite of
> the sprd's fix. What do you think?

Yes, as our previous offline discussion, we should have cid in ril_worker being cleared when data is disconnected no matter how modem treats their cid assignment.

Comment 11

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

Comment on attachment 8356937 [details] [diff] [review]
proposed patch.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3972,5 @@
> +    if (this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED ||
> +        this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) {
> +      this.cid = null;
> +      this.connectedTypes = [];
> +      if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) {

Why don't we need to unregisterNetworkInterface when state == DISCONNECTED? Why not supposed to be:

if (this.registeredAsNetworkInterface) {
  gNetworkManager.unregisterNetworkInterface(this);
}

Comment 12

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #11)
> Comment on attachment 8356937 [details] [diff] [review]
> proposed patch.
> 
> Review of attachment 8356937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +3972,5 @@
> > +    if (this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED ||
> > +        this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) {
> > +      this.cid = null;
> > +      this.connectedTypes = [];
> > +      if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) {
> 
> Why don't we need to unregisterNetworkInterface when state == DISCONNECTED?
> Why not supposed to be:
> 
> if (this.registeredAsNetworkInterface) {
>   gNetworkManager.unregisterNetworkInterface(this);
> }

Oh, it should be:

  if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN &&
      this.registeredAsNetworkInterface) {
    gNetworkManager.unregisterNetworkInterface(this);
    this.registeredAsNetworkInterface = false;
  }

I will correct it in the next patch.

In our current design, state becomes DISCONNECTED when data call is disconnected unexpectedly, so it should be reconnected pretty soon, therefore I was thinking not to unregister network interface in this case. But on second thought, the reconnection might fail, so maybe we should unregister on DISCONNECT? What do you think?

Comment 13

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #12)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #11)
> > Comment on attachment 8356937 [details] [diff] [review]
> > proposed patch.
> > 
> > Review of attachment 8356937 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +3972,5 @@
> > > +    if (this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED ||
> > > +        this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) {
> > > +      this.cid = null;
> > > +      this.connectedTypes = [];
> > > +      if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) {
> > 
> > Why don't we need to unregisterNetworkInterface when state == DISCONNECTED?
> > Why not supposed to be:
> > 
> > if (this.registeredAsNetworkInterface) {
> >   gNetworkManager.unregisterNetworkInterface(this);
> > }
> 
> Oh, it should be:
> 
>   if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN &&
>       this.registeredAsNetworkInterface) {
>     gNetworkManager.unregisterNetworkInterface(this);
>     this.registeredAsNetworkInterface = false;
>   }
> 
> I will correct it in the next patch.
> 
> In our current design, state becomes DISCONNECTED when data call is
> disconnected unexpectedly, so it should be reconnected pretty soon,
> therefore I was thinking not to unregister network interface in this case.
> But on second thought, the reconnection might fail, so maybe we should
> unregister on DISCONNECT? What do you think?

Yeah, I'd prefer to unregister on DISCONNECT and register it when necessary on CONNECTED.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8356937 [details] [diff] [review]
proposed patch.

Thank you, Hsinyi.
I will submit another patch addressing comment 11 and 13.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v2.

Address review comment 11 and 13.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8361595 [details] [diff] [review]
proposed patch, v2.

I have moved 'this.radioInterface.updateRILNetworkInterface();' to the bottom, cause it may trigger 'setupDataCallByType()' which then calls 'connect()', where apntype is added to 'connectedTypes'. This will cause problems if we clear 'connectedTypes' afterwards.

Comment 17

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

Comment on attachment 8361595 [details] [diff] [review]
proposed patch, v2.

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

Great!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4110,5 @@
> +                                 kNetworkInterfaceStateChangedTopic,
> +                                 null);
> +
> +    if ((this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN ||
> +        this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED) &&

nit: align the two lines "this.state == ..."

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch (final)

Address review comment 17.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=a8a207ed4474
Green! \o/

Comment 20

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

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc35ec07c7c9