[Fugu] data connectivity lost after left idle

RESOLVED FIXED in 1.3 C3/1.4 S3(31jan)

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

unspecified
1.3 C3/1.4 S3(31jan)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 8350448 [details]
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8356937 [details] [diff] [review]
proposed patch.
(Assignee)

Comment 2

5 years ago
Created attachment 8356939 [details]
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.
Flags: needinfo?(sam.hua)

Comment 3

5 years ago
pls look 943180
Flags: needinfo?(sam.hua)
(Assignee)

Comment 4

5 years ago
(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.
Flags: needinfo?(sam.hua)

Comment 5

5 years ago
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.
Flags: needinfo?(sam.hua)
(Assignee)

Comment 6

5 years ago
(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

5 years ago
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

5 years ago
AT< ^CEND is the at command to report the changes of data calls
(Assignee)

Comment 9

5 years ago
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?
Flags: needinfo?(htsai)
(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.
Flags: needinfo?(htsai)
(Assignee)

Updated

5 years ago
Attachment #8356937 - Flags: review?(htsai)
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);
}
(Assignee)

Comment 12

5 years ago
(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?
(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.
(Assignee)

Comment 14

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

Thank you, Hsinyi.
I will submit another patch addressing comment 11 and 13.
Attachment #8356937 - Flags: review?(htsai)
(Assignee)

Comment 15

5 years ago
Created attachment 8361595 [details] [diff] [review]
proposed patch, v2.

Address review comment 11 and 13.
Attachment #8356937 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
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.
Attachment #8361595 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Assignee: nobody → jjong
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 == ..."
Attachment #8361595 - Flags: review?(htsai) → review+
(Assignee)

Comment 18

5 years ago
Created attachment 8364134 [details] [diff] [review]
proposed patch (final)

Address review comment 17.
Attachment #8361595 - Attachment is obsolete: true
Attachment #8364134 - Flags: review+
(Assignee)

Comment 19

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=a8a207ed4474
Green! \o/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cc35ec07c7c9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)

Updated

5 years ago
Blocks: 905568
You need to log in before you can comment on or make changes to this bug.