usbnet: allow rx_process() to ignore packets
PATCH 1/1 usbnet: allow rx_process() to ignore packets
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).
drivers/net/usb/usbnet.c +16 -10
@@ -385,18 +385,24 @@ static int rx_submit (struct usbnet *devstatic 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); } }
drivers/net/usb/asix.c +1 -1
@@ -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 ) {
Signature · 1 line
Ondrej Zary
Quoted 2 lines · Ondrej Zary
From: Ondrej Zary <[email protected]>
Subject: [PATCH] usbnet: allowrx_process()to ignore packets
It already can ... I'm already not
liking this patch...
You've not convinced me this is even necessary.
Quoted 10 lines
To: "David Brownell" <[email protected]>
Cc: [email protected], "Kernel development list" <[email protected]>
Date: Saturday, September 4, 2010, 2:52 PM
Allowrx_process()to ignore a packet
without incrementing error counters ifrx_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
Quoted 2 lines
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
Quoted 6 lines
easily possible
when a driver (like the newcx82310_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.)
Quoted 4 lines
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?
Quoted 2 lines
Quoted 6 lines · On Sunday 05 September 2010 01:24:46 David Brownell wrote:
Quoted 2 lines · Ondrej Zary
From: Ondrej Zary <[email protected]>
Subject: [PATCH] usbnet: allowrx_process()to ignore packetsIt 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.
Quoted 15 lines
You've not convinced me this is even necessary.
Quoted 10 lines
To: "David Brownell" <[email protected]>
Cc: [email protected], "Kernel development list" <[email protected]> Date: Saturday, September 4, 2010, 2:52 PM Allowrx_process()to ignore a packet
without incrementing error counters ifrx_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 processNot many drivers. Or even most.
Quoted 7 lines
the last
Quoted 2 lines
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".
Quoted 7 lines
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).
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 11 lines
This is not
Quoted 6 lines
easily possible
when a driver (like the newcx82310_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.
Quoted 3 lines
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.
Quoted 8 lines
Quoted 3 lines
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).
Signature · 1 line
Ondrej Zary
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
From: Ondrej Zary <[email protected]>
Subject: [PATCH] usbnet: allowrx_process()toignore packets
Quoted 3 lines
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:
Quoted 3 lines
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.
Quoted 4 lines
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
Quoted 3 lines
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.
Quoted 3 lines
Quoted 3 lines
You've not convinced me this is even necessary.
Quoted 5 lines · On Sunday 05 September 2010 23:35:15 David Brownell wrote:
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
From: Ondrej Zary <[email protected]>
Subject: [PATCH] usbnet: allowrx_process()toignore packets
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 7 lines
rx_process()knows only two cases:Quoted 3 lines
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.
Quoted 10 lines
Quoted 4 lines
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.
Quoted 6 lines
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?
Signature · 1 line
Ondrej Zary
PATCH v2 1/1 usbnet: do not count empty skbs as errors in rx_process()
In reply to David BrownellIf 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).
---
The patch does not pass checkpatch.pl because the function calls match the style used in usbnet.c.
drivers/net/usb/usbnet.c +4 -10
@@ -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); }/*-------------------------------------------------------------------------*/
Signature · 1 line
Ondrej Zary
Re: [PATCH v2] usbnet: do not count empty skbs as errors in rx_process()
Quoted 2 lines · Ondrej Zary
From: Ondrej Zary <[email protected]>
Subject: [PATCH v2] usbnet: do not count empty skbs as errors inrx_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.
Quoted 2 lines
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).
Quoted 3 lines
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()
Quoted 22 lines · On Saturday 11 September 2010 21:07:59 David Brownell wrote:
Quoted 3 lines · Ondrej Zary
From: Ondrej Zary <[email protected]>
Subject: [PATCH v2] usbnet: do not count empty skbs as errors inrx_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.
Quoted 2 lines
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).
Quoted 7 lines
Quoted 3 lines
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.
Signature · 1 line
Ondrej Zary
Re: [PATCH v2] usbnet: do not count empty skbs as errors in rx_process()
Quoted 8 lines · Ondrej Zary
Date: Saturday, September 11, 2010, 1:22 PM
Quoted 4 lines · On Saturday 11 September 2010 21:07:59 David Brownell wrote:
NAK to this backwards-incompatible change.
Quoted 2 lines
This is needed for
cx82310_eth.
Backwards-incompatible changes are NOT "needed"
Quoted 6 lines
Quoted 4 lines
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".
Quoted 3 lines
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()
Quoted 20 lines · On Saturday 11 September 2010 23:15:29 David Brownell wrote:
Quoted 7 lines · Ondrej Zary
Date: Saturday, September 11, 2010, 1:22 PM On Saturday 11 September 2010
Quoted 3 lines · 21:07:59 David Brownell wrote:
NAK to this backwards-incompatible change.
Quoted 1 line
This is needed for
cx82310_eth.Backwards-incompatible changes are NOT "needed"
Quoted 5 lines
Quoted 3 lines
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".
Quoted 8 lines
Quoted 3 lines
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 torx_fixup(), used
in the cx82310 code. Why didn't you try that?
Something like the first patch that started this thread?
Signature · 1 line
Ondrej Zary