Hi Oliver, ...
On Fri, Dec 18, 2009 at 10:28:53AM -0800, Oliver Neukum wrote:
> [...]
> > 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);
> + }
I think you're too late at this point, the real
delay is when we give up the cpu when we submit
the command. Here we're just making sure that
we run the data phase without delays. But the
chip doesn't seem to have a problem with that.
The chip bug (prob f/w bug) is hit by inserting
a delay between the command and the data phase...
Arthur
> 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
