[PATCH] net: usb: allow MTU that is a multiple of USB packet size

[PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Ruslan Bilovol]

Current usbnet driver rejects setting MTU that is a multiple
of USB endpoint's wMaxPacketSize size. However, it may only
lead to possible performance degradation but is not so
critical that its using should be prohibited. So allow it
but also warn user about possible issue.

Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
---
 drivers/net/usb/usbnet.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 733f4fe..09dc848 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -382,9 +382,11 @@ int usbnet_change_mtu (struct net_device *net, int new_mtu)
 
 	if (new_mtu <= 0)
 		return -EINVAL;
-	// no second zero-length packet read wanted after mtu-sized packets
+	/* warn about second zero-length packet read after mtu-sized packets */
 	if ((ll_mtu % dev->maxpacket) == 0)
-		return -EDOM;
+		netdev_warn(dev->net, "MTU %d is a multiple of USB wMaxPacketSize (%d),"
+				" consider possible performance degradation\n",
+				ll_mtu, dev->maxpacket);
 	net->mtu = new_mtu;
 
 	dev->hard_mtu = net->mtu + net->hard_header_len;
-- 
1.7.9.5

Re: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Oliver Neukum]

On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
> Current usbnet driver rejects setting MTU that is a multiple
> of USB endpoint's wMaxPacketSize size. However, it may only
> lead to possible performance degradation but is not so
> critical that its using should be prohibited. So allow it
> but also warn user about possible issue.

We have reports about devices reacting badly to ZLPs.
Unless you have a compelling reasons for this change
I have to reject it.

	Regards
		Oliver

NACK

Re: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Ruslan Bilovol]

Hi Oliver,

On Thu, May 7, 2015 at 1:11 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
>> Current usbnet driver rejects setting MTU that is a multiple
>> of USB endpoint's wMaxPacketSize size. However, it may only
>> lead to possible performance degradation but is not so
>> critical that its using should be prohibited. So allow it
>> but also warn user about possible issue.
>
> We have reports about devices reacting badly to ZLPs.
> Unless you have a compelling reasons for this change
> I have to reject it.

What devices do you mean here: USB network adapters or USB host controllers?
If it's just network adapters, then we can create some kind of quirk handling
for them and do not disable this functionality just because some buggy device
can't work with it.
My device works fine with ZLPs so I want to use this particular MTU under Linux
like I do it under other operation systems.

Regards,
Ruslan


>
>         Regards
>                 Oliver
>
> NACK
>

Re: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Ruslan Bilovol]

Hi Oliver,

On Thu, May 7, 2015 at 1:11 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
>> Current usbnet driver rejects setting MTU that is a multiple
>> of USB endpoint's wMaxPacketSize size. However, it may only
>> lead to possible performance degradation but is not so
>> critical that its using should be prohibited. So allow it
>> but also warn user about possible issue.
>
> We have reports about devices reacting badly to ZLPs.
> Unless you have a compelling reasons for this change
> I have to reject it.

What devices do you mean here: USB network adapters or USB host controllers?
If it's just network adapters, then we can create some kind of quirk handling
for them and do not disable this functionality just because some buggy device
can't work with it.
My device works fine with ZLPs so I want to use this particular MTU under Linux
like I do it under other operation systems.

Regards,
Ruslan


>
>         Regards
>                 Oliver
>
> NACK
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Oliver Neukum]

On Thu, 2015-05-07 at 13:51 +0300, Ruslan Bilovol wrote:
> Hi Oliver,
> 
> On Thu, May 7, 2015 at 1:11 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
> >> Current usbnet driver rejects setting MTU that is a multiple
> >> of USB endpoint's wMaxPacketSize size. However, it may only
> >> lead to possible performance degradation but is not so
> >> critical that its using should be prohibited. So allow it
> >> but also warn user about possible issue.
> >
> > We have reports about devices reacting badly to ZLPs.
> > Unless you have a compelling reasons for this change
> > I have to reject it.
> 
> What devices do you mean here: USB network adapters or USB host controllers?

The network adapters

> If it's just network adapters, then we can create some kind of quirk handling
> for them and do not disable this functionality just because some buggy device
> can't work with it.

No. They are too many.

> My device works fine with ZLPs so I want to use this particular MTU under Linux
> like I do it under other operation systems.

We can have a white list, but the general case is just too dangerous.

	Sorry
		Oliver

Re: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Sergei Shtylyov]

Hello.

On 5/7/2015 12:49 PM, Ruslan Bilovol wrote:

> Current usbnet driver rejects setting MTU that is a multiple
> of USB endpoint's wMaxPacketSize size. However, it may only
> lead to possible performance degradation but is not so
> critical that its using should be prohibited. So allow it
> but also warn user about possible issue.

> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> ---
>   drivers/net/usb/usbnet.c |    6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 733f4fe..09dc848 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -382,9 +382,11 @@ int usbnet_change_mtu (struct net_device *net, int new_mtu)
>
>   	if (new_mtu <= 0)
>   		return -EINVAL;
> -	// no second zero-length packet read wanted after mtu-sized packets
> +	/* warn about second zero-length packet read after mtu-sized packets */
>   	if ((ll_mtu % dev->maxpacket) == 0)
> -		return -EDOM;
> +		netdev_warn(dev->net, "MTU %d is a multiple of USB wMaxPacketSize (%d),"
> +				" consider possible performance degradation\n",

    Please do not wrap the kernel messages, it impedes grepping for them. 
scripts/checkpatch.pl is aware of this rule.

WBR, Sergei

RE: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1703 bytes --]

From: Oliver Neukum
> Sent: 07 May 2015 11:11
> On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
> > Current usbnet driver rejects setting MTU that is a multiple
> > of USB endpoint's wMaxPacketSize size. However, it may only
> > lead to possible performance degradation but is not so
> > critical that its using should be prohibited. So allow it
> > but also warn user about possible issue.
> 
> We have reports about devices reacting badly to ZLPs.
> Unless you have a compelling reasons for this change
> I have to reject it.

I don't remember seeing a fix for xhci to even send ZLP go through.
If the ZLP are sent then the device will behave badly.

The receive side code in usbnet also needs a lot of TLC.
(I don't remember anything major happening to it in the last couple
of years.)
I think it would be better if it didn't try to use skb for receive URB.
Instead it should supply URB that are always multiples of the USB
buffer size and then generate receive skb from the receive USB data.
This would require code that correctly processes ethernet frames that
span URB boundaries.
This would remove the need for the 16k+ sized skb used by some of the
sub-drivers and the typical lying about the 'true size'.

For xhci it really ought to be possible to remove a lot of code from the
tx and rx data paths and just write the buffer descriptors directly into
the ring entries (as is a typical ethernet driver).
That might get the performance (and software overhead) of USB3 Ge much
nearer that of a normal ethernet device.

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[David Laight]

From: Oliver Neukum
> Sent: 07 May 2015 11:11
> On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
> > Current usbnet driver rejects setting MTU that is a multiple
> > of USB endpoint's wMaxPacketSize size. However, it may only
> > lead to possible performance degradation but is not so
> > critical that its using should be prohibited. So allow it
> > but also warn user about possible issue.
> 
> We have reports about devices reacting badly to ZLPs.
> Unless you have a compelling reasons for this change
> I have to reject it.

I don't remember seeing a fix for xhci to even send ZLP go through.
If the ZLP are sent then the device will behave badly.

The receive side code in usbnet also needs a lot of TLC.
(I don't remember anything major happening to it in the last couple
of years.)
I think it would be better if it didn't try to use skb for receive URB.
Instead it should supply URB that are always multiples of the USB
buffer size and then generate receive skb from the receive USB data.
This would require code that correctly processes ethernet frames that
span URB boundaries.
This would remove the need for the 16k+ sized skb used by some of the
sub-drivers and the typical lying about the 'true size'.

For xhci it really ought to be possible to remove a lot of code from the
tx and rx data paths and just write the buffer descriptors directly into
the ring entries (as is a typical ethernet driver).
That might get the performance (and software overhead) of USB3 Ge much
nearer that of a normal ethernet device.

	David