On Wed, Dec 18, 2013 at 11:24:47AM -0000, David Laight wrote:
> This saves a kzalloc() call on every transfer and some memory
> indirections.
>
> The only possible downside is for isochronous tranfers with 64 td
> when the allocate is 8+4096 bytes (on 64bit systems) so requires
> an additional page.
>
> Signed-off-by: David Laight <david.lai...@aculab.com>
> ---
>
> v2: Added signed-off-by line
Hi David,
The patch looks good in general and applies fine. However, in testing
this with a USB mass storage device, I ran across a warning, followed by
an oops that hung the system:
Dec 19 11:17:58 xanatos kernel: [ 111.379696] ------------[ cut here
]------------
Dec 19 11:17:58 xanatos kernel: [ 111.379723] WARNING: CPU: 3 PID: 2646 at
drivers/usb/host/xhci-ring.c:568 xhci_find_new_dequeue_state+0xcd/0x330
[xhci_hcd]()
Dec 19 11:17:58 xanatos kernel: [ 111.379729] Modules linked in: usb_storage
cuse dm_crypt uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_core
videodev x86_pkg_temp_thermal coretemp ghash_clmulni_intel joydev aesni_intel
aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd arc4 iwldvm mac80211
snd_hda_codec_hdmi iwlwifi microcode snd_hda_codec_realtek cfg80211
thinkpad_acpi psmouse nvram snd_hda_intel serio_raw snd_hda_codec snd_seq_midi
snd_hwdep snd_seq_midi_event snd_pcm lpc_ich ehci_pci ehci_hcd snd_page_alloc
snd_rawmidi bnep rfcomm bluetooth snd_seq snd_seq_device snd_timer snd
soundcore tpm_tis binfmt_misc btrfs libcrc32c xor raid6_pq i915 i2c_algo_bit
drm_kms_helper e1000e sdhci_pci ahci drm sdhci libahci ptp xhci_hcd pps_core
video
Dec 19 11:17:58 xanatos kernel: [ 111.379860] CPU: 3 PID: 2646 Comm:
usb-storage Not tainted 3.13.0-rc1+ #144
Dec 19 11:17:58 xanatos kernel: [ 111.379865] Hardware name: LENOVO
2325AP7/2325AP7, BIOS G2ET82WW (2.02 ) 09/11/2012
Dec 19 11:17:58 xanatos kernel: [ 111.379870] 0000000000000009
ffff8800946dbb38 ffffffff8165839e 0000000000000000
Dec 19 11:17:58 xanatos kernel: [ 111.379882] ffff8800946dbb70
ffffffff81048c3d ffff880030df8000 ffff8800946dbbe0
Dec 19 11:17:58 xanatos kernel: [ 111.379892] ffff880094668000
ffff88009a083130 0000000000000002 ffff8800946dbb80
Dec 19 11:17:58 xanatos kernel: [ 111.379903] Call Trace:
Dec 19 11:17:58 xanatos kernel: [ 111.379917] [<ffffffff8165839e>]
dump_stack+0x4d/0x66
Dec 19 11:17:58 xanatos kernel: [ 111.379928] [<ffffffff81048c3d>]
warn_slowpath_common+0x7d/0xa0
Dec 19 11:17:58 xanatos kernel: [ 111.379937] [<ffffffff81048d1a>]
warn_slowpath_null+0x1a/0x20
Dec 19 11:17:58 xanatos kernel: [ 111.379955] [<ffffffffa0021ebd>]
xhci_find_new_dequeue_state+0xcd/0x330 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.379972] [<ffffffffa0019f5f>]
xhci_cleanup_stalled_ring+0x6f/0x200 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.379990] [<ffffffffa0021290>] ?
queue_command+0x80/0xe0 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.380006] [<ffffffffa001a21e>]
xhci_endpoint_reset+0x12e/0x1a0 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.380018] [<ffffffff814b1b15>]
usb_hcd_reset_endpoint+0x25/0x70
Dec 19 11:17:58 xanatos kernel: [ 111.380025] [<ffffffff814b3f88>]
usb_reset_endpoint+0x28/0x40
Dec 19 11:17:58 xanatos kernel: [ 111.380036] [<ffffffffa06d6081>]
usb_stor_clear_halt+0x71/0x80 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380046] [<ffffffffa06d61c8>]
interpret_urb_result+0x48/0x60 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380057] [<ffffffffa06d633c>]
usb_stor_bulk_transfer_buf+0x8c/0xa0 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380067] [<ffffffffa06d65fc>]
usb_stor_Bulk_transport+0x15c/0x330 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380078] [<ffffffffa06d7005>]
usb_stor_invoke_transport+0x255/0x550 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380089] [<ffffffffa06d5b7e>]
usb_stor_transparent_scsi_command+0xe/0x10 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380100] [<ffffffffa06d7cc3>]
usb_stor_control_thread+0x173/0x290 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380110] [<ffffffffa06d7b50>] ?
fill_inquiry_response+0x20/0x20 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380121] [<ffffffffa06d7b50>] ?
fill_inquiry_response+0x20/0x20 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.380131] [<ffffffff8106dc8c>]
kthread+0xfc/0x120
Dec 19 11:17:58 xanatos kernel: [ 111.380142] [<ffffffff8106db90>] ?
kthread_create_on_node+0x230/0x230
Dec 19 11:17:58 xanatos kernel: [ 111.380150] [<ffffffff816695ec>]
ret_from_fork+0x7c/0xb0
Dec 19 11:17:58 xanatos kernel: [ 111.380159] [<ffffffff8106db90>] ?
kthread_create_on_node+0x230/0x230
Dec 19 11:17:58 xanatos kernel: [ 111.380164] ---[ end trace 0c5666a6387712b8
]---
Dec 19 11:17:58 xanatos kernel: [ 111.380176] BUG: unable to handle kernel
NULL pointer dereference at 0000000000000010
Dec 19 11:17:58 xanatos kernel: [ 111.380279] IP: [<ffffffffa002219f>]
xhci_queue_new_dequeue_state+0x7f/0x1e0 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.380388] PGD 0
Dec 19 11:17:58 xanatos kernel: [ 111.380418] Oops: 0000 [#1] SMP
Dec 19 11:17:58 xanatos kernel: [ 111.380462] Modules linked in: usb_storage
cuse dm_crypt uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_core
videodev x86_pkg_temp_thermal coretemp ghash_clmulni_intel joydev aesni_intel
aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd arc4 iwldvm mac80211
snd_hda_codec_hdmi iwlwifi microcode snd_hda_codec_realtek cfg80211
thinkpad_acpi psmouse nvram snd_hda_intel serio_raw snd_hda_codec snd_seq_midi
snd_hwdep snd_seq_midi_event snd_pcm lpc_ich ehci_pci ehci_hcd snd_page_alloc
snd_rawmidi bnep rfcomm bluetooth snd_seq snd_seq_device snd_timer snd
soundcore tpm_tis binfmt_misc btrfs libcrc32c xor raid6_pq i915 i2c_algo_bit
drm_kms_helper e1000e sdhci_pci ahci drm sdhci libahci ptp xhci_hcd pps_core
video
Dec 19 11:17:58 xanatos kernel: [ 111.381242] CPU: 3 PID: 2646 Comm:
usb-storage Tainted: G W 3.13.0-rc1+ #144
Dec 19 11:17:58 xanatos kernel: [ 111.381336] Hardware name: LENOVO
2325AP7/2325AP7, BIOS G2ET82WW (2.02 ) 09/11/2012
Dec 19 11:17:58 xanatos kernel: [ 111.381430] task: ffff8800b1f9c120 ti:
ffff8800946da000 task.ti: ffff8800946da000
Dec 19 11:17:58 xanatos kernel: [ 111.381521] RIP: 0010:[<ffffffffa002219f>]
[<ffffffffa002219f>] xhci_queue_new_dequeue_state+0x7f/0x1e0 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.381661] RSP: 0018:ffff8800946dbb68
EFLAGS: 00010046
Dec 19 11:17:58 xanatos kernel: [ 111.381726] RAX: 0000000000000000 RBX:
0000000000000002 RCX: 0000000000000000
Dec 19 11:17:58 xanatos kernel: [ 111.381812] RDX: 0000000000000001 RSI:
0000000000000001 RDI: ffff880030df8000
Dec 19 11:17:58 xanatos kernel: [ 111.381899] RBP: ffff8800946dbbc8 R08:
ffff8800946dbbe0 R09: ffff880030df8000
Dec 19 11:17:58 xanatos kernel: [ 111.381984] R10: 0000000000000000 R11:
0000000000000000 R12: 0000000000000001
Dec 19 11:17:58 xanatos kernel: [ 111.382071] R13: ffff880094668238 R14:
ffff8800946dbbe0 R15: ffff880030df8000
Dec 19 11:17:58 xanatos kernel: [ 111.382158] FS: 0000000000000000(0000)
GS:ffff88011e2c0000(0000) knlGS:0000000000000000
Dec 19 11:17:58 xanatos kernel: [ 111.382256] CS: 0010 DS: 0000 ES: 0000 CR0:
0000000080050033
Dec 19 11:17:58 xanatos kernel: [ 111.382327] CR2: 0000000000000010 CR3:
0000000001c0c000 CR4: 00000000001407e0
Dec 19 11:17:58 xanatos kernel: [ 111.382412] Stack:
Dec 19 11:17:58 xanatos kernel: [ 111.382439] ffff8800946dbb70
ffff880000000018 ffff8800946dbbd8 ffff8800946dbb88
Dec 19 11:17:58 xanatos kernel: [ 111.382542] ffffffffa0021ebd
ffff880000000000 0000000200000002 ffff880030df8000
Dec 19 11:17:58 xanatos kernel: [ 111.382645] 0000000000000002
ffff880094668238 ffff8800bbe93000 ffff880094668238
Dec 19 11:17:58 xanatos kernel: [ 111.382747] Call Trace:
Dec 19 11:17:58 xanatos kernel: [ 111.382798] [<ffffffffa0021ebd>] ?
xhci_find_new_dequeue_state+0xcd/0x330 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.382908] [<ffffffffa001a0b2>]
xhci_cleanup_stalled_ring+0x1c2/0x200 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.383013] [<ffffffffa0021290>] ?
queue_command+0x80/0xe0 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.383105] [<ffffffffa001a21e>]
xhci_endpoint_reset+0x12e/0x1a0 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.383199] [<ffffffff814b1b15>]
usb_hcd_reset_endpoint+0x25/0x70
Dec 19 11:17:58 xanatos kernel: [ 111.383280] [<ffffffff814b3f88>]
usb_reset_endpoint+0x28/0x40
Dec 19 11:17:58 xanatos kernel: [ 111.383358] [<ffffffffa06d6081>]
usb_stor_clear_halt+0x71/0x80 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.383450] [<ffffffffa06d61c8>]
interpret_urb_result+0x48/0x60 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.383543] [<ffffffffa06d633c>]
usb_stor_bulk_transfer_buf+0x8c/0xa0 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.383643] [<ffffffffa06d65fc>]
usb_stor_Bulk_transport+0x15c/0x330 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.383743] [<ffffffffa06d7005>]
usb_stor_invoke_transport+0x255/0x550 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.383842] [<ffffffffa06d5b7e>]
usb_stor_transparent_scsi_command+0xe/0x10 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.383946] [<ffffffffa06d7cc3>]
usb_stor_control_thread+0x173/0x290 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.384046] [<ffffffffa06d7b50>] ?
fill_inquiry_response+0x20/0x20 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.384141] [<ffffffffa06d7b50>] ?
fill_inquiry_response+0x20/0x20 [usb_storage]
Dec 19 11:17:58 xanatos kernel: [ 111.384236] [<ffffffff8106dc8c>]
kthread+0xfc/0x120
Dec 19 11:17:58 xanatos kernel: [ 111.384303] [<ffffffff8106db90>] ?
kthread_create_on_node+0x230/0x230
Dec 19 11:17:58 xanatos kernel: [ 111.384383] [<ffffffff816695ec>]
ret_from_fork+0x7c/0xb0
Dec 19 11:17:58 xanatos kernel: [ 111.384452] [<ffffffff8106db90>] ?
kthread_create_on_node+0x230/0x230
Dec 19 11:17:58 xanatos kernel: [ 111.384529] Code: 00 48 85 c9 0f 84 72 01 00
00 4c 89 ce 48 89 cf 89 55 b4 4c 89 4d b8 48 89 4d c0 e8 0c f7 ff ff 48 8b 4d
c0 4c 8b 4d b8 8b 55 b4 <4c> 8b 41 10 48 c7 c6 b0 15 02 a0 89 54 24 08 48 89 04
24 48 c7
Dec 19 11:17:58 xanatos kernel: [ 111.388053] RIP [<ffffffffa002219f>]
xhci_queue_new_dequeue_state+0x7f/0x1e0 [xhci_hcd]
Dec 19 11:17:58 xanatos kernel: [ 111.390151] RSP <ffff8800946dbb68>
Dec 19 11:17:58 xanatos kernel: [ 111.391772] CR2: 0000000000000010
Dec 19 11:17:58 xanatos kernel: [ 111.402876] ---[ end trace 0c5666a6387712b9
]---
At this point I'm not sure if this is new behavior introduced by the
patch, or the patch simply uncovering a bug that's always been there.
The code paths involved have been known to cause oopses before (which
was why the WARN calls were introduced).
I'll do some more digging into this bug, but the holidays are
approaching, so I may not be able to triage it until after the new year.
Sarah Sharp
>
> drivers/usb/host/xhci-mem.c | 4 +---
> drivers/usb/host/xhci-ring.c | 22 ++++++++++------------
> drivers/usb/host/xhci.c | 24 ++++++------------------
> drivers/usb/host/xhci.h | 2 +-
> 4 files changed, 18 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 79cdcc2..9b5d1c3 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1675,10 +1675,8 @@ struct xhci_command *xhci_alloc_command(struct
> xhci_hcd *xhci,
>
> void xhci_urb_free_priv(struct xhci_hcd *xhci, struct urb_priv *urb_priv)
> {
> - if (urb_priv) {
> - kfree(urb_priv->td[0]);
> + if (urb_priv)
> kfree(urb_priv);
> - }
> }
>
> void xhci_free_command(struct xhci_hcd *xhci,
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index e38abc2..d3f4a9a 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3007,7 +3007,7 @@ static int prepare_transfer(struct xhci_hcd *xhci,
> return ret;
>
> urb_priv = urb->hcpriv;
> - td = urb_priv->td[td_index];
> + td = &urb_priv->td[td_index];
>
> INIT_LIST_HEAD(&td->td_list);
> INIT_LIST_HEAD(&td->cancelled_td_list);
> @@ -3024,8 +3024,6 @@ static int prepare_transfer(struct xhci_hcd *xhci,
> td->start_seg = ep_ring->enq_seg;
> td->first_trb = ep_ring->enqueue;
>
> - urb_priv->td[td_index] = td;
> -
> return 0;
> }
>
> @@ -3216,7 +3214,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci,
> gfp_t mem_flags,
> return trb_buff_len;
>
> urb_priv = urb->hcpriv;
> - td = urb_priv->td[0];
> + td = &urb_priv->td[0];
>
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> @@ -3387,7 +3385,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
> mem_flags,
> return ret;
>
> urb_priv = urb->hcpriv;
> - td = urb_priv->td[0];
> + td = &urb_priv->td[0];
>
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> @@ -3517,7 +3515,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> mem_flags,
> return ret;
>
> urb_priv = urb->hcpriv;
> - td = urb_priv->td[0];
> + td = &urb_priv->td[0];
>
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> @@ -3729,7 +3727,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
> gfp_t mem_flags,
> goto cleanup;
> }
>
> - td = urb_priv->td[i];
> + td = &urb_priv->td[i];
> for (j = 0; j < trbs_per_td; j++) {
> u32 remainder = 0;
> field = 0;
> @@ -3829,20 +3827,20 @@ cleanup:
> /* Clean up a partially enqueued isoc transfer. */
>
> for (i--; i >= 0; i--)
> - list_del_init(&urb_priv->td[i]->td_list);
> + list_del_init(&urb_priv->td[i].td_list);
>
> /* Use the first TD as a temporary variable to turn the TDs we've queued
> * into No-ops with a software-owned cycle bit. That way the hardware
> * won't accidentally start executing bogus TDs when we partially
> * overwrite them. td->first_trb and td->start_seg are already set.
> */
> - urb_priv->td[0]->last_trb = ep_ring->enqueue;
> + urb_priv->td[0].last_trb = ep_ring->enqueue;
> /* Every TRB except the first & last will have its cycle bit flipped. */
> - td_to_noop(xhci, ep_ring, urb_priv->td[0], true);
> + td_to_noop(xhci, ep_ring, &urb_priv->td[0], true);
>
> /* Reset the ring enqueue back to the first TRB and its cycle bit. */
> - ep_ring->enqueue = urb_priv->td[0]->first_trb;
> - ep_ring->enq_seg = urb_priv->td[0]->start_seg;
> + ep_ring->enqueue = urb_priv->td[0].first_trb;
> + ep_ring->enq_seg = urb_priv->td[0].start_seg;
> ep_ring->cycle_state = start_cycle;
> ep_ring->num_trbs_free = ep_ring->num_trbs_free_temp;
> usb_hcd_unlink_urb_from_ep(bus_to_hcd(urb->dev->bus), urb);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 6e0d886..0969f74 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1248,12 +1248,11 @@ static int xhci_check_maxpacket(struct xhci_hcd
> *xhci, unsigned int slot_id,
> int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
> {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> - struct xhci_td *buffer;
> unsigned long flags;
> int ret = 0;
> unsigned int slot_id, ep_index;
> struct urb_priv *urb_priv;
> - int size, i;
> + int size;
>
> if (!urb || xhci_check_args(hcd, urb->dev, urb->ep,
> true, true, __func__) <= 0)
> @@ -1275,21 +1274,10 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb
> *urb, gfp_t mem_flags)
> size = 1;
>
> urb_priv = kzalloc(sizeof(struct urb_priv) +
> - size * sizeof(struct xhci_td *), mem_flags);
> + size * sizeof(struct xhci_td), mem_flags);
> if (!urb_priv)
> return -ENOMEM;
>
> - buffer = kzalloc(size * sizeof(struct xhci_td), mem_flags);
> - if (!buffer) {
> - kfree(urb_priv);
> - return -ENOMEM;
> - }
> -
> - for (i = 0; i < size; i++) {
> - urb_priv->td[i] = buffer;
> - buffer++;
> - }
> -
> urb_priv->length = size;
> urb_priv->td_cnt = 0;
> urb->hcpriv = urb_priv;
> @@ -1470,7 +1458,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb
> *urb, int status)
> "HW died, freeing TD.");
> urb_priv = urb->hcpriv;
> for (i = urb_priv->td_cnt; i < urb_priv->length; i++) {
> - td = urb_priv->td[i];
> + td = &urb_priv->td[i];
> if (!list_empty(&td->td_list))
> list_del_init(&td->td_list);
> if (!list_empty(&td->cancelled_td_list))
> @@ -1514,11 +1502,11 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb
> *urb, int status)
> urb, urb->dev->devpath,
> urb->ep->desc.bEndpointAddress,
> (unsigned long long) xhci_trb_virt_to_dma(
> - urb_priv->td[i]->start_seg,
> - urb_priv->td[i]->first_trb));
> + urb_priv->td[i].start_seg,
> + urb_priv->td[i].first_trb));
>
> for (; i < urb_priv->length; i++) {
> - td = urb_priv->td[i];
> + td = &urb_priv->td[i];
> list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
> }
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 72ad988..a7d8fdd 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1363,7 +1363,7 @@ struct xhci_scratchpad {
> struct urb_priv {
> int length;
> int td_cnt;
> - struct xhci_td *td[0];
> + struct xhci_td td[0];
> };
>
> /*
> --
> 1.8.1.2
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
