Re: [PATCH v2 01/11] scsi: Convert structScsi_Host->cmd_serial_number to atomic_t
From: Matthew Wilcox
Date: Mon Sep 27 2010 - 23:19:37 EST
On Fri, Sep 17, 2010 at 09:45:10PM -0500, Mike Christie wrote:
> You could also convert drivers to the host tagging if you needed a
> unique id for each command sent to a host.
I just wanted to make people aware of a problem with the host tagging
-- scsi_cmnd->tag is always 0!
Watch this:
scsi_prep_fn:
ret = scsi_setup_blk_pc_cmnd(sdev, req);
scsi_setup_blk_pc_cmnd:
cmd = scsi_get_cmd_from_req(sdev, req);
scsi_get_cmd_from_req:
cmd->tag = req->tag;
scsi_request_fn:
if (!(blk_queue_tagged(q) && !blk_queue_start_tag(q, req)))
blk_start_request(req);
blk_queue_start_tag
rq->tag = tag;
(and yes, they're called in this order)
I encountered this while writing the UAS driver, and decided to just
use the rq->tag directly instead. I would favour deleting scmd->tag in
order to save others the confusion.
In fact, looking at the users of scmd->tag, it appears there is a common
misconception that it contains the tag type (HEAD / SIMPLE / ORDERED). I
have no idea where this comes from, but it means the following patch to
delete scmd's ->tag is almost certainly wrong ... but it preserves the
existing behaviour!
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 5d2f148..37ff654 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1531,7 +1531,6 @@ part2:
tmp[0] = IDENTIFY(((instance->irq == SCSI_IRQ_NONE) ? 0 : 1), cmd->device->lun);
len = 1;
- cmd->tag = 0;
/* Send message(s) */
data = tmp;
@@ -2240,7 +2239,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) {
}
initialize_SCp(cmd->next_link);
/* The next command is still part of this process */
- cmd->next_link->tag = cmd->tag;
+ cmd->next_link->tag = 0;
cmd->result = cmd->SCp.Status | (cmd->SCp.Message << 8);
dprintk(NDEBUG_LINKED, ("scsi%d : target %d lun %d linked request done, calling scsi_done().\n", instance->host_no, cmd->device->id, cmd->device->lun));
collect_stats(hostdata, cmd);
@@ -2604,7 +2603,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance) {
do_abort(instance);
} else {
hostdata->connected = tmp;
- dprintk(NDEBUG_RESELECTION, ("scsi%d : nexus established, target = %d, lun = %d, tag = %d\n", instance->host_no, tmp->target, tmp->lun, tmp->tag));
+ dprintk(NDEBUG_RESELECTION, ("scsi%d : nexus established, target = %d, lun = %d, tag = %d\n", instance->host_no, tmp->target, tmp->lun, 0));
}
}
@@ -2789,7 +2788,7 @@ static int NCR5380_abort(Scsi_Cmnd * cmd) {
if (cmd == tmp) {
dprintk(NDEBUG_ABORT, ("scsi%d : aborting disconnected command.\n", instance->host_no));
- if (NCR5380_select(instance, cmd, (int) cmd->tag))
+ if (NCR5380_select(instance, cmd, 0))
return FAILED;
dprintk(NDEBUG_ABORT, ("scsi%d : nexus reestablished.\n", instance->host_no));
diff --git a/drivers/scsi/bfa/bfa_cb_ioim_macros.h b/drivers/scsi/bfa/bfa_cb_ioim_macros.h
index 3906ed9..57f2f7d 100644
--- a/drivers/scsi/bfa/bfa_cb_ioim_macros.h
+++ b/drivers/scsi/bfa/bfa_cb_ioim_macros.h
@@ -143,19 +143,8 @@ bfa_cb_ioim_get_taskattr(struct bfad_ioim_s *dio)
struct scsi_cmnd *cmnd = (struct scsi_cmnd *)dio;
u8 task_attr = UNTAGGED;
- if (cmnd->device->tagged_supported) {
- switch (cmnd->tag) {
- case HEAD_OF_QUEUE_TAG:
- task_attr = HEAD_OF_Q;
- break;
- case ORDERED_QUEUE_TAG:
- task_attr = ORDERED_Q;
- break;
- default:
- task_attr = SIMPLE_Q;
- break;
- }
- }
+ if (cmnd->device->tagged_supported)
+ task_attr = SIMPLE_Q;
return task_attr;
}
diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index ff6a28c..f506970 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -431,7 +431,7 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
sc->sdb.length = len;
sc->sdb.table.sgl = (void *) (unsigned long) addr;
- sc->tag = tag;
+ sc->request->tag = tag;
err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
cmd->tag);
if (err)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ee02d38..42e40de 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1031,8 +1031,6 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
cmd = req->special;
}
- /* pull a tag out of the request if we have one */
- cmd->tag = req->tag;
cmd->request = req;
cmd->cmnd = req->cmd;
diff --git a/drivers/scsi/scsi_tgt_if.c b/drivers/scsi/scsi_tgt_if.c
index a87e21c..c00e363 100644
--- a/drivers/scsi/scsi_tgt_if.c
+++ b/drivers/scsi/scsi_tgt_if.c
@@ -117,7 +117,7 @@ int scsi_tgt_uspace_send_cmd(struct scsi_cmnd *cmd, u64 itn_id,
ev.p.cmd_req.data_len = scsi_bufflen(cmd);
memcpy(ev.p.cmd_req.scb, cmd->cmnd, sizeof(ev.p.cmd_req.scb));
memcpy(ev.p.cmd_req.lun, lun, sizeof(ev.p.cmd_req.lun));
- ev.p.cmd_req.attribute = cmd->tag;
+ ev.p.cmd_req.attribute = 0;
ev.p.cmd_req.tag = tag;
dprintk("%p %d %u %x %llx\n", cmd, shost->host_no,
diff --git a/drivers/scsi/vmw_pvscsi.c b/drivers/scsi/vmw_pvscsi.c
index 2689445..39cae91 100644
--- a/drivers/scsi/vmw_pvscsi.c
+++ b/drivers/scsi/vmw_pvscsi.c
@@ -665,10 +665,6 @@ static int pvscsi_queue_ring(struct pvscsi_adapter *adapter,
memcpy(e->cdb, cmd->cmnd, e->cdbLen);
e->tag = SIMPLE_QUEUE_TAG;
- if (sdev->tagged_supported &&
- (cmd->tag == HEAD_OF_QUEUE_TAG ||
- cmd->tag == ORDERED_QUEUE_TAG))
- e->tag = cmd->tag;
if (cmd->sc_data_direction == DMA_FROM_DEVICE)
e->flags = PVSCSI_FLAG_CMD_DIR_TOHOST;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a5e885a..829e341 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -127,8 +127,6 @@ struct scsi_cmnd {
* to be at an address < 16Mb). */
int result; /* Status code from lower level driver */
-
- unsigned char tag; /* SCSI-II queued command tag */
};
extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/