Am Freitag, 18. Dezember 2009 01:07:55 schrieb Matthew Dharm:
> On Thu, Dec 17, 2009 at 03:13:22PM -0800, Arthur Jones wrote:
> > The Intel Value SSD has a bug where it will lock
> > solid if it encounters a delay between the command
> > and data phases of the usb bulk storage transactions.
> > This bug is easily reproduced by adding a 2.5sec
> > delay just before the comment labelled DATA STAGE
> > in drivers/usb/storage/transport.c in
> > usb_stor_Bulk_transport().
> >
> > We can't really guarantee to not hit this bug, but
> > if we decrease our nice level, we may be somewhat
> > less likely to see this already rare issue surface.
>
> I have two issues with this:
>
> 1) I'm not clear on the side effects of changing our priority. We've had
> issues in the past because block-storage devices are tied closely into the
> memory management system, and I'd hate to see another issue like those crop
> up because we're wagging our priority around. That said, I have no direct
> knowledge of anything that would cause an issue...
It seems to me that we'd have a serious problem if we were lowering our
priority. In fact I am not sure whether we correctly work as is if an RT
task does IO to a storage device.
> 2) All this patch does is cover up a pretty serious bug, without completely
> covering it up. The end-user still winds up with an unreliable storage
> device. With your patch, it may simply be a little less unreliable.
It may be possible to do this with an RT thread. Maybe something like
this untested version?
Regards
Oliver
diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index cc313d1..1de70a2 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -46,6 +46,7 @@
#include <linux/sched.h>
#include <linux/errno.h>
#include <linux/slab.h>
+#include <linux/sched.h>
#include <scsi/scsi.h>
#include <scsi/scsi_eh.h>
@@ -1029,6 +1030,8 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
int fake_sense = 0;
unsigned int cswlen;
unsigned int cbwlen = US_BULK_CB_WRAP_LEN;
+ struct sched_param paramrt = { .sched_priority = MAX_RT_PRIO-1 };
+ struct sched_param paramrr = { .sched_priority = 0 };
/* Take care of BULK32 devices; set extra byte to 0 */
if (unlikely(us->fflags & US_FL_BULK32)) {
@@ -1056,11 +1059,18 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
le32_to_cpu(bcb->DataTransferLength), bcb->Flags,
(bcb->Lun >> 4), (bcb->Lun & 0x0F),
bcb->Length);
+
+ /* some devices are critical in timing and do not allow delays*/
+ if (us->fflags & US_FL_NO_CMD_DELAY) {
+ preempt_disable();
+ /* we need to run again as soon as possible */
+ sched_setscheduler_nocheck(current, SCHED_FIFO, ¶mrt);
+ }
result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe,
bcb, cbwlen, NULL);
US_DEBUGP("Bulk command transfer result=%d\n", result);
if (result != USB_STOR_XFER_GOOD)
- return USB_STOR_TRANSPORT_ERROR;
+ goto err_out;
/* DATA STAGE */
/* send/receive data payload, if there is any */
@@ -1077,7 +1087,7 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
result = usb_stor_bulk_srb(us, pipe, srb);
US_DEBUGP("Bulk data transfer result 0x%x\n", result);
if (result == USB_STOR_XFER_ERROR)
- return USB_STOR_TRANSPORT_ERROR;
+ goto err_out;
/* If the device tried to send back more data than the
* amount requested, the spec requires us to transfer
@@ -1088,6 +1098,9 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
if (result == USB_STOR_XFER_LONG)
fake_sense = 1;
}
+
+ if (us->fflags & US_FL_NO_CMD_DELAY)
+ sched_setscheduler_nocheck(current, SCHED_NORMAL, ¶mrr);
/* See flow chart on pg 15 of the Bulk Only Transport spec for
* an explanation of how this code works.
@@ -1116,6 +1129,9 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe,
bcs, US_BULK_CS_WRAP_LEN, NULL);
}
+
+ if (us->fflags & US_FL_NO_CMD_DELAY)
+ preempt_enable();
/* if we still have a failure at this point, we're in trouble */
US_DEBUGP("Bulk status result = %d\n", result);
@@ -1199,6 +1215,13 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us)
/* we should never get here, but if we do, we're in trouble */
return USB_STOR_TRANSPORT_ERROR;
+
+err_out:
+ if (us->fflags & US_FL_NO_CMD_DELAY) {
+ sched_setscheduler_nocheck(current, SCHED_NORMAL, ¶mrr);
+ preempt_enable();
+ }
+ return USB_STOR_TRANSPORT_ERROR;
}
EXPORT_SYMBOL_GPL(usb_stor_Bulk_transport);
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index a4b947e..3c2ae4f 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -58,8 +58,9 @@
US_FLAG(CAPACITY_OK, 0x00010000) \
/* READ CAPACITY response is correct */ \
US_FLAG(BAD_SENSE, 0x00020000) \
- /* Bad Sense (never more than 18 bytes) */
-
+ /* Bad Sense (never more than 18 bytes) */ \
+ US_FLAG(NO_CMD_DELAY, 0x00040000) \
+ /* Do not to delay after command phase */
#define US_FLAG(name, value) US_FL_##name = value ,
enum { US_DO_ALL_FLAGS };
#undef US_FLAG
diff --git a/kernel/sched.c b/kernel/sched.c
index 18cceee..4496435 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6395,6 +6395,7 @@ int sched_setscheduler_nocheck(struct task_struct *p, int policy,
{
return __sched_setscheduler(p, policy, param, false);
}
+EXPORT_SYMBOL_GPL(sched_setscheduler_nocheck);
static int
do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
