On Thu, Apr 01, 2010 at 10:38:51AM -0700, Hrant Dalalyan wrote:
> Implementation of USB Attached SCSI Protocol per UASP Specification
> (Rev.1, July 9, 2008). Below is the list of the enhancements made to
> the usb-storage driver.
I've only started reviewing this. See comments inline.
One overarching comment -- I think you're overreaching with this single
patch. Implementing UASP support should be done first. Then, we can look
at adding multiple commands in-flight separately.
Most specifically, having both multi-command-queueing AND support for UASP
added in a single mega-patch makes it very very difficult to review. It
looks like there is a LOT of code duplication to avoid disturbing existing
code paths; that's an admirable goal, but I think this is NOT the way to go
about doing it.
If the existing infrastructure of the usb-storage driver, with a single
protocol and transport pointer isn't sufficient, then let's enhance the
framework. Otherwise, this driver might as well as be completely separate
from usb-storage, as just about the only parts it shares with the other
protocol/transport drivers is the top-level interface into the SCSI core.
This either needs to exist in a unified framework with all the other
sub-drivers, or should just be a separate driver completely. I would
prefer the former.
> diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
> index fc310f7..a8882f1 100644
> --- a/drivers/usb/storage/protocol.c
> +++ b/drivers/usb/storage/protocol.c
> @@ -119,7 +119,10 @@ void usb_stor_transparent_scsi_command(struct scsi_cmnd *srb,
> struct us_data *us)
> {
> /* send the command to the transport layer */
> - usb_stor_invoke_transport(srb, us);
> + if (us->protocol == US_PR_UASP)
> + usb_stor_invoke_UASP_transport(us);
> + else
> + usb_stor_invoke_transport(srb, us);
> }
> EXPORT_SYMBOL_GPL(usb_stor_transparent_scsi_command);
>
Why is this required? The first thing usb_stor_invoke_transport is call
whatever is in us->transport() -- why not just set us->transport to your
new usb_stor_USAP_transport()?
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index cfa26d5..dc8c602 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -299,10 +301,33 @@ static int queuecommand(struct scsi_cmnd *srb,
> return 0;
> }
>
> - /* enqueue the command and wake up the control thread */
> - srb->scsi_done = done;
> - us->srb = srb;
> - complete(&us->cmnd_ready);
> + if (us->protocol != US_PR_UASP) {
> + /* enqueue the command and wake up the control thread */
> + srb->scsi_done = done;
> + us->srb = srb;
> + complete(&us->cmnd_ready);
> + } else {
> + cmdiu = kzalloc(sizeof(struct cmd_iu), GFP_ATOMIC);
> + if (!cmdiu)
> + return SCSI_MLQUEUE_HOST_BUSY;
> +
> + cmdiu->cmd_iu_id = IU_ID_COMMAND;
> + cmdiu->ipt_tag = cpu_to_be16(usb_stor_get_tag(us));
> + cmdiu->length = cpu_to_be16(30);
> + cmdiu->lun[7] = srb->device->lun;
> + memcpy(cmdiu->cdb, srb->cmnd, srb->cmd_len);
> + cmdiu->cmd = srb;
> + cmdiu->cmd->scsi_done = done;
> + cmdiu->state = COMMAND_STATE_IDLE;
> + cmdiu->us = us;
> +
> + spin_lock_irqsave(&us->lock, flags);
> + list_add_tail(&cmdiu->node, &us->temp_scsi_cmnd_queue);
> + us->new_command = 1;
> + spin_unlock_irqrestore(&us->lock, flags);
> +
> + wake_up(&us->uasp_wq);
> + }
>
> return 0;
> }
Why are you doing this here, and not in your transport function? This
looks like you are framing the command for transit over the wire -- this is
a transport-level function, then.
Also, you should avoid allocation/deallocation in the command paths.
Allocate one (or a series of) struct cmd_iu when the module is loaded -- I
believe there is still a pointer in the struct us_data for such use.
> /* Command timeout and abort */
> static int command_abort(struct scsi_cmnd *srb)
> {
> struct us_data *us = host_to_us(srb->device->host);
> + struct cmd_iu *cmdiu;
> + unsigned long flags;
>
> US_DEBUGP("%s called\n", __func__);
>
> - /* us->srb together with the TIMED_OUT, RESETTING, and ABORTING
> - * bits are protected by the host lock. */
> - scsi_lock(us_to_host(us));
> + if (us->protocol != US_PR_UASP) {
> + /* us->srb together with the TIMED_OUT, RESETTING, and
> + * ABORTING bits are protected by the host lock.
> + */
> + scsi_lock(us_to_host(us));
>
> - /* Is this command still active? */
> - if (us->srb != srb) {
> + /* Is this command still active? */
> + if (us->srb != srb) {
> + scsi_unlock(us_to_host(us));
> + US_DEBUGP("-- nothing to abort\n");
> + return FAILED;
> + }
> + /* Set the TIMED_OUT bit. Also set the ABORTING bit, but only
> + * if a device reset isn't already in progress (to avoid
> + * interfering with the reset). Note that we must retain the
> + * host lock while calling usb_stor_stop_transport();
> + * otherwise it might interfere with an auto-reset that
> + * begins as soon as we release the lock.
> + */
> + set_bit(US_FLIDX_TIMED_OUT, &us->dflags);
> + if (!test_bit(US_FLIDX_RESETTING, &us->dflags)) {
> + set_bit(US_FLIDX_ABORTING, &us->dflags);
> + usb_stor_stop_transport(us);
> + }
> scsi_unlock(us_to_host(us));
> - US_DEBUGP ("-- nothing to abort\n");
> - return FAILED;
> - }
> -
> - /* Set the TIMED_OUT bit. Also set the ABORTING bit, but only if
> - * a device reset isn't already in progress (to avoid interfering
> - * with the reset). Note that we must retain the host lock while
> - * calling usb_stor_stop_transport(); otherwise it might interfere
> - * with an auto-reset that begins as soon as we release the lock. */
> - set_bit(US_FLIDX_TIMED_OUT, &us->dflags);
> - if (!test_bit(US_FLIDX_RESETTING, &us->dflags)) {
> + } else {
> + /* If we are disconnecting */
> + if (test_bit(US_FLIDX_DISCONNECTING, &us->dflags))
> + return FAILED;
> +
> + /* If reset bit is set */
> + if (test_bit(US_FLIDX_RESETTING, &us->dflags))
> + return FAILED;
> +
> + spin_lock_irqsave(&us->lock, flags);
> + cmdiu = find_cmd_by_ptr(us, srb);
> + spin_unlock_irqrestore(&us->lock, flags);
> + /* Is this command still active? */
> + if (!cmdiu)
> + return FAILED;
> +
> + spin_lock_irqsave(&us->lock, flags);
> + memset(us->abort_task_tmf, 0, TM_FUNCTION_IU_SIZE);
> + us->abort_task_tmf->cmdiu = cmdiu;
> + us->abort_task_tmf->tm_iu_id = IU_ID_TASK_MANAGEMENT;
> + us->abort_task_tmf->ipt_tag = cpu_to_be16(usb_stor_get_tag(us));
> + us->abort_task_tmf->tm_function = TM_FUNCTION_ABORT_TASK;
> + us->abort_task_tmf->task_tag = cmdiu->ipt_tag;
> + memcpy(us->abort_task_tmf->lun, cmdiu->lun, 8);
> + us->abort_task_tmf->state = COMMAND_STATE_IDLE;
> set_bit(US_FLIDX_ABORTING, &us->dflags);
> - usb_stor_stop_transport(us);
> + spin_unlock_irqrestore(&us->lock, flags);
> +
> + wake_up(&us->uasp_wq);
> }
> - scsi_unlock(us_to_host(us));
>
> /* Wait for the aborted command to finish */
> wait_for_completion(&us->notify);
> +
> return SUCCESS;
> }
This looks like a pretty major change to the entire error handling concept.
I mean, why would, when the SCSI layer attempts to cancel a command, you
need to do all this work? At the very least, comments explaining what is
going on here are necessary.
> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> index 589f6b4..e24a02f 100644
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
> @@ -175,7 +175,7 @@ static int usb_stor_msg_common(struct us_data *us, int timeout)
> /* wait for the completion of the URB */
> timeleft = wait_for_completion_interruptible_timeout(
> &urb_done, timeout ? : MAX_SCHEDULE_TIMEOUT);
> -
> +
> clear_bit(US_FLIDX_URB_ACTIVE, &us->dflags);
>
> if (timeleft <= 0) {
Patches shouldn't needlessly change whitespace.
> @@ -1305,3 +1305,1269 @@ int usb_stor_port_reset(struct us_data *us)
> }
> return result;
> }
> +
> +void usb_stor_transfer_UASP_sglist(struct work_struct *work)
> +{
> + struct stor_sg_req *sg_req = container_of(work,
> + struct stor_sg_req,
> + work);
> + struct us_data *us = sg_req->us;
> + struct cmd_iu *cmdiu = sg_req->cmdiu;
> + unsigned int pipe = cmdiu->cmd->sc_data_direction == DMA_FROM_DEVICE ?
> + us->recv_bulk_pipe : us->send_bulk_pipe;
> + unsigned long flags;
> + int tag;
> + int i;
> +
> + US_DEBUGP("%s called\n", __func__);
> +
> + /* The command is aborted by abort task or reset nexus */
> + if (cmdiu->state == COMMAND_STATE_ABORTED)
> + goto ret;
> +
> + /* The command is halted by abort task or reset nexus */
> + if (cmdiu->state == COMMAND_STATE_HALTED)
> + goto ret;
> +
> + /* Sense iu received earlier */
> + if (cmdiu->state == COMMAND_STATE_STATUS)
> + goto ret;
> +
> + /* Disconnect bit is set */
> + if (test_bit(US_FLIDX_DISCONNECTING, &us->dflags))
> + goto ret;
> +
> + /* Reset bit is set */
> + if (test_bit(US_FLIDX_RESETTING, &us->dflags))
> + goto ret;
> +
> + sg_req->result = usb_sg_init(&sg_req->sg_req,
> + us->pusb_dev,
> + pipe,
> + 0,
> + scsi_sglist(cmdiu->cmd),
> + scsi_sg_count(cmdiu->cmd),
> + scsi_bufflen(cmdiu->cmd),
> + GFP_NOIO);
> +
> + if (sg_req->result)
> + goto ret;
> +
> + /*
> + * workaround for setting stream_id for each urb of sg_request,
> + * this should be implemented in usbcore driver.
> + */
> + tag = be16_to_cpu(cmdiu->ipt_tag);
> + for (i = 0; i < sg_req->sg_req.entries; i++)
> + sg_req->sg_req.urbs[i]->stream_id = tag;
> +
> + /* wait for the completion of the transfer */
> + usb_sg_wait(&sg_req->sg_req);
> + scsi_set_resid(cmdiu->cmd, scsi_bufflen(cmdiu->cmd) -
> + sg_req->sg_req.bytes);
If this "should be implemented in usbcore driver", then implement it there.
Don't put it in usb-storage, or we're only going to have to go back and do
twice the work to fix it properly.
> +
> +ret:
> + spin_lock_irqsave(&us->lock, flags);
> + /* This means that status received earlier with error code */
> + if (cmdiu->state == COMMAND_STATE_STATUS)
> + cmdiu->iobuf_sts = REQ_COMPLETED;
> +
> + cmdiu->sgreq_sts = REQ_COMPLETED;
> +
> + us->active_requests--;
> + us->pending_requests++;
> + spin_unlock_irqrestore(&us->lock, flags);
> +
> + wake_up(&us->uasp_wq);
> +}
> +
> +static int usb_stor_transfer_UASP_buf(struct us_data *us,
> + unsigned int pipe,
> + struct urb *cur_urb,
> + struct stor_iobuf *iobuf,
> + unsigned int length,
> + unsigned short stream_id,
> + void (*urb_complete)(struct urb *urb),
> + void *context)
> +{
> + int result;
> +
> + US_DEBUGP("%s called\n", __func__);
> +
> + /* fill and submit the URB */
> + usb_fill_bulk_urb(cur_urb,
> + us->pusb_dev,
> + pipe,
> + iobuf->buf,
> + length,
> + urb_complete,
> + context);
> +
> + /* fill the common fields in the URB */
> + cur_urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
> + cur_urb->transfer_dma = iobuf->dma;
> + cur_urb->actual_length = 0;
> + cur_urb->error_count = 0;
> + cur_urb->status = 0;
> + cur_urb->stream_id = stream_id;
> +
> + /* submit the URB */
> + result = usb_submit_urb(cur_urb, GFP_NOIO);
> + if (result)
> + return result;
> +
> + return 0;
> +}
Why isn't this just a modification to usb_stor_msg_common() ??
> +static void usb_stor_cmd_urb_complete(struct urb *urb)
> +{
> + unsigned long flags;
> + struct cmd_iu *cmdiu = urb->context;
> + struct us_data *us = cmdiu->us;
> +
> + US_DEBUGP("%s called\n", __func__);
> +
> + us->command_pipe_sts = COMMAND_PIPE_IDLE;
> +
> + spin_lock_irqsave(&us->lock, flags);
> + cmdiu->iobuf_sts = REQ_COMPLETED;
> + us->active_requests--;
> + us->pending_requests++;
> + spin_unlock_irqrestore(&us->lock, flags);
> +
> + wake_up(&us->uasp_wq);
> +}
As I said earlier, UASP support should be implemented separately from all
the command-queuing stuff.
> +unsigned int usb_stor_get_tag(struct us_data *us)
> +{
> + US_DEBUGP("%s called\n", __func__);
> +
> + us->tag++;
> + if (us->tag > USB_STOR_NUM_STREAMS)
> + us->tag = 0;
> +
> + if (!us->tag)
> + us->tag++;
> +
> + return us->tag;
> +}
I would think a function like this should be declared static, not have the
usb_stor prefix on the name, and have a BIG comment on it noting what locks
need to be held when it is called.
This is just one of many functions to which this comment applies.
> diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
> index d4f034e..b9809d5 100644
> --- a/drivers/usb/storage/unusual_devs.h
> +++ b/drivers/usb/storage/unusual_devs.h
> @@ -1875,3 +1875,6 @@ USUAL_DEV(US_SC_QIC, US_PR_BULK, USB_US_TYPE_STOR),
> USUAL_DEV(US_SC_UFI, US_PR_BULK, USB_US_TYPE_STOR),
> USUAL_DEV(US_SC_8070, US_PR_BULK, USB_US_TYPE_STOR),
> USUAL_DEV(US_SC_SCSI, US_PR_BULK, 0),
> +
> +/* UASP transport */
> +USUAL_DEV(US_SC_SCSI, US_PR_UASP, 0),
Are these no longer Storage Class devices?
Skipped the rest for now pending addressing of earlier comments.
Matt
--
Matthew Dharm Home: mdharm-usb@xxxxxxxxxxxxxxxxxx
Maintainer, Linux USB Mass Storage Driver
A: The most ironic oxymoron wins ...
DP: "Microsoft Works"
A: Uh, okay, you win.
-- A.J. & Dust Puppy
User Friendly, 1/18/1998
Attachment:
pgpTr7hZ5oN4R.pgp
Description: PGP signature
