On Sun, Jun 27, 2010 at 11:41:15AM -0400, Alan Stern wrote:
> On Sun, 27 Jun 2010, Andrea Righi wrote:
> > [ 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.
OK, thanks for the clarifications.
>
> > [ 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.
OK.
>
> > 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.
Agreed. I'll add the WARN() and post another patch soon.
>
> > 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?
Right. We already check this condition in hcd_alloc_coherent().
Thanks for reviewing,
-Andrea
--
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
