UAS: eliminate infinite loop; add debug print

PATCH In review Luben Tuikov 5 participants 8 messages Oct 30, 2010 – Nov 8, 2010 1 message not in the archive

Subsystems (4)

SCSI USB USB mass storage driver USB attached SCSI

lore

Share & tools

lore

Luben Tuikov

author

2010-10-30 00:33 UTC #0 lore raw

PATCH 1/1 UAS: eliminate infinite loop; add debug print

Eliminate an infinite loop whereby the SCSI layer would reissue a command (which would be failed by the driver) ad infinitum. (Invariably due to the driver's profuse use of the SCSI_MLQUEUE_DEVICE_BUSY returned result in its queuecommand() method.)

Also add a debug option and a few debug prints.

Signed-off-by Luben Tuikov <[email protected]>

---
 drivers/usb/storage/Kconfig  |   10 ++++
 drivers/usb/storage/Makefile |    4 ++
 drivers/usb/storage/uas.c    |  102 +++++++++++++++++++++++++++--------------
 3 files changed, 81 insertions(+), 35 deletions(-)

drivers/usb/storage/Kconfig +10 -0

index 49a489e..eb9f6af 100644
@@ -185,6 +185,16 @@ config USB_UAS
 
 	  If you compile this driver as a module, it will be named uas.
 
+config USB_UAS_DEBUG
+       bool "Compile in debug mode"
+       default n
+       depends on USB_UAS
+       help
+	 Compiles the uas driver in debug mode. In debug mode,
+	 the driver prints debug messages to the console.
+
+	 If unsure, say 'N'.
+
 config USB_LIBUSUAL
 	bool "The shared table of common (or usual) storage devices"
 	depends on USB

drivers/usb/storage/Makefile +4 -0

index fcf14cd..16715ab 100644
@@ -10,6 +10,10 @@ ccflags-y := -Idrivers/scsi
 obj-$(CONFIG_USB_UAS)		+= uas.o
 obj-$(CONFIG_USB_STORAGE)	+= usb-storage.o
 
+ifeq ($(CONFIG_USB_UAS_DEBUG),y)
+	EXTRA_CFLAGS += -DUAS_DEBUG
+endif
+
 usb-storage-y := scsiglue.o protocol.o transport.o usb.o
 usb-storage-y += initializers.o sierra_ms.o option_ms.o

drivers/usb/storage/uas.c +67 ~~-35~~

217 lines not on this page · raw message

Signature · 1 line

1.7.0.1

Greg KH

2010-11-03 16:23 UTC #1 lore raw

Quoted 8 lines · Luben Tuikov

Eliminate an infinite loop whereby the SCSI layer would reissue a command (which would be failed by the driver) ad infinitum. (Invariably due to the driver's profuse use of the SCSI_MLQUEUE_DEVICE_BUSY returned result in its queuecommand() method.)
Also add a debug option and a few debug prints.

Matthew, any thoughts about this, and the other ones by Luben? I need your ack before I can take them.

thanks,

greg k-h

Matthew Wilcox

2010-11-08 11:24 UTC #2 lore raw

In reply to Luben Tuikov

Quoted 8 lines · Luben Tuikov

Eliminate an infinite loop whereby the SCSI layer would reissue a command (which would be failed by the driver) ad infinitum. (Invariably due to the driver's profuse use of the SCSI_MLQUEUE_DEVICE_BUSY returned result in its queuecommand() method.)
Also add a debug option and a few debug prints.

Why have you added your own debug scheme instead of using dev_dbg? You've made a lot of other random whitespace changes too. I'll pick through this and see what I like in it.

Quoted 279 lines

Signed-off-by Luben Tuikov <[email protected]>

---
 drivers/usb/storage/Kconfig  |   10 ++++
 drivers/usb/storage/Makefile |    4 ++
 drivers/usb/storage/uas.c    |  102 +++++++++++++++++++++++++++--------------
 3 files changed, 81 insertions(+), 35 deletions(-)

drivers/usb/storage/Kconfig +10 -0

index 49a489e..eb9f6af 100644
@@ -185,6 +185,16 @@ config USB_UAS
 
 	  If you compile this driver as a module, it will be named uas.
 
+config USB_UAS_DEBUG
+       bool "Compile in debug mode"
+       default n
+       depends on USB_UAS
+       help
+	 Compiles the uas driver in debug mode. In debug mode,
+	 the driver prints debug messages to the console.
+
+	 If unsure, say 'N'.
+
 config USB_LIBUSUAL
 	bool "The shared table of common (or usual) storage devices"
 	depends on USB

drivers/usb/storage/Makefile +4 -0

index fcf14cd..16715ab 100644
@@ -10,6 +10,10 @@ ccflags-y := -Idrivers/scsi
 obj-$(CONFIG_USB_UAS)		+= uas.o
 obj-$(CONFIG_USB_STORAGE)	+= usb-storage.o
 
+ifeq ($(CONFIG_USB_UAS_DEBUG),y)
+	EXTRA_CFLAGS += -DUAS_DEBUG
+endif
+
 usb-storage-y := scsiglue.o protocol.o transport.o usb.o
 usb-storage-y += initializers.o sierra_ms.o option_ms.o

drivers/usb/storage/uas.c +67 ~~-35~~

index 48dc2b8..ef6e707 100644
@@ -21,6 +21,14 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_tcq.h>
 
+#define uas_printk(fmt, ...)	printk(KERN_NOTICE "uas: " fmt, ## __VA_ARGS__)
+
+#ifdef UAS_DEBUG
+#define UAS_DPRINTK uas_printk
+#else
+#define UAS_DPRINTK(fmt, ...)
+#endif
+
 /* Common header for all IUs */
 struct iu {
 	__u8 iu_id;

@@ -128,6 +136,7 @@ static void uas_do_work(struct work_struct *work)
 {
 	struct uas_cmd_info *cmdinfo;
 	struct list_head list;
+	int res;
 
 	spin_lock_irq(&uas_work_lock);
 	list_replace_init(&uas_work_list, &list);

@@ -136,8 +145,10 @@ static void uas_do_work(struct work_struct *work)
 	list_for_each_entry(cmdinfo, &list, list) {
 		struct scsi_pointer *scp = (void *)cmdinfo;
 		struct scsi_cmnd *cmnd = container_of(scp,
-							struct scsi_cmnd, SCp);
-		uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_KERNEL);
+						      struct scsi_cmnd, SCp);
+		res = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_KERNEL);
+		UAS_DPRINTK("%s: cmd:%p, res:%d, state:0x%x\n", __FUNCTION__,
+			    cmnd, res, cmdinfo->state);
 	}
 }

@@ -198,13 +209,15 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
 }
 
 static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
-							unsigned direction)
+			  unsigned direction)
 {
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	int err;
 
 	cmdinfo->state = direction | SUBMIT_STATUS_URB;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
+	UAS_DPRINTK("%s: cmd:%p, err:%d, state:0x%x\n", __FUNCTION__,
+		    cmnd, err, cmdinfo->state);
 	if (err) {
 		spin_lock(&uas_work_lock);
 		list_add_tail(&cmdinfo->list, &uas_work_list);

@@ -222,7 +235,8 @@ 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);
+		dev_err(&urb->dev->dev, "%s: URB BAD STATUS %d\n",
+			__FUNCTION__, urb->status);
 		usb_free_urb(urb);
 		return;
 	}

@@ -259,6 +273,14 @@ static void uas_stat_cmplt(struct urb *urb)
 static void uas_data_cmplt(struct urb *urb)
 {
 	struct scsi_data_buffer *sdb = urb->context;
+
+	if (urb->status) {
+		dev_err(&urb->dev->dev, "%s: URB BAD STATUS %d\n",
+			__FUNCTION__, urb->status);
+		usb_free_urb(urb);
+		return;
+	}
+	
 	sdb->resid = sdb->length - urb->actual_length;
 	usb_free_urb(urb);
 }

@@ -339,7 +361,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 	memcpy(iu->cdb, cmnd->cmnd, cmnd->cmd_len);
 
 	usb_fill_bulk_urb(urb, udev, devinfo->cmd_pipe, iu, sizeof(*iu) + len,
-							usb_free_urb, NULL);
+			  usb_free_urb, NULL);
 	urb->transfer_flags |= URB_FREE_BUFFER;
  out:
 	return urb;

@@ -355,23 +377,26 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
  */
 
 static int uas_submit_urbs(struct scsi_cmnd *cmnd,
-					struct uas_dev_info *devinfo, gfp_t gfp)
+			   struct uas_dev_info *devinfo, gfp_t gfp)
 {
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+	int res;
 
 	if (cmdinfo->state & ALLOC_STATUS_URB) {
 		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
 							  cmdinfo->stream);
 		if (!cmdinfo->status_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return -ENOMEM;
 		cmdinfo->state &= ~ALLOC_STATUS_URB;
 	}
 
 	if (cmdinfo->state & SUBMIT_STATUS_URB) {
-		if (usb_submit_urb(cmdinfo->status_urb, gfp)) {
+		res = usb_submit_urb(cmdinfo->status_urb, gfp);
+		if (res) {
 			scmd_printk(KERN_INFO, cmnd,
-					"sense urb submission failure\n");
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+				    "sense urb submission failure (%d)\n",
+				    res);
+			return res;
 		}
 		cmdinfo->state &= ~SUBMIT_STATUS_URB;
 	}

@@ -381,15 +406,17 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 					devinfo->data_in_pipe, cmdinfo->stream,
 					scsi_in(cmnd), DMA_FROM_DEVICE);
 		if (!cmdinfo->data_in_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return -ENOMEM;
 		cmdinfo->state &= ~ALLOC_DATA_IN_URB;
 	}
 
 	if (cmdinfo->state & SUBMIT_DATA_IN_URB) {
-		if (usb_submit_urb(cmdinfo->data_in_urb, gfp)) {
+		res = usb_submit_urb(cmdinfo->data_in_urb, gfp);
+		if (res) {
 			scmd_printk(KERN_INFO, cmnd,
-					"data in urb submission failure\n");
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+				    "data in urb submission failure (%d)\n",
+				    res);
+			return res;
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_IN_URB;
 	}

@@ -399,15 +426,17 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 					devinfo->data_out_pipe, cmdinfo->stream,
 					scsi_out(cmnd), DMA_TO_DEVICE);
 		if (!cmdinfo->data_out_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return -ENOMEM;
 		cmdinfo->state &= ~ALLOC_DATA_OUT_URB;
 	}
 
 	if (cmdinfo->state & SUBMIT_DATA_OUT_URB) {
-		if (usb_submit_urb(cmdinfo->data_out_urb, gfp)) {
+		res = usb_submit_urb(cmdinfo->data_out_urb, gfp);
+		if (res) {
 			scmd_printk(KERN_INFO, cmnd,
-					"data out urb submission failure\n");
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+				    "data out urb submission failure (%d)\n",
+				    res);
+			return res;
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_OUT_URB;
 	}

@@ -416,15 +445,16 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 		cmdinfo->cmd_urb = uas_alloc_cmd_urb(devinfo, gfp, cmnd,
 							cmdinfo->stream);
 		if (!cmdinfo->cmd_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return -ENOMEM;
 		cmdinfo->state &= ~ALLOC_CMD_URB;
 	}
 
 	if (cmdinfo->state & SUBMIT_CMD_URB) {
-		if (usb_submit_urb(cmdinfo->cmd_urb, gfp)) {
+		res = usb_submit_urb(cmdinfo->cmd_urb, gfp);
+		if (res) {
 			scmd_printk(KERN_INFO, cmnd,
-					"cmd urb submission failure\n");
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+				    "cmd urb submission failure (%d)\n", res);
+			return res;
 		}
 		cmdinfo->state &= ~SUBMIT_CMD_URB;
 	}

@@ -433,12 +463,12 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 }
 
 static int uas_queuecommand(struct scsi_cmnd *cmnd,
-					void (*done)(struct scsi_cmnd *))
+			    void (*done)(struct scsi_cmnd *))
 {
 	struct scsi_device *sdev = cmnd->device;
 	struct uas_dev_info *devinfo = sdev->hostdata;
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
-	int err;
+	int res;
 
 	BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));

@@ -474,17 +504,19 @@ static int uas_queuecommand(struct scsi_cmnd *cmnd,
 		cmdinfo->stream = 0;
 	}
 
-	err = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
-	if (err) {
-		/* If we did nothing, give up now */
-		if (cmdinfo->state & SUBMIT_STATUS_URB) {
-			usb_free_urb(cmdinfo->status_urb);
-			return SCSI_MLQUEUE_DEVICE_BUSY;
-		}
-		spin_lock(&uas_work_lock);
-		list_add_tail(&cmdinfo->list, &uas_work_list);
-		spin_unlock(&uas_work_lock);
-		schedule_work(&uas_work);
+	res = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
+	UAS_DPRINTK("%s: cmd:%p (0x%02x), err:%d, state:0x%x\n", __FUNCTION__,
+		    cmnd, cmnd->cmnd[0], res, cmdinfo->state);
+	if (res) {
+		usb_unlink_urb(cmdinfo->status_urb);
+		usb_unlink_urb(cmdinfo->data_in_urb);
+		usb_unlink_urb(cmdinfo->data_out_urb);
+		usb_unlink_urb(cmdinfo->cmd_urb);
+
+		sdev->current_cmnd = NULL;
+
+		cmnd->result = DID_NO_CONNECT << 16;
+		done(cmnd);
 	}
 
 	return 0;

Signature · 1 line

1.7.0.1

Greg KH

2010-11-08 16:00 UTC #3 lore raw

Quoted 11 lines · Matthew Wilcox

Quoted 8 lines · Luben Tuikov
Eliminate an infinite loop whereby the SCSI layer would reissue a command (which would be failed by the driver) ad infinitum. (Invariably due to the driver's profuse use of the SCSI_MLQUEUE_DEVICE_BUSY returned result in its queuecommand() method.)
Also add a debug option and a few debug prints.
Why have you added your own debug scheme instead of using dev_dbg?

I agree, don't ever do that.

thanks,

greg k-h

Luben Tuikov

author

2010-11-08 17:27 UTC #4 lore raw

In reply to Matthew Wilcox

Quoted 8 lines · Matthew Wilcox

Quoted 7 lines · Luben Tuikov
Eliminate an infinite loop whereby the SCSI layer would reissue a command (which would be failed by the driver) ad infinitum. (Invariably due to the driver's profuse use of the SCSI_MLQUEUE_DEVICE_BUSY returned result in its queuecommand() method.)

Quoted 4 lines

Quoted 1 line
Also add a debug option and a few debug prints.
Why have you added your own debug scheme instead of using dev_dbg?

Because, this debug "scheme" produces copious and otherwise unnecessary volume of debug information in a working driver. And because you can turn it on/off just for this driver. It is intended to be used only for debugging this driver and the UAS device(s) to which the driver communicates. 99.9% of the time, this setting will be a 'N' out there. And this is what the Kconfig help entry says: "If unsure, say 'N'."

Quoted 2 lines

You've made a lot of other random whitespace changes too. I'll pick through this and see what I like in it.

Yes, it's my emacs. I didn't find a setting to right-justify a continuing argument list of a function definition, nor have I seen this in the kernel before (since 2000 A.D. at least). Have you tried a function declaration instead, in this style? Do you think it would look good in a header file with more than one declaration?

Quoted 459 lines

Quoted 4 lines
Signed-off-by Luben Tuikov <[email protected]>
---
drivers/usb/storage/Kconfig
| 10 ++++
Quoted 1 line
drivers/usb/storage/Makefile | 4
++
Quoted 1 line
drivers/usb/storage/uas.c |
102 +++++++++++++++++++++++++++--------------
Quoted 1 line
  3 files changed, 81 insertions(+), 35
deletions(-)
Review
Quoted 2 lines
(quoted diff) +0 -0
b/drivers/usb/storage/Kconfig
Quoted 6 lines
index 49a489e..eb9f6af 100644
drivers/usb/storage/Kconfig +0 -0
@@ -185,6 +185,16 @@ config USB_UAS
If you compile this
driver as a module, it will be named uas.
Quoted 3 lines

+config USB_UAS_DEBUG
+    bool "Compile in
debug mode"
Quoted 4 lines
+    default n
+    depends on USB_UAS
+    help
+     Compiles the uas driver
in debug mode. In debug mode,
Quoted 1 line
+     the driver prints debug
messages to the console.
Quoted 5 lines
drivers/usb/storage/Kconfig +3 -0
+
+     If unsure, say 'N'.
+
config USB_LIBUSUAL
bool "The shared table of
common (or usual) storage devices"
Review
Quoted 2 lines
depends on USB
(quoted diff) +0 -0
b/drivers/usb/storage/Makefile
drivers/usb/storage/Makefile ccflags-y := -Idrivers/scsi · line 10
Quoted 5 lines
index fcf14cd..16715ab 100644
drivers/usb/storage/Makefile +0 -0
@@ -10,6 +10,10 @@ ccflags-y := -Idrivers/scsi
   obj-$(CONFIG_USB_UAS)   
+= uas.o
Quoted 1 line
obj-$(CONFIG_USB_STORAGE) +=
usb-storage.o
Quoted 6 lines
drivers/usb/storage/Makefile +4 -0
+ifeq ($(CONFIG_USB_UAS_DEBUG),y)
+    EXTRA_CFLAGS += -DUAS_DEBUG
+endif
+
usb-storage-y := scsiglue.o protocol.o
transport.o usb.o
Quoted 1 line
usb-storage-y += initializers.o sierra_ms.o
option_ms.o
Review
Quoted 2 lines
(quoted diff) +0 -0
b/drivers/usb/storage/uas.c
Quoted 8 lines
index 48dc2b8..ef6e707 100644
drivers/usb/storage/uas.c +0 -0
@@ -21,6 +21,14 @@
   #include <scsi/scsi_host.h>
   #include <scsi/scsi_tcq.h>
+#define uas_printk(fmt, ...)
printk(KERN_NOTICE "uas: " fmt, ## __VA_ARGS__)
drivers/usb/storage/uas.c static void uas_do_work(struct
Quoted 11 lines
drivers/usb/storage/uas.c +7 -0
+
+#ifdef UAS_DEBUG
+#define UAS_DPRINTK uas_printk
+#else
+#define UAS_DPRINTK(fmt, ...)
+#endif
+
/* Common header for all IUs */
struct iu {
__u8 iu_id;
drivers/usb/storage/uas.c +0 -0
@@ -128,6 +136,7 @@ static void uas_do_work(struct
work_struct *work)
Quoted 2 lines
drivers/usb/storage/uas.c +0 -0
   {
struct uas_cmd_info
*cmdinfo;
Quoted 4 lines
struct list_head list;
+ int res;

spin_lock_irq(&uas_work_lock);
Quoted 1 line
list_replace_init(&uas_work_list, &list);
drivers/usb/storage/uas.c static void uas_do_work(struct
Quoted 1 line
drivers/usb/storage/uas.c +0 -0
@@ -136,8 +145,10 @@ static void uas_do_work(struct
work_struct *work)
drivers/usb/storage/uas.c static void uas_do_work(struct · line 145
Quoted 1 line
drivers/usb/storage/uas.c +0 -0
       list_for_each_entry(cmdinfo,
&list, list) {
Quoted 1 line
        struct
scsi_pointer *scp = (void *)cmdinfo;
Quoted 1 line
        struct
scsi_cmnd *cmnd = container_of(scp,
Quoted 1 line
-

        struct scsi_cmnd,
SCp);
Quoted 1 line
-
uas_submit_urbs(cmnd, cmnd->device->hostdata,
GFP_KERNEL);
Quoted 1 line
+

    struct scsi_cmnd,
SCp);
Quoted 1 line
+        res =
uas_submit_urbs(cmnd, cmnd->device->hostdata,
GFP_KERNEL);
Quoted 1 line
+
UAS_DPRINTK("%s: cmd:%p, res:%d, state:0x%x\n",
__FUNCTION__,
Quoted 1 line
+
    cmnd, res,
cmdinfo->state);
drivers/usb/storage/uas.c static void uas_sense_old(struct
Quoted 4 lines
}
}
drivers/usb/storage/uas.c +0 -0
@@ -198,13 +209,15 @@ static void uas_sense_old(struct
urb *urb, struct scsi_cmnd *cmnd)
Quoted 3 lines
drivers/usb/storage/uas.c +0 -0
   }
static void uas_xfer_data(struct urb *urb,
struct scsi_cmnd *cmnd,
Quoted 1 line
-

        unsigned direction)
Quoted 1 line
+
    unsigned direction)
Quoted 2 lines
{
    struct uas_cmd_info *cmdinfo
= (void *)&cmnd->SCp;
Quoted 3 lines
    int err;

    cmdinfo->state = direction
| SUBMIT_STATUS_URB;
Quoted 1 line
    err = uas_submit_urbs(cmnd,
cmnd->device->hostdata, GFP_ATOMIC);
Quoted 1 line
+    UAS_DPRINTK("%s: cmd:%p, err:%d,
state:0x%x\n", __FUNCTION__,
Quoted 1 line
+
cmnd, err, cmdinfo->state);
Quoted 2 lines
    if (err) {
spin_lock(&uas_work_lock);
Quoted 1 line
list_add_tail(&cmdinfo->list, &uas_work_list);
drivers/usb/storage/uas.c static void uas_stat_cmplt(struct
Quoted 1 line
drivers/usb/storage/uas.c +0 -0
@@ -222,7 +235,8 @@ static void uas_stat_cmplt(struct
urb *urb)
Quoted 4 lines
drivers/usb/storage/uas.c +0 -0
       u16 tag;
if (urb->status) {
-
dev_err(&urb->dev->dev, "URB BAD STATUS %d\n",
urb->status);
Quoted 1 line
+
dev_err(&urb->dev->dev, "%s: URB BAD STATUS
%d\n",
Quoted 1 line
+
__FUNCTION__, urb->status);
Quoted 1 line
usb_free_urb(urb);
drivers/usb/storage/uas.c static void uas_stat_cmplt(struct
Quoted 3 lines
return;
}
drivers/usb/storage/uas.c +0 -0
@@ -259,6 +273,14 @@ static void uas_stat_cmplt(struct
urb *urb)
Quoted 3 lines
drivers/usb/storage/uas.c +0 -0
   static void uas_data_cmplt(struct urb *urb)
   {
struct scsi_data_buffer *sdb
= urb->context;
Quoted 3 lines
+
+    if (urb->status) {
+
dev_err(&urb->dev->dev, "%s: URB BAD STATUS
%d\n",
Quoted 1 line
+
    __FUNCTION__, urb->status);
Quoted 1 line
+
usb_free_urb(urb);
Quoted 4 lines
+        return;
+    }
+
    sdb->resid =
sdb->length - urb->actual_length;
drivers/usb/storage/uas.c static struct urb
Quoted 3 lines
usb_free_urb(urb);
}
drivers/usb/storage/uas.c +0 -0
@@ -339,7 +361,7 @@ static struct urb
*uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
drivers/usb/storage/uas.c static struct urb · line 361
Quoted 1 line
drivers/usb/storage/uas.c +0 -0
       memcpy(iu->cdb,
cmnd->cmnd, cmnd->cmd_len);
Quoted 2 lines

    usb_fill_bulk_urb(urb, udev,
devinfo->cmd_pipe, iu, sizeof(*iu) + len,
Quoted 1 line
-

        usb_free_urb, NULL);
Quoted 1 line
+
    usb_free_urb, NULL);
Quoted 1 line
    urb->transfer_flags |=
URB_FREE_BUFFER;
drivers/usb/storage/uas.c static struct urb
Quoted 3 lines
out:
return urb;
drivers/usb/storage/uas.c +0 -0
@@ -355,23 +377,26 @@ static struct urb
*uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
Quoted 3 lines
drivers/usb/storage/uas.c +0 -0
    */
static int uas_submit_urbs(struct scsi_cmnd
*cmnd,
Quoted 1 line
-

struct uas_dev_info *devinfo, gfp_t gfp)
Quoted 1 line
+
       struct uas_dev_info
*devinfo, gfp_t gfp)
Quoted 2 lines
{
    struct uas_cmd_info *cmdinfo
= (void *)&cmnd->SCp;
Quoted 3 lines
+    int res;

    if (cmdinfo->state &
ALLOC_STATUS_URB) {
Quoted 1 line
cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
Quoted 1 line

cmdinfo->stream);
Quoted 1 line
        if
(!cmdinfo->status_urb)
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
    return -ENOMEM;
Quoted 1 line
cmdinfo->state &= ~ALLOC_STATUS_URB;
Quoted 3 lines
    }

    if (cmdinfo->state &
SUBMIT_STATUS_URB) {
Quoted 1 line
-        if
(usb_submit_urb(cmdinfo->status_urb, gfp)) {
Quoted 1 line
+        res =
usb_submit_urb(cmdinfo->status_urb, gfp);
Quoted 2 lines
+        if (res) {
    scmd_printk(KERN_INFO, cmnd,
Quoted 1 line
-

"sense urb submission failure\n");
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
        "sense
urb submission failure (%d)\n",
Quoted 1 line
+
        res);
Quoted 1 line
+
    return res;
Quoted 2 lines
        }
cmdinfo->state &= ~SUBMIT_STATUS_URB;
drivers/usb/storage/uas.c static int
Quoted 2 lines
}
drivers/usb/storage/uas.c +0 -0
@@ -381,15 +406,17 @@ static int
uas_submit_urbs(struct scsi_cmnd *cmnd,
Quoted 1 line

devinfo->data_in_pipe, cmdinfo->stream,
Quoted 1 line

scsi_in(cmnd), DMA_FROM_DEVICE);
Quoted 1 line
        if
(!cmdinfo->data_in_urb)
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
    return -ENOMEM;
Quoted 1 line
cmdinfo->state &= ~ALLOC_DATA_IN_URB;
Quoted 3 lines
    }

    if (cmdinfo->state &
SUBMIT_DATA_IN_URB) {
Quoted 1 line
-        if
(usb_submit_urb(cmdinfo->data_in_urb, gfp)) {
Quoted 1 line
+        res =
usb_submit_urb(cmdinfo->data_in_urb, gfp);
Quoted 2 lines
+        if (res) {
    scmd_printk(KERN_INFO, cmnd,
Quoted 1 line
-

"data in urb submission failure\n");
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
        "data in
urb submission failure (%d)\n",
Quoted 1 line
+
        res);
Quoted 1 line
+
    return res;
Quoted 2 lines
        }
cmdinfo->state &= ~SUBMIT_DATA_IN_URB;
drivers/usb/storage/uas.c static int
Quoted 2 lines
}
drivers/usb/storage/uas.c +0 -0
@@ -399,15 +426,17 @@ static int
uas_submit_urbs(struct scsi_cmnd *cmnd,
Quoted 1 line

devinfo->data_out_pipe, cmdinfo->stream,
Quoted 1 line

scsi_out(cmnd), DMA_TO_DEVICE);
Quoted 1 line
        if
(!cmdinfo->data_out_urb)
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
    return -ENOMEM;
Quoted 1 line
cmdinfo->state &= ~ALLOC_DATA_OUT_URB;
Quoted 3 lines
    }

    if (cmdinfo->state &
SUBMIT_DATA_OUT_URB) {
Quoted 1 line
-        if
(usb_submit_urb(cmdinfo->data_out_urb, gfp)) {
Quoted 1 line
+        res =
usb_submit_urb(cmdinfo->data_out_urb, gfp);
Quoted 2 lines
+        if (res) {
    scmd_printk(KERN_INFO, cmnd,
Quoted 1 line
-

"data out urb submission failure\n");
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
        "data
out urb submission failure (%d)\n",
Quoted 1 line
+
        res);
Quoted 1 line
+
    return res;
Quoted 2 lines
        }
cmdinfo->state &= ~SUBMIT_DATA_OUT_URB;
drivers/usb/storage/uas.c static int
Quoted 2 lines
}
drivers/usb/storage/uas.c +0 -0
@@ -416,15 +445,16 @@ static int
uas_submit_urbs(struct scsi_cmnd *cmnd,
Quoted 1 line
cmdinfo->cmd_urb = uas_alloc_cmd_urb(devinfo, gfp, cmnd,
Quoted 1 line

        cmdinfo->stream);
Quoted 1 line
        if
(!cmdinfo->cmd_urb)
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
    return -ENOMEM;
Quoted 1 line
cmdinfo->state &= ~ALLOC_CMD_URB;
Quoted 3 lines
    }

    if (cmdinfo->state &
SUBMIT_CMD_URB) {
Quoted 1 line
-        if
(usb_submit_urb(cmdinfo->cmd_urb, gfp)) {
Quoted 1 line
+        res =
usb_submit_urb(cmdinfo->cmd_urb, gfp);
Quoted 2 lines
+        if (res) {
    scmd_printk(KERN_INFO, cmnd,
Quoted 1 line
-

"cmd urb submission failure\n");
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 1 line
+
        "cmd urb
submission failure (%d)\n", res);
Quoted 1 line
+
    return res;
Quoted 2 lines
        }
cmdinfo->state &= ~SUBMIT_CMD_URB;
drivers/usb/storage/uas.c static int
Quoted 2 lines
}
drivers/usb/storage/uas.c +0 -0
@@ -433,12 +463,12 @@ static int
uas_submit_urbs(struct scsi_cmnd *cmnd,
Quoted 3 lines
drivers/usb/storage/uas.c +0 -0
   }
static int uas_queuecommand(struct scsi_cmnd
*cmnd,
Quoted 1 line
-

void (*done)(struct scsi_cmnd *))
Quoted 1 line
+
    void (*done)(struct
scsi_cmnd *))
Quoted 2 lines
{
    struct scsi_device *sdev =
cmnd->device;
Quoted 1 line
    struct uas_dev_info *devinfo
= sdev->hostdata;
Quoted 1 line
    struct uas_cmd_info *cmdinfo
= (void *)&cmnd->SCp;
Quoted 4 lines
-    int err;
+    int res;

    BUILD_BUG_ON(sizeof(struct
uas_cmd_info) > sizeof(struct scsi_pointer));
drivers/usb/storage/uas.c static int
Quoted 2 lines
drivers/usb/storage/uas.c +0 -0
@@ -474,17 +504,19 @@ static int
uas_queuecommand(struct scsi_cmnd *cmnd,
Quoted 1 line
cmdinfo->stream = 0;
Quoted 3 lines
    }

-    err = uas_submit_urbs(cmnd,
devinfo, GFP_ATOMIC);
Quoted 2 lines
-    if (err) {
-        /* If we did
nothing, give up now */
Quoted 1 line
-        if
(cmdinfo->state & SUBMIT_STATUS_URB) {
Quoted 1 line
-
    usb_free_urb(cmdinfo->status_urb);
Quoted 1 line
-
    return SCSI_MLQUEUE_DEVICE_BUSY;
Quoted 2 lines
-        }
-
spin_lock(&uas_work_lock);
Quoted 1 line
-
list_add_tail(&cmdinfo->list, &uas_work_list);
Quoted 1 line
-
spin_unlock(&uas_work_lock);
Quoted 1 line
-
schedule_work(&uas_work);
Quoted 1 line
+    res = uas_submit_urbs(cmnd,
devinfo, GFP_ATOMIC);
Quoted 1 line
+    UAS_DPRINTK("%s: cmd:%p (0x%02x),
err:%d, state:0x%x\n", __FUNCTION__,
Quoted 1 line
+
cmnd, cmnd->cmnd[0], res, cmdinfo->state);
Quoted 2 lines
+    if (res) {
+
usb_unlink_urb(cmdinfo->status_urb);
Quoted 1 line
+
usb_unlink_urb(cmdinfo->data_in_urb);
Quoted 1 line
+
usb_unlink_urb(cmdinfo->data_out_urb);

Quoted 15 lines

Quoted 1 line
+
usb_unlink_urb(cmdinfo->cmd_urb);
Quoted 2 lines
+
+
sdev->current_cmnd = NULL;
Quoted 2 lines
+
+ cmnd->result
= DID_NO_CONNECT << 16;
Quoted 6 lines
+        done(cmnd);
    }

    return 0;
Signature · 1 line
1.7.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 272723.33242.qm-5Es2LHxk9sSvuULXzWHTWIglqE1Y4D90QQ4Iyu8u01E@public.gmane.org

Greg KH

2010-11-08 17:38 UTC #6 lore raw

Quoted 14 lines · Luben Tuikov

Quoted 8 lines · Matthew Wilcox
Quoted 7 lines · Luben Tuikov
Eliminate an infinite loop whereby the SCSI layer would reissue a command (which would be failed by the driver) ad infinitum. (Invariably due to the driver's profuse use of the SCSI_MLQUEUE_DEVICE_BUSY returned result in its queuecommand() method.)
Quoted 4 lines
Quoted 1 line
Also add a debug option and a few debug prints.
Why have you added your own debug scheme instead of using dev_dbg?

Quoted 7 lines

Because, this debug "scheme" produces copious and otherwise unnecessary volume of debug information in a working driver. And because you can turn it on/off just for this driver. It is intended to be used only for debugging this driver and the UAS device(s) to which the driver communicates. 99.9% of the time, this setting will be a 'N' out there. And this is what the Kconfig help entry says: "If unsure, say 'N'."

dev_dbg() is able to be turned on and off dynamically when the kernel is running without rebuilding anything, which is what you really want to have happen for a driver being used by normal users.

It also produces the output in a standard manner that we have all agreed to follow.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Luben Tuikov

author

2010-11-08 17:57 UTC #7 lore raw

Quoted 43 lines · Greg KH

Quoted 1 line · Luben Tuikov
--- On Mon, 11/8/10, Matthew Wilcox <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
Quoted 1 line · Luben Tuikov
Eliminate an infinite loop whereby the SCSI
layer
Quoted 1 line
Quoted 1 line
Quoted 1 line
would reissue a command (which would be
failed by
Quoted 1 line
Quoted 1 line
Quoted 1 line
the driver) ad infinitum. (Invariably due to
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
driver's profuse use of the
SCSI_MLQUEUE_DEVICE_BUSY returned result in
its
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
queuecommand() method.)
Also add a debug option and a few debug
prints.
Quoted 2 lines
Quoted 2 lines
Why have you added your own debug scheme instead
of using
Quoted 3 lines
Quoted 1 line
dev_dbg?
Because, this debug "scheme" produces copious and
otherwise
Quoted 1 line
unnecessary volume of debug information in a working
driver. And
Quoted 1 line
because you can turn it on/off just for this driver.
It is intended to
Quoted 1 line
be used only for debugging this driver and the UAS
device(s) to which
Quoted 1 line
the driver communicates. 99.9% of the time, this
setting will be a 'N'
Quoted 1 line
out there. And this is what the Kconfig help
entry says: "If unsure,
Quoted 1 line
say 'N'."
dev_dbg() is able to be turned on and off dynamically when the kernel is
running without rebuilding anything, which is what you really want to
have happen for a driver being used by normal users.

Quoted 3 lines

It also produces the output in a standard manner that we have all agreed
to follow.

I'll modify the code in my tree and resubmit what I have now using dev_dbg. I'll also integrate the hint from Michal.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michał Nazarewicz

2010-11-08 17:40 UTC #8 lore raw

In reply to Luben Tuikov

Quoted 6 lines

Quoted 2 lines
Quoted 1 line · Luben Tuikov
Also add a debug option and a few debug prints.
Quoted 2 lines · Matthew Wilcox
Why have you added your own debug scheme instead of using dev_dbg?

Quoted 6 lines · Luben Tuikov

Because, this debug "scheme" produces copious and otherwise unnecessary volume of debug information in a working driver. And because you can turn it on/off just for this driver. It is intended to be used only for debugging this driver and the UAS device(s) to which the driver communicates. 99.9% of the time, this setting will be a 'N' out there. And this is what the Kconfig help entry says: "If unsure, say 'N'."

dev_dbg() and pr_debug() are no-ops when DEBUG is not defined, so proper way to do things is to use dev_dbg() and put the following before any #includes:

#ifdef CONFIG_USB_UAS_DEBUG
# define DEBUG 1
#endif

Signature · 8 lines

Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html