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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

+++ 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]

Comment 1

[Jessica Jong [:jessica] (inactive)]

I'll take care of this.

Comment 2

[Jessica Jong [:jessica] (inactive)]

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

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 4

[Jessica Jong [:jessica] (inactive)]

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

Comment 5

[Jessica Jong [:jessica] (inactive)]

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

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add setMtu() support in NetworkService, v1.

Edgar, may I have your review on this? Thanks.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: set mtu for connected network interfaces. r=echen

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: read data call's mtu from network/apn settings, v1.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: test cases for network interface's mtu, v1.

Comment 10

[Edgar Chen [:edgar]]

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.

Comment 11

[Edgar Chen [:edgar]]

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.

Comment 12

[Jessica Jong [:jessica] (inactive)]

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

Comment 13

[Jessica Jong [:jessica] (inactive)]

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

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add setMtu() support in NetworkService. r=echen,smaug

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.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: read data call's mtu from network/apn settings. r=echen

Address review comment 10:
- have consistent comments/define with the mtu in nsINetworkInterface where -1 means not set/invalid

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: test cases for network interface's mtu. r=echen

Address review comment 11:
- return network info directly in setDataEnabledAndWait() and setupDataCallAndWait() and adjust comments

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8664131 [details] [diff] [review]
Part 1: add setMtu() support in NetworkService. r=echen,smaug

r+ for the .webidl

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: test cases for network interface's mtu. r=echen

Use the compact version for return in arrow function.

Comment 19

[Jessica Jong [:jessica] (inactive)]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a045d550a0cb&group_state=expanded&exclusion_profile=false

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/88018925cf28
https://hg.mozilla.org/integration/b2g-inbound/rev/e3de9e349f1d
https://hg.mozilla.org/integration/b2g-inbound/rev/6367ac9f290c
https://hg.mozilla.org/integration/b2g-inbound/rev/ba660084227d

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88018925cf28
https://hg.mozilla.org/mozilla-central/rev/e3de9e349f1d
https://hg.mozilla.org/mozilla-central/rev/6367ac9f290c
https://hg.mozilla.org/mozilla-central/rev/ba660084227d