On Thu, 22 Mar 2012, Ming Lei wrote: > On Thu, Mar 22, 2012 at 3:30 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > This patch (as1517) fixes an error in the USB scatter-gather library. > > The library code uses urb->dev to determine whether or nor an URB is > > currently active; the completion handler sets urb->dev to NULL. > > However the core unlinking routines need to use urb->dev. �Since > > unlinking always racing with completion, the completion handler must > > not clear urb->dev -- it can lead to invalid memory accesses when a > > transfer has to be cancelled. > > > > This patch fixes the problem by getting rid of the lines that clear > > urb->dev after urb has been submitted. �As a result we may end up > > trying to unlink an URB that failed in submission or that has already > > completed, so an extra check is added after each unlink to avoid > > printing an error message when this happens. > > > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Reported-and-tested-by: Illia Zaitsev <I.Zaitsev@xxxxxxxxxxxxx> > > CC: <stable@xxxxxxxxxxxxxxx> > > > > --- > > > > �drivers/usb/core/message.c | � �6 ++---- > > �1 file changed, 2 insertions(+), 4 deletions(-) > > > > Index: usb-3.3/drivers/usb/core/message.c > > =================================================================== > > --- usb-3.3.orig/drivers/usb/core/message.c > > +++ usb-3.3/drivers/usb/core/message.c > > @@ -308,7 +308,8 @@ static void sg_complete(struct urb *urb) > > � � � � � � � � � � � � � � � �retval = usb_unlink_urb(io->urbs [i]); > > � � � � � � � � � � � � � � � �if (retval != -EINPROGRESS && > > � � � � � � � � � � � � � � � � � �retval != -ENODEV && > > - � � � � � � � � � � � � � � � � � retval != -EBUSY) > > + � � � � � � � � � � � � � � � � � retval != -EBUSY && > > + � � � � � � � � � � � � � � � � � retval != -EIDRM) > > � � � � � � � � � � � � � � � � � � � �dev_err(&io->dev->dev, > > � � � � � � � � � � � � � � � � � � � � � � � �"%s, unlink --> %d\n", > > � � � � � � � � � � � � � � � � � � � � � � � �__func__, retval); > > @@ -317,7 +318,6 @@ static void sg_complete(struct urb *urb) > > � � � � � � � �} > > � � � � � � � �spin_lock(&io->lock); > > � � � �} > > - � � � urb->dev = NULL; > > Given the above is removed, the check on urb->dev in sg_complete > and sg_cancel may be removed too. They could be removed, but leaving them in place stops the code from trying to unlink URBs that haven't been submitted yet. They don't do any harm, so they may as well stay. > > � � � �/* on the last completion, signal usb_sg_wait() */ > > � � � �io->bytes += urb->actual_length; > > @@ -524,7 +524,6 @@ void usb_sg_wait(struct usb_sg_request * > > � � � � � � � �case -ENXIO: � �/* hc didn't queue this one */ > > � � � � � � � �case -EAGAIN: > > � � � � � � � �case -ENOMEM: > > - � � � � � � � � � � � io->urbs[i]->dev = NULL; > > � � � � � � � � � � � �retval = 0; > > � � � � � � � � � � � �yield(); > > � � � � � � � � � � � �break; > > @@ -542,7 +541,6 @@ void usb_sg_wait(struct usb_sg_request * > > > > � � � � � � � � � � � �/* fail any uncompleted urbs */ > > � � � � � � � �default: > > - � � � � � � � � � � � io->urbs[i]->dev = NULL; > > Maybe the check of (retval != -EIDRM) should be added in sg_cancel() too, > otherwise warning will be printed even though the URB has been > completed. The same is true for -ENODEV. And in fact, those error messages do show up if you unplug a USB flash drive while a data transfer is in progress. Do you think checks for both values should be added in this patch, or just -EIDRM? 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
