| Message ID | 201009102336.00722.linux@rainbow-software.org |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
--- 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 ... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
--- 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
--- 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); } /*-------------------------------------------------------------------------*/
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.