[PATCH] usbnet: allow rx_process() to ignore packets

[PATCH] usbnet: allow rx_process() to ignore packets

[Ondrej Zary]

Allow rx_process() to ignore a packet without incrementing error counters if 
rx_fixup() returns value other than 0 or 1 (e.g. 2).

This allows to simplify rx_fixup() functions of drivers who do complex 
processing there. Currently, drivers must process the last packet in a 
special way - leave it for usbnet to process. This is not easily possible 
when a driver (like the new cx82310_eth) needs to process packets that cross 
URB (and thus skb) boundaries. With this patch, the driver can process all 
packets in the skb and just return 2 at the end.

Also fix asix driver that was returning 2 at one place before this change 
(probably by mistake).

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

--- linux-2.6.36-rc3-orig/drivers/net/usb/usbnet.c	2010-08-29 17:36:04.000000000 +0200
+++ linux-2.6.36-rc3/drivers/net/usb/usbnet.c	2010-09-04 23:47:14.000000000 +0200
@@ -385,18 +385,24 @@ static int rx_submit (struct usbnet *dev
 
 static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
 {
-	if (dev->driver_info->rx_fixup &&
-	    !dev->driver_info->rx_fixup (dev, skb))
-		goto error;
-	// else network stack removes extra byte if we forced a short packet
+	int fixup = 1;
 
-	if (skb->len)
-		usbnet_skb_return (dev, skb);
-	else {
-		netif_dbg(dev, rx_err, dev->net, "drop\n");
-error:
+	if (dev->driver_info->rx_fixup)
+		fixup = dev->driver_info->rx_fixup(dev, skb);
+
+	switch (fixup) {
+	case 1:	/* skb is correct */
+		if (skb->len) {
+			usbnet_skb_return(dev, skb);
+			break;
+		} else
+			netif_dbg(dev, rx_err, dev->net, "drop\n");
+		/* fall through */
+	case 0: /* skb is incorrect */
 		dev->net->stats.rx_errors++;
-		skb_queue_tail (&dev->done, skb);
+		/* fall through */
+	default: /* skb does not need processing */
+		skb_queue_tail(&dev->done, skb);
 	}
 }
 
--- linux-2.6.36-rc3-orig/drivers/net/usb/asix.c	2010-08-29 17:36:04.000000000 +0200
+++ linux-2.6.36-rc3/drivers/net/usb/asix.c	2010-09-04 23:48:42.000000000 +0200
@@ -341,7 +341,7 @@ static int asix_rx_fixup(struct usbnet *
 				skb->data -= realignment;
 				skb_set_tail_pointer(skb, size);
 			}
-			return 2;
+			return 1;
 		}
 
 		if (size > dev->net->mtu + ETH_HLEN) {


-- 
Ondrej Zary

Re: [PATCH] usbnet: allow rx_process() to ignore packets

[David Brownell]

--- On Sat, 9/4/10, Ondrej Zary <linux@rainbow-software.org> wrote:

> From: Ondrej Zary <linux@rainbow-software.org>
> Subject: [PATCH] usbnet: allow rx_process() to ignore packets

It already can ... I'm already not
liking this patch...

You've not convinced me this is even necessary.


> To: "David Brownell" <dbrownell@users.sourceforge.net>
> Cc: netdev@vger.kernel.org, "Kernel development list" <linux-kernel@vger.kernel.org>
> Date: Saturday, September 4, 2010, 2:52 PM
> Allow rx_process() to ignore a packet
> without incrementing error counters if 
> rx_fixup() returns value other than 0 or 1 (e.g. 2).
> 
> This allows to simplify rx_fixup() functions of drivers who
> do complex 
> processing there. Currently, drivers must process

Not many drivers.  Or even most.

 the last
> packet in a 
> special way - leave it for usbnet to process.

Don't you mean "clean up"?  The usbnet core knows
exactly zero about packet framing, 

Which in your scenario -- packet crosses URB
boundaries, can't be handled by usbnet at all,
since it's specific to the framing used by the
protocol the driver understands.

The way to handle such perversity (or is it bad
design ... just use large-enough RX urbs!

Or ... have the usbnet minidriver queue up the
packets it's got to re-assemble, and do that
work the next time rx_fixup() is called.  Very
straightforward, and doesn't affect the core
usbnet framework at all.

Better of course is to stick to the simple
framing model that places the least load on the
whole stack:  one Ethernet packet per URB...

This is not
> easily possible 
> when a driver (like the new cx82310_eth) needs to process
> packets that cross 
> URB (and thus skb) boundaries. With this patch, the driver
> can process all 
> packets in the skb and just return 2 at the end.

With "2" signifying just what?  And what's keeping
that routine from handing up multiple SKBs *NOW* ??

ISTR nothing is keeping it from doing that, since
the RNDIS code has done so forever.  (More evidence
that this change is not needed.)
> 
> Also fix asix driver that was returning 2 at one place
> before this change 
> (probably by mistake).


If that's worth fixing, it's worth doing it
in a patch by itself, instead of glommed into
an otherwise unrelated patch.  Does that really
break that driver?


> 
> 

Re: [PATCH] usbnet: allow rx_process() to ignore packets

[Ondrej Zary]

On Sunday 05 September 2010 01:24:46 David Brownell wrote:
> --- On Sat, 9/4/10, Ondrej Zary <linux@rainbow-software.org> wrote:
> > From: Ondrej Zary <linux@rainbow-software.org>
> > Subject: [PATCH] usbnet: allow rx_process() to ignore packets
>
> It already can ... I'm already not
> liking this patch...

How it can? Currently, rx_process() knows only two cases: either rx_fixup() 
returns 0 or a non-zero value. If I return 0, the error counter is 
incremented. If I return non-zero value, packet is processed ("passed up the 
stack" - usbnet_skb_return() called) if the skb has non-zero length, 
otherwise the error counter is incremented. There's no way to not pass the 
packet up the stack without incrementing the error counter.

> You've not convinced me this is even necessary.
>
> > To: "David Brownell" <dbrownell@users.sourceforge.net>
> > Cc: netdev@vger.kernel.org, "Kernel development list"
> > <linux-kernel@vger.kernel.org> Date: Saturday, September 4, 2010, 2:52 PM
> > Allow rx_process() to ignore a packet
> > without incrementing error counters if
> > rx_fixup() returns value other than 0 or 1 (e.g. 2).
> >
> > This allows to simplify rx_fixup() functions of drivers who
> > do complex
> > processing there. Currently, drivers must process
>
> Not many drivers.  Or even most.
>
>  the last
>
> > packet in a
> > special way - leave it for usbnet to process.
>
> Don't you mean "clean up"?  The usbnet core knows
> exactly zero about packet framing,

I mean "pass up the stack".

> Which in your scenario -- packet crosses URB
> boundaries, can't be handled by usbnet at all,
> since it's specific to the framing used by the
> protocol the driver understands.
>
> The way to handle such perversity (or is it bad
> design ... just use large-enough RX urbs!

I cannot change the HW. cx82310 merges the packets in URBs and does not care 
about URB boundaries. If a packet does not fit the URB (4KB), it simply 
continues in the next URB (without any header).

> Or ... have the usbnet minidriver queue up the
> packets it's got to re-assemble, and do that
> work the next time rx_fixup() is called.  Very
> straightforward, and doesn't affect the core
> usbnet framework at all.

That's exactly what I'm already doing. When the packet is not complete, I copy 
the partial data. And return what? I cannot return 1 as the packet is not 
complete. If I return 0, the error counter gets incremented even if this is 
not an error condition. This is why this patch was created.

The cx82310_eth (mini)driver already works
(see http://www.spinics.net/lists/netdev/msg139950.html)
and the only problem I can see are the error statistics.

> Better of course is to stick to the simple
> framing model that places the least load on the
> whole stack:  one Ethernet packet per URB...

The device probably tries to maximize the throughput by merging everything it 
can.

> This is not
>
> > easily possible
> > when a driver (like the new cx82310_eth) needs to process
> > packets that cross
> > URB (and thus skb) boundaries. With this patch, the driver
> > can process all
> > packets in the skb and just return 2 at the end.
>
> With "2" signifying just what?  And what's keeping
> that routine from handing up multiple SKBs *NOW* ??

"2" means that rx_process() should neither call usbnet_skb_return() nor 
increment the error counter.

> ISTR nothing is keeping it from doing that, since
> the RNDIS code has done so forever.  (More evidence
> that this change is not needed.)

RNDIS processes multiple packets per skb but not packets that cross skb 
boundaries.
cdc_eem, smsc75xx, smsc95xx and also rndis_host treat last packet differently 
just because usbnet need to pass it up the stack by itself.

> > Also fix asix driver that was returning 2 at one place
> > before this change
> > (probably by mistake).
>
> If that's worth fixing, it's worth doing it
> in a patch by itself, instead of glommed into
> an otherwise unrelated patch.  Does that really
> break that driver?

When semantic of "return 2" changes, the asix driver would be doing something 
other that it's doing now. I don't want to break anything so the driver 
should be fixed at the same time (or before).

-- 
Ondrej Zary

Re: [PATCH] usbnet: allow rx_process() to ignore packets

[David Brownell]

> > > From: Ondrej Zary <linux@rainbow-software.org>
> > > Subject: [PATCH] usbnet: allow rx_process() to
> ignore packets
> >
> > It already can ... I'm already not
> > liking this patch...

You didn't explain why "ignore".  As a rule, if
the network peer is sending garbage, that needs
to be accounted as an error, not igored.  You seem
to be complaining about accounting garbage as such.

 rx_process() knows only two cases:
> either rx_fixup() 
> returns 0 or a non-zero value. If I return 0,
>  the error counter is  incremented.

So don't return zero, when you're not trying to
indicate an error. ... easy.


> If I return non-zero value, packet is
> processed ("passed up the 
> stack" - usbnet_skb_return() called)
> if the skb has non-zero length,

Exactly -- that's how the minidriver says that
it stripped framing off the packet, so other
code should pass the packet up the stack.


Have you tried emptying the SKB (len zero) to
indicate you've consumed all of its contents?
(Or in your case, "ignored").  That would seem to
be more like what you want to do ...  ISTR that the
network stack cleanly handles empty SKBs; if not,
maybe it should.

There's no way
> to not pass the 
> packet up the stack without incrementing the error
> counter.

   If rx_fixup leaves a single packet in SKB, that
gets passed up the stack. and not treated as any
kind of error.



> 
> > You've not convinced me this is even necessary.
> >

Re: [PATCH] usbnet: allow rx_process() to ignore packets

[Ondrej Zary]

On Sunday 05 September 2010 23:35:15 David Brownell wrote:
> > > > From: Ondrej Zary <linux@rainbow-software.org>
> > > > Subject: [PATCH] usbnet: allow rx_process() to
> >
> > ignore packets
> >
> > > It already can ... I'm already not
> > > liking this patch...
>
> You didn't explain why "ignore".  As a rule, if
> the network peer is sending garbage, that needs
> to be accounted as an error, not igored.  You seem
> to be complaining about accounting garbage as such.

It's not a garbage, just a packet that is not yet complete.

>  rx_process() knows only two cases:
> > either rx_fixup()
> > returns 0 or a non-zero value. If I return 0,
> >  the error counter is  incremented.
>
> So don't return zero, when you're not trying to
> indicate an error. ... easy.

If I return 1, the incomplete packet would be passed up the stack.

> > If I return non-zero value, packet is
> > processed ("passed up the
> > stack" - usbnet_skb_return() called)
> > if the skb has non-zero length,
>
> Exactly -- that's how the minidriver says that
> it stripped framing off the packet, so other
> code should pass the packet up the stack.
>
>
> Have you tried emptying the SKB (len zero) to
> indicate you've consumed all of its contents?
> (Or in your case, "ignored").  That would seem to
> be more like what you want to do ...  ISTR that the
> network stack cleanly handles empty SKBs; if not,
> maybe it should.

Yes, I have tried it - in fact, this is that cx82310_eth does now. It does not 
work because rx_process() in usbnet.c checks if the skb is empty - and 
increments the error counter if it is. Maybe it should not?

-- 
Ondrej Zary

[PATCH v2] usbnet: do not count empty skbs as errors in rx_process()

[Ondrej Zary]

If rx_fixup() returns an empty skb (because it consumed all data inside), do
not count it as error.

This is needed for cx82310_eth. It also may allow other usbnet minidrivers'
rx_fixup() functions to be simplified (no need to leave the last packet in the
skb anymore).

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
The patch does not pass checkpatch.pl because the function calls match the
style used in usbnet.c.

--- linux-2.6.36-rc3-orig/drivers/net/usb/usbnet.c	2010-08-29 17:36:04.000000000 +0200
+++ linux-2.6.36-rc3/drivers/net/usb/usbnet.c	2010-09-10 20:10:10.000000000 +0200
@@ -387,17 +387,11 @@
 {
 	if (dev->driver_info->rx_fixup &&
 	    !dev->driver_info->rx_fixup (dev, skb))
-		goto error;
-	// else network stack removes extra byte if we forced a short packet
-
-	if (skb->len)
-		usbnet_skb_return (dev, skb);
-	else {
-		netif_dbg(dev, rx_err, dev->net, "drop\n");
-error:
 		dev->net->stats.rx_errors++;
-		skb_queue_tail (&dev->done, skb);
-	}
+	else if (skb->len)
+		return usbnet_skb_return (dev, skb);
+
+	skb_queue_tail (&dev->done, skb);
 }
 
 /*-------------------------------------------------------------------------*/


-- 
Ondrej Zary

Re: [PATCH v2] usbnet: do not count empty skbs as errors in rx_process()

[David Brownell]

--- On Fri, 9/10/10, Ondrej Zary <linux@rainbow-software.org> wrote:

> From: Ondrej Zary <linux@rainbow-software.org>
> Subject: [PATCH v2] usbnet: do not count empty skbs as errors in rx_process()

NAK to this backwards-incompatible change.

At this point there's no way to know how many
drivers it breaks ... I do know that counting
such SKBs as errors has previously turned up
link-level errors, and thus led to bugfixes.


> If rx_fixup() returns an empty skb
> (because it consumed all data inside),

The canonical reason the SKB would be empty is
because after rx_fixup() removed all header and
trailer data, there was no packet body left.
Which is likely a link error, and has been
worth accounting as such in the past (turned up
bugs on TX or RX sides).

> do not count it as error.
> 
> This is needed for cx82310_eth.

I'd far rather see that driver fixed, than see
the core usbnet framework broken to avoid such
fixes to a very new driver ...

Re: [PATCH v2] usbnet: do not count empty skbs as errors in rx_process()

[Ondrej Zary]

On Saturday 11 September 2010 21:07:59 David Brownell wrote:
> --- On Fri, 9/10/10, Ondrej Zary <linux@rainbow-software.org> wrote:
> > From: Ondrej Zary <linux@rainbow-software.org>
> > Subject: [PATCH v2] usbnet: do not count empty skbs as errors in
> > rx_process()
>
> NAK to this backwards-incompatible change.
>
> At this point there's no way to know how many
> drivers it breaks ... I do know that counting
> such SKBs as errors has previously turned up
> link-level errors, and thus led to bugfixes.
>
> > If rx_fixup() returns an empty skb
> > (because it consumed all data inside),
>
> The canonical reason the SKB would be empty is
> because after rx_fixup() removed all header and
> trailer data, there was no packet body left.
> Which is likely a link error, and has been
> worth accounting as such in the past (turned up
> bugs on TX or RX sides).
>
> > do not count it as error.
> >
> > This is needed for cx82310_eth.
>
> I'd far rather see that driver fixed, than see
> the core usbnet framework broken to avoid such
> fixes to a very new driver ...

I already tried to explain that the driver is not broken and so it cannot be 
fixed. It's the way the hardware works and usbnet is not ready for it. I see 
no way to process a packet that crosses URBs (and thus SKBs) without changing 
usbnet.

-- 
Ondrej Zary

Re: [PATCH v2] usbnet: do not count empty skbs as errors in rx_process()

[David Brownell]

--- On Sat, 9/11/10, Ondrej Zary wrote:

> Date: Saturday, September 11, 2010, 1:22 PM
> On Saturday 11 September 2010
> 21:07:59 David Brownell wrote:
> 
> > NAK to this backwards-incompatible change.
> >
> > >
> > > This is needed for cx82310_eth.

Backwards-incompatible changes are NOT "needed"
> >
> > I'd far rather see that driver fixed, than see
> > the core usbnet framework broken to avoid such
> > fixes to a very new driver ...
> 
> I already tried to explain that the driver is not broken

Yet you keep saying it doesn't work right, which
is the classic definition of "broken, needs fix".


> and so it cannot be 
> fixed. It's the way the hardware works and usbnet
> is not > ready for it.

And when we went over this before, I said how
to resolve this:   a *BACKWARD-COMPATIBLE change
that adds a new return state to rx_fixup(), used
in the cx82310 code.  Why didn't you try that?

This proposed patch doesn't do it, since it punts
on backwards compatibility ... which is a HUGE issue
when we can't easily test all the permutations.
Best to not risk breaking things; and in this case
there's not even any need to take such risks.

- Dave

Re: [PATCH v2] usbnet: do not count empty skbs as errors in rx_process()

[Ondrej Zary]

On Saturday 11 September 2010 23:15:29 David Brownell wrote:
> --- On Sat, 9/11/10, Ondrej Zary wrote:
> > Date: Saturday, September 11, 2010, 1:22 PM
> > On Saturday 11 September 2010
> >
> > 21:07:59 David Brownell wrote:
> > > NAK to this backwards-incompatible change.
> > >
> > > > This is needed for cx82310_eth.
>
> Backwards-incompatible changes are NOT "needed"
>
> > > I'd far rather see that driver fixed, than see
> > > the core usbnet framework broken to avoid such
> > > fixes to a very new driver ...
> >
> > I already tried to explain that the driver is not broken
>
> Yet you keep saying it doesn't work right, which
> is the classic definition of "broken, needs fix".
>
> > and so it cannot be
> > fixed. It's the way the hardware works and usbnet
> > is not > ready for it.
>
> And when we went over this before, I said how
> to resolve this:   a *BACKWARD-COMPATIBLE change
> that adds a new return state to rx_fixup(), used
> in the cx82310 code.  Why didn't you try that?

Something like the first patch that started this thread?

-- 
Ondrej Zary

Re: [PATCH] usbnet: allow rx_process() to ignore packets

[David Brownell]

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

--- On Tue, 9/7/10, Ondrej Zary <linux@rainbow-software.org> wrote:
> 
> It's not a garbage, just a packet that is not yet
> complete.

Sorry, I guess I was being dense.

I thought packets were by definition complete... :)

I think you're saying this is one of N fragments
which the minidriver will be re-assembling into
a bigger packet, before somehow passing it up
the stack...

I attach my doc update patch.  (should be the
same as what I sent before.  Consider my apology
for using an attachment as repeated here; my email
setup remains partly broken).  Can you send
a version of your defrag-enabling patch which
applies on top of it, and documents your new
calling convention (with a third return case
for rx_process() too?

[-- 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) */