* [PATCH] [USB] UAS: eliminate infinite loop; add debug print
@ 2010-10-30 0:33 Luben Tuikov
2010-11-03 16:24 ` Greg KH
2010-11-08 11:24 ` Matthew Wilcox
0 siblings, 2 replies; 9+ messages in thread
From: Luben Tuikov @ 2010-10-30 0:33 UTC (permalink / raw)
To: Greg KH, linux-usb, linux-scsi, Matthew Wilcox
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 <ltuikov@yahoo.com>
---
drivers/usb/storage/Kconfig | 10 ++++
drivers/usb/storage/Makefile | 4 ++
drivers/usb/storage/uas.c | 102 +++++++++++++++++++++++++++--------------
3 files changed, 81 insertions(+), 35 deletions(-)
diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
index 49a489e..eb9f6af 100644
--- a/drivers/usb/storage/Kconfig
+++ b/drivers/usb/storage/Kconfig
@@ -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
diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
index fcf14cd..16715ab 100644
--- a/drivers/usb/storage/Makefile
+++ b/drivers/usb/storage/Makefile
@@ -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
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 48dc2b8..ef6e707 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -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;
--
1.7.0.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
2010-10-30 0:33 [PATCH] [USB] UAS: eliminate infinite loop; add debug print Luben Tuikov
@ 2010-11-03 16:24 ` Greg KH
2010-11-08 11:24 ` Matthew Wilcox
1 sibling, 0 replies; 9+ messages in thread
From: Greg KH @ 2010-11-03 16:24 UTC (permalink / raw)
To: Luben Tuikov, Matthew Wilcox; +Cc: linux-usb, linux-scsi
On Fri, Oct 29, 2010 at 05:33:19PM -0700, Luben Tuikov wrote:
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
2010-10-30 0:33 [PATCH] [USB] UAS: eliminate infinite loop; add debug print Luben Tuikov
2010-11-03 16:24 ` Greg KH
@ 2010-11-08 11:24 ` Matthew Wilcox
2010-11-08 15:55 ` Greg KH
2010-11-08 17:27 ` Luben Tuikov
1 sibling, 2 replies; 9+ messages in thread
From: Matthew Wilcox @ 2010-11-08 11:24 UTC (permalink / raw)
To: Luben Tuikov; +Cc: Greg KH, linux-usb, linux-scsi
On Fri, Oct 29, 2010 at 05:33:19PM -0700, Luben Tuikov wrote:
> 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.
> Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> ---
> drivers/usb/storage/Kconfig | 10 ++++
> drivers/usb/storage/Makefile | 4 ++
> drivers/usb/storage/uas.c | 102 +++++++++++++++++++++++++++--------------
> 3 files changed, 81 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
> index 49a489e..eb9f6af 100644
> --- a/drivers/usb/storage/Kconfig
> +++ b/drivers/usb/storage/Kconfig
> @@ -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
> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
> index fcf14cd..16715ab 100644
> --- a/drivers/usb/storage/Makefile
> +++ b/drivers/usb/storage/Makefile
> @@ -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
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 48dc2b8..ef6e707 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -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;
> --
> 1.7.0.1
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
2010-11-08 11:24 ` Matthew Wilcox
@ 2010-11-08 15:55 ` Greg KH
2010-11-08 17:27 ` Luben Tuikov
1 sibling, 0 replies; 9+ messages in thread
From: Greg KH @ 2010-11-08 15:55 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Luben Tuikov, linux-usb, linux-scsi
On Mon, Nov 08, 2010 at 06:24:56AM -0500, Matthew Wilcox wrote:
> On Fri, Oct 29, 2010 at 05:33:19PM -0700, Luben Tuikov wrote:
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
2010-11-08 11:24 ` Matthew Wilcox
2010-11-08 15:55 ` Greg KH
@ 2010-11-08 17:27 ` Luben Tuikov
[not found] ` <272723.33242.qm-5Es2LHxk9sSvuULXzWHTWIglqE1Y4D90QQ4Iyu8u01E@public.gmane.org>
2010-11-08 17:40 ` Michał Nazarewicz
1 sibling, 2 replies; 9+ messages in thread
From: Luben Tuikov @ 2010-11-08 17:27 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Greg KH, linux-usb, linux-scsi
--- On Mon, 11/8/10, Matthew Wilcox <willy@linux.intel.com> wrote:
> -0700, Luben Tuikov wrote:
> > 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?
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'."
> 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?
>
> > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > ---
> > drivers/usb/storage/Kconfig
> | 10 ++++
> > drivers/usb/storage/Makefile | 4
> ++
> > drivers/usb/storage/uas.c |
> 102 +++++++++++++++++++++++++++--------------
> > 3 files changed, 81 insertions(+), 35
> deletions(-)
> >
> > diff --git a/drivers/usb/storage/Kconfig
> b/drivers/usb/storage/Kconfig
> > index 49a489e..eb9f6af 100644
> > --- a/drivers/usb/storage/Kconfig
> > +++ b/drivers/usb/storage/Kconfig
> > @@ -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
> > diff --git a/drivers/usb/storage/Makefile
> b/drivers/usb/storage/Makefile
> > index fcf14cd..16715ab 100644
> > --- a/drivers/usb/storage/Makefile
> > +++ b/drivers/usb/storage/Makefile
> > @@ -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
> >
> > diff --git a/drivers/usb/storage/uas.c
> b/drivers/usb/storage/uas.c
> > index 48dc2b8..ef6e707 100644
> > --- a/drivers/usb/storage/uas.c
> > +++ b/drivers/usb/storage/uas.c
> > @@ -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;
> > --
> > 1.7.0.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread[parent not found: <272723.33242.qm-5Es2LHxk9sSvuULXzWHTWIglqE1Y4D90QQ4Iyu8u01E@public.gmane.org>]
* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
[not found] ` <272723.33242.qm-5Es2LHxk9sSvuULXzWHTWIglqE1Y4D90QQ4Iyu8u01E@public.gmane.org>
@ 2010-11-08 17:38 ` Greg KH
2010-11-08 17:57 ` Luben Tuikov
0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2010-11-08 17:38 UTC (permalink / raw)
To: Luben Tuikov
Cc: Matthew Wilcox, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-scsi-u79uwXL29TY76Z2rM5mHXA
On Mon, Nov 08, 2010 at 09:27:22AM -0800, Luben Tuikov wrote:
> --- On Mon, 11/8/10, Matthew Wilcox <willy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> > -0700, Luben Tuikov wrote:
> > > 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?
>
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
2010-11-08 17:38 ` Greg KH
@ 2010-11-08 17:57 ` Luben Tuikov
0 siblings, 0 replies; 9+ messages in thread
From: Luben Tuikov @ 2010-11-08 17:57 UTC (permalink / raw)
To: Greg KH; +Cc: Matthew Wilcox, linux-usb, linux-scsi
--- On Mon, 11/8/10, Greg KH <greg@kroah.com> wrote:
> On Mon, Nov 08, 2010 at 09:27:22AM
> -0800, Luben Tuikov wrote:
> > --- On Mon, 11/8/10, Matthew Wilcox <willy@linux.intel.com>
> wrote:
> > > -0700, Luben Tuikov wrote:
> > > > 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?
> >
> > 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.
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
2010-11-08 17:27 ` Luben Tuikov
[not found] ` <272723.33242.qm-5Es2LHxk9sSvuULXzWHTWIglqE1Y4D90QQ4Iyu8u01E@public.gmane.org>
@ 2010-11-08 17:40 ` Michał Nazarewicz
1 sibling, 0 replies; 9+ messages in thread
From: Michał Nazarewicz @ 2010-11-08 17:40 UTC (permalink / raw)
To: Matthew Wilcox, Luben Tuikov; +Cc: Greg KH, linux-usb, linux-scsi
>> -0700, Luben Tuikov wrote:
>> > Also add a debug option and a few debug prints.
> --- On Mon, 11/8/10, Matthew Wilcox <willy@linux.intel.com> wrote:
>> Why have you added your own debug scheme instead of using
>> dev_dbg?
On Mon, 08 Nov 2010 18:27:22 +0100, Luben Tuikov <ltuikov@yahoo.com> wrote:
> 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
--
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
@ 2010-12-10 10:51 Luben Tuikov
0 siblings, 0 replies; 9+ messages in thread
From: Luben Tuikov @ 2010-12-10 10:51 UTC (permalink / raw)
To: Greg KH, linux-usb, linux-scsi, Matthew Wilcox
Patch retracted for this still-born driver. Instead please use superior uasp.c which was recently submitted.
--- On Fri, 10/29/10, Luben Tuikov <ltuikov@yahoo.com> wrote:
> From: Luben Tuikov <ltuikov@yahoo.com>
> Subject: [PATCH] [USB] UAS: eliminate infinite loop; add debug print
> To: "Greg KH" <greg@kroah.com>, linux-usb@vger.kernel.org, linux-scsi@vger.kernel.org, "Matthew Wilcox" <willy@linux.intel.com>
> Date: Friday, October 29, 2010, 5:33 PM
> 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 <ltuikov@yahoo.com>
> ---
> drivers/usb/storage/Kconfig | 10
> ++++
> drivers/usb/storage/Makefile | 4 ++
> drivers/usb/storage/uas.c | 102
> +++++++++++++++++++++++++++--------------
> 3 files changed, 81 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/storage/Kconfig
> b/drivers/usb/storage/Kconfig
> index 49a489e..eb9f6af 100644
> --- a/drivers/usb/storage/Kconfig
> +++ b/drivers/usb/storage/Kconfig
> @@ -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
> diff --git a/drivers/usb/storage/Makefile
> b/drivers/usb/storage/Makefile
> index fcf14cd..16715ab 100644
> --- a/drivers/usb/storage/Makefile
> +++ b/drivers/usb/storage/Makefile
> @@ -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
>
> diff --git a/drivers/usb/storage/uas.c
> b/drivers/usb/storage/uas.c
> index 48dc2b8..ef6e707 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -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;
> --
> 1.7.0.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-12-10 10:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-30 0:33 [PATCH] [USB] UAS: eliminate infinite loop; add debug print Luben Tuikov
2010-11-03 16:24 ` Greg KH
2010-11-08 11:24 ` Matthew Wilcox
2010-11-08 15:55 ` Greg KH
2010-11-08 17:27 ` Luben Tuikov
[not found] ` <272723.33242.qm-5Es2LHxk9sSvuULXzWHTWIglqE1Y4D90QQ4Iyu8u01E@public.gmane.org>
2010-11-08 17:38 ` Greg KH
2010-11-08 17:57 ` Luben Tuikov
2010-11-08 17:40 ` Michał Nazarewicz
-- strict thread matches above, loose matches on Subject: below --
2010-12-10 10:51 Luben Tuikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).