Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759495Ab3ICRjM (ORCPT ); Tue, 3 Sep 2013 13:39:12 -0400 Received: from mga09.intel.com ([134.134.136.24]:37358 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756920Ab3ICRjJ (ORCPT ); Tue, 3 Sep 2013 13:39:09 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.89,1015,1367996400"; d="scan'208";a="372854280" Date: Tue, 3 Sep 2013 10:39:08 -0700 From: Sarah Sharp To: Gerd Hoffmann Cc: linux-usb@vger.kernel.org, Matthew Wilcox , Matthew Dharm , Greg Kroah-Hartman , "open list:USB ATTACHED SCSI" , "open list:USB MASS STORAGE..." , open list Subject: Re: [PATCH 4/5] uas: add dead request list Message-ID: <20130903173908.GA4965@xanatos> References: <1378121129-32594-1-git-send-email-kraxel@redhat.com> <1378121129-32594-5-git-send-email-kraxel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1378121129-32594-5-git-send-email-kraxel@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7095 Lines: 188 Don't you need to send an ABORT TASK message to the device to cancel the outstanding request for that stream ID? I don't see that in this code. I see lots of URB cancellation code, but nothing to remove the request from the device-side queue. Does this code currently handle the case where a device refuses to respond to a SCSI request, and the upper layers attempt to cancel that request? Or does it simply ensure that SCSI bus reset works? I'm not comfortable with removing CONFIG_BROKEN until the upper layers can cancel one outstanding request. Plus, as Joe mentioned, this code is full of BUG_ON(), which is not friendly to users, and doesn't increase my confidence that the driver is ready to have CONFIG_BROKEN removed. Sarah Sharp On Mon, Sep 02, 2013 at 01:25:28PM +0200, Gerd Hoffmann wrote: > This patch adds a new list where all requests which are canceled are > added to, so we don't loose them. Then, after killing all inflight > urbs on bus reset (and disconnect) we'll walk over the list and clean > them up. > > Without this we can end up with aborted requests lingering around in > case of status pipe transfer errors. > > Signed-off-by: Gerd Hoffmann > --- > drivers/usb/storage/uas.c | 69 +++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 61 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index a63972a..9dfb8f9 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -78,6 +78,7 @@ struct uas_cmd_info { > struct urb *data_in_urb; > struct urb *data_out_urb; > struct list_head work; > + struct list_head dead; > }; > > /* I hate forward declarations, but I actually have a loop */ > @@ -87,10 +88,12 @@ static void uas_do_work(struct work_struct *work); > static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller); > static void uas_configure_endpoints(struct uas_dev_info *devinfo); > static void uas_free_streams(struct uas_dev_info *devinfo); > +static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller); > > static DECLARE_WORK(uas_work, uas_do_work); > static DEFINE_SPINLOCK(uas_lists_lock); > static LIST_HEAD(uas_work_list); > +static LIST_HEAD(uas_dead_list); > > static void uas_unlink_data_urbs(struct uas_dev_info *devinfo, > struct uas_cmd_info *cmdinfo) > @@ -167,15 +170,13 @@ static void uas_abort_work(struct uas_dev_info *devinfo) > struct uas_dev_info *di = (void *)cmnd->device->hostdata; > > if (di == devinfo) { > + uas_log_cmd_state(cmnd, __func__); > + BUG_ON(cmdinfo->state & COMMAND_ABORTED); > cmdinfo->state |= COMMAND_ABORTED; > + spin_lock_irq(&uas_lists_lock); > + list_add_tail(&cmdinfo->dead, &uas_dead_list); > + spin_unlock_irq(&uas_lists_lock); > cmdinfo->state &= ~IS_IN_WORK_LIST; > - if (devinfo->resetting) { > - /* uas_stat_cmplt() will not do that > - * when a device reset is in > - * progress */ > - cmdinfo->state &= ~COMMAND_INFLIGHT; > - } > - uas_try_complete(cmnd, __func__); > } else { > /* not our uas device, relink into list */ > list_del(&cmdinfo->work); > @@ -187,6 +188,43 @@ static void uas_abort_work(struct uas_dev_info *devinfo) > spin_unlock_irqrestore(&devinfo->lock, flags); > } > > +static void uas_zap_dead(struct uas_dev_info *devinfo) > +{ > + struct uas_cmd_info *cmdinfo; > + struct uas_cmd_info *temp; > + struct list_head list; > + unsigned long flags; > + > + spin_lock_irq(&uas_lists_lock); > + list_replace_init(&uas_dead_list, &list); > + spin_unlock_irq(&uas_lists_lock); > + > + spin_lock_irqsave(&devinfo->lock, flags); > + list_for_each_entry_safe(cmdinfo, temp, &list, dead) { > + struct scsi_pointer *scp = (void *)cmdinfo; > + struct scsi_cmnd *cmnd = container_of(scp, > + struct scsi_cmnd, SCp); > + struct uas_dev_info *di = (void *)cmnd->device->hostdata; > + > + if (di == devinfo) { > + uas_log_cmd_state(cmnd, __func__); > + BUG_ON(!(cmdinfo->state & COMMAND_ABORTED)); > + /* all urbs are killed, clear inflight bits */ > + cmdinfo->state &= ~(COMMAND_INFLIGHT | > + DATA_IN_URB_INFLIGHT | > + DATA_OUT_URB_INFLIGHT); > + uas_try_complete(cmnd, __func__); > + } else { > + /* not our uas device, relink into list */ > + list_del(&cmdinfo->dead); > + spin_lock_irq(&uas_lists_lock); > + list_add_tail(&cmdinfo->dead, &uas_dead_list); > + spin_unlock_irq(&uas_lists_lock); > + } > + } > + spin_unlock_irqrestore(&devinfo->lock, flags); > +} > + > static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd) > { > struct sense_iu *sense_iu = urb->transfer_buffer; > @@ -274,6 +312,9 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller) > if (cmdinfo->state & COMMAND_ABORTED) { > scmd_printk(KERN_INFO, cmnd, "abort completed\n"); > cmnd->result = DID_ABORT << 16; > + spin_lock_irq(&uas_lists_lock); > + list_del(&cmdinfo->dead); > + spin_unlock_irq(&uas_lists_lock); > } > cmnd->scsi_done(cmnd); > return 0; > @@ -307,7 +348,12 @@ static void uas_stat_cmplt(struct urb *urb) > u16 tag; > > if (urb->status) { > - dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status); > + if (urb->status == -ENOENT) { > + dev_err(&urb->dev->dev, "stat urb: killed (stream %d\n", > + urb->stream_id); > + } else { > + dev_err(&urb->dev->dev, "stat urb: status %d\n", urb->status); > + } > usb_free_urb(urb); > return; > } > @@ -762,7 +808,11 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd) > > uas_log_cmd_state(cmnd, __func__); > spin_lock_irqsave(&devinfo->lock, flags); > + BUG_ON(cmdinfo->state & COMMAND_ABORTED); > cmdinfo->state |= COMMAND_ABORTED; > + spin_lock_irq(&uas_lists_lock); > + list_add_tail(&cmdinfo->dead, &uas_dead_list); > + spin_unlock_irq(&uas_lists_lock); > if (cmdinfo->state & IS_IN_WORK_LIST) { > spin_lock(&uas_lists_lock); > list_del(&cmdinfo->work); > @@ -797,11 +847,13 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd) > struct usb_device *udev = devinfo->udev; > int err; > > + shost_printk(KERN_INFO, sdev->host, "%s start\n", __func__); > devinfo->resetting = 1; > uas_abort_work(devinfo); > usb_kill_anchored_urbs(&devinfo->cmd_urbs); > usb_kill_anchored_urbs(&devinfo->sense_urbs); > usb_kill_anchored_urbs(&devinfo->data_urbs); > + uas_zap_dead(devinfo); > uas_free_streams(devinfo); > err = usb_reset_device(udev); > if (!err) { > @@ -1055,6 +1107,7 @@ static void uas_disconnect(struct usb_interface *intf) > usb_kill_anchored_urbs(&devinfo->cmd_urbs); > usb_kill_anchored_urbs(&devinfo->sense_urbs); > usb_kill_anchored_urbs(&devinfo->data_urbs); > + uas_zap_dead(devinfo); > scsi_remove_host(shost); > uas_free_streams(devinfo); > kfree(devinfo); > -- > 1.8.3.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/