On Fri, Dec 09, 2011 at 12:17:51PM +0100, Sebastian Andrzej Siewior wrote:
> * 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.
Or I can just rip the workqueue out all 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.
No, SUBMIT_CMD_URB and SUBMIT_STATUS_URB mean that the URBs have been
allocated, but they need to be submitted by the workqueue. So I won't
be freeing the buffer twice because the completion handler will never
run.
> >+ }
> >+}
> >+
> > 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().
Ok, sure.
Sarah Sharp
--
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
