On Sun, 27 Jun 2010, Andrea Righi wrote:
> I did many tests in these past days, unfortunately without any
> significant result. :(
>
> The only "bad" thing I've found is that the communication via
> usb_stor_bulk_transfer_sglist() seems to fail only with "large"
> transfers (even 192 is sufficient to trigger the problem, while small
> transfers seems to work fine):
>
> $ dmesg | grep "Status code\|bulk_transfer"
> [ 3199.220552] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.220761] usb-storage: Status code 0; transferred 31/31
> [ 3199.220767] usb-storage: usb_stor_bulk_transfer_sglist: xfer 36 bytes, 1 entries
> [ 3199.257161] usb-storage: Status code 0; transferred 36/36
> [ 3199.257168] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.257380] usb-storage: Status code 0; transferred 13/13
> [ 3199.269690] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.269896] usb-storage: Status code 0; transferred 31/31
> [ 3199.269903] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.270141] usb-storage: Status code 0; transferred 13/13
> [ 3199.270173] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.270390] usb-storage: Status code 0; transferred 31/31
> [ 3199.270396] usb-storage: usb_stor_bulk_transfer_sglist: xfer 18 bytes, 1 entries
> [ 3199.647916] usb-storage: Status code 0; transferred 18/18
> [ 3199.647924] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.648143] usb-storage: Status code 0; transferred 13/13
> [ 3199.648223] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.648393] usb-storage: Status code 0; transferred 31/31
> [ 3199.648399] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.648642] usb-storage: Status code 0; transferred 13/13
> [ 3199.648702] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.648892] usb-storage: Status code 0; transferred 31/31
> [ 3199.648897] usb-storage: usb_stor_bulk_transfer_sglist: xfer 8 bytes, 1 entries
> [ 3199.649648] usb-storage: Status code 0; transferred 8/8
> [ 3199.649655] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.649891] usb-storage: Status code 0; transferred 13/13
> [ 3199.649952] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.658891] usb-storage: Status code 0; transferred 31/31
> [ 3199.658897] usb-storage: usb_stor_bulk_transfer_sglist: xfer 192 bytes, 1 entries
> [ 3199.659233] usb-storage: Status code -121; transferred 36/192
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That isn't bad. It is correct: The host asked for 192 bytes and the
device sent only 36.
> [ 3199.659240] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.659401] usb-storage: Status code 0; transferred 13/13
> [ 3199.659544] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.659787] usb-storage: Status code 0; transferred 31/31
> [ 3199.659794] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.660054] usb-storage: Status code 0; transferred 13/13
> [ 3199.660914] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.661184] usb-storage: Status code 0; transferred 31/31
> [ 3199.661191] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.661793] usb-storage: Status code 0; transferred 13/13
> [ 3199.661824] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.662027] usb-storage: Status code 0; transferred 31/31
> [ 3199.662032] usb-storage: usb_stor_bulk_transfer_sglist: xfer 18 bytes, 1 entries
> [ 3199.662289] usb-storage: Status code 0; transferred 18/18
> [ 3199.662296] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.662526] usb-storage: Status code 0; transferred 13/13
> [ 3199.662614] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.662775] usb-storage: Status code 0; transferred 31/31
> [ 3199.662782] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.663025] usb-storage: Status code 0; transferred 13/13
> [ 3199.663086] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.663273] usb-storage: Status code 0; transferred 31/31
> [ 3199.663279] usb-storage: usb_stor_bulk_transfer_sglist: xfer 8 bytes, 1 entries
> [ 3199.664035] usb-storage: Status code 0; transferred 8/8
> [ 3199.664042] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.664277] usb-storage: Status code 0; transferred 13/13
> [ 3199.664338] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.664522] usb-storage: Status code 0; transferred 31/31
> [ 3199.664528] usb-storage: usb_stor_bulk_transfer_sglist: xfer 192 bytes, 1 entries
> [ 3199.664840] usb-storage: Status code -121; transferred 36/192
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That's not bad either, for the same reason.
> [ 3199.664848] usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
> [ 3199.665043] usb-storage: Status code 0; transferred 13/13
> [ 3199.665149] usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
> [ 3199.665273] usb-storage: Status code 0; transferred 31/31
> [ 3199.665279] usb-storage: usb_stor_bulk_transfer_sglist: xfer 4096 bytes, 1 entries
> [ 3230.081270] usb-storage: Status code -104; transferred 0/4096
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This probably _is_ an error.
> The error pattern seems quite clear looking at this particular log.
>
> Maybe the controller is just buggy and it's not able to handle "large"
> bulk transfers... I don't see a quick way to limit the size of bulk
> transfers, so I'd like to do a test adding an intermediate layer to
> split large bulk transfers into smaller one.
>
> BTW I also found this issue in the current scatter-gather communication
> in the EHCI layer:
>
> ---
> USB: EHCI: fix NULL pointer dererence in HCDs that use HCD_LOCAL_MEM
>
> If we use HCD_LOCAL_MEM flag and dma_declare_coherent_memory() to
> enforce local memory utilization we also need to disable native
> scatter-gather support, otherwise hcd_alloc_coherent() in
> map_urb_for_dma() is called with urb->transfer_buffer == NULL, that
> triggers a NULL pointer dereference.
>
> We can also consider to add some BUG_ON() to better catch this problem
> in the future.
That is generally not a good idea. Instead getting a warning with a
traceback message, the BUG_ON will crash the whole machine.
> [ At the moment this bug is not triggered by any driver, and probably
> this is not the best way to fix it, but at least this is sufficient to
> avoid annoying kernel oops for future drivers. ]
>
> Signed-off-by: Andrea Righi <arighi@xxxxxxxxxxx>
> ---
> drivers/usb/core/hcd.c | 4 ++++
> drivers/usb/host/ehci-hcd.c | 3 ++-
> 2 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 12742f1..4f1d9a2 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1219,6 +1219,8 @@ static int hcd_alloc_coherent(struct usb_bus *bus,
> {
> unsigned char *vaddr;
>
> + BUG_ON(*vaddr_handle == NULL);
> +
You'd be a lot better off putting in a WARN() message and returning an
error code.
> vaddr = hcd_buffer_alloc(bus, size + sizeof(vaddr),
> mem_flags, dma_handle);
> if (!vaddr)
> @@ -1248,6 +1250,8 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
> {
> unsigned char *vaddr = *vaddr_handle;
>
> + BUG_ON(*vaddr_handle == NULL);
> +
How can this ever happen?
> vaddr = (void *)get_unaligned((unsigned long *)(vaddr + size));
>
> if (dir == DMA_FROM_DEVICE)
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index a3ef2a9..27e0351 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -606,7 +606,8 @@ static int ehci_init(struct usb_hcd *hcd)
> ehci->command = temp;
>
> /* Accept arbitrarily long scatter-gather lists */
> - hcd->self.sg_tablesize = ~0;
> + if (!(hcd->driver->flags & HCD_LOCAL_MEM))
> + hcd->self.sg_tablesize = ~0;
> return 0;
> }
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
