Re: [PATCH] net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.

Re: [PATCH] net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.

[Bjørn Mork]

Paul Gildea <paul.gildea@gmail.com> writes:

> When MTU of modem is set to less than 1500 and a packet larger than MTU
> arrives in Linux from a modem, it is discarded with -EOVERFLOW error
> (Babble error). This is seen on USB3.0 and USB2.0 busses. This is
> essentially because the MRU (Max Receive Size) is not a separate entity to
> the MTU (Max Transmit Size) and the received packets can be larger than
> those transmitted. Following the babble error there were an endless supply
> of zero-length URBs which are rejected with -EPROTO (increasing the rx
> input error counter each time). This is only seen on USB3.0. These continue
> to come ad infinitum until the modem is shutdown, rendering the modem
> unusable. There is a bug in the core USB handling code in Linux that
> doesn't deal well with network MTUs smaller than 1500 bytes. By default the
> dev->hard_mtu (the "real" MTU) is in lockstep with dev->rx_urb_size
> (essentially an MRU), and it's the latter that is causing trouble. This has
> nothing to do with the modems; the issue can be reproduced by getting a
> USB-Ethernet dongle, setting the MTU to 1430, and pinging with size greater
> than 1406.
>
> Signed-off-by: Paul Gildea <Paul.Gildea@gmail.com>
> ---
> drivers/net/usb/qmi_wwan.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 5754bb6..545c772 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -815,6 +815,13 @@ static int qmi_wwan_bind(struct usbnet *dev, struct
> usb_interface *intf)
>     }
>     dev->net->netdev_ops = &qmi_wwan_netdev_ops;
>     dev->net->sysfs_groups[0] = &qmi_wwan_sysfs_attr_group;
> +    /* LTE Networks don't always respect their own MTU on receive side;
> +    * e.g. AT&T pushes 1430 MTU but still allows 1500 byte packets from
> +    * far-end network. Make receive buffer large enough to accommodate
> +    * them, and add four bytes so MTU does not equal MRU on network
> +    * with 1500 MTU otherwise usbnet_change_mtu() will change both.
> +    */
> +   dev->rx_urb_size = ETH_DATA_LEN + 4;
>  err:
>     return status;
>  }
> --
> 1.9.1


This is fine as a first step towards saner buffer handling in qmi_wwan.
If real world devices use asymmetric MTUs, then we should just deal with
that.

So I was going to add my ack.  But the patch does not apply:


 bjorn@miraculix:/usr/local/src/git/linux$ git am /tmp/l
 Applying: net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.
 error: corrupt patch at line 10

and checkpatch says why:

 bjorn@miraculix:/usr/local/src/git/linux$ scripts/checkpatch.pl /tmp/l
 ERROR: patch seems to be corrupt (line wrapped?)
 #34: FILE: drivers/net/usb/qmi_wwan.c:814:
 usb_interface *intf)


Could you fix up and resend? You might have to use a different email
client.  See
https://www.kernel.org/doc/html/latest/process/email-clients.html#email-clients


Bjørn

Re: [PATCH] net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.

[Daniele Palmas]

Hi Bjørn and Paul,

Il giorno dom 8 mar 2020 alle ore 16:28 Bjørn Mork <bjorn@mork.no> ha scritto:
>
> Paul Gildea <paul.gildea@gmail.com> writes:
>
> > When MTU of modem is set to less than 1500 and a packet larger than MTU
> > arrives in Linux from a modem, it is discarded with -EOVERFLOW error
> > (Babble error). This is seen on USB3.0 and USB2.0 busses. This is
> > essentially because the MRU (Max Receive Size) is not a separate entity to
> > the MTU (Max Transmit Size) and the received packets can be larger than
> > those transmitted. Following the babble error there were an endless supply
> > of zero-length URBs which are rejected with -EPROTO (increasing the rx
> > input error counter each time). This is only seen on USB3.0. These continue
> > to come ad infinitum until the modem is shutdown, rendering the modem
> > unusable. There is a bug in the core USB handling code in Linux that
> > doesn't deal well with network MTUs smaller than 1500 bytes. By default the
> > dev->hard_mtu (the "real" MTU) is in lockstep with dev->rx_urb_size
> > (essentially an MRU), and it's the latter that is causing trouble. This has
> > nothing to do with the modems; the issue can be reproduced by getting a
> > USB-Ethernet dongle, setting the MTU to 1430, and pinging with size greater
> > than 1406.
> >
> > Signed-off-by: Paul Gildea <Paul.Gildea@gmail.com>
> > ---
> > drivers/net/usb/qmi_wwan.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > index 5754bb6..545c772 100644
> > --- a/drivers/net/usb/qmi_wwan.c
> > +++ b/drivers/net/usb/qmi_wwan.c
> > @@ -815,6 +815,13 @@ static int qmi_wwan_bind(struct usbnet *dev, struct
> > usb_interface *intf)
> >     }
> >     dev->net->netdev_ops = &qmi_wwan_netdev_ops;
> >     dev->net->sysfs_groups[0] = &qmi_wwan_sysfs_attr_group;
> > +    /* LTE Networks don't always respect their own MTU on receive side;
> > +    * e.g. AT&T pushes 1430 MTU but still allows 1500 byte packets from
> > +    * far-end network. Make receive buffer large enough to accommodate
> > +    * them, and add four bytes so MTU does not equal MRU on network
> > +    * with 1500 MTU otherwise usbnet_change_mtu() will change both.
> > +    */
> > +   dev->rx_urb_size = ETH_DATA_LEN + 4;

Isn't this going to break the change MTU workaround for dl data
aggregation when using qmap?

Regards,
Daniele

> >  err:
> >     return status;
> >  }
> > --
> > 1.9.1
>
>
> This is fine as a first step towards saner buffer handling in qmi_wwan.
> If real world devices use asymmetric MTUs, then we should just deal with
> that.
>
> So I was going to add my ack.  But the patch does not apply:
>
>
>  bjorn@miraculix:/usr/local/src/git/linux$ git am /tmp/l
>  Applying: net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.
>  error: corrupt patch at line 10
>
> and checkpatch says why:
>
>  bjorn@miraculix:/usr/local/src/git/linux$ scripts/checkpatch.pl /tmp/l
>  ERROR: patch seems to be corrupt (line wrapped?)
>  #34: FILE: drivers/net/usb/qmi_wwan.c:814:
>  usb_interface *intf)
>
>
> Could you fix up and resend? You might have to use a different email
> client.  See
> https://www.kernel.org/doc/html/latest/process/email-clients.html#email-clients
>
>
> Bjørn

Re: [PATCH] net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.

[Kristian Evensen]

Hi all,

I was able to trigger the same issue as reported by Paul, and came
across this patch (+ Daniele's other patch and thread on the libqmi
mailing list). Applying Paul's fix solved the problem for me, changing
the MTU of the QMI interface now works fine. Thanks a lot to everyone
involved!

I just have one question, is there a specific reason for the patch not
being resubmitted or Daniele's work not resumed? I do not use any of
the aggregation-stuff, so I don't know how that is affected by for
example Paul's change. If there is anything I can do to help, please
let me know.

BR,
Kristian

Re: [PATCH] net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.

[Bjørn Mork]

Kristian Evensen <kristian.evensen@gmail.com> writes:

> Hi all,
>
> I was able to trigger the same issue as reported by Paul, and came
> across this patch (+ Daniele's other patch and thread on the libqmi
> mailing list). Applying Paul's fix solved the problem for me, changing
> the MTU of the QMI interface now works fine. Thanks a lot to everyone
> involved!
>
> I just have one question, is there a specific reason for the patch not
> being resubmitted or Daniele's work not resumed? I do not use any of
> the aggregation-stuff, so I don't know how that is affected by for
> example Paul's change. If there is anything I can do to help, please
> let me know.

Thanks for bringing this back into our collective memory.  The patch
never made it to patchwork, probably due to the formatting issues, and
was just forgotten.

There are no other reasons than Daniele's concerns in the email you are
replying to, AFAIK.  The issue pointed out by Paull should be fixed, but
the fix must not break aggregation..

This is a great opportunity for you to play with QMAP aggregation :-)
Wouldn't it be good to do some measurements to document why it is such a
bad idea?


Bjørn

Re: [PATCH] net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.

[Daniele Palmas]

Hi Kristian and Bjørn,

Il giorno lun 7 set 2020 alle ore 09:45 Bjørn Mork <bjorn@mork.no> ha scritto:
>
> Kristian Evensen <kristian.evensen@gmail.com> writes:
>
> > Hi all,
> >
> > I was able to trigger the same issue as reported by Paul, and came
> > across this patch (+ Daniele's other patch and thread on the libqmi
> > mailing list). Applying Paul's fix solved the problem for me, changing
> > the MTU of the QMI interface now works fine. Thanks a lot to everyone
> > involved!
> >
> > I just have one question, is there a specific reason for the patch not
> > being resubmitted or Daniele's work not resumed? I do not use any of
> > the aggregation-stuff, so I don't know how that is affected by for
> > example Paul's change. If there is anything I can do to help, please
> > let me know.
>
> Thanks for bringing this back into our collective memory.  The patch
> never made it to patchwork, probably due to the formatting issues, and
> was just forgotten.
>
> There are no other reasons than Daniele's concerns in the email you are
> replying to, AFAIK.  The issue pointed out by Paull should be fixed, but
> the fix must not break aggregation..
>
> This is a great opportunity for you to play with QMAP aggregation :-)
> Wouldn't it be good to do some measurements to document why it is such a
> bad idea?
>

there was also another recent thread about this and the final plan was
to simply increase the rx urb size setting to the highest value we are
aware of (see https://www.spinics.net/lists/linux-usb/msg198858.html):
this should solve the babble issue without breaking aggregation.

The change should be simple, I was just waiting to perform some sanity
tests with different models I have. Hope to have it done by this week.

Regards,
Daniele

>
> Bjørn
>

Re: [PATCH] net: usb: qmi_wwan: Fix for packets being rejected in the ring buffer used by the xHCI controller.

[Kristian Evensen]

Hi Daniele,

On Mon, Sep 7, 2020 at 10:35 AM Daniele Palmas <dnlplm@gmail.com> wrote:
> there was also another recent thread about this and the final plan was
> to simply increase the rx urb size setting to the highest value we are
> aware of (see https://www.spinics.net/lists/linux-usb/msg198858.html):
> this should solve the babble issue without breaking aggregation.
>
> The change should be simple, I was just waiting to perform some sanity
> tests with different models I have. Hope to have it done by this week.

Thanks for letting me know. Looking forward to the patch and dropping
my own work-around :)

Kristian