From: Aleksander Morgado <hidden> Date: 2021-07-31 22:50:58
Hey Subash,
I'm playing with the whole QMAP data aggregation setup with a USB
connected Fibocom FM150-AE module (SDX55).
See https://gitlab.freedesktop.org/mobile-broadband/libqmi/-/issues/71
for some details on how I tested all this.
This module reports a "Downlink Data Aggregation Max Size" of 32768
via the "QMI WDA Get Data Format" request/response, and therefore I
configured the MTU of the master wwan0 interface with that same value
(while in 802.3 mode, before switching to raw-ip and enabling
qmap-pass-through in qmi_wwan).
When attempting to create a new link using netlink, the operation
fails with -EINVAL, and following the code path in the kernel driver,
it looks like there is a check in rmnet_vnd_change_mtu() where the
master interface MTU is checked against the RMNET_MAX_PACKET_SIZE
value, defined as 16384.
If I setup the master interface with MTU 16384 before creating the
links with netlink, there's no error reported anywhere. The FM150
module crashes as soon as I connect it with data aggregation enabled,
but that's a different story...
Is this limitation imposed by the RMNET_MAX_PACKET_SIZE value still a
valid one in this case? Should changing the max packet size to 32768
be a reasonable approach? Am I doing something wrong? :)
This previous discussion for the qmi_wwan add_mux/del_mux case is
relevant: https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/..
The suggested patch was not included yet in the qmi_wwan driver and
therefore the user still needs to manually configure the MTU of the
master interface before setting up all the links, but at least there
seems to be no maximum hardcoded limit.
Cheers!
--
Aleksander
https://aleksander.es
Hey Subash,
I'm playing with the whole QMAP data aggregation setup with a USB
connected Fibocom FM150-AE module (SDX55).
See https://gitlab.freedesktop.org/mobile-broadband/libqmi/-/issues/71
for some details on how I tested all this.
This module reports a "Downlink Data Aggregation Max Size" of 32768
via the "QMI WDA Get Data Format" request/response, and therefore I
configured the MTU of the master wwan0 interface with that same value
(while in 802.3 mode, before switching to raw-ip and enabling
qmap-pass-through in qmi_wwan).
When attempting to create a new link using netlink, the operation
fails with -EINVAL, and following the code path in the kernel driver,
it looks like there is a check in rmnet_vnd_change_mtu() where the
master interface MTU is checked against the RMNET_MAX_PACKET_SIZE
value, defined as 16384.
If I setup the master interface with MTU 16384 before creating the
links with netlink, there's no error reported anywhere. The FM150
module crashes as soon as I connect it with data aggregation enabled,
but that's a different story...
Is this limitation imposed by the RMNET_MAX_PACKET_SIZE value still a
valid one in this case? Should changing the max packet size to 32768
be a reasonable approach? Am I doing something wrong? :)
This previous discussion for the qmi_wwan add_mux/del_mux case is
relevant:
https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/..
The suggested patch was not included yet in the qmi_wwan driver and
therefore the user still needs to manually configure the MTU of the
master interface before setting up all the links, but at least there
seems to be no maximum hardcoded limit.
Cheers!
Hi Aleksander
The downlink data aggregation size shouldn't affect the MTU.
MTU applies for uplink only and there is no correlation with the
downlink path.
Ideally, you should be able to use standard 1500 bytes (+ additional
size for MAP header)
for the master device. Is there some specific network which is using
greater than 1500 for the IP packet itself in uplink.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
From: Aleksander Morgado <hidden> Date: 2021-08-05 20:33:01
Hey Subash,
quoted
I'm playing with the whole QMAP data aggregation setup with a USB
connected Fibocom FM150-AE module (SDX55).
See https://gitlab.freedesktop.org/mobile-broadband/libqmi/-/issues/71
for some details on how I tested all this.
This module reports a "Downlink Data Aggregation Max Size" of 32768
via the "QMI WDA Get Data Format" request/response, and therefore I
configured the MTU of the master wwan0 interface with that same value
(while in 802.3 mode, before switching to raw-ip and enabling
qmap-pass-through in qmi_wwan).
When attempting to create a new link using netlink, the operation
fails with -EINVAL, and following the code path in the kernel driver,
it looks like there is a check in rmnet_vnd_change_mtu() where the
master interface MTU is checked against the RMNET_MAX_PACKET_SIZE
value, defined as 16384.
If I setup the master interface with MTU 16384 before creating the
links with netlink, there's no error reported anywhere. The FM150
module crashes as soon as I connect it with data aggregation enabled,
but that's a different story...
Is this limitation imposed by the RMNET_MAX_PACKET_SIZE value still a
valid one in this case? Should changing the max packet size to 32768
be a reasonable approach? Am I doing something wrong? :)
This previous discussion for the qmi_wwan add_mux/del_mux case is
relevant:
https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/..
The suggested patch was not included yet in the qmi_wwan driver and
therefore the user still needs to manually configure the MTU of the
master interface before setting up all the links, but at least there
seems to be no maximum hardcoded limit.
Cheers!
Hi Aleksander
The downlink data aggregation size shouldn't affect the MTU.
MTU applies for uplink only and there is no correlation with the
downlink path.
Ideally, you should be able to use standard 1500 bytes (+ additional
size for MAP header)
for the master device. Is there some specific network which is using
greater than 1500 for the IP packet itself in uplink.
I may be mistaken then in how this should be setup when using rmnet.
For the qmi_wwan case using add_mux/del_mux (Daniele correct me if
wrong!), we do need to configure the MTU of the master interface to be
equal to the aggregation data size reported via QMI WDA before
creating any mux link; see
http://paldan.altervista.org/linux-qmap-qmi_wwan-multiple-pdn-setup/
I ended up doing the same here for the rmnet case; but if it's not
needed I can definitely change that. I do recall that I originally had
left the master MTU untouched in the rmnet case and users had issues,
and increasing it to the aggregation size solved that; I assume that's
because the MTU should have been increased to accommodate the extra
MAP header as you said. How much more size does it need on top of the
1500 bytes?
--
Aleksander
https://aleksander.es
I may be mistaken then in how this should be setup when using rmnet.
For the qmi_wwan case using add_mux/del_mux (Daniele correct me if
wrong!), we do need to configure the MTU of the master interface to be
equal to the aggregation data size reported via QMI WDA before
creating any mux link; see
http://paldan.altervista.org/linux-qmap-qmi_wwan-multiple-pdn-setup/
I ended up doing the same here for the rmnet case; but if it's not
needed I can definitely change that. I do recall that I originally had
left the master MTU untouched in the rmnet case and users had issues,
and increasing it to the aggregation size solved that; I assume that's
because the MTU should have been increased to accommodate the extra
MAP header as you said. How much more size does it need on top of the
1500 bytes?
You need to use an additional 4 bytes for MAPv1 and 8 bytes for
MAPv4/v5.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
From: Aleksander Morgado <hidden> Date: 2021-08-06 14:15:35
Hey Subash,
quoted
I may be mistaken then in how this should be setup when using rmnet.
For the qmi_wwan case using add_mux/del_mux (Daniele correct me if
wrong!), we do need to configure the MTU of the master interface to be
equal to the aggregation data size reported via QMI WDA before
creating any mux link; see
http://paldan.altervista.org/linux-qmap-qmi_wwan-multiple-pdn-setup/
I ended up doing the same here for the rmnet case; but if it's not
needed I can definitely change that. I do recall that I originally had
left the master MTU untouched in the rmnet case and users had issues,
and increasing it to the aggregation size solved that; I assume that's
because the MTU should have been increased to accommodate the extra
MAP header as you said. How much more size does it need on top of the
1500 bytes?
You need to use an additional 4 bytes for MAPv1 and 8 bytes for
MAPv4/v5.
I tried with a SIMCOM 7600E, with data aggregation enabled with QMAPv1:
$ sudo qmicli -d /dev/cdc-wdm0 -p --wda-get-data-format
[/dev/cdc-wdm0] Successfully got data format
QoS flow header: no
Link layer protocol: 'raw-ip'
Uplink data aggregation protocol: 'qmap'
Downlink data aggregation protocol: 'qmap'
NDP signature: '0'
Downlink data aggregation max datagrams: '10'
Downlink data aggregation max size: '4096'
As you suggested, the MTU of the new muxed interface is set to 1500
and the MTU of the master interface to only 4 more bytes (1504):
# ip link
8: wwp0s20f0u8u4i5: <POINTOPOINT,UP,LOWER_UP> mtu 1504 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/none
9: qmapmux0.0@wwp0s20f0u8u4i5: <UP,LOWER_UP> mtu 1500 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/[519]
Under this scenario, the downlink is completely broken (speedtest
0.39Mbps), while the uplink seems to work (speedtest 13Mbps).
If I use the logic I had before, associating the downlink data
aggregation max size reported by the module to the MTU of the master
interface, same as I had to do when using qmi_wwan add_mux/del_mux,
then it works properly:
# ip link
14: wwp0s20f0u8u4i5: <POINTOPOINT,UP,LOWER_UP> mtu 4096 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/none
15: qmapmux0.0@wwp0s20f0u8u4i5: <UP,LOWER_UP> mtu 1500 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/[519]
Downlink is now 26Mbps and uplink still 13Mbps.
Is there something I'm doing wrong? Or do we really need to do the
same thing as in qmi_wwan add_mux/del_mux; i.e. configuring the master
interface MTU to be the same as the downlink max aggregation data size
so that we change the rx_urb_size?
--
Aleksander
https://aleksander.es
I tried with a SIMCOM 7600E, with data aggregation enabled with QMAPv1:
$ sudo qmicli -d /dev/cdc-wdm0 -p --wda-get-data-format
[/dev/cdc-wdm0] Successfully got data format
QoS flow header: no
Link layer protocol: 'raw-ip'
Uplink data aggregation protocol: 'qmap'
Downlink data aggregation protocol: 'qmap'
NDP signature: '0'
Downlink data aggregation max datagrams: '10'
Downlink data aggregation max size: '4096'
As you suggested, the MTU of the new muxed interface is set to 1500
and the MTU of the master interface to only 4 more bytes (1504):
# ip link
8: wwp0s20f0u8u4i5: <POINTOPOINT,UP,LOWER_UP> mtu 1504 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/none
9: qmapmux0.0@wwp0s20f0u8u4i5: <UP,LOWER_UP> mtu 1500 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/[519]
Under this scenario, the downlink is completely broken (speedtest
0.39Mbps), while the uplink seems to work (speedtest 13Mbps).
If I use the logic I had before, associating the downlink data
aggregation max size reported by the module to the MTU of the master
interface, same as I had to do when using qmi_wwan add_mux/del_mux,
then it works properly:
# ip link
14: wwp0s20f0u8u4i5: <POINTOPOINT,UP,LOWER_UP> mtu 4096 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/none
15: qmapmux0.0@wwp0s20f0u8u4i5: <UP,LOWER_UP> mtu 1500 qdisc fq_codel
state UNKNOWN mode DEFAULT group default qlen 1000
link/[519]
Downlink is now 26Mbps and uplink still 13Mbps.
Is there something I'm doing wrong? Or do we really need to do the
same thing as in qmi_wwan add_mux/del_mux; i.e. configuring the master
interface MTU to be the same as the downlink max aggregation data size
so that we change the rx_urb_size?
Unfortunately, this seems to be a limitation of qmi_wwan (usbnet)
where its tying the RX to the TX size through usbnet_change_mtu.
Ideally, we should break this dependency and have a sysfs or some other
configuration scheme to set the rx_urb_size.
Looks like this discussion has happened a while back and the option to
use
a configurable scheme for rx_urb_size was rejected by Bjorn and Greg KH.
The summary of the thread was to set a large rx_urb_size during probe
itself for qmi_wwan.
https://patchwork.kernel.org/project/linux-usb/patch/20200803065105.8997-1-yzc666@netease.com/
We could try setting a large value as suggested there and it should
hopefully
solve the issue you are seeing.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
Unfortunately, this seems to be a limitation of qmi_wwan (usbnet)
where its tying the RX to the TX size through usbnet_change_mtu.
Ideally, we should break this dependency and have a sysfs or some other
configuration scheme to set the rx_urb_size.
Looks like this discussion has happened a while back and the option to
use
a configurable scheme for rx_urb_size was rejected by Bjorn and Greg KH.
Ouch, I never meant to shoot down the proposal. I had to go back and
read my comments. I see that it might have come out that way...
My main point was that I'd prefer this to work without any userspace
invervention, if possible. And I'm still not sure we've explored that
alternative to the end?
The summary of the thread was to set a large rx_urb_size during probe
itself for qmi_wwan.
Yes, I think it would be good to make the driver DTRT automatically.
Coding driver specific quirks into ModemManager might work, but it feels
wrong to work around a Linux driver bug. We don't have to do that. We
can fix the driver.
Why can't we break the rx_urb_size dependency on MTU automatically when
pass_through or qmi_wwan internal muxing is enabled? Preferably with
some fixed default size which would Just Work for everyone.
I'm not rejecting a rx_urb_size knob if it is absolutely necessary. But
I hope someone will write a good explanation on how to tune that value
then. Knobs are only useful if the user can make an intelligent choice.
This should go into the ABI docs.
Bjørn
From: Aleksander Morgado <hidden> Date: 2021-08-06 20:22:16
Hey,
quoted
The summary of the thread was to set a large rx_urb_size during probe
itself for qmi_wwan.
Yes, I think it would be good to make the driver DTRT automatically.
Coding driver specific quirks into ModemManager might work, but it feels
wrong to work around a Linux driver bug. We don't have to do that. We
can fix the driver.
Why can't we break the rx_urb_size dependency on MTU automatically when
pass_through or qmi_wwan internal muxing is enabled? Preferably with
some fixed default size which would Just Work for everyone.
That default fixed size you're suggesting for the rx_urb_size, isn't
it supposed to have the same logical meaning as RMNET_MAX_PACKET_SIZE
at the end?
--
Aleksander
https://aleksander.es
That default fixed size you're suggesting for the rx_urb_size, isn't
it supposed to have the same logical meaning as RMNET_MAX_PACKET_SIZE
at the end?
RMNET_MAX_PACKET_SIZE is the maximum size for uplink. I probably should
have
named it more clearly. We would need a different value for downlink.
quoted
Yes, I think it would be good to make the driver DTRT automatically.
Coding driver specific quirks into ModemManager might work, but it
feels
wrong to work around a Linux driver bug. We don't have to do that. We
can fix the driver.
Why can't we break the rx_urb_size dependency on MTU automatically
when
pass_through or qmi_wwan internal muxing is enabled? Preferably with
some fixed default size which would Just Work for everyone.
qmi_wwan_netdev_ops = {
.ndo_stop = usbnet_stop,
.ndo_start_xmit = usbnet_start_xmit,
.ndo_tx_timeout = usbnet_tx_timeout,
- .ndo_change_mtu = usbnet_change_mtu,
+ .ndo_change_mtu = qmi_wwan_change_mtu,
.ndo_get_stats64 = dev_get_tstats64,
.ndo_set_mac_address = qmi_wwan_mac_addr,
.ndo_validate_addr = eth_validate_addr,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
net_device *dev,
dev_put(real_dev);
}
+static int qmi_wwan_change_mtu(struct net_device *net, int new_mtu)
+{
+ struct usbnet *dev = netdev_priv(net);
+ struct qmi_wwan_state *info = (void *)&dev->data;
+ int old_rx_urb_size = dev->rx_urb_size;
+
+ /* mux and pass through modes use a fixed rx_urb_size and the
value
+ * is independent of mtu
+ */
+ if (info->flags & (QMI_WWAN_FLAG_MUX |
QMI_WWAN_FLAG_PASS_THROUGH)) {
+ if (old_rx_urb_size == MAP_DL_URB_SIZE)
+ return 0;
+
+ if (old_rx_urb_size < MAP_DL_URB_SIZE) {
+ usbnet_pause_rx(dev);
+ usbnet_unlink_rx_urbs(dev);
+ usbnet_resume_rx(dev);
+ usbnet_update_max_qlen(dev);
+ }
+
+ return 0;
+ }
+
+ /* rawip mode uses existing logic of setting rx_urb_size based
on mtu */
+ return usbnet_change_mtu(net, new_mtu);
+}
Either I'm blind, or you don't actuelly change the rx_urb_size for the
mux and pass through modes?
I'd also prefer this to reset back to syncing with hard_mtu if/when
muxing or passthrough is disabled. Calling usbnet_change_mtu() won't do
that. It doesn't touch rx_urb_size if it is different from hard_mtu.
I also think that it might be useful to keep the mtu/hard_mtu control,
wouldn't it?
Something like
old_rx_urb_size = dev->rx_urb_size;
if (mux|passthrough)
dev->rx_urb_size = MAP_DL_URB_SIZE;
else
dev->rx_urb_size = dev->hard_mtu;
if (dev->rx_urb_size > old_rx_urb_size)
unlink_urbs etc;
return usbnet_change_mtu(net, new_mtu);
should do that, I think. Completely untested....
No need for () around a constant, is there?
Either I'm blind, or you don't actuelly change the rx_urb_size for the
mux and pass through modes?
I seem to have missed this and the other stuff you have pointed out.
Can you please review this update-
I'd also prefer this to reset back to syncing with hard_mtu if/when
muxing or passthrough is disabled. Calling usbnet_change_mtu() won't
do
that. It doesn't touch rx_urb_size if it is different from hard_mtu.
I also think that it might be useful to keep the mtu/hard_mtu control,
wouldn't it?
Something like
old_rx_urb_size = dev->rx_urb_size;
if (mux|passthrough)
dev->rx_urb_size = MAP_DL_URB_SIZE;
else
dev->rx_urb_size = dev->hard_mtu;
if (dev->rx_urb_size > old_rx_urb_size)
unlink_urbs etc;
return usbnet_change_mtu(net, new_mtu);
should do that, I think. Completely untested....
And add it to the (!qmimux_has_slaves(dev)) cas in del_mux_store() too.
Assuming this patch doesn't have too many other issues, can I request
Aleksander / Daniele to try this out.
Hi Subash,
Il giorno lun 9 ago 2021 alle ore 23:40 [off-list ref] ha scritto:
quoted
No need for () around a constant, is there?
Either I'm blind, or you don't actuelly change the rx_urb_size for the
mux and pass through modes?
I seem to have missed this and the other stuff you have pointed out.
Can you please review this update-
quoted
I'd also prefer this to reset back to syncing with hard_mtu if/when
muxing or passthrough is disabled. Calling usbnet_change_mtu() won't
do
that. It doesn't touch rx_urb_size if it is different from hard_mtu.
I also think that it might be useful to keep the mtu/hard_mtu control,
wouldn't it?
Something like
old_rx_urb_size = dev->rx_urb_size;
if (mux|passthrough)
dev->rx_urb_size = MAP_DL_URB_SIZE;
else
dev->rx_urb_size = dev->hard_mtu;
if (dev->rx_urb_size > old_rx_urb_size)
unlink_urbs etc;
return usbnet_change_mtu(net, new_mtu);
should do that, I think. Completely untested....
And add it to the (!qmimux_has_slaves(dev)) cas in del_mux_store() too.
Assuming this patch doesn't have too many other issues, can I request
Aleksander / Daniele to try this out.
I'm currently in vacation for a few weeks, so I can't test the change
at the moment, will try to do when I come back.
Thanks for bringing this up.
Looks like a tunable would be needed to satisfy all users.
Perhaps we can use 32k as default in mux and passthrough mode but allow
for changes
there if needed through a sysfs.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
Thanks for bringing this up.
Looks like a tunable would be needed to satisfy all users.
Perhaps we can use 32k as default in mux and passthrough mode but
allow for changes
there if needed through a sysfs.
Thanks for bringing this up.
Looks like a tunable would be needed to satisfy all users.
Perhaps we can use 32k as default in mux and passthrough mode but
allow for changes
there if needed through a sysfs.
Sounds reasonable to me.
I have done a bit of testing both with qmi_wwann qmap implementation
and rmnet, so far everything seems to be working fine.
Thanks,
Daniele
I have done a bit of testing both with qmi_wwann qmap implementation
and rmnet, so far everything seems to be working fine.
Thanks,
Daniele
Thanks, I'll send out the patch with additional sysfs once net-next
is open.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project