PATCH: usbnet: doc updates

PATCH: usbnet: doc updates

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 135 bytes --]

Apologies in advance for sending this as an
attachment, but I think it's less likely to get
angled (except quoted-printable?) this way.

[-- Attachment #2: rxfix --]
[-- Type: application/octet-stream, Size: 2461 bytes --]

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Subject: usbnet: doc updates

Provide more documentation about the rx_fixup() calling convention,
to help reduce some recently-observed confusion.

Also summarize the equivalence of USB and network TX/RX queues, to
help highlight the roles of the fixup() routines with link protocols
using the CPU-intensive packet batching models.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---


 include/linux/usb/usbnet.h |   30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -22,8 +22,20 @@
 #ifndef	__LINUX_USB_USBNET_H
 #define	__LINUX_USB_USBNET_H
 
-/* interface from usbnet core to each USB networking link we handle */
+/** interface from usbnet core to each USB networking link we handle
+ *
+ * Note that the design center for usbnet has one IEEE packet per URB, and
+maps network TX and RX queues directly to the USB hardware TX/RX queues,
+ * , minimizing software and hardware overhead.  Some link protocols
+ * (like RNDIS and CDC NCM) promote  less efficient designs which involve
+ * batching multiple IEEE packets in each URB, with increased TX overhead
+to copy IEEE packets into TX URBs' data buffer in tx_fixup() routines,
+plus unbatching work in rx_fixup() as well.  Sometimes the TX overhead can
+be minimized on Linux by not batching, but this usually won't work for RX
+because of the need to interoperate with implementations which batch on TX.
+ */
 struct usbnet {
+
 	/* housekeeping */
 	struct usb_device	*udev;
 	struct usb_interface	*intf;
@@ -121,7 +133,21 @@ struct driver_info {
 	/* link reset handling, called from defer_kevent */
 	int	(*link_reset)(struct usbnet *);
 
-	/* fixup rx packet (strip framing) */
+	/* fixup rx packet ( by stripping framing from URB/SKB.
+	 *
+	 - If SKB batches one or more packets, pass all but one of them
+	   up the stack (clone SKB and fixup each clone before netif_rx)).
+
+
+
+	 - When you leave one packet in SKB (intended usage), return
+	 nonzero and ensure skb-len is nonzero.  the packet will be passed
+	 up the network stack and accounted properly.
+
+	- return zero to indicate errors such as too much
+	header or data, or a bad checksum; the packet will be dropped
+	and accounted as an error.
+	*/
 	int	(*rx_fixup)(struct usbnet *dev, struct sk_buff *skb);
 
 	/* fixup tx packet (add framing) */

Re: PATCH: usbnet: doc updates

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Sun, 5 Sep 2010 17:06:10 -0700 (PDT)

> Apologies in advance for sending this as an
> attachment, but I think it's less likely to get
> angled (except quoted-printable?) this way.

Well, patchwork didn't grok it at all, and this means your patch is a
lot more work for me.

Actually looking at the patch, the comment formatting is not all
that pretty:

+ * Note that the design center for usbnet has one IEEE packet per URB, and
+maps network TX and RX queues directly to the USB hardware TX/RX queues,

Please start every line of the comment with the
proper leading " * " rather than going straight to
the text.

+	/* fixup rx packet ( by stripping framing from URB/SKB.
+	 *
+	 - If SKB batches one or more packets, pass all but one of them
+	   up the stack (clone SKB and fixup each clone before netif_rx)).

Please also do the same here.

Re: PATCH: usbnet: doc updates

[David Brownell]

--- On Sun, 9/5/10, David Miller <davem@davemloft.net> wrote:

> > Apologies in advance for sending this as an
> > attachment, but I think it's less likely to get
> > angled (except quoted-printable?) this way.
> 
> Well, patchwork didn't grok it at all, and this means your
> patch is a
> lot more work for me.

More than having to fix up a patch that's had
tabs morphed to spaces, and line-endings broken?

I'm sending a new version in a followup, direct
to you (and list) which I hope will be easier to
cope with.


> 
> Actually looking at the patch, the comment formatting is
> not all
> that pretty:
> 
> + * Note that the design center for usbnet has one IEEE
> packet per URB, and
> +maps network TX and RX queues directly to the USB hardware
> TX/RX queues,
> 
> Please start every line of the comment with the
> proper leading " * " rather than going straight to
> the text.
> 

I guess you don't like

Re: PATCH: usbnet: doc updates

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

--- On Sun, 9/5/10, David Miller <davem@davemloft.net> wrote:

> From: David Miller <davem@davemloft.net>
> Subject: Re: PATCH: usbnet: doc updates
> To: david-b@pacbell.net
> Cc: netdev@vger.kernel.org, linux@rainbow-software.org
> Date: Sunday, September 5, 2010, 5:12 PM
> From: David Brownell <david-b@pacbell.net>
> Date: Sun, 5 Sep 2010 17:06:10 -0700 (PDT)
> 
> > Apologies in advance for sending this as an
> > attachment, but I think it's less likely to get
> > angled (except quoted-printable?) this way.
> 
> Well, patchwork didn't grok it at all, and this means your
> patch is a
> lot more work for me.

If I had my normal mail access, I'd send this
patch inline; but I don't.


> 
> Actually looking at the patch, the comment formatting is
> not all
> that pretty:


I guess you don't like

    /*
     preformatted text ...
    */

as a comment style.  Fixed in the attached.

[-- Attachment #2: rxfix --]
[-- Type: application/octet-stream, Size: 2492 bytes --]

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Subject: usbnet: doc updates

Provide more documentation about the rx_fixup() calling convention,
to help reduce some recently-observed confusion.

Also summarize the equivalence of USB and network TX/RX queues, to
help highlight the roles of the fixup() routines with link protocols
using the CPU-intensive packet batching models.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---


 include/linux/usb/usbnet.h |   31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -22,8 +22,22 @@
 #ifndef	__LINUX_USB_USBNET_H
 #define	__LINUX_USB_USBNET_H
 
-/* interface from usbnet core to each USB networking link we handle */
+/** interface from usbnet core to each USB networking link we handle.
+ *
+ * Note that the design center for usbnet has one IEEE packet per URB,
+ * and maps network TX and RX queues directly to the
+ * USB hardware TX/RX queues, minimizing software and hardware overhead.
+ * Some link protocols (like RNDIS and CDC NCM) promote  less efficient
+ * designs which involve batching multiple IEEE packets in each URB,
+ * with increased TX overhead to copy IEEE packets into TX URBs' data
+ * buffers in tx_fixup() routines, plus unbatching work in rx_fixup() too.
+ * Sometimes the TX overhead can be minimized on Linux by not batching, but
+ * this usually won't work for RX because of the need to interoperate with
+ * implementations which batch on TX.
+ */
+
 struct usbnet {
+
 	/* housekeeping */
 	struct usb_device	*udev;
 	struct usb_interface	*intf;
@@ -113,7 +127,20 @@ struct driver_info {
 	/* link reset handling, called from defer_kevent */
 	int	(*link_reset)(struct usbnet *);
 
-	/* fixup rx packet (strip framing) */
+	/* fixup rx packet ( by stripping framing from URB/SKB.
+	 *
+	 * If SKB batches one or more packets, pass all but one of them
+	 * up the stack (clone SKB and fixup each clone before netif_rx)).
+	 *
+
+	 * When you leave one packet in SKB (intended usage), return
+	 * nonzero and ensure skb-len is nonzero.  the packet will be
+	 * passed up the network stack and accounted properly.
+	 *
+	 * return zero to indicate errors such as too much header or
+	 * data, or a bad checksum; the packet will be dropped
+	 * and accounted as an error.
+	*/
 	int	(*rx_fixup)(struct usbnet *dev, struct sk_buff *skb);
 
 	/* fixup tx packet (add framing) */

Re: PATCH: usbnet: doc updates

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Sun, 5 Sep 2010 21:48:01 -0700 (PDT)

> If I had my normal mail access, I'd send this
> patch inline; but I don't.

The problem is that you set the content type to octet stream with
base64 encoding, which patchwork can't handle.

Set it to something like text/plain with UTF8 encoding, and all should
be well.

Thanks.

Re: PATCH: usbnet: doc updates

[David Brownell]

--- On Sun, 9/5/10, David Miller <davem@davemloft.net> wrote:

> From: David Brownell <david-b@pacbell.net>
> Date: Sun, 5 Sep 2010 21:48:01 -0700 (PDT)
> 
> > If I had my normal mail access, I'd send this
> > patch inline; but I don't.
> 
> The problem is that you set the content type to octet stream with
> base64 encoding,

I did no such thing; the mailer must have that
hard-wired.  The only option I have is whether
to send HTML mail (never!!) or plain text.

I would really like to have something like my
normail email setup back again... but too many
key machines died at the same time, and putting
such a  setup back together is painful.

You wouldn't know of a Linux distro that comes
with a "this box will be a NAT firewall" setup
option?  Ubuntu only has that aftermarket AFAICT.

Re: PATCH: usbnet: doc updates

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Mon, 6 Sep 2010 11:28:48 -0700 (PDT)

> You wouldn't know of a Linux distro that comes
> with a "this box will be a NAT firewall" setup
> option?  Ubuntu only has that aftermarket AFAICT.

Just do a bare debian install and put something like the file below in
your /etc/network/interfaces, it's what I use.

eth0 is outgoing, eth1 is internal network.  Change eth0 to a dhcp
config if that is what you use.

I also use dnsmasq as the dhcp server for the internal network.

IP addresses variable'ized to protect the innocent :-)

--------------------
# This file describes the network interfaces available on your system
# and how to activate them. For more information, see interfaces(5).

# The loopback network interface
auto lo
iface lo inet loopback

# The primary network interface
allow-hotplug eth0
iface eth0 inet static
      address $(INTERNET_IP)
      netmask $(INTERNET_NETMASK)
      network $(INTERNET_NETWORK)
      broadcast $(INTERNET_BROADCAST)
      gateway $(INTERNET_GATEWAY)
      dns-nameservers $(INTERNET_DNS_1) $(INTERNET_DNS_2)

allow-hotplug eth1
iface eth1 inet static
      address 11.0.0.1
      netmask 255.0.0.0
      network 11.0.0.0
      broadcast 11.255.255.255
      up iptables -t nat -A POSTROUTING -o eth0 -j MASQUERADE