Re: [PATCH 2/2] uas: Make uas work with blk-mq
On Thu, Oct 02, 2014 at 09:25:50AM -0400, James Bottomley wrote:
OK, so 3.17 should be on sunday giving three days or so to sort this
out. If you're happy with blk-mq being disabled so the bug never
triggers unless the user activates it, doing nothing is my preferred
outcome ... but that also means no need to backport to stable.
If you want a it fixed in 3.17, then we need a patch from you now.
Ultimately you have to judge the necessity for us, since it's your
driver.
scsi-mq might be new and not default, but I wouldn't really consider it
experimental in the sense that people should expect crashes if they
attach a usb device.
Please consider applying the patch below for 3.17, as there is a cxgb
patch pending anyway:
---
From 8a42ea1369701960a821b7fa9a3da5c68e7e4366 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig h...@lst.de
Date: Fri, 3 Oct 2014 10:45:10 +0200
Subject: uas: disable use of blk-mq I/O path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The uas driver uses the block layer tag for USB3 stream IDs. With
blk-mq we can get larger tag numbers that the queue depth, which breaks
this assumption. A fix is under way for 3.18, but sits on top of
large changes so can't easily be backported. Set the disable_blk_mq
path so that a uas device can't easily crash the system when using
blk-mq for SCSI.
Signed-off-by: Christoph Hellwig h...@lst.de
Acked-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/storage/uas.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..9bfa725 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -970,6 +970,13 @@ static struct scsi_host_template uas_host_template = {
.cmd_per_lun = 1, /* until we override it */
.skip_settle_delay = 1,
.ordered_tag = 1,
+
+ /*
+* The uas drivers expects tags not to be bigger than the maximum
+* per-device queue depth, which is not true with the blk-mq tag
+* allocator.
+*/
+ .disable_blk_mq = true,
};
#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
Hi,
On 10/03/2014 10:47 AM, Christoph Hellwig wrote:
On Thu, Oct 02, 2014 at 09:25:50AM -0400, James Bottomley wrote:
OK, so 3.17 should be on sunday giving three days or so to sort this
out. If you're happy with blk-mq being disabled so the bug never
triggers unless the user activates it, doing nothing is my preferred
outcome ... but that also means no need to backport to stable.
If you want a it fixed in 3.17, then we need a patch from you now.
Ultimately you have to judge the necessity for us, since it's your
driver.
scsi-mq might be new and not default, but I wouldn't really consider it
experimental in the sense that people should expect crashes if they
attach a usb device.
Please consider applying the patch below for 3.17, as there is a cxgb
patch pending anyway:
For the record, and since I said before that I'm fine with leaving this as
is. As Christoph believes it is important to get this fixed for 3.17, lets
try to get this fix in.
So my Acked-by which is included below still stands.
Regards,
Hans
---
From 8a42ea1369701960a821b7fa9a3da5c68e7e4366 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig h...@lst.de
Date: Fri, 3 Oct 2014 10:45:10 +0200
Subject: uas: disable use of blk-mq I/O path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The uas driver uses the block layer tag for USB3 stream IDs. With
blk-mq we can get larger tag numbers that the queue depth, which breaks
this assumption. A fix is under way for 3.18, but sits on top of
large changes so can't easily be backported. Set the disable_blk_mq
path so that a uas device can't easily crash the system when using
blk-mq for SCSI.
Signed-off-by: Christoph Hellwig h...@lst.de
Acked-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/storage/uas.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..9bfa725 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -970,6 +970,13 @@ static struct scsi_host_template uas_host_template = {
.cmd_per_lun = 1, /* until we override it */
.skip_settle_delay = 1,
.ordered_tag = 1,
+
+ /*
+ * The uas drivers expects tags not to be bigger than the maximum
+ * per-device queue depth, which is not true with the blk-mq tag
+ * allocator.
+ */
+ .disable_blk_mq = true,
};
#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] uas: Make uas work with blk-mq
With uas over usb-3 the tags inside the uas iu-s must match the usb-3 stream
ids, and those go from 1 - qdepth.
Before blk-mq calling scsi_activate_tcq(sdev, qdepth) guaranteed that we would
only get cmnd-request-tag from 0 - (qdepth - 1), and we used those as
uas-tags / stream-ids.
With blk-mq however we are guaranteed to never get more then qdepth commands
queued at the same time, but the cmnd-request-tag values may be much larger,
which breaks uas.
This commit fixes this by generating uas tags in the 1 - qdepth range ourselves
instead of using cmnd-request-tag.
While touching all involved code anyways also rename the uas_cmd_info stream
field to uas_tag, because when using uas over usb-2 streams are not used.
Cc: Christoph Hellwig h...@infradead.org
Reported-by: Douglas Gilbert dgilb...@interlog.com
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/storage/uas.c | 57 +++
1 file changed, 23 insertions(+), 34 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d1dbe88..004ebc1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -66,7 +66,7 @@ enum {
/* Overrides scsi_pointer */
struct uas_cmd_info {
unsigned int state;
- unsigned int stream;
+ unsigned int uas_tag;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -173,30 +173,15 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd
*cmnd)
cmnd-result = sense_iu-status;
}
-/*
- * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
- * which go from 1 - nr_streams. And we use 1 for untagged commands.
- */
-static int uas_get_tag(struct scsi_cmnd *cmnd)
-{
- int tag;
-
- if (blk_rq_tagged(cmnd-request))
- tag = cmnd-request-tag + 2;
- else
- tag = 1;
-
- return tag;
-}
-
static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
int status)
{
struct uas_cmd_info *ci = (void *)cmnd-SCp;
+ struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
scmd_printk(KERN_INFO, cmnd,
- %s %d tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
- prefix, status, uas_get_tag(cmnd),
+ %s %d uas-tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
+ prefix, status, cmdinfo-uas_tag,
(ci-state SUBMIT_STATUS_URB) ? s-st : ,
(ci-state ALLOC_DATA_IN_URB) ? a-in : ,
(ci-state SUBMIT_DATA_IN_URB)? s-in : ,
@@ -242,7 +227,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
DATA_OUT_URB_INFLIGHT |
COMMAND_ABORTED))
return -EBUSY;
- devinfo-cmnd[uas_get_tag(cmnd) - 1] = NULL;
+ devinfo-cmnd[cmdinfo-uas_tag - 1] = NULL;
uas_free_unsubmitted_urbs(cmnd);
cmnd-scsi_done(cmnd);
return 0;
@@ -289,7 +274,7 @@ static void uas_stat_cmplt(struct urb *urb)
idx = be16_to_cpup(iu-tag) - 1;
if (idx = MAX_CMNDS || !devinfo-cmnd[idx]) {
dev_err(urb-dev-dev,
- stat urb: no pending cmd for tag %d\n, idx + 1);
+ stat urb: no pending cmd for uas-tag %d\n, idx + 1);
goto out;
}
@@ -427,7 +412,8 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info
*devinfo, gfp_t gfp,
goto out;
usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb-length,
uas_data_cmplt, cmnd);
- urb-stream_id = cmdinfo-stream;
+ if (devinfo-use_streams)
+ urb-stream_id = cmdinfo-uas_tag;
urb-num_sgs = udev-bus-sg_tablesize ? sdb-table.nents : 0;
urb-sg = sdb-table.sgl;
out:
@@ -451,7 +437,8 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info
*devinfo, gfp_t gfp,
usb_fill_bulk_urb(urb, udev, devinfo-status_pipe, iu, sizeof(*iu),
uas_stat_cmplt, cmnd-device-host);
- urb-stream_id = cmdinfo-stream;
+ if (devinfo-use_streams)
+ urb-stream_id = cmdinfo-uas_tag;
urb-transfer_flags |= URB_FREE_BUFFER;
out:
return urb;
@@ -465,6 +452,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info
*devinfo, gfp_t gfp,
{
struct usb_device *udev = devinfo-udev;
struct scsi_device *sdev = cmnd-device;
+ struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct command_iu *iu;
int len;
@@ -481,7 +469,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info
*devinfo, gfp_t gfp,
goto free;
iu-iu_id = IU_ID_COMMAND;
- iu-tag = cpu_to_be16(uas_get_tag(cmnd));
+ iu-tag = cpu_to_be16(cmdinfo-uas_tag);
iu-prio_attr = UAS_SIMPLE_TAG;
Re: [PATCH 2/2] uas: Make uas work with blk-mq
Looks fine to me (and actually removes code..) Reviewed-by: Christoph Hellwig h...@lst.de It's fairly late in 3.17 to get something like this in, but would you consider Ccing it to stable so we can get it into the first 3.17 stable release? -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
Hi, On 10/02/2014 01:26 PM, Christoph Hellwig wrote: Looks fine to me (and actually removes code..) Reviewed-by: Christoph Hellwig h...@lst.de It's fairly late in 3.17 to get something like this in, but would you consider Ccing it to stable so we can get it into the first 3.17 stable release? This patch sits on top of a whole bunch of uas cleanups / fixes which are going into 3.18, and which are to big of a change to backport. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
On Thu, Oct 02, 2014 at 02:35:10PM +0200, Hans de Goede wrote: This patch sits on top of a whole bunch of uas cleanups / fixes which are going into 3.18, and which are to big of a change to backport. Can add a patch to set disable_blk_mq for uas on 3.17 then? -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
Hi, On 10/02/2014 03:06 PM, Christoph Hellwig wrote: On Thu, Oct 02, 2014 at 02:35:10PM +0200, Hans de Goede wrote: This patch sits on top of a whole bunch of uas cleanups / fixes which are going into 3.18, and which are to big of a change to backport. Can add a patch to set disable_blk_mq for uas on 3.17 then? Yes, although that likely would need to go through stable, without going into 3.18 . Greg, is this ok with you and how do I get a patch in stable which is not in upstream (because upstream will get a better fix) ? Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
On Thu, Oct 02, 2014 at 03:08:40PM +0200, Hans de Goede wrote: Yes, although that likely would need to go through stable, without going into 3.18 . Greg, is this ok with you and how do I get a patch in stable which is not in upstream (because upstream will get a better fix) ? This is a trivial one liner and 3.17 isn't released yet, so can you just send it to Linus now? -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
Hi, On 10/02/2014 03:12 PM, Christoph Hellwig wrote: On Thu, Oct 02, 2014 at 03:08:40PM +0200, Hans de Goede wrote: Yes, although that likely would need to go through stable, without going into 3.18 . Greg, is this ok with you and how do I get a patch in stable which is not in upstream (because upstream will get a better fix) ? This is a trivial one liner and 3.17 isn't released yet, so can you just send it to Linus now? I don't think it is that urgent, given that block-mq is disabled by default in 3.17 AFAIK. But feel free to send such a patch directly to Linus with my: Acked-by: Hans de Goede hdego...@redhat.com Added, if that gets in, I guess I then need to do a v2 of the patch from $subject, reverting it. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
On Thu, 2014-10-02 at 15:18 +0200, Hans de Goede wrote: Hi, On 10/02/2014 03:12 PM, Christoph Hellwig wrote: On Thu, Oct 02, 2014 at 03:08:40PM +0200, Hans de Goede wrote: Yes, although that likely would need to go through stable, without going into 3.18 . Greg, is this ok with you and how do I get a patch in stable which is not in upstream (because upstream will get a better fix) ? This is a trivial one liner and 3.17 isn't released yet, so can you just send it to Linus now? I don't think it is that urgent, given that block-mq is disabled by default in 3.17 AFAIK. But feel free to send such a patch directly to Linus with my: Acked-by: Hans de Goede hdego...@redhat.com Added, if that gets in, I guess I then need to do a v2 of the patch from $subject, reverting it. OK, so 3.17 should be on sunday giving three days or so to sort this out. If you're happy with blk-mq being disabled so the bug never triggers unless the user activates it, doing nothing is my preferred outcome ... but that also means no need to backport to stable. If you want a it fixed in 3.17, then we need a patch from you now. Ultimately you have to judge the necessity for us, since it's your driver. James -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
On Thu, Oct 02, 2014 at 06:12:59AM -0700, Christoph Hellwig wrote: On Thu, Oct 02, 2014 at 03:08:40PM +0200, Hans de Goede wrote: Yes, although that likely would need to go through stable, without going into 3.18 . Greg, is this ok with you and how do I get a patch in stable which is not in upstream (because upstream will get a better fix) ? This is a trivial one liner and 3.17 isn't released yet, so can you just send it to Linus now? Let's just wait for these patches to get into 3.18-rc1, and then, Hans, you can send me a patch just for 3.17-stable with a note saying why it is different from what is in Linus's tree. Or send me that patch now and I can queue it up at the proper time. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] uas: Make uas work with blk-mq
Hi, On 10/02/2014 03:25 PM, James Bottomley wrote: On Thu, 2014-10-02 at 15:18 +0200, Hans de Goede wrote: Hi, On 10/02/2014 03:12 PM, Christoph Hellwig wrote: On Thu, Oct 02, 2014 at 03:08:40PM +0200, Hans de Goede wrote: Yes, although that likely would need to go through stable, without going into 3.18 . Greg, is this ok with you and how do I get a patch in stable which is not in upstream (because upstream will get a better fix) ? This is a trivial one liner and 3.17 isn't released yet, so can you just send it to Linus now? I don't think it is that urgent, given that block-mq is disabled by default in 3.17 AFAIK. But feel free to send such a patch directly to Linus with my: Acked-by: Hans de Goede hdego...@redhat.com Added, if that gets in, I guess I then need to do a v2 of the patch from $subject, reverting it. OK, so 3.17 should be on sunday giving three days or so to sort this out. If you're happy with blk-mq being disabled so the bug never triggers unless the user activates it, doing nothing is my preferred outcome ... but that also means no need to backport to stable. If you want a it fixed in 3.17, then we need a patch from you now. Ultimately you have to judge the necessity for us, since it's your driver. Fair enough. Given that this only triggers if the user enables an experimental option, and then mixes that with uas usage, my preferred way to deal with this is also doing nothing. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
