* Sarah Sharp | 2011-12-02 11:55:54 [-0800]:
>index 2598cbf..f61de1f 100644
>--- a/drivers/usb/storage/uas.c
>+++ b/drivers/usb/storage/uas.c
>@@ -144,19 +144,19 @@ static void uas_do_work(struct work_struct *work)
> list_replace_init(&uas_work_list, &list);
> spin_unlock_irq(&uas_work_lock);
>
>+ spin_lock_irq(&uas_work_lock);
> list_for_each_entry_safe(cmdinfo, temp, &list, list) {
> struct scsi_pointer *scp = (void *)cmdinfo;
> struct scsi_cmnd *cmnd = container_of(scp,
> struct scsi_cmnd, SCp);
> err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO);
>+ list_del_init(&cmdinfo->list);
> if (err) {
>- list_del(&cmdinfo->list);
>- spin_lock_irq(&uas_work_lock);
> list_add_tail(&cmdinfo->list, &uas_work_list);
>- spin_unlock_irq(&uas_work_lock);
> schedule_work(&uas_work);
> }
> }
>+ spin_unlock_irq(&uas_work_lock);
If you end up in the workqueue due to -ENOMEM and you downgrade
youreself to GFP_ATOMIC (like suggested in this thread) you probably do
better by just dropping the request :)
My recommendation:
- use a mutex while in the list_for_each loop to protect the wq against
uas_eh_abort_handler().
- add failed again on a new (local) list.
- once the loop is done, grab the spin lock and list_splice() them
together.
> }
>
> static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
>@@ -537,11 +538,47 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
>
> static DEF_SCSI_QCMD(uas_queuecommand)
>
>+static void uas_kill_tagged_urbs(struct usb_anchor *anchor,
>+ struct uas_cmd_info *cmdinfo)
>+{
>+ usb_kill_anchored_urbs(anchor);
>+
>+ /* Free any URBs that weren't submitted. */
>+ if (cmdinfo->state & SUBMIT_STATUS_URB) {
>+ kfree(cmdinfo->status_urb->transfer_buffer);
>+ usb_free_urb(cmdinfo->status_urb);
>+ }
>+ if (cmdinfo->state & SUBMIT_DATA_IN_URB) {
>+ kfree(cmdinfo->data_in_urb->transfer_buffer);
>+ usb_free_urb(cmdinfo->data_in_urb);
>+ }
>+ if (cmdinfo->state & SUBMIT_DATA_OUT_URB) {
>+ kfree(cmdinfo->data_out_urb->transfer_buffer);
>+ usb_free_urb(cmdinfo->data_out_urb);
>+ }
>+ if (cmdinfo->state & SUBMIT_CMD_URB) {
>+ kfree(cmdinfo->cmd_urb->transfer_buffer);
>+ usb_free_urb(cmdinfo->cmd_urb);
The command and status urb has the URB_FREE_BUFFER flag set. So you
kfree the tranfer_buffer twice for those two.
>+ }
>+}
>+
> static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
> {
> struct scsi_device *sdev = cmnd->device;
>+ struct uas_dev_info *devinfo = sdev->hostdata;
>+ struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
>+
> sdev_printk(KERN_INFO, sdev, "%s tag %d\n", __func__,
> cmnd->request->tag);
>+ spin_lock_irq(&uas_work_lock);
>+ if (!list_empty(&cmdinfo->list))
>+ list_del_init(&cmdinfo->list);
You used INIT_LIST_HEAD() on that one so you can use list_del() on it
without checking. If you plan to do this more than once you have to use
list_del_init().
>+ spin_unlock_irq(&uas_work_lock);
>+
>+ if (blk_rq_tagged(cmnd->request))
>+ uas_kill_tagged_urbs(&devinfo->anchors[cmnd->request->tag], cmdinfo);
>+ else
>+ uas_kill_tagged_urbs(&devinfo->anchors[0], cmdinfo);
>
> /* XXX: Send ABORT TASK Task Management command */
> return FAILED;
Sebastian
--
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
