RMNET QMAP data aggregation with size greater than 16384

RMNET QMAP data aggregation with size greater than 16384

[Aleksander Morgado]

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

Re: RMNET QMAP data aggregation with size greater than 16384

[subashab]

On 2021-07-31 16:45, Aleksander Morgado wrote:
> 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

Re: RMNET QMAP data aggregation with size greater than 16384

[Aleksander Morgado]

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

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

Re: RMNET QMAP data aggregation with size greater than 16384

[Aleksander Morgado]

>> > > 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/
>>
>
> Right: it's not for the MTU itself, but for changing the rx_urb_size, since usbnet_change_mtu has that side effect.
>

I knew there was a reason even if not obvious. Should we fix that rx
urb size value to 16384 to avoid needing that extra step? Was that
what you were suggesting in that patch that was never merged?

-- 
Aleksander
https://aleksander.es

Re: RMNET QMAP data aggregation with size greater than 16384

[Daniele Palmas]

Il giorno gio 5 ago 2021 alle ore 23:01 Aleksander Morgado
<aleksander@aleksander.es> ha scritto:
>
> >> > > 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/
> >>
> >
> > Right: it's not for the MTU itself, but for changing the rx_urb_size, since usbnet_change_mtu has that side effect.
> >
>
> I knew there was a reason even if not obvious. Should we fix that rx
> urb size value to 16384 to avoid needing that extra step? Was that
> what you were suggesting in that patch that was never merged?
>

Yes, that was my purpose, but thinking about it twice, I thought it
was not a very good idea.

There are too many modems/hosts/firmware versions combinations, each
one of them with its own bunch of bugs/quirks (daily experienced):
after all I think it's better to have the possibility to have the urb
size configurable (even if through an odd way), than to have it fixed.

Regards,
Daniele

>
> --
> Aleksander
> https://aleksander.es

Re: RMNET QMAP data aggregation with size greater than 16384

[subashab]

> 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

Re: RMNET QMAP data aggregation with size greater than 16384

[Aleksander Morgado]

Hey Subash,

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

Re: RMNET QMAP data aggregation with size greater than 16384

[subashab]

> 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

Re: RMNET QMAP data aggregation with size greater than 16384

[Bjørn Mork]

subashab@codeaurora.org writes:

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

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

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

Re: RMNET QMAP data aggregation with size greater than 16384

[Aleksander Morgado]

Hey,

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

Re: RMNET QMAP data aggregation with size greater than 16384

[subashab]

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

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

Would something like this work-

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 6a2e4f8..c49827e 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -75,6 +75,8 @@ struct qmimux_priv {
         u8 mux_id;
  };

+#define MAP_DL_URB_SIZE (32768)
+
  static int qmimux_open(struct net_device *dev)
  {
         struct qmimux_priv *priv = netdev_priv(dev);
@@ -303,6 +305,33 @@ static void qmimux_unregister_device(struct 
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);
+}
+
  static void qmi_wwan_netdev_setup(struct net_device *net)
  {
         struct usbnet *dev = netdev_priv(net);
@@ -326,7 +355,7 @@ static void qmi_wwan_netdev_setup(struct net_device 
*net)
         }

         /* recalculate buffers after changing hard_header_len */
-       usbnet_change_mtu(net, net->mtu);
+       qmi_wwan_change_mtu(net, net->mtu);
  }

  static ssize_t raw_ip_show(struct device *d, struct device_attribute 
*attr, char *buf)
@@ -433,6 +462,7 @@ static ssize_t add_mux_store(struct device *d,  
struct device_attribute *attr, c
         if (!ret) {
                 info->flags |= QMI_WWAN_FLAG_MUX;
                 ret = len;
+               qmi_wwan_change_mtu(dev->net, dev->net->mtu);
         }
  err:
         rtnl_unlock();
@@ -514,6 +544,8 @@ static ssize_t pass_through_store(struct device *d,
         else
                 info->flags &= ~QMI_WWAN_FLAG_PASS_THROUGH;

+       qmi_wwan_change_mtu(dev->net, dev->net->mtu);
+
         return len;
  }

@@ -643,7 +675,7 @@ static const struct net_device_ops 
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

Re: RMNET QMAP data aggregation with size greater than 16384

[Bjørn Mork]

subashab@codeaurora.org writes:

> Would something like this work-


Looking pretty good to me, but I believe it needs some polishing.

> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 6a2e4f8..c49827e 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -75,6 +75,8 @@ struct qmimux_priv {
>         u8 mux_id;
>  };
>
> +#define MAP_DL_URB_SIZE (32768)

No need for () around a constant, is there?

>  static int qmimux_open(struct net_device *dev)
>  {
>         struct qmimux_priv *priv = netdev_priv(dev);
> @@ -303,6 +305,33 @@ static void qmimux_unregister_device(struct
> 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....



> +
>  static void qmi_wwan_netdev_setup(struct net_device *net)
>  {
>         struct usbnet *dev = netdev_priv(net);
> @@ -326,7 +355,7 @@ static void qmi_wwan_netdev_setup(struct
> net_device *net)
>         }
>
>         /* recalculate buffers after changing hard_header_len */
> -       usbnet_change_mtu(net, net->mtu);
> +       qmi_wwan_change_mtu(net, net->mtu);
>  }
>
>  static ssize_t raw_ip_show(struct device *d, struct device_attribute
>  *attr, char *buf)
> @@ -433,6 +462,7 @@ static ssize_t add_mux_store(struct device *d,
> struct device_attribute *attr, c
>         if (!ret) {
>                 info->flags |= QMI_WWAN_FLAG_MUX;
>                 ret = len;
> +               qmi_wwan_change_mtu(dev->net, dev->net->mtu);
>         }
>  err:
>         rtnl_unlock();
> @@ -514,6 +544,8 @@ static ssize_t pass_through_store(struct device *d,
>         else
>                 info->flags &= ~QMI_WWAN_FLAG_PASS_THROUGH;
>
> +       qmi_wwan_change_mtu(dev->net, dev->net->mtu);
> +
>         return len;
>  }


And add it to the (!qmimux_has_slaves(dev)) cas in del_mux_store() too.


Bjørn

Re: RMNET QMAP data aggregation with size greater than 16384

[subashab]

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

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 6a2e4f8..4676544 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -75,6 +75,8 @@ struct qmimux_priv {
         u8 mux_id;
  };

+#define MAP_DL_URB_SIZE 32768
+
  static int qmimux_open(struct net_device *dev)
  {
         struct qmimux_priv *priv = netdev_priv(dev);
@@ -303,6 +305,39 @@ static void qmimux_unregister_device(struct 
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) {
+                       dev->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.
+        * rx_urb_size will be updated within usbnet_change_mtu only if 
it is
+        * equal to existing hard_mtu
+        */
+       dev->rx_urb_size = dev->hard_mtu;
+       return usbnet_change_mtu(net, new_mtu);
+}
+
  static void qmi_wwan_netdev_setup(struct net_device *net)
  {
         struct usbnet *dev = netdev_priv(net);
@@ -326,7 +361,7 @@ static void qmi_wwan_netdev_setup(struct net_device 
*net)
         }

         /* recalculate buffers after changing hard_header_len */
-       usbnet_change_mtu(net, net->mtu);
+       qmi_wwan_change_mtu(net, net->mtu);
  }

  static ssize_t raw_ip_show(struct device *d, struct device_attribute 
*attr, char *buf)
@@ -433,6 +468,7 @@ static ssize_t add_mux_store(struct device *d,  
struct device_attribute *attr, c
         if (!ret) {
                 info->flags |= QMI_WWAN_FLAG_MUX;
                 ret = len;
+               qmi_wwan_change_mtu(dev->net, dev->net->mtu);
         }
  err:
         rtnl_unlock();
@@ -466,8 +502,11 @@ static ssize_t del_mux_store(struct device *d,  
struct device_attribute *attr, c
         }
         qmimux_unregister_device(del_dev, NULL);

-       if (!qmimux_has_slaves(dev))
+       if (!qmimux_has_slaves(dev)) {
                 info->flags &= ~QMI_WWAN_FLAG_MUX;
+               qmi_wwan_change_mtu(dev->net, dev->net->mtu);
+       }
+
         ret = len;
  err:
         rtnl_unlock();
@@ -514,6 +553,8 @@ static ssize_t pass_through_store(struct device *d,
         else
                 info->flags &= ~QMI_WWAN_FLAG_PASS_THROUGH;

+       qmi_wwan_change_mtu(dev->net, dev->net->mtu);
+
         return len;
  }

@@ -643,7 +684,7 @@ static const struct net_device_ops 
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,

Re: RMNET QMAP data aggregation with size greater than 16384

[Daniele Palmas]

Hi Subash,

Il giorno lun 9 ago 2021 alle ore 23:40 <subashab@codeaurora.org> ha scritto:
>
> > 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.
>

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.

> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 6a2e4f8..4676544 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -75,6 +75,8 @@ struct qmimux_priv {
>          u8 mux_id;
>   };
>
> +#define MAP_DL_URB_SIZE 32768

Just an heads-up that when I proposed that urb size there were doubts
about the value (see
https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523774
and https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523958):
it is true that this time you are setting the size just when qmap is
enabled, but I think that Carl's comment about low-cat chipsets could
still apply.

Thanks,
Daniele

> +
>   static int qmimux_open(struct net_device *dev)
>   {
>          struct qmimux_priv *priv = netdev_priv(dev);
> @@ -303,6 +305,39 @@ static void qmimux_unregister_device(struct
> 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) {
> +                       dev->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.
> +        * rx_urb_size will be updated within usbnet_change_mtu only if
> it is
> +        * equal to existing hard_mtu
> +        */
> +       dev->rx_urb_size = dev->hard_mtu;
> +       return usbnet_change_mtu(net, new_mtu);
> +}
> +
>   static void qmi_wwan_netdev_setup(struct net_device *net)
>   {
>          struct usbnet *dev = netdev_priv(net);
> @@ -326,7 +361,7 @@ static void qmi_wwan_netdev_setup(struct net_device
> *net)
>          }
>
>          /* recalculate buffers after changing hard_header_len */
> -       usbnet_change_mtu(net, net->mtu);
> +       qmi_wwan_change_mtu(net, net->mtu);
>   }
>
>   static ssize_t raw_ip_show(struct device *d, struct device_attribute
> *attr, char *buf)
> @@ -433,6 +468,7 @@ static ssize_t add_mux_store(struct device *d,
> struct device_attribute *attr, c
>          if (!ret) {
>                  info->flags |= QMI_WWAN_FLAG_MUX;
>                  ret = len;
> +               qmi_wwan_change_mtu(dev->net, dev->net->mtu);
>          }
>   err:
>          rtnl_unlock();
> @@ -466,8 +502,11 @@ static ssize_t del_mux_store(struct device *d,
> struct device_attribute *attr, c
>          }
>          qmimux_unregister_device(del_dev, NULL);
>
> -       if (!qmimux_has_slaves(dev))
> +       if (!qmimux_has_slaves(dev)) {
>                  info->flags &= ~QMI_WWAN_FLAG_MUX;
> +               qmi_wwan_change_mtu(dev->net, dev->net->mtu);
> +       }
> +
>          ret = len;
>   err:
>          rtnl_unlock();
> @@ -514,6 +553,8 @@ static ssize_t pass_through_store(struct device *d,
>          else
>                  info->flags &= ~QMI_WWAN_FLAG_PASS_THROUGH;
>
> +       qmi_wwan_change_mtu(dev->net, dev->net->mtu);
> +
>          return len;
>   }
>
> @@ -643,7 +684,7 @@ static const struct net_device_ops
> 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,

Re: RMNET QMAP data aggregation with size greater than 16384

[subashab]

> Just an heads-up that when I proposed that urb size there were doubts
> about the value (see
> https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523774
> and
> https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523958):
> it is true that this time you are setting the size just when qmap is
> enabled, but I think that Carl's comment about low-cat chipsets could
> still apply.
> 
> Thanks,
> Daniele
> 

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

Re: RMNET QMAP data aggregation with size greater than 16384

[Bjørn Mork]

subashab@codeaurora.org writes:

>> Just an heads-up that when I proposed that urb size there were doubts
>> about the value (see
>> https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523774
>> and
>> https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523958):
>> it is true that this time you are setting the size just when qmap is
>> enabled, but I think that Carl's comment about low-cat chipsets could
>> still apply.
>> Thanks,
>> Daniele
>> 
>
> 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.



Bjørn

Re: RMNET QMAP data aggregation with size greater than 16384

[Daniele Palmas]

Hi all,

Il giorno ven 13 ago 2021 alle ore 08:25 Bjørn Mork <bjorn@mork.no> ha scritto:
>
> subashab@codeaurora.org writes:
>
> >> Just an heads-up that when I proposed that urb size there were doubts
> >> about the value (see
> >> https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523774
> >> and
> >> https://patchwork.ozlabs.org/project/netdev/patch/20200909091302.20992-1-dnlplm@gmail.com/#2523958):
> >> it is true that this time you are setting the size just when qmap is
> >> enabled, but I think that Carl's comment about low-cat chipsets could
> >> still apply.
> >> Thanks,
> >> Daniele
> >>
> >
> > 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

>
>
> Bjørn

Re: RMNET QMAP data aggregation with size greater than 16384

[subashab]

> 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