| Message ID | 6020948.EvS2esPpc0@linux-lqwf.site |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
> -----Original Message----- > From: Oliver Neukum [mailto:oneukum@suse.de] > Sent: Friday, September 21, 2012 10:35 AM > To: Alexey ORISHKO; bjorn@mork.no; netdev@vger.kernel.org; linux- > usb@vger.kernel.org > Subject: removing the timer from cdc-ncm > > Hi, > > here is the patch that does everything I consider theoretically > necessary to have bundling of frames in usbnet and adapting cdc-ncm to > it. > > I'd appreciate any review in case I am doing something stupid. Hi Oliver, I've tried to apply it, but always got a message that "patch does not apply". What is the base for this patch? Regards, Alexey -- 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 Monday 24 September 2012 14:13:50 Alexey ORISHKO wrote: > I've tried to apply it, but always got a message that "patch does not apply". > What is the base for this patch? Greg KH's tree, specifically commit 6dab7ede9390d4d937cb89feca932e4fd575d2da HTH Oliver -- 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
> -----Original Message----- > From: Oliver Neukum [mailto:oneukum@suse.de] > > here is the patch that does everything I consider theoretically > necessary to have bundling of frames in usbnet and adapting cdc-ncm to > it. > > I'd appreciate any review in case I am doing something stupid. > I had a brief look at cdc_ncm and a few corrections needed: - remove the following: #include <linux/hrtimer.h> ... /* Restart the timer, if amount of datagrams is less than given value */ #define CDC_NCM_RESTART_TIMER_DATAGRAM_CNT 3 #define CDC_NCM_TIMER_PENDING_CNT 2 #define CDC_NCM_TIMER_INTERVAL (400UL * NSEC_PER_USEC) ... In struct cdc_ncm_ctx { ... struct hrtimer tx_timer; struct tasklet_struct bh; ... In cdc_ncm_unbind(): if (hrtimer_active(&ctx->tx_timer)) hrtimer_cancel(&ctx->tx_timer); tasklet_kill(&ctx->bh); I didn't have time to check the new logic for data path, but I've tried to run it on Ubuntu 12.04. Linux host got panic right after data path has been established (i.e. connected to mobile network). Regards, Alexey -- 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 Tuesday 25 September 2012 13:18:10 Alexey ORISHKO wrote: > > -----Original Message----- > > From: Oliver Neukum [mailto:oneukum@suse.de] > > > > here is the patch that does everything I consider theoretically > > necessary to have bundling of frames in usbnet and adapting cdc-ncm to > > it. > > > > I'd appreciate any review in case I am doing something stupid. > > > > I had a brief look at cdc_ncm and a few corrections needed: > - remove the following: > #include <linux/hrtimer.h> > ... > /* Restart the timer, if amount of datagrams is less than given value */ > #define CDC_NCM_RESTART_TIMER_DATAGRAM_CNT 3 > #define CDC_NCM_TIMER_PENDING_CNT 2 > #define CDC_NCM_TIMER_INTERVAL (400UL * NSEC_PER_USEC) > ... > In struct cdc_ncm_ctx { > ... > struct hrtimer tx_timer; > struct tasklet_struct bh; > ... > > In cdc_ncm_unbind(): > if (hrtimer_active(&ctx->tx_timer)) > hrtimer_cancel(&ctx->tx_timer); > > tasklet_kill(&ctx->bh); Roger > I didn't have time to check the new logic for data path, but I've > tried to run it on Ubuntu 12.04. > Linux host got panic right after data path has been established > (i.e. connected to mobile network). Thank you. Worse than I hoped, but not unexpected. I'll stare at the code a bit. Regards Oliver -- 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
> -----Original Message----- > From: Oliver Neukum [mailto:oneukum@suse.de] > Thank you. Worse than I hoped, but not unexpected. I'll stare at the > code a bit. Just a small clarification: on Ellisys trace I see only ConnectionSpeedChange and NetworkConnection (Connected). No data was sent on the bulk pipe before it crashed. /alexey -- 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
Oliver Neukum <oneukum@suse.de> writes: > On Tuesday 25 September 2012 13:18:10 Alexey ORISHKO wrote: > >> Linux host got panic right after data path has been established >> (i.e. connected to mobile network). > > Thank you. Worse than I hoped, but not unexpected. I'll stare at the > code a bit. I had the same experience, but thought I was going to look a bit more at it before sending anything and forgot all about it. So this time I am just sending you my preliminary results instead of waiting. I am running with your initial patch, Alexeys suggested cleanup, and some additional debug printks. The driver receives a few ARPs from the device, but crashes on the very first outgoing dhcp packet (which is dumped with a ">>" prefix from cdc_ncm_tx_bundle just before calling cdc_ncm_fill_tx_frame): [48880.037638] cdc_ncm: wwan0: network connection: connected [48880.044038] IPv6: ADDRCONF(NETDEV_CHANGE): wwan0: link becomes ready [48880.048351] >> 00000000: ff ff ff ff ff ff 02 80 37 ec 02 00 08 00 45 10 ........7.....E. [48880.048361] >> 00000010: 01 48 00 00 00 00 80 11 39 96 00 00 00 00 ff ff .H......9....... [48880.048365] >> 00000020: ff ff 00 44 00 43 01 34 9e fd 01 01 06 00 9e 31 ...D.C.4.......1 [48880.048370] >> 00000030: 64 26 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d&.............. [48880.048374] >> 00000040: 00 00 00 00 00 00 02 80 37 ec 02 00 00 00 00 00 ........7....... [48880.048378] >> 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048382] >> 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048386] >> 00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048390] >> 00000080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048395] >> 00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048398] >> 000000a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048406] >> 000000b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048411] >> 000000c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048414] >> 000000d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048418] >> 000000e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048422] >> 000000f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048427] >> 00000100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048431] >> 00000110: 00 00 00 00 00 00 63 82 53 63 35 01 01 37 07 01 ......c.Sc5..7.. [48880.048435] >> 00000120: 1c 02 03 0f 06 0c ff 00 00 00 00 00 00 00 00 00 ................ [48880.048439] >> 00000130: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048444] >> 00000140: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [48880.048448] >> 00000150: 00 00 00 00 00 00 ...... [48880.048453] cdc_ncm_fill_tx_frame: ctx=ffff880162c97600, skb=ffff88018237bac0 [48880.048494] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068 [48880.048573] IP: [<ffffffffa06ba879>] cdc_ncm_tx_bundle+0x168/0x43b [cdc_ncm] [48880.048638] PGD 0 [48880.048663] Oops: 0000 [#1] SMP [48880.048702] Modules linked in: cdc_wdm cdc_ncm(O) netconsole configfs usbnet(O) mii cdc_acm usbhid hid option usb_storage uas nfsv3 nfsv4 auth_rpcgss udf crc_itu_t xt_multiport iptable_filter ip_tables cpufreq_userspace cpufreq_stats cpufreq_conservative cpufreq_powersave rfcomm bnep xt_hl binfmt_misc ip6table_filter ip6_tables x_tables fuse nfsd nfs_acl nfs lockd fscache sunrpc 8021q garp stp llc tun ext2 loop snd_hda_codec_conexant snd_hda_intel snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss iTCO_wdt snd_pcm iTCO_vendor_support thinkpad_acpi nvram snd_page_alloc snd_seq_midi snd_seq_midi_event snd_rawmidi arc4 iwldvm mac80211 snd_seq snd_timer snd_seq_device qcserial usb_wwan coretemp kvm_intel usbserial uvcvideo videobuf2_vmalloc btusb kvm videobuf2_memops videobuf2_core bluetooth psmouse i2c_i801 serio_raw videodev evdev crc16 lpc_ich acpi_cpufreq mfd_core ac battery snd iwlwifi mperf wmi i915 cfg80211 rfkill video processor button drm_kms_helper drm soundcore mei i2c_algo_bit i2c_core ext3 mbcache jbd sha256_generic ablk_helper cryptd aes_x86_64 aes_generic cbc dm_crypt dm_mod nbd sg sd_mod sr_mod crc_t10dif cdrom microcode thermal thermal_sys uhci_hcd ahci ehci_hcd libahci libata e1000e scsi_mod usbcore usb_common [last unloaded: cdc_ncm] [48880.050129] CPU 1 [48880.050151] Pid: 5467, comm: dhclient Tainted: G W O 3.6.0 #36 LENOVO 2776LEG/2776LEG [48880.050218] RIP: 0010:[<ffffffffa06ba879>] [<ffffffffa06ba879>] cdc_ncm_tx_bundle+0x168/0x43b [cdc_ncm] [48880.050297] RSP: 0018:ffff880232189ab8 EFLAGS: 00010287 [48880.050340] RAX: 0000000000000000 RBX: ffff880162c97600 RCX: 00000000ffffffff [48880.050395] RDX: 0000000000000800 RSI: ffff8802310bac00 RDI: 0000000000000246 [48880.050448] RBP: ffff880230a0e8c0 R08: 0000000000001100 R09: 000000015eb6d202 [48880.050502] R10: 00000000ffffffff R11: ffff880230a0e8c0 R12: 0000000000000000 [48880.050556] R13: 0000000000000001 R14: 0000000000000212 R15: 00000000000001e8 [48880.050610] FS: 00007fb27436d700(0000) GS:ffff88023bc80000(0000) knlGS:0000000000000000 [48880.050673] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [48880.050717] CR2: 0000000000000068 CR3: 0000000226196000 CR4: 00000000000007e0 [48880.050771] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [48880.050827] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [48880.050882] Process dhclient (pid: 5467, threadinfo ffff880232188000, task ffff88020f4f0810) [48880.050945] Stack: [48880.050964] 0000000000000156 0000000000000001 0000000000000000 ffffffff812a8b00 [48880.051039] ffff8801b2b70980 0000000000000000 0000000000000000 0000000000004000 [48880.051114] 0000000000000001 ffff88016f63f000 ffff88018237bac0 ffffffffa03ac5fc [48880.051193] Call Trace: [48880.051221] [<ffffffff812a8b00>] ? build_skb+0x7b/0xa9 [48880.051266] [<ffffffffa03ac5fc>] ? usbnet_start_xmit+0x99/0x4e9 [usbnet] [48880.051321] [<ffffffff812b50b1>] ? dev_hard_start_xmit+0x3db/0x533 [48880.051373] [<ffffffff812c9d5a>] ? sch_direct_xmit+0x64/0x13a [48880.051426] [<ffffffff812b5521>] ? dev_queue_xmit+0x318/0x4eb [48880.051477] [<ffffffff81347a8a>] ? packet_sendmsg_spkt+0x268/0x297 [48880.051528] [<ffffffff812a0644>] ? sock_sendmsg+0x53/0x6b [48880.051576] [<ffffffff8124bb5d>] ? pty_write+0x48/0x53 [48880.051619] [<ffffffff8129f9f6>] ? copy_from_user+0x18/0x30 [48880.051669] [<ffffffff8129fa46>] ? move_addr_to_kernel+0x2a/0x65 [48880.051718] [<ffffffff812a0b6a>] ? sys_sendto+0xf7/0x137 [48880.051765] [<ffffffff8110b27b>] ? vfs_write+0xc9/0xff [48880.051810] [<ffffffff8136f779>] ? system_call_fastpath+0x16/0x1b [48880.051859] Code: 44 89 f1 31 c0 48 89 d7 f3 aa c7 83 58 01 00 00 00 00 00 00 45 31 ed e9 d9 00 00 00 8b 93 68 01 00 00 41 39 d6 0f 83 e2 00 00 00 <41> 8b 4c 24 68 44 29 f2 39 d1 76 28 66 45 85 ed 0f 85 cc 00 00 [48880.052010] RIP [<ffffffffa06ba879>] cdc_ncm_tx_bundle+0x168/0x43b [cdc_ncm] [48880.052010] RSP <ffff880232189ab8> [48880.052010] CR2: 0000000000000068 [48880.079274] ---[ end trace 993ca2b17e744958 ]--- [48880.079284] Kernel panic - not syncing: Fatal exception in interrupt [48880.079340] panic occurred, switching back to text console [48880.079405] ------------[ cut here ]------------ [48880.079479] WARNING: at drivers/gpu/drm/i915/intel_display.c:1225 intel_crtc_disable+0x52/0x86 [i915]() [48880.079549] Hardware name: 2776LEG [48880.079578] pipe B assertion failure (expected off, current on) [48880.080007] Modules linked in: cdc_wdm cdc_ncm(O) netconsole configfs usbnet(O) mii cdc_acm usbhid hid option usb_storage uas nfsv3 nfsv4 auth_rpcgss udf crc_itu_t xt_multiport iptable_filter ip_tables cpufreq_userspace cpufreq_stats cpufreq_conservative cpufreq_powersave rfcomm bnep xt_hl binfmt_misc ip6table_filter ip6_tables x_tables fuse nfsd nfs_acl nfs lockd fscache sunrpc 8021q garp stp llc tun ext2 loop snd_hda_codec_conexant snd_hda_intel snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss iTCO_wdt snd_pcm iTCO_vendor_support thinkpad_acpi nvram snd_page_alloc snd_seq_midi snd_seq_midi_event snd_rawmidi arc4 iwldvm mac80211 snd_seq snd_timer snd_seq_device qcserial usb_wwan coretemp kvm_intel usbserial uvcvideo videobuf2_vmalloc btusb kvm videobuf2_memops videobuf2_core bluetooth psmouse i2c_i801 serio_raw videodev evdev crc16 lpc_ich acpi_cpufreq mfd_core ac battery snd iwlwifi mperf wmi i915 cfg80211 rfkill video processor button drm_kms_helper drm soundcore mei i2c_algo_bit i2c_core ext3 mbcache jbd sha256_generic ablk_helper cryptd aes_x86_64 aes_generic cbc dm_crypt dm_mod nbd sg sd_mod sr_mod crc_t10dif cdrom microcode thermal thermal_sys uhci_hcd ahci ehci_hcd libahci libata e1000e scsi_mod usbcore usb_common [last unloaded: cdc_ncm] [48880.080007] Pid: 5467, comm: dhclient Tainted: G D W O 3.6.0 #36 [48880.080007] Call Trace: [48880.080007] [<ffffffff8103e0ed>] ? warn_slowpath_common+0x78/0x8c [48880.080007] [<ffffffff8103e19f>] ? warn_slowpath_fmt+0x45/0x4a [48880.080007] [<ffffffffa02b5ce1>] ? intel_crtc_disable+0x52/0x86 [i915] [48880.080007] [<ffffffffa0212f0a>] ? drm_helper_disable_unused_functions+0xf1/0x133 [drm_kms_helper] [48880.080007] [<ffffffffa0213dd0>] ? drm_crtc_helper_set_config+0x185/0x919 [drm_kms_helper] [48880.080007] [<ffffffffa021113a>] ? drm_fb_helper_restore_fbdev_mode+0x30/0x4a [drm_kms_helper] [48880.080007] [<ffffffffa021118b>] ? drm_fb_helper_force_kernel_mode+0x37/0x62 [drm_kms_helper] [48880.080007] [<ffffffffa0211aa2>] ? drm_fb_helper_panic+0x20/0x26 [drm_kms_helper] [48880.080007] [<ffffffff8136d416>] ? notifier_call_chain+0x2e/0x5b [48880.080007] [<ffffffff81368236>] ? panic+0xf3/0x1dd [48880.080007] [<ffffffff8136b013>] ? oops_end+0xaa/0xb7 [48880.080007] [<ffffffff8103319a>] ? no_context+0x254/0x263 [48880.080007] [<ffffffff8136d249>] ? do_page_fault+0x1ad/0x34c [48880.080007] [<ffffffffa0699240>] ? write_msg+0x9f/0x102 [netconsole] [48880.080007] [<ffffffff81101319>] ? ____cache_alloc+0x3f/0x246 [48880.080007] [<ffffffff8136a5a5>] ? page_fault+0x25/0x30 [48880.080007] [<ffffffffa06ba879>] ? cdc_ncm_tx_bundle+0x168/0x43b [cdc_ncm] [48880.080007] [<ffffffffa06ba93a>] ? cdc_ncm_tx_bundle+0x229/0x43b [cdc_ncm] [48880.080007] [<ffffffff812a8b00>] ? build_skb+0x7b/0xa9 [48880.080007] [<ffffffffa03ac5fc>] ? usbnet_start_xmit+0x99/0x4e9 [usbnet] [48880.080007] [<ffffffff812b50b1>] ? dev_hard_start_xmit+0x3db/0x533 [48880.080007] [<ffffffff812c9d5a>] ? sch_direct_xmit+0x64/0x13a [48880.080007] [<ffffffff812b5521>] ? dev_queue_xmit+0x318/0x4eb [48880.080007] [<ffffffff81347a8a>] ? packet_sendmsg_spkt+0x268/0x297 [48880.080007] [<ffffffff812a0644>] ? sock_sendmsg+0x53/0x6b [48880.080007] [<ffffffff8124bb5d>] ? pty_write+0x48/0x53 [48880.080007] [<ffffffff8129f9f6>] ? copy_from_user+0x18/0x30 [48880.080007] [<ffffffff8129fa46>] ? move_addr_to_kernel+0x2a/0x65 [48880.080007] [<ffffffff812a0b6a>] ? sys_sendto+0xf7/0x137 [48880.080007] [<ffffffff8110b27b>] ? vfs_write+0xc9/0xff [48880.080007] [<ffffffff8136f779>] ? system_call_fastpath+0x16/0x1b [48880.080007] ---[ end trace 993ca2b17e744959 ]--- -- 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
Bjørn Mork <bjorn@mork.no> writes: > [48880.048494] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068 > [48880.048573] IP: [<ffffffffa06ba879>] cdc_ncm_tx_bundle+0x168/0x43b [cdc_ncm] This one is because you removed the "if (skb == NULL)" from the for loop, but left the "skb = NULL;" at the end: @@ -719,28 +707,15 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) /* compute maximum buffer size */ rem = ctx->tx_max - offset; - if (skb == NULL) { - skb = ctx->tx_rem_skb; - ctx->tx_rem_skb = NULL; - - /* check for end of skb */ - if (skb == NULL) - break; - } - if (skb->len > rem) { if (n == 0) { /* won't fit, MTU problem? */ dev_kfree_skb_any(skb); skb = NULL; ctx->netdev->stats.tx_dropped++; + error = 1; } else { - /* no room for skb - store for later */ - if (ctx->tx_rem_skb != NULL) { - dev_kfree_skb_any(ctx->tx_rem_skb); - ctx->netdev->stats.tx_dropped++; - } - ctx->tx_rem_skb = skb; + skb = NULL; ready2send = 1; } @@ -768,13 +743,6 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) skb = NULL; } If I understand your intentions with this code, then the for loop should probably just go away completely? Anyway, after avoiding that one, I ended up with [ 857.112692] cdc_ncm_fill_tx_frame: skb_out=ffff880218624cc0, length=2048 [ 857.112696] cdc_ncm_tx_fixup: returning (null) [ 857.112731] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068 [ 857.112807] IP: [<ffffffffa030e68b>] usbnet_start_xmit+0x128/0x4e9 [usbnet] No need for the reset of the trace here. Removing the "goto not_drop" makes usbnet_start_xmit continue after tx_fixup returns NULL: - if (info->tx_fixup) { + if (transmit_now && info->tx_fixup) { skb = info->tx_fixup (dev, skb, GFP_ATOMIC); if (!skb) { if (netif_msg_tx_err(dev)) { netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n"); goto drop; - } else { - /* cdc_ncm collected packet; waits for more */ - goto not_drop; } } } // some devices want funky USB-level framing, for // win32 driver (usually) and/or hardware quirks if (transmit_now && info->tx_fixup) { skb = info->tx_fixup (dev, skb, GFP_ATOMIC); if (!skb) { if (netif_msg_tx_err(dev)) { netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n"); goto drop; } } } length = skb->len; I stopped there. I am not exactly sure where you were going with this anymore. tx_fixup will return the tx_curr_skb being currently built every time it is called, without anything resetting it at that point. Then when finally cdc_ncm_fill_tx_frame has been called enough times to fill it completely, you end up just dropping it on the floor. The result is that nothing is ever transmitted even after fixing the above errors because the tx_curr_skb has an incomplete header every time tx_fixup returns it. I do like your idea, but if you don't mind I think we'll go ahead and base the upcoming cdc_mbim driver on the current cdc_ncm *with* the timer. The plan is to reuse as much of cdc_ncm as possible, so in theory it should be easy to update both drivers when the timer goes away (there is no need for cdc_mbim be aware of the timer at all). But unfortunately it seems that cdc_ncm_fill_tx_frame must be changed a lot to support the concept of multiple NDPs in a single NTB. This means that the timer removal patches will conflict heavily with the preparations for MBIM support. With some luck and acceptance from all contributors, I hope to be able to post the first version of that as the merge window closes. Bjørn -- 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
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 4cd582a..56ef743 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -135,9 +135,6 @@ struct cdc_ncm_ctx { u16 connected; }; -static void cdc_ncm_txpath_bh(unsigned long param); -static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx); -static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer); static struct usb_driver cdc_ncm_driver; static void @@ -464,10 +461,6 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf) if (ctx == NULL) return -ENODEV; - hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); - ctx->tx_timer.function = &cdc_ncm_tx_timer_cb; - ctx->bh.data = (unsigned long)ctx; - ctx->bh.func = cdc_ncm_txpath_bh; atomic_set(&ctx->stop, 0); spin_lock_init(&ctx->mtx); ctx->netdev = dev->net; @@ -650,7 +643,7 @@ static void cdc_ncm_zero_fill(u8 *ptr, u32 first, u32 end, u32 max) memset(ptr + first, 0, end - first); } -static struct sk_buff * +static int cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) { struct sk_buff *skb_out; @@ -659,12 +652,7 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) u32 last_offset; u16 n = 0, index; u8 ready2send = 0; - - /* if there is a remaining skb, it gets priority */ - if (skb != NULL) - swap(skb, ctx->tx_rem_skb); - else - ready2send = 1; + u8 error = 0; /* * +----------------+ @@ -690,7 +678,7 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) dev_kfree_skb_any(skb); ctx->netdev->stats.tx_dropped++; } - goto exit_no_skb; + return -EBUSY; } /* make room for NTH and NDP */ @@ -719,28 +707,15 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) /* compute maximum buffer size */ rem = ctx->tx_max - offset; - if (skb == NULL) { - skb = ctx->tx_rem_skb; - ctx->tx_rem_skb = NULL; - - /* check for end of skb */ - if (skb == NULL) - break; - } - if (skb->len > rem) { if (n == 0) { /* won't fit, MTU problem? */ dev_kfree_skb_any(skb); skb = NULL; ctx->netdev->stats.tx_dropped++; + error = 1; } else { - /* no room for skb - store for later */ - if (ctx->tx_rem_skb != NULL) { - dev_kfree_skb_any(ctx->tx_rem_skb); - ctx->netdev->stats.tx_dropped++; - } - ctx->tx_rem_skb = skb; + skb = NULL; ready2send = 1; } @@ -768,13 +743,6 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) skb = NULL; } - /* free up any dangling skb */ - if (skb != NULL) { - dev_kfree_skb_any(skb); - skb = NULL; - ctx->netdev->stats.tx_dropped++; - } - ctx->tx_curr_frame_num = n; if (n == 0) { @@ -791,9 +759,7 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) ctx->tx_curr_skb = skb_out; ctx->tx_curr_offset = offset; ctx->tx_curr_last_offset = last_offset; - /* set the pending count */ - if (n < CDC_NCM_RESTART_TIMER_DATAGRAM_CNT) - ctx->tx_timer_pending = CDC_NCM_TIMER_PENDING_CNT; + goto exit_no_skb; } else { @@ -874,71 +840,37 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb) /* return skb */ ctx->tx_curr_skb = NULL; ctx->netdev->stats.tx_packets += ctx->tx_curr_frame_num; - return skb_out; -exit_no_skb: - /* Start timer, if there is a remaining skb */ - if (ctx->tx_curr_skb != NULL) - cdc_ncm_tx_timeout_start(ctx); - return NULL; -} - -static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx) -{ - /* start timer, if not already started */ - if (!(hrtimer_active(&ctx->tx_timer) || atomic_read(&ctx->stop))) - hrtimer_start(&ctx->tx_timer, - ktime_set(0, CDC_NCM_TIMER_INTERVAL), - HRTIMER_MODE_REL); -} + if (error) + return -EBUSY; + if (ready2send) + return -EBUSY; + else + return 0; -static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer) -{ - struct cdc_ncm_ctx *ctx = - container_of(timer, struct cdc_ncm_ctx, tx_timer); +exit_no_skb: - if (!atomic_read(&ctx->stop)) - tasklet_schedule(&ctx->bh); - return HRTIMER_NORESTART; + return -EAGAIN; } -static void cdc_ncm_txpath_bh(unsigned long param) +static int cdc_ncm_tx_bundle(struct usbnet *dev, struct sk_buff *skb, gfp_t flags) { - struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)param; - - spin_lock_bh(&ctx->mtx); - if (ctx->tx_timer_pending != 0) { - ctx->tx_timer_pending--; - cdc_ncm_tx_timeout_start(ctx); - spin_unlock_bh(&ctx->mtx); - } else if (ctx->netdev != NULL) { - spin_unlock_bh(&ctx->mtx); - netif_tx_lock_bh(ctx->netdev); - usbnet_start_xmit(NULL, ctx->netdev); - netif_tx_unlock_bh(ctx->netdev); - } + int err; + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; + + err = cdc_ncm_fill_tx_frame(ctx, skb); + return err; } static struct sk_buff * cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags) { - struct sk_buff *skb_out; struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; - /* - * The Ethernet API we are using does not support transmitting - * multiple Ethernet frames in a single call. This driver will - * accumulate multiple Ethernet frames and send out a larger - * USB frame when the USB buffer is full or when a single jiffies - * timeout happens. - */ if (ctx == NULL) goto error; - spin_lock_bh(&ctx->mtx); - skb_out = cdc_ncm_fill_tx_frame(ctx, skb); - spin_unlock_bh(&ctx->mtx); - return skb_out; + return ctx->tx_curr_skb; error: if (skb != NULL) @@ -1197,6 +1129,7 @@ static const struct driver_info cdc_ncm_info = { .manage_power = cdc_ncm_manage_power, .status = cdc_ncm_status, .rx_fixup = cdc_ncm_rx_fixup, + .tx_bundle = cdc_ncm_tx_bundle, .tx_fixup = cdc_ncm_tx_fixup, }; @@ -1211,6 +1144,7 @@ static const struct driver_info wwan_info = { .manage_power = cdc_ncm_manage_power, .status = cdc_ncm_status, .rx_fixup = cdc_ncm_rx_fixup, + .tx_bundle = cdc_ncm_tx_bundle, .tx_fixup = cdc_ncm_tx_fixup, }; diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 8531c1c..67b6ea0 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -86,6 +86,9 @@ static u8 node_id [ETH_ALEN]; static const char driver_name [] = "usbnet"; +static void tx_complete(struct urb *urb); +static int submit_next_bundle(struct usbnet *dev, struct sk_buff *skb); + /* use ethtool to change the level for any given device */ static int msg_level = -1; module_param (msg_level, int, 0); @@ -923,7 +926,9 @@ kevent (struct work_struct *work) { struct usbnet *dev = container_of(work, struct usbnet, kevent); + struct driver_info *info = dev->driver_info; int status; + struct sk_buff *skb; /* usb_clear_halt() needs a thread context */ if (test_bit (EVENT_TX_HALT, &dev->flags)) { @@ -942,8 +947,13 @@ fail_pipe: status); } else { clear_bit (EVENT_TX_HALT, &dev->flags); - if (status != -ESHUTDOWN) + if (status != -ESHUTDOWN) { + /* queue halted, no race */ + skb = info->tx_fixup(dev, NULL, GFP_KERNEL); + if (skb) + submit_next_bundle(dev, skb); netif_wake_queue (dev->net); + } } } if (test_bit (EVENT_RX_HALT, &dev->flags)) { @@ -1008,6 +1018,11 @@ skip_reset: dev->udev->devpath, info->description); } else { + spin_lock_irq(&dev->txlock); + skb = info->tx_fixup(dev, NULL, GFP_ATOMIC); + if (skb) + submit_next_bundle(dev, skb); + spin_unlock_irq(&dev->txlock); usb_autopm_put_interface(dev->intf); } } @@ -1016,17 +1031,78 @@ skip_reset: netdev_dbg(dev->net, "kevent done, flags = 0x%lx\n", dev->flags); } +static int submit_next_bundle(struct usbnet *dev, struct sk_buff *skb) +{ + struct skb_data *entry; + int length = skb->len; + int retval; + struct driver_info *info = dev->driver_info; + struct urb *urb; + + urb = usb_alloc_urb(0, GFP_ATOMIC); + if (!urb) + return -ENOMEM; + + entry = (struct skb_data *) skb->cb; + entry->urb = urb; + entry->dev = dev; + entry->length = length; + + usb_fill_bulk_urb (urb, dev->udev, dev->out, + skb->data, skb->len, tx_complete, skb); + + /* don't assume the hardware handles USB_ZERO_PACKET + * NOTE: strictly conforming cdc-ether devices should expect + * the ZLP here, but ignore the one-byte packet. + * NOTE2: CDC NCM specification is different from CDC ECM when + * handling ZLP/short packets, so cdc_ncm driver will make short + * packet itself if needed. + */ + if (length % dev->maxpacket == 0) { + if (!(info->flags & FLAG_SEND_ZLP)) { + if (!(info->flags & FLAG_MULTI_PACKET)) { + urb->transfer_buffer_length++; + if (skb_tailroom(skb)) { + skb->data[skb->len] = 0; + __skb_put(skb, 1); + } + } + } else + urb->transfer_flags |= URB_ZERO_PACKET; + } + + atomic_inc(&dev->tx_in_flight); + retval = usb_submit_urb(urb, GFP_ATOMIC); + if (retval < 0) + atomic_dec(&dev->tx_in_flight); + //FIXME: full error handling + + return retval; +} /*-------------------------------------------------------------------------*/ static void tx_complete (struct urb *urb) { struct sk_buff *skb = (struct sk_buff *) urb->context; + struct sk_buff *skb2; struct skb_data *entry = (struct skb_data *) skb->cb; struct usbnet *dev = entry->dev; + struct driver_info *info = dev->driver_info; + int in_flight; + in_flight = atomic_dec_return(&dev->tx_in_flight); if (urb->status == 0) { - if (!(dev->driver_info->flags & FLAG_MULTI_PACKET)) + if (!(dev->driver_info->flags & FLAG_MULTI_PACKET)) { dev->net->stats.tx_packets++; + } else { + if (in_flight < 2) { + spin_lock(&dev->txlock); + skb2 = info->tx_fixup(dev, NULL, GFP_ATOMIC); + if (skb2) + submit_next_bundle(dev, skb2); + spin_unlock(&dev->txlock); + } + } dev->net->stats.tx_bytes += entry->length; } else { dev->net->stats.tx_errors++; @@ -1089,23 +1165,51 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, struct urb *urb = NULL; struct skb_data *entry; struct driver_info *info = dev->driver_info; + struct sk_buff *skb_old = NULL; unsigned long flags; int retval; + int transmit_now = 1; + int bundle_again = 0; if (skb) skb_tx_timestamp(skb); + /* + * first we allow drivers to bundle packets together + * maintainance of the buffer is the responsibility + * of the lower layer + */ + spin_lock(&dev->txlock); +rebundle: + if (info->tx_bundle) { + bundle_again = 0; + retval = info->tx_bundle(dev, skb, GFP_ATOMIC); + + switch (retval) { + case 0: /* the package has been bundled */ + if (atomic_read(&dev->tx_in_flight) < 2) + transmit_now = 1; + else + transmit_now = 0; + break; + case -EAGAIN: + transmit_now = 1; + bundle_again = 1; + skb_old = skb; + break; + case -EBUSY: + transmit_now = 1; + break; + } + } // some devices want funky USB-level framing, for // win32 driver (usually) and/or hardware quirks - if (info->tx_fixup) { + if (transmit_now && info->tx_fixup) { skb = info->tx_fixup (dev, skb, GFP_ATOMIC); if (!skb) { if (netif_msg_tx_err(dev)) { netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n"); goto drop; - } else { - /* cdc_ncm collected packet; waits for more */ - goto not_drop; } } } @@ -1164,14 +1268,17 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, } #endif + atomic_inc(&dev->tx_in_flight); switch ((retval = usb_submit_urb (urb, GFP_ATOMIC))) { case -EPIPE: netif_stop_queue (net); usbnet_defer_kevent (dev, EVENT_TX_HALT); + atomic_dec(&dev->tx_in_flight); usb_autopm_put_interface_async(dev->intf); break; default: usb_autopm_put_interface_async(dev->intf); + atomic_dec(&dev->tx_in_flight); netif_dbg(dev, tx_err, dev->net, "tx: submit urb err %d\n", retval); break; @@ -1187,7 +1294,6 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, netif_dbg(dev, tx_err, dev->net, "drop, code %d\n", retval); drop: dev->net->stats.tx_dropped++; -not_drop: if (skb) dev_kfree_skb_any (skb); usb_free_urb (urb); @@ -1197,6 +1303,11 @@ not_drop: #ifdef CONFIG_PM deferred: #endif + if (bundle_again) { + skb = skb_old; + goto rebundle; + } + spin_unlock(&dev->txlock); return NETDEV_TX_OK; } EXPORT_SYMBOL_GPL(usbnet_start_xmit); @@ -1393,6 +1504,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) dev->delay.data = (unsigned long) dev; init_timer (&dev->delay); mutex_init (&dev->phy_mutex); + spin_lock_init(&dev->txlock); + atomic_set(&dev->tx_in_flight, 0); dev->net = net; strcpy (net->name, "usb%d"); diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index f87cf62..544ddd2 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -33,6 +33,8 @@ struct usbnet { wait_queue_head_t *wait; struct mutex phy_mutex; unsigned char suspend_count; + atomic_t tx_in_flight; + spinlock_t txlock; /* i/o info: pipes etc */ unsigned in, out; @@ -133,6 +135,12 @@ struct driver_info { /* fixup rx packet (strip framing) */ int (*rx_fixup)(struct usbnet *dev, struct sk_buff *skb); + /* bundle individual package for transmission as + * larger package. This cannot sleep + */ + int (*tx_bundle)(struct usbnet *dev, + struct sk_buff *skb, gfp_t flags); + /* fixup tx packet (add framing) */ struct sk_buff *(*tx_fixup)(struct usbnet *dev, struct sk_buff *skb, gfp_t flags);