Closed Bug 1174998 Opened 6 years ago Closed 5 years ago

Configure MTU from apn settings or the one reported from modem.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, firefox44 fixed)

RESOLVED FIXED
FxOS-S8 (02Oct)
tracking-b2g backlog
Tracking Status
firefox44 --- fixed

People

(Reporter: edgar, Assigned: jessica)

References

Details

Attachments

(4 files, 4 obsolete files)

+++ This bug was initially created as a follow-up of Bug #1171283 +++

MTU configuration is important for the network, specially in IPv6 [1].
Now we didn't do any special handling for MTU, in some ipv6 network, we may get 'Packet Too Big" error from ICMPv6. To adopt correct MTU for a network, we can use MTU information from,
1. Apn setting: some apn settings includes mtu information [2].
2. Modem: mtu will be provided in RIL_Data_Call_Response, but it is only available since RIL version 11 [3].
3. Or use the IPv6 default minimum MTU (1280).

[1] http://www.tcpipguide.com/free/t_ICMPv6PacketTooBigMessages.htm
[2] https://github.com/mozilla-b2g/gaia/blob/7631a7d17093e5bdf4968edd032e848ee75d48fb/shared/resources/apn.json#L1244
[3] https://github.com/android/platform_hardware_ril/commit/b44dda3ea5bbbadb910479019f967b52cf9d69c7
[3]
Blocks: 1183002
I'll take care of this.
Assignee: nobody → jjong
If we'd like to read mtu from data call response, we'll need to read pcscf [1] available in ril version 10 first. I'd prefer to do it in another bug though.

[1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L380
[Tracking Requested - why for this release]:
(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> If we'd like to read mtu from data call response, we'll need to read pcscf
> [1] available in ril version 10 first. I'd prefer to do it in another bug
> though.
> 
> [1]
> https://github.com/android/platform_hardware_ril/blob/master/include/
> telephony/ril.h#L380

Filed bug 1185406 for reading pcscf from data call response.
netd set/get mtu command is supported only since Android version 4.4 (Kitkat) [1], so we'll need to skip this in older versions, like in current emulator (ics).

[1] https://github.com/mozilla-b2g/platform_system_netd/commit/6d6c0e6f1164e3182538cb48c2b95d90a2eb780c
Edgar, may I have your review on this? Thanks.
Attachment #8660611 - Flags: review?(echen)
Attachment #8660615 - Attachment description: Part 3: read data call's mtu from network/apn settings. → Part 3: read data call's mtu from network/apn settings, v1.
See Also: → 1200937
Attachment #8660611 - Flags: review?(echen) → review+
Attachment #8660612 - Flags: review?(echen) → review+
Comment on attachment 8660615 [details] [diff] [review]
Part 3: read data call's mtu from network/apn settings, v1.

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

::: dom/system/gonk/nsIDataCallInterfaceService.idl
@@ +62,5 @@
>     */
>    readonly attribute DOMString pcscf;
> +
> +  /**
> +   * MTU received from network. Value <= 0 means network has either not sent a

It will be great if we could have consistent comments/define with the mtu in nsINetworkInterface where -1 means invalid.
Attachment #8660615 - Flags: review?(echen) → review+
Comment on attachment 8660617 [details] [diff] [review]
Part 4: test cases for network interface's mtu, v1.

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

Thank you.

::: dom/system/gonk/tests/marionette/head.js
@@ +215,4 @@
>      }));
>    promises.push(setSettings(SETTINGS_KEY_DATA_ENABLED, aEnabled));
>  
>    return Promise.all(promises);

Nit: What about retrieve what we need here, e.g. `return Promise.all(promises).then(aValues => aValues[0]);`? And please also update the comments.

@@ +246,2 @@
>      }));
>    promises.push(radioInterface.setupDataCallByType(aNetworkType));

Ditto.
Attachment #8660617 - Flags: review?(echen) → review+
Thank you, Edgar.

(In reply to Edgar Chen [:edgar][:echen] from comment #10)
> Comment on attachment 8660615 [details] [diff] [review]
> Part 3: read data call's mtu from network/apn settings, v1.
> 
> Review of attachment 8660615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIDataCallInterfaceService.idl
> @@ +62,5 @@
> >     */
> >    readonly attribute DOMString pcscf;
> > +
> > +  /**
> > +   * MTU received from network. Value <= 0 means network has either not sent a
> 
> It will be great if we could have consistent comments/define with the mtu in
> nsINetworkInterface where -1 means invalid.

Actually, this comes from [1], but since the negative value is not really meaningful, we can just set it to -1 if value is <= 0. Will update this on the next patch.

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/b2g-lollipop-mr1/include/telephony/ril.h#L416
(In reply to Edgar Chen [:edgar][:echen] from comment #11)
> Comment on attachment 8660617 [details] [diff] [review]
> Part 4: test cases for network interface's mtu, v1.
> 
> Review of attachment 8660617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you.
> 
> ::: dom/system/gonk/tests/marionette/head.js
> @@ +215,4 @@
> >      }));
> >    promises.push(setSettings(SETTINGS_KEY_DATA_ENABLED, aEnabled));
> >  
> >    return Promise.all(promises);
> 
> Nit: What about retrieve what we need here, e.g. `return
> Promise.all(promises).then(aValues => aValues[0]);`? And please also update
> the comments.

Sounds good to me, will update this on the upcoming patch. Thanks.

> 
> @@ +246,2 @@
> >      }));
> >    promises.push(radioInterface.setupDataCallByType(aNetworkType));
> 
> Ditto.
Hi Olli, we are adding a new attribute 'mtu' in NetworkOptions.webidl for setting Maximum Transmit Unit on a network interface. Would you mind reviewing the webidl part? Thanks.
Attachment #8660611 - Attachment is obsolete: true
Attachment #8664131 - Flags: review?(bugs)
Attachment #8660612 - Attachment description: Part 2: set mtu for connected network interfaces, v1. → Part 2: set mtu for connected network interfaces. r=echen
Address review comment 10:
- have consistent comments/define with the mtu in nsINetworkInterface where -1 means not set/invalid
Attachment #8660615 - Attachment is obsolete: true
Attachment #8664132 - Flags: review+
Address review comment 11:
- return network info directly in setDataEnabledAndWait() and setupDataCallAndWait() and adjust comments
Attachment #8660617 - Attachment is obsolete: true
Attachment #8664145 - Flags: review+
Comment on attachment 8664131 [details] [diff] [review]
Part 1: add setMtu() support in NetworkService. r=echen,smaug

r+ for the .webidl
Attachment #8664131 - Flags: review?(bugs) → review+
Use the compact version for return in arrow function.
Attachment #8664145 - Attachment is obsolete: true
Attachment #8664654 - Flags: review+
Attachment #8664131 - Attachment description: Part 1: add setMtu() support in NetworkService, v2. → Part 1: add setMtu() support in NetworkService. r=echen,smaug
You need to log in before you can comment on or make changes to this bug.