* [PATCH, RFC] hide EH backup data outside the scsi_cmnd
@ 2006-06-03 11:26 Christoph Hellwig
2006-06-03 11:31 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Christoph Hellwig @ 2006-06-03 11:26 UTC (permalink / raw)
To: jejb; +Cc: fischer, linux-kernel
Currently struct scsi_cmnd has various fields that are used to backup
original data after the corresponding fields have been overridden for
EH commands. This means drivers can easily get at it and misuse it.
Due to the old_ naming this doesn't happen for most of them, but two
that have different names have been used wrong a lot (see previous
patch). Another downside is that they unessecarily bloat the scsi_cmnd
size.
This patch moves them onstack in scsi_send_eh_cmnd to fix those two
issues above.
Note that this breaks the aha152x and 53x700 drivers because they're
plaing with those fields internally.
J"urgen and James, could you take a look at whether it's feasible to
fix those drivers?
Otherwise any comments on the general approach?
(Note: you probably want the patch to remove the wrong buffer and
bufflen patches applied before this, else lots of drivers will stop
compiling)
Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_error.c 2006-06-02 18:20:23.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_error.c 2006-06-03 12:31:05.000000000 +0200
@@ -438,19 +438,67 @@
* Return value:
* SUCCESS or FAILED or NEEDS_RETRY
**/
-static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, int timeout)
+static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, int timeout, int copy_sense)
{
struct scsi_device *sdev = scmd->device;
struct Scsi_Host *shost = sdev->host;
+ int old_result = scmd->result;
DECLARE_COMPLETION(done);
unsigned long timeleft;
unsigned long flags;
+ unsigned char old_cmnd[MAX_COMMAND_SIZE];
+ enum dma_data_direction old_data_direction;
+ unsigned short old_use_sg;
+ unsigned char old_cmd_len;
+ unsigned old_bufflen;
+ void *old_buffer;
int rtn;
+ /*
+ * We need saved copies of a number of fields - this is because
+ * error handling may need to overwrite these with different values
+ * to run different commands, and once error handling is complete,
+ * we will need to restore these values prior to running the actual
+ * command.
+ */
+ old_buffer = scmd->request_buffer;
+ old_bufflen = scmd->request_bufflen;
+ memcpy(old_cmnd, scmd->cmnd, sizeof(scmd->cmnd));
+ old_data_direction = scmd->sc_data_direction;
+ old_cmd_len = scmd->cmd_len;
+ old_use_sg = scmd->use_sg;
+
+ if (copy_sense) {
+ int gfp_mask = GFP_ATOMIC;
+
+ if (shost->hostt->unchecked_isa_dma)
+ gfp_mask |= __GFP_DMA;
+
+ scmd->sc_data_direction = DMA_FROM_DEVICE;
+ scmd->request_bufflen = 252;
+ scmd->request_buffer = kzalloc(scmd->request_bufflen, gfp_mask);
+ if (!scmd->request_buffer)
+ return FAILED;
+ } else {
+ scmd->request_buffer = NULL;
+ scmd->request_bufflen = 0;
+ scmd->sc_data_direction = DMA_NONE;
+ }
+
+ scmd->underflow = 0;
+ scmd->use_sg = 0;
+ scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
+
if (sdev->scsi_level <= SCSI_2)
scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
(sdev->lun << 5 & 0xe0);
+ /*
+ * Zero the sense buffer. The scsi spec mandates that any
+ * untransferred sense data should be interpreted as being zero.
+ */
+ memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
+
shost->eh_action = &done;
scmd->request->rq_status = RQ_SCSI_BUSY;
@@ -502,6 +550,27 @@
rtn = FAILED;
}
+
+ /*
+ * Last chance to have valid sense data.
+ */
+ if (copy_sense && !SCSI_SENSE_VALID(scmd)) {
+ memcpy(scmd->sense_buffer, scmd->request_buffer,
+ sizeof(scmd->sense_buffer));
+ kfree(scmd->request_buffer);
+ }
+
+
+ /*
+ * Restore original data
+ */
+ scmd->request_buffer = old_buffer;
+ scmd->request_bufflen = old_bufflen;
+ memcpy(scmd->cmnd, old_cmnd, sizeof(scmd->cmnd));
+ scmd->sc_data_direction = old_data_direction;
+ scmd->cmd_len = old_cmd_len;
+ scmd->use_sg = old_use_sg;
+ scmd->result = old_result;
return rtn;
}
@@ -517,56 +586,10 @@
static int scsi_request_sense(struct scsi_cmnd *scmd)
{
static unsigned char generic_sense[6] =
- {REQUEST_SENSE, 0, 0, 0, 252, 0};
- unsigned char *scsi_result;
- int saved_result;
- int rtn;
+ {REQUEST_SENSE, 0, 0, 0, 252, 0};
memcpy(scmd->cmnd, generic_sense, sizeof(generic_sense));
-
- scsi_result = kmalloc(252, GFP_ATOMIC | ((scmd->device->host->hostt->unchecked_isa_dma) ? __GFP_DMA : 0));
-
-
- if (unlikely(!scsi_result)) {
- printk(KERN_ERR "%s: cannot allocate scsi_result.\n",
- __FUNCTION__);
- return FAILED;
- }
-
- /*
- * zero the sense buffer. some host adapters automatically always
- * request sense, so it is not a good idea that
- * scmd->request_buffer and scmd->sense_buffer point to the same
- * address (db). 0 is not a valid sense code.
- */
- memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
- memset(scsi_result, 0, 252);
-
- saved_result = scmd->result;
- scmd->request_buffer = scsi_result;
- scmd->request_bufflen = 252;
- scmd->use_sg = 0;
- scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
- scmd->sc_data_direction = DMA_FROM_DEVICE;
- scmd->underflow = 0;
-
- rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
-
- /* last chance to have valid sense data */
- if(!SCSI_SENSE_VALID(scmd)) {
- memcpy(scmd->sense_buffer, scmd->request_buffer,
- sizeof(scmd->sense_buffer));
- }
-
- kfree(scsi_result);
-
- /*
- * when we eventually call scsi_finish, we really wish to complete
- * the original request, so let's restore the original data. (db)
- */
- scsi_setup_cmd_retry(scmd);
- scmd->result = saved_result;
- return rtn;
+ return scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT, 1);
}
/**
@@ -585,12 +608,6 @@
{
scmd->device->host->host_failed--;
scmd->eh_eflags = 0;
-
- /*
- * set this back so that the upper level can correctly free up
- * things.
- */
- scsi_setup_cmd_retry(scmd);
list_move_tail(&scmd->eh_entry, done_q);
}
EXPORT_SYMBOL(scsi_eh_finish_cmd);
@@ -695,47 +712,26 @@
{
static unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
int retry_cnt = 1, rtn;
- int saved_result;
retry_tur:
memcpy(scmd->cmnd, tur_command, sizeof(tur_command));
- /*
- * zero the sense buffer. the scsi spec mandates that any
- * untransferred sense data should be interpreted as being zero.
- */
- memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
- saved_result = scmd->result;
- scmd->request_buffer = NULL;
- scmd->request_bufflen = 0;
- scmd->use_sg = 0;
- scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
- scmd->underflow = 0;
- scmd->sc_data_direction = DMA_NONE;
+ rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT, 0);
- rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
-
- /*
- * when we eventually call scsi_finish, we really wish to complete
- * the original request, so let's restore the original data. (db)
- */
- scsi_setup_cmd_retry(scmd);
- scmd->result = saved_result;
-
- /*
- * hey, we are done. let's look to see what happened.
- */
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd %p rtn %x\n",
__FUNCTION__, scmd, rtn));
- if (rtn == SUCCESS)
- return 0;
- else if (rtn == NEEDS_RETRY) {
+
+ switch (rtn) {
+ case NEEDS_RETRY:
if (retry_cnt--)
goto retry_tur;
+ /*FALLTHRU*/
+ case SUCCESS:
return 0;
+ default:
+ return 1;
}
- return 1;
}
/**
@@ -817,44 +813,16 @@
static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
{
static unsigned char stu_command[6] = {START_STOP, 0, 0, 0, 1, 0};
- int rtn;
- int saved_result;
-
- if (!scmd->device->allow_restart)
- return 1;
-
- memcpy(scmd->cmnd, stu_command, sizeof(stu_command));
-
- /*
- * zero the sense buffer. the scsi spec mandates that any
- * untransferred sense data should be interpreted as being zero.
- */
- memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
- saved_result = scmd->result;
- scmd->request_buffer = NULL;
- scmd->request_bufflen = 0;
- scmd->use_sg = 0;
- scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
- scmd->underflow = 0;
- scmd->sc_data_direction = DMA_NONE;
+ if (scmd->device->allow_restart) {
+ int rtn;
- rtn = scsi_send_eh_cmnd(scmd, START_UNIT_TIMEOUT);
-
- /*
- * when we eventually call scsi_finish, we really wish to complete
- * the original request, so let's restore the original data. (db)
- */
- scsi_setup_cmd_retry(scmd);
- scmd->result = saved_result;
+ memcpy(scmd->cmnd, stu_command, sizeof(stu_command));
+ rtn = scsi_send_eh_cmnd(scmd, START_UNIT_TIMEOUT, 0);
+ if (rtn == SUCCESS)
+ return 0;
+ }
- /*
- * hey, we are done. let's look to see what happened.
- */
- SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd %p rtn %x\n",
- __FUNCTION__, scmd, rtn));
- if (rtn == SUCCESS)
- return 0;
return 1;
}
@@ -1663,8 +1631,6 @@
scmd->scsi_done = scsi_reset_provider_done_command;
scmd->done = NULL;
- scmd->buffer = NULL;
- scmd->bufflen = 0;
scmd->request_buffer = NULL;
scmd->request_bufflen = 0;
Index: scsi-misc-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_lib.c 2006-06-02 18:20:23.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_lib.c 2006-06-03 12:31:05.000000000 +0200
@@ -502,60 +502,16 @@
*
* Arguments: cmd - command that is ready to be queued.
*
- * Returns: Nothing
- *
* Notes: This function has the job of initializing a number of
* fields related to error handling. Typically this will
* be called once for each command, as required.
*/
-static int scsi_init_cmd_errh(struct scsi_cmnd *cmd)
+static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
{
cmd->serial_number = 0;
-
memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);
-
if (cmd->cmd_len == 0)
cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
-
- /*
- * We need saved copies of a number of fields - this is because
- * error handling may need to overwrite these with different values
- * to run different commands, and once error handling is complete,
- * we will need to restore these values prior to running the actual
- * command.
- */
- cmd->old_use_sg = cmd->use_sg;
- cmd->old_cmd_len = cmd->cmd_len;
- cmd->sc_old_data_direction = cmd->sc_data_direction;
- cmd->old_underflow = cmd->underflow;
- memcpy(cmd->data_cmnd, cmd->cmnd, sizeof(cmd->cmnd));
- cmd->buffer = cmd->request_buffer;
- cmd->bufflen = cmd->request_bufflen;
-
- return 1;
-}
-
-/*
- * Function: scsi_setup_cmd_retry()
- *
- * Purpose: Restore the command state for a retry
- *
- * Arguments: cmd - command to be restored
- *
- * Returns: Nothing
- *
- * Notes: Immediately prior to retrying a command, we need
- * to restore certain fields that we saved above.
- */
-void scsi_setup_cmd_retry(struct scsi_cmnd *cmd)
-{
- memcpy(cmd->cmnd, cmd->data_cmnd, sizeof(cmd->data_cmnd));
- cmd->request_buffer = cmd->buffer;
- cmd->request_bufflen = cmd->bufflen;
- cmd->use_sg = cmd->old_use_sg;
- cmd->cmd_len = cmd->old_cmd_len;
- cmd->sc_data_direction = cmd->sc_old_data_direction;
- cmd->underflow = cmd->old_underflow;
}
void scsi_device_unbusy(struct scsi_device *sdev)
@@ -873,22 +829,13 @@
*/
static void scsi_release_buffers(struct scsi_cmnd *cmd)
{
- struct request *req = cmd->request;
-
- /*
- * Free up any indirection buffers we allocated for DMA purposes.
- */
if (cmd->use_sg)
scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
- else if (cmd->request_buffer != req->buffer)
- kfree(cmd->request_buffer);
/*
* Zero these out. They now point to freed memory, and it is
* dangerous to hang onto the pointers.
*/
- cmd->buffer = NULL;
- cmd->bufflen = 0;
cmd->request_buffer = NULL;
cmd->request_bufflen = 0;
}
@@ -925,7 +872,7 @@
unsigned int block_bytes)
{
int result = cmd->result;
- int this_count = cmd->bufflen;
+ int this_count = cmd->request_bufflen;
request_queue_t *q = cmd->device->request_queue;
struct request *req = cmd->request;
int clear_errors = 1;
@@ -933,28 +880,14 @@
int sense_valid = 0;
int sense_deferred = 0;
- /*
- * Free up any indirection buffers we allocated for DMA purposes.
- * For the case of a READ, we need to copy the data out of the
- * bounce buffer and into the real buffer.
- */
- if (cmd->use_sg)
- scsi_free_sgtable(cmd->buffer, cmd->sglist_len);
- else if (cmd->buffer != req->buffer) {
- if (rq_data_dir(req) == READ) {
- unsigned long flags;
- char *to = bio_kmap_irq(req->bio, &flags);
- memcpy(to, cmd->buffer, cmd->bufflen);
- bio_kunmap_irq(to, &flags);
- }
- kfree(cmd->buffer);
- }
+ scsi_release_buffers(cmd);
if (result) {
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
if (sense_valid)
sense_deferred = scsi_sense_is_deferred(&sshdr);
}
+
if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
req->errors = result;
if (result) {
@@ -975,15 +908,6 @@
}
/*
- * Zero these out. They now point to freed memory, and it is
- * dangerous to hang onto the pointers.
- */
- cmd->buffer = NULL;
- cmd->bufflen = 0;
- cmd->request_buffer = NULL;
- cmd->request_bufflen = 0;
-
- /*
* Next deal with any sectors which we were able to correctly
* handle.
*/
@@ -1083,7 +1007,7 @@
if (!(req->flags & REQ_QUIET)) {
scmd_printk(KERN_INFO, cmd,
"Volume overflow, CDB: ");
- __scsi_print_command(cmd->data_cmnd);
+ __scsi_print_command(cmd->cmnd);
scsi_print_sense("", cmd);
}
scsi_end_request(cmd, 0, block_bytes, 1);
@@ -1222,7 +1146,7 @@
* successfully. Since this is a REQ_BLOCK_PC command the
* caller should check the request's errors value
*/
- scsi_io_completion(cmd, cmd->bufflen, 0);
+ scsi_io_completion(cmd, cmd->request_bufflen, 0);
}
static void scsi_setup_blk_pc_cmnd(struct scsi_cmnd *cmd)
Index: scsi-misc-2.6/drivers/scsi/scsi.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi.c 2006-06-02 18:20:23.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi.c 2006-06-03 12:31:05.000000000 +0200
@@ -420,7 +420,7 @@
if (level > 3) {
printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
" done = 0x%p, queuecommand 0x%p\n",
- cmd->buffer, cmd->bufflen,
+ cmd->request_buffer, cmd->request_bufflen,
cmd->done,
sdev->host->hostt->queuecommand);
@@ -678,10 +678,7 @@
cmd->use_sg = sreq->sr_use_sg;
cmd->request = sreq->sr_request;
- memcpy(cmd->data_cmnd, sreq->sr_cmnd, sizeof(cmd->data_cmnd));
cmd->serial_number = 0;
- cmd->bufflen = sreq->sr_bufflen;
- cmd->buffer = sreq->sr_buffer;
cmd->retries = 0;
cmd->allowed = sreq->sr_allowed;
cmd->done = sreq->sr_done;
@@ -699,12 +696,8 @@
memset(cmd->sense_buffer, 0, sizeof(sreq->sr_sense_buffer));
cmd->request_buffer = sreq->sr_buffer;
cmd->request_bufflen = sreq->sr_bufflen;
- cmd->old_use_sg = cmd->use_sg;
if (cmd->cmd_len == 0)
cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
- cmd->old_cmd_len = cmd->cmd_len;
- cmd->sc_old_data_direction = cmd->sc_data_direction;
- cmd->old_underflow = cmd->underflow;
/*
* Start the timer ticking.
@@ -784,11 +777,6 @@
*/
int scsi_retry_command(struct scsi_cmnd *cmd)
{
- /*
- * Restore the SCSI command state.
- */
- scsi_setup_cmd_retry(cmd);
-
/*
* Zero the sense information from the last time we tried
* this command.
@@ -836,11 +824,6 @@
"(result %x)\n", cmd->result));
/*
- * We can get here with use_sg=0, causing a panic in the upper level
- */
- cmd->use_sg = cmd->old_use_sg;
-
- /*
* If there is an associated request structure, copy the data over
* before we call the completion function.
*/
Index: scsi-misc-2.6/include/scsi/scsi_cmnd.h
===================================================================
--- scsi-misc-2.6.orig/include/scsi/scsi_cmnd.h 2006-06-03 12:07:51.000000000 +0200
+++ scsi-misc-2.6/include/scsi/scsi_cmnd.h 2006-06-03 12:31:05.000000000 +0200
@@ -64,9 +64,7 @@
int timeout_per_command;
unsigned char cmd_len;
- unsigned char old_cmd_len;
enum dma_data_direction sc_data_direction;
- enum dma_data_direction sc_old_data_direction;
/* These elements define the operation we are about to perform */
#define MAX_COMMAND_SIZE 16
@@ -77,18 +75,11 @@
void *request_buffer; /* Actual requested buffer */
/* These elements define the operation we ultimately want to perform */
- unsigned char data_cmnd[MAX_COMMAND_SIZE];
- unsigned short old_use_sg; /* We save use_sg here when requesting
- * sense info */
unsigned short use_sg; /* Number of pieces of scatter-gather */
unsigned short sglist_len; /* size of malloc'd scatter-gather list */
- unsigned bufflen; /* Size of data buffer */
- void *buffer; /* Data buffer */
unsigned underflow; /* Return error if less than
this amount is transferred */
- unsigned old_underflow; /* save underflow here when reusing the
- * command for error handling */
unsigned transfersize; /* How much we are guaranteed to
transfer with each SCSI transfer
Index: scsi-misc-2.6/drivers/scsi/scsi_priv.h
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_priv.h 2006-06-02 18:20:23.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_priv.h 2006-06-03 12:31:05.000000000 +0200
@@ -68,7 +68,6 @@
/* scsi_lib.c */
extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
-extern void scsi_setup_cmd_retry(struct scsi_cmnd *cmd);
extern void scsi_device_unbusy(struct scsi_device *sdev);
extern int scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
extern void scsi_next_command(struct scsi_cmnd *cmd);
Index: scsi-misc-2.6/drivers/scsi/sd.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/sd.c 2006-06-03 12:25:32.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/sd.c 2006-06-03 12:31:05.000000000 +0200
@@ -477,8 +477,7 @@
SCpnt->cmnd[4] = (unsigned char) this_count;
SCpnt->cmnd[5] = 0;
}
- SCpnt->request_bufflen = SCpnt->bufflen =
- this_count * sdp->sector_size;
+ SCpnt->request_bufflen = this_count * sdp->sector_size;
/*
* We shouldn't disconnect in the middle of a sector, so with a dumb
Index: scsi-misc-2.6/drivers/scsi/sr.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/sr.c 2006-06-03 12:25:32.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/sr.c 2006-06-03 12:31:05.000000000 +0200
@@ -360,7 +360,7 @@
"mismatch count %d, bytes %d\n",
size, SCpnt->request_bufflen);
if (SCpnt->request_bufflen > size)
- SCpnt->request_bufflen = SCpnt->bufflen = size;
+ SCpnt->request_bufflen = size;
}
}
@@ -387,8 +387,7 @@
if (this_count > 0xffff) {
this_count = 0xffff;
- SCpnt->request_bufflen = SCpnt->bufflen =
- this_count * s_size;
+ SCpnt->request_bufflen = this_count * s_size;
}
SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
Index: scsi-misc-2.6/drivers/scsi/aha152x.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/aha152x.c 2006-06-02 18:20:22.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/aha152x.c 2006-06-03 12:39:30.000000000 +0200
@@ -1165,6 +1165,10 @@
DECLARE_MUTEX_LOCKED(sem);
struct timer_list timer;
int ret, issued, disconnected;
+ unsigned char old_cmd_len = SCpnt->cmd_len;
+ unsigned short old_use_sg = SCpnt->use_sg;
+ void *old_buffer = SCpnt->request_buffer;
+ unsigned old_bufflen = SCpnt->request_bufflen;
unsigned long flags;
#if defined(AHA152X_DEBUG)
@@ -1198,11 +1202,11 @@
add_timer(&timer);
down(&sem);
del_timer(&timer);
-
- SCpnt->cmd_len = SCpnt->old_cmd_len;
- SCpnt->use_sg = SCpnt->old_use_sg;
- SCpnt->request_buffer = SCpnt->buffer;
- SCpnt->request_bufflen = SCpnt->bufflen;
+
+ SCpnt->cmd_len = old_cmd_len;
+ SCpnt->use_sg = old_use_sg;
+ SCpnt->request_buffer = old_buffer;
+ SCpnt->request_bufflen = old_bufflen;
DO_LOCK(flags);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-03 11:26 [PATCH, RFC] hide EH backup data outside the scsi_cmnd Christoph Hellwig
@ 2006-06-03 11:31 ` Christoph Hellwig
2006-06-10 16:08 ` Christoph Hellwig
2006-07-24 17:27 ` Tony Luck
2 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2006-06-03 11:31 UTC (permalink / raw)
To: jejb; +Cc: fischer, linux-kernel, linux-scsi
This should have gone to linux-scsi of course. Fup'to to -scsi instead
of -kernel please.
On Sat, Jun 03, 2006 at 01:26:10PM +0200, Christoph Hellwig wrote:
> Currently struct scsi_cmnd has various fields that are used to backup
> original data after the corresponding fields have been overridden for
> EH commands. This means drivers can easily get at it and misuse it.
> Due to the old_ naming this doesn't happen for most of them, but two
> that have different names have been used wrong a lot (see previous
> patch). Another downside is that they unessecarily bloat the scsi_cmnd
> size.
>
> This patch moves them onstack in scsi_send_eh_cmnd to fix those two
> issues above.
>
> Note that this breaks the aha152x and 53x700 drivers because they're
> plaing with those fields internally.
>
> J"urgen and James, could you take a look at whether it's feasible to
> fix those drivers?
>
> Otherwise any comments on the general approach?
>
> (Note: you probably want the patch to remove the wrong buffer and
> bufflen patches applied before this, else lots of drivers will stop
> compiling)
>
> Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/scsi_error.c 2006-06-02 18:20:23.000000000 +0200
> +++ scsi-misc-2.6/drivers/scsi/scsi_error.c 2006-06-03 12:31:05.000000000 +0200
> @@ -438,19 +438,67 @@
> * Return value:
> * SUCCESS or FAILED or NEEDS_RETRY
> **/
> -static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, int timeout)
> +static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, int timeout, int copy_sense)
> {
> struct scsi_device *sdev = scmd->device;
> struct Scsi_Host *shost = sdev->host;
> + int old_result = scmd->result;
> DECLARE_COMPLETION(done);
> unsigned long timeleft;
> unsigned long flags;
> + unsigned char old_cmnd[MAX_COMMAND_SIZE];
> + enum dma_data_direction old_data_direction;
> + unsigned short old_use_sg;
> + unsigned char old_cmd_len;
> + unsigned old_bufflen;
> + void *old_buffer;
> int rtn;
>
> + /*
> + * We need saved copies of a number of fields - this is because
> + * error handling may need to overwrite these with different values
> + * to run different commands, and once error handling is complete,
> + * we will need to restore these values prior to running the actual
> + * command.
> + */
> + old_buffer = scmd->request_buffer;
> + old_bufflen = scmd->request_bufflen;
> + memcpy(old_cmnd, scmd->cmnd, sizeof(scmd->cmnd));
> + old_data_direction = scmd->sc_data_direction;
> + old_cmd_len = scmd->cmd_len;
> + old_use_sg = scmd->use_sg;
> +
> + if (copy_sense) {
> + int gfp_mask = GFP_ATOMIC;
> +
> + if (shost->hostt->unchecked_isa_dma)
> + gfp_mask |= __GFP_DMA;
> +
> + scmd->sc_data_direction = DMA_FROM_DEVICE;
> + scmd->request_bufflen = 252;
> + scmd->request_buffer = kzalloc(scmd->request_bufflen, gfp_mask);
> + if (!scmd->request_buffer)
> + return FAILED;
> + } else {
> + scmd->request_buffer = NULL;
> + scmd->request_bufflen = 0;
> + scmd->sc_data_direction = DMA_NONE;
> + }
> +
> + scmd->underflow = 0;
> + scmd->use_sg = 0;
> + scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
> +
> if (sdev->scsi_level <= SCSI_2)
> scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
> (sdev->lun << 5 & 0xe0);
>
> + /*
> + * Zero the sense buffer. The scsi spec mandates that any
> + * untransferred sense data should be interpreted as being zero.
> + */
> + memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
> +
> shost->eh_action = &done;
> scmd->request->rq_status = RQ_SCSI_BUSY;
>
> @@ -502,6 +550,27 @@
> rtn = FAILED;
> }
>
> +
> + /*
> + * Last chance to have valid sense data.
> + */
> + if (copy_sense && !SCSI_SENSE_VALID(scmd)) {
> + memcpy(scmd->sense_buffer, scmd->request_buffer,
> + sizeof(scmd->sense_buffer));
> + kfree(scmd->request_buffer);
> + }
> +
> +
> + /*
> + * Restore original data
> + */
> + scmd->request_buffer = old_buffer;
> + scmd->request_bufflen = old_bufflen;
> + memcpy(scmd->cmnd, old_cmnd, sizeof(scmd->cmnd));
> + scmd->sc_data_direction = old_data_direction;
> + scmd->cmd_len = old_cmd_len;
> + scmd->use_sg = old_use_sg;
> + scmd->result = old_result;
> return rtn;
> }
>
> @@ -517,56 +586,10 @@
> static int scsi_request_sense(struct scsi_cmnd *scmd)
> {
> static unsigned char generic_sense[6] =
> - {REQUEST_SENSE, 0, 0, 0, 252, 0};
> - unsigned char *scsi_result;
> - int saved_result;
> - int rtn;
> + {REQUEST_SENSE, 0, 0, 0, 252, 0};
>
> memcpy(scmd->cmnd, generic_sense, sizeof(generic_sense));
> -
> - scsi_result = kmalloc(252, GFP_ATOMIC | ((scmd->device->host->hostt->unchecked_isa_dma) ? __GFP_DMA : 0));
> -
> -
> - if (unlikely(!scsi_result)) {
> - printk(KERN_ERR "%s: cannot allocate scsi_result.\n",
> - __FUNCTION__);
> - return FAILED;
> - }
> -
> - /*
> - * zero the sense buffer. some host adapters automatically always
> - * request sense, so it is not a good idea that
> - * scmd->request_buffer and scmd->sense_buffer point to the same
> - * address (db). 0 is not a valid sense code.
> - */
> - memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
> - memset(scsi_result, 0, 252);
> -
> - saved_result = scmd->result;
> - scmd->request_buffer = scsi_result;
> - scmd->request_bufflen = 252;
> - scmd->use_sg = 0;
> - scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
> - scmd->sc_data_direction = DMA_FROM_DEVICE;
> - scmd->underflow = 0;
> -
> - rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
> -
> - /* last chance to have valid sense data */
> - if(!SCSI_SENSE_VALID(scmd)) {
> - memcpy(scmd->sense_buffer, scmd->request_buffer,
> - sizeof(scmd->sense_buffer));
> - }
> -
> - kfree(scsi_result);
> -
> - /*
> - * when we eventually call scsi_finish, we really wish to complete
> - * the original request, so let's restore the original data. (db)
> - */
> - scsi_setup_cmd_retry(scmd);
> - scmd->result = saved_result;
> - return rtn;
> + return scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT, 1);
> }
>
> /**
> @@ -585,12 +608,6 @@
> {
> scmd->device->host->host_failed--;
> scmd->eh_eflags = 0;
> -
> - /*
> - * set this back so that the upper level can correctly free up
> - * things.
> - */
> - scsi_setup_cmd_retry(scmd);
> list_move_tail(&scmd->eh_entry, done_q);
> }
> EXPORT_SYMBOL(scsi_eh_finish_cmd);
> @@ -695,47 +712,26 @@
> {
> static unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
> int retry_cnt = 1, rtn;
> - int saved_result;
>
> retry_tur:
> memcpy(scmd->cmnd, tur_command, sizeof(tur_command));
>
> - /*
> - * zero the sense buffer. the scsi spec mandates that any
> - * untransferred sense data should be interpreted as being zero.
> - */
> - memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
>
> - saved_result = scmd->result;
> - scmd->request_buffer = NULL;
> - scmd->request_bufflen = 0;
> - scmd->use_sg = 0;
> - scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
> - scmd->underflow = 0;
> - scmd->sc_data_direction = DMA_NONE;
> + rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT, 0);
>
> - rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
> -
> - /*
> - * when we eventually call scsi_finish, we really wish to complete
> - * the original request, so let's restore the original data. (db)
> - */
> - scsi_setup_cmd_retry(scmd);
> - scmd->result = saved_result;
> -
> - /*
> - * hey, we are done. let's look to see what happened.
> - */
> SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd %p rtn %x\n",
> __FUNCTION__, scmd, rtn));
> - if (rtn == SUCCESS)
> - return 0;
> - else if (rtn == NEEDS_RETRY) {
> +
> + switch (rtn) {
> + case NEEDS_RETRY:
> if (retry_cnt--)
> goto retry_tur;
> + /*FALLTHRU*/
> + case SUCCESS:
> return 0;
> + default:
> + return 1;
> }
> - return 1;
> }
>
> /**
> @@ -817,44 +813,16 @@
> static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
> {
> static unsigned char stu_command[6] = {START_STOP, 0, 0, 0, 1, 0};
> - int rtn;
> - int saved_result;
> -
> - if (!scmd->device->allow_restart)
> - return 1;
> -
> - memcpy(scmd->cmnd, stu_command, sizeof(stu_command));
> -
> - /*
> - * zero the sense buffer. the scsi spec mandates that any
> - * untransferred sense data should be interpreted as being zero.
> - */
> - memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
>
> - saved_result = scmd->result;
> - scmd->request_buffer = NULL;
> - scmd->request_bufflen = 0;
> - scmd->use_sg = 0;
> - scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
> - scmd->underflow = 0;
> - scmd->sc_data_direction = DMA_NONE;
> + if (scmd->device->allow_restart) {
> + int rtn;
>
> - rtn = scsi_send_eh_cmnd(scmd, START_UNIT_TIMEOUT);
> -
> - /*
> - * when we eventually call scsi_finish, we really wish to complete
> - * the original request, so let's restore the original data. (db)
> - */
> - scsi_setup_cmd_retry(scmd);
> - scmd->result = saved_result;
> + memcpy(scmd->cmnd, stu_command, sizeof(stu_command));
> + rtn = scsi_send_eh_cmnd(scmd, START_UNIT_TIMEOUT, 0);
> + if (rtn == SUCCESS)
> + return 0;
> + }
>
> - /*
> - * hey, we are done. let's look to see what happened.
> - */
> - SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd %p rtn %x\n",
> - __FUNCTION__, scmd, rtn));
> - if (rtn == SUCCESS)
> - return 0;
> return 1;
> }
>
> @@ -1663,8 +1631,6 @@
>
> scmd->scsi_done = scsi_reset_provider_done_command;
> scmd->done = NULL;
> - scmd->buffer = NULL;
> - scmd->bufflen = 0;
> scmd->request_buffer = NULL;
> scmd->request_bufflen = 0;
>
> Index: scsi-misc-2.6/drivers/scsi/scsi_lib.c
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/scsi_lib.c 2006-06-02 18:20:23.000000000 +0200
> +++ scsi-misc-2.6/drivers/scsi/scsi_lib.c 2006-06-03 12:31:05.000000000 +0200
> @@ -502,60 +502,16 @@
> *
> * Arguments: cmd - command that is ready to be queued.
> *
> - * Returns: Nothing
> - *
> * Notes: This function has the job of initializing a number of
> * fields related to error handling. Typically this will
> * be called once for each command, as required.
> */
> -static int scsi_init_cmd_errh(struct scsi_cmnd *cmd)
> +static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
> {
> cmd->serial_number = 0;
> -
> memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);
> -
> if (cmd->cmd_len == 0)
> cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
> -
> - /*
> - * We need saved copies of a number of fields - this is because
> - * error handling may need to overwrite these with different values
> - * to run different commands, and once error handling is complete,
> - * we will need to restore these values prior to running the actual
> - * command.
> - */
> - cmd->old_use_sg = cmd->use_sg;
> - cmd->old_cmd_len = cmd->cmd_len;
> - cmd->sc_old_data_direction = cmd->sc_data_direction;
> - cmd->old_underflow = cmd->underflow;
> - memcpy(cmd->data_cmnd, cmd->cmnd, sizeof(cmd->cmnd));
> - cmd->buffer = cmd->request_buffer;
> - cmd->bufflen = cmd->request_bufflen;
> -
> - return 1;
> -}
> -
> -/*
> - * Function: scsi_setup_cmd_retry()
> - *
> - * Purpose: Restore the command state for a retry
> - *
> - * Arguments: cmd - command to be restored
> - *
> - * Returns: Nothing
> - *
> - * Notes: Immediately prior to retrying a command, we need
> - * to restore certain fields that we saved above.
> - */
> -void scsi_setup_cmd_retry(struct scsi_cmnd *cmd)
> -{
> - memcpy(cmd->cmnd, cmd->data_cmnd, sizeof(cmd->data_cmnd));
> - cmd->request_buffer = cmd->buffer;
> - cmd->request_bufflen = cmd->bufflen;
> - cmd->use_sg = cmd->old_use_sg;
> - cmd->cmd_len = cmd->old_cmd_len;
> - cmd->sc_data_direction = cmd->sc_old_data_direction;
> - cmd->underflow = cmd->old_underflow;
> }
>
> void scsi_device_unbusy(struct scsi_device *sdev)
> @@ -873,22 +829,13 @@
> */
> static void scsi_release_buffers(struct scsi_cmnd *cmd)
> {
> - struct request *req = cmd->request;
> -
> - /*
> - * Free up any indirection buffers we allocated for DMA purposes.
> - */
> if (cmd->use_sg)
> scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
> - else if (cmd->request_buffer != req->buffer)
> - kfree(cmd->request_buffer);
>
> /*
> * Zero these out. They now point to freed memory, and it is
> * dangerous to hang onto the pointers.
> */
> - cmd->buffer = NULL;
> - cmd->bufflen = 0;
> cmd->request_buffer = NULL;
> cmd->request_bufflen = 0;
> }
> @@ -925,7 +872,7 @@
> unsigned int block_bytes)
> {
> int result = cmd->result;
> - int this_count = cmd->bufflen;
> + int this_count = cmd->request_bufflen;
> request_queue_t *q = cmd->device->request_queue;
> struct request *req = cmd->request;
> int clear_errors = 1;
> @@ -933,28 +880,14 @@
> int sense_valid = 0;
> int sense_deferred = 0;
>
> - /*
> - * Free up any indirection buffers we allocated for DMA purposes.
> - * For the case of a READ, we need to copy the data out of the
> - * bounce buffer and into the real buffer.
> - */
> - if (cmd->use_sg)
> - scsi_free_sgtable(cmd->buffer, cmd->sglist_len);
> - else if (cmd->buffer != req->buffer) {
> - if (rq_data_dir(req) == READ) {
> - unsigned long flags;
> - char *to = bio_kmap_irq(req->bio, &flags);
> - memcpy(to, cmd->buffer, cmd->bufflen);
> - bio_kunmap_irq(to, &flags);
> - }
> - kfree(cmd->buffer);
> - }
> + scsi_release_buffers(cmd);
>
> if (result) {
> sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
> if (sense_valid)
> sense_deferred = scsi_sense_is_deferred(&sshdr);
> }
> +
> if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
> req->errors = result;
> if (result) {
> @@ -975,15 +908,6 @@
> }
>
> /*
> - * Zero these out. They now point to freed memory, and it is
> - * dangerous to hang onto the pointers.
> - */
> - cmd->buffer = NULL;
> - cmd->bufflen = 0;
> - cmd->request_buffer = NULL;
> - cmd->request_bufflen = 0;
> -
> - /*
> * Next deal with any sectors which we were able to correctly
> * handle.
> */
> @@ -1083,7 +1007,7 @@
> if (!(req->flags & REQ_QUIET)) {
> scmd_printk(KERN_INFO, cmd,
> "Volume overflow, CDB: ");
> - __scsi_print_command(cmd->data_cmnd);
> + __scsi_print_command(cmd->cmnd);
> scsi_print_sense("", cmd);
> }
> scsi_end_request(cmd, 0, block_bytes, 1);
> @@ -1222,7 +1146,7 @@
> * successfully. Since this is a REQ_BLOCK_PC command the
> * caller should check the request's errors value
> */
> - scsi_io_completion(cmd, cmd->bufflen, 0);
> + scsi_io_completion(cmd, cmd->request_bufflen, 0);
> }
>
> static void scsi_setup_blk_pc_cmnd(struct scsi_cmnd *cmd)
> Index: scsi-misc-2.6/drivers/scsi/scsi.c
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/scsi.c 2006-06-02 18:20:23.000000000 +0200
> +++ scsi-misc-2.6/drivers/scsi/scsi.c 2006-06-03 12:31:05.000000000 +0200
> @@ -420,7 +420,7 @@
> if (level > 3) {
> printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
> " done = 0x%p, queuecommand 0x%p\n",
> - cmd->buffer, cmd->bufflen,
> + cmd->request_buffer, cmd->request_bufflen,
> cmd->done,
> sdev->host->hostt->queuecommand);
>
> @@ -678,10 +678,7 @@
> cmd->use_sg = sreq->sr_use_sg;
>
> cmd->request = sreq->sr_request;
> - memcpy(cmd->data_cmnd, sreq->sr_cmnd, sizeof(cmd->data_cmnd));
> cmd->serial_number = 0;
> - cmd->bufflen = sreq->sr_bufflen;
> - cmd->buffer = sreq->sr_buffer;
> cmd->retries = 0;
> cmd->allowed = sreq->sr_allowed;
> cmd->done = sreq->sr_done;
> @@ -699,12 +696,8 @@
> memset(cmd->sense_buffer, 0, sizeof(sreq->sr_sense_buffer));
> cmd->request_buffer = sreq->sr_buffer;
> cmd->request_bufflen = sreq->sr_bufflen;
> - cmd->old_use_sg = cmd->use_sg;
> if (cmd->cmd_len == 0)
> cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
> - cmd->old_cmd_len = cmd->cmd_len;
> - cmd->sc_old_data_direction = cmd->sc_data_direction;
> - cmd->old_underflow = cmd->underflow;
>
> /*
> * Start the timer ticking.
> @@ -784,11 +777,6 @@
> */
> int scsi_retry_command(struct scsi_cmnd *cmd)
> {
> - /*
> - * Restore the SCSI command state.
> - */
> - scsi_setup_cmd_retry(cmd);
> -
> /*
> * Zero the sense information from the last time we tried
> * this command.
> @@ -836,11 +824,6 @@
> "(result %x)\n", cmd->result));
>
> /*
> - * We can get here with use_sg=0, causing a panic in the upper level
> - */
> - cmd->use_sg = cmd->old_use_sg;
> -
> - /*
> * If there is an associated request structure, copy the data over
> * before we call the completion function.
> */
> Index: scsi-misc-2.6/include/scsi/scsi_cmnd.h
> ===================================================================
> --- scsi-misc-2.6.orig/include/scsi/scsi_cmnd.h 2006-06-03 12:07:51.000000000 +0200
> +++ scsi-misc-2.6/include/scsi/scsi_cmnd.h 2006-06-03 12:31:05.000000000 +0200
> @@ -64,9 +64,7 @@
> int timeout_per_command;
>
> unsigned char cmd_len;
> - unsigned char old_cmd_len;
> enum dma_data_direction sc_data_direction;
> - enum dma_data_direction sc_old_data_direction;
>
> /* These elements define the operation we are about to perform */
> #define MAX_COMMAND_SIZE 16
> @@ -77,18 +75,11 @@
> void *request_buffer; /* Actual requested buffer */
>
> /* These elements define the operation we ultimately want to perform */
> - unsigned char data_cmnd[MAX_COMMAND_SIZE];
> - unsigned short old_use_sg; /* We save use_sg here when requesting
> - * sense info */
> unsigned short use_sg; /* Number of pieces of scatter-gather */
> unsigned short sglist_len; /* size of malloc'd scatter-gather list */
> - unsigned bufflen; /* Size of data buffer */
> - void *buffer; /* Data buffer */
>
> unsigned underflow; /* Return error if less than
> this amount is transferred */
> - unsigned old_underflow; /* save underflow here when reusing the
> - * command for error handling */
>
> unsigned transfersize; /* How much we are guaranteed to
> transfer with each SCSI transfer
> Index: scsi-misc-2.6/drivers/scsi/scsi_priv.h
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/scsi_priv.h 2006-06-02 18:20:23.000000000 +0200
> +++ scsi-misc-2.6/drivers/scsi/scsi_priv.h 2006-06-03 12:31:05.000000000 +0200
> @@ -68,7 +68,6 @@
>
> /* scsi_lib.c */
> extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
> -extern void scsi_setup_cmd_retry(struct scsi_cmnd *cmd);
> extern void scsi_device_unbusy(struct scsi_device *sdev);
> extern int scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
> extern void scsi_next_command(struct scsi_cmnd *cmd);
> Index: scsi-misc-2.6/drivers/scsi/sd.c
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/sd.c 2006-06-03 12:25:32.000000000 +0200
> +++ scsi-misc-2.6/drivers/scsi/sd.c 2006-06-03 12:31:05.000000000 +0200
> @@ -477,8 +477,7 @@
> SCpnt->cmnd[4] = (unsigned char) this_count;
> SCpnt->cmnd[5] = 0;
> }
> - SCpnt->request_bufflen = SCpnt->bufflen =
> - this_count * sdp->sector_size;
> + SCpnt->request_bufflen = this_count * sdp->sector_size;
>
> /*
> * We shouldn't disconnect in the middle of a sector, so with a dumb
> Index: scsi-misc-2.6/drivers/scsi/sr.c
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/sr.c 2006-06-03 12:25:32.000000000 +0200
> +++ scsi-misc-2.6/drivers/scsi/sr.c 2006-06-03 12:31:05.000000000 +0200
> @@ -360,7 +360,7 @@
> "mismatch count %d, bytes %d\n",
> size, SCpnt->request_bufflen);
> if (SCpnt->request_bufflen > size)
> - SCpnt->request_bufflen = SCpnt->bufflen = size;
> + SCpnt->request_bufflen = size;
> }
> }
>
> @@ -387,8 +387,7 @@
>
> if (this_count > 0xffff) {
> this_count = 0xffff;
> - SCpnt->request_bufflen = SCpnt->bufflen =
> - this_count * s_size;
> + SCpnt->request_bufflen = this_count * s_size;
> }
>
> SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> Index: scsi-misc-2.6/drivers/scsi/aha152x.c
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/aha152x.c 2006-06-02 18:20:22.000000000 +0200
> +++ scsi-misc-2.6/drivers/scsi/aha152x.c 2006-06-03 12:39:30.000000000 +0200
> @@ -1165,6 +1165,10 @@
> DECLARE_MUTEX_LOCKED(sem);
> struct timer_list timer;
> int ret, issued, disconnected;
> + unsigned char old_cmd_len = SCpnt->cmd_len;
> + unsigned short old_use_sg = SCpnt->use_sg;
> + void *old_buffer = SCpnt->request_buffer;
> + unsigned old_bufflen = SCpnt->request_bufflen;
> unsigned long flags;
>
> #if defined(AHA152X_DEBUG)
> @@ -1198,11 +1202,11 @@
> add_timer(&timer);
> down(&sem);
> del_timer(&timer);
> -
> - SCpnt->cmd_len = SCpnt->old_cmd_len;
> - SCpnt->use_sg = SCpnt->old_use_sg;
> - SCpnt->request_buffer = SCpnt->buffer;
> - SCpnt->request_bufflen = SCpnt->bufflen;
> +
> + SCpnt->cmd_len = old_cmd_len;
> + SCpnt->use_sg = old_use_sg;
> + SCpnt->request_buffer = old_buffer;
> + SCpnt->request_bufflen = old_bufflen;
>
> DO_LOCK(flags);
>
---end quoted text---
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-03 11:26 [PATCH, RFC] hide EH backup data outside the scsi_cmnd Christoph Hellwig
2006-06-03 11:31 ` Christoph Hellwig
@ 2006-06-10 16:08 ` Christoph Hellwig
2006-06-12 19:19 ` Christoph Hellwig
2006-07-24 17:27 ` Tony Luck
2 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2006-06-10 16:08 UTC (permalink / raw)
To: jejb; +Cc: fischer, linux-scsi
On Sat, Jun 03, 2006 at 01:26:10PM +0200, Christoph Hellwig wrote:
> Currently struct scsi_cmnd has various fields that are used to backup
> original data after the corresponding fields have been overridden for
> EH commands. This means drivers can easily get at it and misuse it.
> Due to the old_ naming this doesn't happen for most of them, but two
> that have different names have been used wrong a lot (see previous
> patch). Another downside is that they unessecarily bloat the scsi_cmnd
> size.
>
> This patch moves them onstack in scsi_send_eh_cmnd to fix those two
> issues above.
>
> Note that this breaks the aha152x and 53x700 drivers because they're
> plaing with those fields internally.
>
> J"urgen and James, could you take a look at whether it's feasible to
> fix those drivers?
>
> Otherwise any comments on the general approach?
>
> (Note: you probably want the patch to remove the wrong buffer and
> bufflen patches applied before this, else lots of drivers will stop
> compiling)
That patch is now in. Below is a patch rediffed against the
scsi_request removal I just posed to linux-scsi.
Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_error.c 2005-10-29 19:22:47.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_error.c 2005-10-29 19:23:00.000000000 +0200
@@ -417,43 +417,15 @@
}
/**
- * scsi_eh_times_out - timeout function for error handling.
- * @scmd: Cmd that is timing out.
- *
- * Notes:
- * During error handling, the kernel thread will be sleeping waiting
- * for some action to complete on the device. our only job is to
- * record that it timed out, and to wake up the thread.
- **/
-static void scsi_eh_times_out(struct scsi_cmnd *scmd)
-{
- scmd->eh_eflags |= SCSI_EH_REC_TIMEOUT;
- SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd:%p\n", __FUNCTION__,
- scmd));
-
- up(scmd->device->host->eh_action);
-}
-
-/**
* scsi_eh_done - Completion function for error handling.
* @scmd: Cmd that is done.
**/
static void scsi_eh_done(struct scsi_cmnd *scmd)
{
- /*
- * if the timeout handler is already running, then just set the
- * flag which says we finished late, and return. we have no
- * way of stopping the timeout handler from running, so we must
- * always defer to it.
- */
- if (del_timer(&scmd->eh_timeout)) {
- scmd->request->rq_status = RQ_SCSI_DONE;
-
- SCSI_LOG_ERROR_RECOVERY(3, printk("%s scmd: %p result: %x\n",
- __FUNCTION__, scmd, scmd->result));
-
- up(scmd->device->host->eh_action);
- }
+ SCSI_LOG_ERROR_RECOVERY(3,
+ printk("%s scmd: %p result: %x\n",
+ __FUNCTION__, scmd, scmd->result));
+ complete(scmd->device->host->eh_action);
}
/**
@@ -461,10 +433,6 @@
* @scmd: SCSI Cmd to send.
* @timeout: Timeout for cmd.
*
- * Notes:
- * The initialization of the structures is quite a bit different in
- * this case, and furthermore, there is a different completion handler
- * vs scsi_dispatch_cmd.
* Return value:
* SUCCESS or FAILED or NEEDS_RETRY
**/
@@ -472,24 +440,16 @@
{
struct scsi_device *sdev = scmd->device;
struct Scsi_Host *shost = sdev->host;
- DECLARE_MUTEX_LOCKED(sem);
+ DECLARE_COMPLETION(done);
+ unsigned long timeleft;
unsigned long flags;
- int rtn = SUCCESS;
+ int rtn;
- /*
- * we will use a queued command if possible, otherwise we will
- * emulate the queuing and calling of completion function ourselves.
- */
if (sdev->scsi_level <= SCSI_2)
scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
(sdev->lun << 5 & 0xe0);
- scsi_add_timer(scmd, timeout, scsi_eh_times_out);
-
- /*
- * set up the semaphore so we wait for the command to complete.
- */
- shost->eh_action = &sem;
+ shost->eh_action = &done;
scmd->request->rq_status = RQ_SCSI_BUSY;
spin_lock_irqsave(shost->host_lock, flags);
@@ -497,47 +457,29 @@
shost->hostt->queuecommand(scmd, scsi_eh_done);
spin_unlock_irqrestore(shost->host_lock, flags);
- down(&sem);
- scsi_log_completion(scmd, SUCCESS);
+ timeleft = wait_for_completion_timeout(&done, timeout);
+ scmd->request->rq_status = RQ_SCSI_DONE;
shost->eh_action = NULL;
- /*
- * see if timeout. if so, tell the host to forget about it.
- * in other words, we don't want a callback any more.
- */
- if (scmd->eh_eflags & SCSI_EH_REC_TIMEOUT) {
- scmd->eh_eflags &= ~SCSI_EH_REC_TIMEOUT;
-
- /*
- * as far as the low level driver is
- * concerned, this command is still active, so
- * we must give the low level driver a chance
- * to abort it. (db)
- *
- * FIXME(eric) - we are not tracking whether we could
- * abort a timed out command or not. not sure how
- * we should treat them differently anyways.
- */
- if (shost->hostt->eh_abort_handler)
- shost->hostt->eh_abort_handler(scmd);
-
- scmd->request->rq_status = RQ_SCSI_DONE;
- rtn = FAILED;
- }
+ scsi_log_completion(scmd, SUCCESS);
- SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd: %p, rtn:%x\n",
- __FUNCTION__, scmd, rtn));
+ SCSI_LOG_ERROR_RECOVERY(3,
+ printk("%s: scmd: %p, timeleft: %ld\n",
+ __FUNCTION__, scmd, timeleft));
/*
- * now examine the actual status codes to see whether the command
- * actually did complete normally.
+ * If there is time left scsi_eh_done got called, and we will
+ * examine the actual status codes to see whether the command
+ * actually did complete normally, else tell the host to forget
+ * about this command.
*/
- if (rtn == SUCCESS) {
+ if (timeleft) {
rtn = scsi_eh_completed_normally(scmd);
SCSI_LOG_ERROR_RECOVERY(3,
printk("%s: scsi_eh_completed_normally %x\n",
__FUNCTION__, rtn));
+
switch (rtn) {
case SUCCESS:
case NEEDS_RETRY:
@@ -547,6 +489,15 @@
rtn = FAILED;
break;
}
+ } else {
+ /*
+ * FIXME(eric) - we are not tracking whether we could
+ * abort a timed out command or not. not sure how
+ * we should treat them differently anyways.
+ */
+ if (shost->hostt->eh_abort_handler)
+ shost->hostt->eh_abort_handler(scmd);
+ rtn = FAILED;
}
return rtn;
Index: scsi-misc-2.6/drivers/scsi/scsi_priv.h
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_priv.h 2005-10-29 19:20:33.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_priv.h 2005-10-29 19:23:00.000000000 +0200
@@ -22,7 +22,6 @@
* Scsi Error Handler Flags
*/
#define SCSI_EH_CANCEL_CMD 0x0001 /* Cancel this cmd */
-#define SCSI_EH_REC_TIMEOUT 0x0002 /* EH retry timed out */
#define SCSI_SENSE_VALID(scmd) \
(((scmd)->sense_buffer[0] & 0x70) == 0x70)
Index: scsi-misc-2.6/include/scsi/scsi_host.h
===================================================================
--- scsi-misc-2.6.orig/include/scsi/scsi_host.h 2005-10-29 19:22:47.000000000 +0200
+++ scsi-misc-2.6/include/scsi/scsi_host.h 2005-10-29 19:23:00.000000000 +0200
@@ -7,6 +7,7 @@
#include <linux/workqueue.h>
struct block_device;
+struct completion;
struct module;
struct scsi_cmnd;
struct scsi_device;
@@ -467,8 +468,8 @@
struct list_head eh_cmd_q;
struct task_struct * ehandler; /* Error recovery thread. */
- struct semaphore * eh_action; /* Wait for specific actions on the
- host. */
+ struct completion * eh_action; /* Wait for specific actions on the
+ host. */
wait_queue_head_t host_wait;
struct scsi_host_template *hostt;
struct scsi_transport_template *transportt;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-10 16:08 ` Christoph Hellwig
@ 2006-06-12 19:19 ` Christoph Hellwig
2006-06-14 2:31 ` James Bottomley
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Christoph Hellwig @ 2006-06-12 19:19 UTC (permalink / raw)
To: jejb; +Cc: fischer, linux-scsi
On Sat, Jun 10, 2006 at 06:08:15PM +0200, Christoph Hellwig wrote:
> On Sat, Jun 03, 2006 at 01:26:10PM +0200, Christoph Hellwig wrote:
> > Currently struct scsi_cmnd has various fields that are used to backup
> > original data after the corresponding fields have been overridden for
> > EH commands. This means drivers can easily get at it and misuse it.
> > Due to the old_ naming this doesn't happen for most of them, but two
> > that have different names have been used wrong a lot (see previous
> > patch). Another downside is that they unessecarily bloat the scsi_cmnd
> > size.
> >
> > This patch moves them onstack in scsi_send_eh_cmnd to fix those two
> > issues above.
> >
> > Note that this breaks the aha152x and 53x700 drivers because they're
> > plaing with those fields internally.
> >
> > J"urgen and James, could you take a look at whether it's feasible to
> > fix those drivers?
> >
> > Otherwise any comments on the general approach?
> >
> > (Note: you probably want the patch to remove the wrong buffer and
> > bufflen patches applied before this, else lots of drivers will stop
> > compiling)
>
> That patch is now in. Below is a patch rediffed against the
> scsi_request removal I just posed to linux-scsi.
looks like no one but James looked at it as I attached a completely
different patch that has been merged long ago. Here's the real one:
Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_error.c 2006-06-10 17:45:59.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_error.c 2006-06-10 17:53:06.000000000 +0200
@@ -438,19 +438,67 @@
* Return value:
* SUCCESS or FAILED or NEEDS_RETRY
**/
-static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, int timeout)
+static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, int timeout, int copy_sense)
{
struct scsi_device *sdev = scmd->device;
struct Scsi_Host *shost = sdev->host;
+ int old_result = scmd->result;
DECLARE_COMPLETION(done);
unsigned long timeleft;
unsigned long flags;
+ unsigned char old_cmnd[MAX_COMMAND_SIZE];
+ enum dma_data_direction old_data_direction;
+ unsigned short old_use_sg;
+ unsigned char old_cmd_len;
+ unsigned old_bufflen;
+ void *old_buffer;
int rtn;
+ /*
+ * We need saved copies of a number of fields - this is because
+ * error handling may need to overwrite these with different values
+ * to run different commands, and once error handling is complete,
+ * we will need to restore these values prior to running the actual
+ * command.
+ */
+ old_buffer = scmd->request_buffer;
+ old_bufflen = scmd->request_bufflen;
+ memcpy(old_cmnd, scmd->cmnd, sizeof(scmd->cmnd));
+ old_data_direction = scmd->sc_data_direction;
+ old_cmd_len = scmd->cmd_len;
+ old_use_sg = scmd->use_sg;
+
+ if (copy_sense) {
+ int gfp_mask = GFP_ATOMIC;
+
+ if (shost->hostt->unchecked_isa_dma)
+ gfp_mask |= __GFP_DMA;
+
+ scmd->sc_data_direction = DMA_FROM_DEVICE;
+ scmd->request_bufflen = 252;
+ scmd->request_buffer = kzalloc(scmd->request_bufflen, gfp_mask);
+ if (!scmd->request_buffer)
+ return FAILED;
+ } else {
+ scmd->request_buffer = NULL;
+ scmd->request_bufflen = 0;
+ scmd->sc_data_direction = DMA_NONE;
+ }
+
+ scmd->underflow = 0;
+ scmd->use_sg = 0;
+ scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
+
if (sdev->scsi_level <= SCSI_2)
scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
(sdev->lun << 5 & 0xe0);
+ /*
+ * Zero the sense buffer. The scsi spec mandates that any
+ * untransferred sense data should be interpreted as being zero.
+ */
+ memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
+
shost->eh_action = &done;
scmd->request->rq_status = RQ_SCSI_BUSY;
@@ -502,6 +550,27 @@
rtn = FAILED;
}
+
+ /*
+ * Last chance to have valid sense data.
+ */
+ if (copy_sense && !SCSI_SENSE_VALID(scmd)) {
+ memcpy(scmd->sense_buffer, scmd->request_buffer,
+ sizeof(scmd->sense_buffer));
+ kfree(scmd->request_buffer);
+ }
+
+
+ /*
+ * Restore original data
+ */
+ scmd->request_buffer = old_buffer;
+ scmd->request_bufflen = old_bufflen;
+ memcpy(scmd->cmnd, old_cmnd, sizeof(scmd->cmnd));
+ scmd->sc_data_direction = old_data_direction;
+ scmd->cmd_len = old_cmd_len;
+ scmd->use_sg = old_use_sg;
+ scmd->result = old_result;
return rtn;
}
@@ -517,56 +586,10 @@
static int scsi_request_sense(struct scsi_cmnd *scmd)
{
static unsigned char generic_sense[6] =
- {REQUEST_SENSE, 0, 0, 0, 252, 0};
- unsigned char *scsi_result;
- int saved_result;
- int rtn;
+ {REQUEST_SENSE, 0, 0, 0, 252, 0};
memcpy(scmd->cmnd, generic_sense, sizeof(generic_sense));
-
- scsi_result = kmalloc(252, GFP_ATOMIC | ((scmd->device->host->hostt->unchecked_isa_dma) ? __GFP_DMA : 0));
-
-
- if (unlikely(!scsi_result)) {
- printk(KERN_ERR "%s: cannot allocate scsi_result.\n",
- __FUNCTION__);
- return FAILED;
- }
-
- /*
- * zero the sense buffer. some host adapters automatically always
- * request sense, so it is not a good idea that
- * scmd->request_buffer and scmd->sense_buffer point to the same
- * address (db). 0 is not a valid sense code.
- */
- memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
- memset(scsi_result, 0, 252);
-
- saved_result = scmd->result;
- scmd->request_buffer = scsi_result;
- scmd->request_bufflen = 252;
- scmd->use_sg = 0;
- scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
- scmd->sc_data_direction = DMA_FROM_DEVICE;
- scmd->underflow = 0;
-
- rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
-
- /* last chance to have valid sense data */
- if(!SCSI_SENSE_VALID(scmd)) {
- memcpy(scmd->sense_buffer, scmd->request_buffer,
- sizeof(scmd->sense_buffer));
- }
-
- kfree(scsi_result);
-
- /*
- * when we eventually call scsi_finish, we really wish to complete
- * the original request, so let's restore the original data. (db)
- */
- scsi_setup_cmd_retry(scmd);
- scmd->result = saved_result;
- return rtn;
+ return scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT, 1);
}
/**
@@ -585,12 +608,6 @@
{
scmd->device->host->host_failed--;
scmd->eh_eflags = 0;
-
- /*
- * set this back so that the upper level can correctly free up
- * things.
- */
- scsi_setup_cmd_retry(scmd);
list_move_tail(&scmd->eh_entry, done_q);
}
EXPORT_SYMBOL(scsi_eh_finish_cmd);
@@ -695,47 +712,26 @@
{
static unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
int retry_cnt = 1, rtn;
- int saved_result;
retry_tur:
memcpy(scmd->cmnd, tur_command, sizeof(tur_command));
- /*
- * zero the sense buffer. the scsi spec mandates that any
- * untransferred sense data should be interpreted as being zero.
- */
- memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
- saved_result = scmd->result;
- scmd->request_buffer = NULL;
- scmd->request_bufflen = 0;
- scmd->use_sg = 0;
- scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
- scmd->underflow = 0;
- scmd->sc_data_direction = DMA_NONE;
+ rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT, 0);
- rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
-
- /*
- * when we eventually call scsi_finish, we really wish to complete
- * the original request, so let's restore the original data. (db)
- */
- scsi_setup_cmd_retry(scmd);
- scmd->result = saved_result;
-
- /*
- * hey, we are done. let's look to see what happened.
- */
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd %p rtn %x\n",
__FUNCTION__, scmd, rtn));
- if (rtn == SUCCESS)
- return 0;
- else if (rtn == NEEDS_RETRY) {
+
+ switch (rtn) {
+ case NEEDS_RETRY:
if (retry_cnt--)
goto retry_tur;
+ /*FALLTHRU*/
+ case SUCCESS:
return 0;
+ default:
+ return 1;
}
- return 1;
}
/**
@@ -817,44 +813,16 @@
static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
{
static unsigned char stu_command[6] = {START_STOP, 0, 0, 0, 1, 0};
- int rtn;
- int saved_result;
-
- if (!scmd->device->allow_restart)
- return 1;
-
- memcpy(scmd->cmnd, stu_command, sizeof(stu_command));
-
- /*
- * zero the sense buffer. the scsi spec mandates that any
- * untransferred sense data should be interpreted as being zero.
- */
- memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
- saved_result = scmd->result;
- scmd->request_buffer = NULL;
- scmd->request_bufflen = 0;
- scmd->use_sg = 0;
- scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
- scmd->underflow = 0;
- scmd->sc_data_direction = DMA_NONE;
+ if (scmd->device->allow_restart) {
+ int rtn;
- rtn = scsi_send_eh_cmnd(scmd, START_UNIT_TIMEOUT);
-
- /*
- * when we eventually call scsi_finish, we really wish to complete
- * the original request, so let's restore the original data. (db)
- */
- scsi_setup_cmd_retry(scmd);
- scmd->result = saved_result;
+ memcpy(scmd->cmnd, stu_command, sizeof(stu_command));
+ rtn = scsi_send_eh_cmnd(scmd, START_UNIT_TIMEOUT, 0);
+ if (rtn == SUCCESS)
+ return 0;
+ }
- /*
- * hey, we are done. let's look to see what happened.
- */
- SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd %p rtn %x\n",
- __FUNCTION__, scmd, rtn));
- if (rtn == SUCCESS)
- return 0;
return 1;
}
@@ -1663,8 +1631,6 @@
scmd->scsi_done = scsi_reset_provider_done_command;
scmd->done = NULL;
- scmd->buffer = NULL;
- scmd->bufflen = 0;
scmd->request_buffer = NULL;
scmd->request_bufflen = 0;
Index: scsi-misc-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_lib.c 2006-06-10 17:46:59.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_lib.c 2006-06-10 17:53:06.000000000 +0200
@@ -436,60 +436,16 @@
*
* Arguments: cmd - command that is ready to be queued.
*
- * Returns: Nothing
- *
* Notes: This function has the job of initializing a number of
* fields related to error handling. Typically this will
* be called once for each command, as required.
*/
-static int scsi_init_cmd_errh(struct scsi_cmnd *cmd)
+static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
{
cmd->serial_number = 0;
-
memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);
-
if (cmd->cmd_len == 0)
cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
-
- /*
- * We need saved copies of a number of fields - this is because
- * error handling may need to overwrite these with different values
- * to run different commands, and once error handling is complete,
- * we will need to restore these values prior to running the actual
- * command.
- */
- cmd->old_use_sg = cmd->use_sg;
- cmd->old_cmd_len = cmd->cmd_len;
- cmd->sc_old_data_direction = cmd->sc_data_direction;
- cmd->old_underflow = cmd->underflow;
- memcpy(cmd->data_cmnd, cmd->cmnd, sizeof(cmd->cmnd));
- cmd->buffer = cmd->request_buffer;
- cmd->bufflen = cmd->request_bufflen;
-
- return 1;
-}
-
-/*
- * Function: scsi_setup_cmd_retry()
- *
- * Purpose: Restore the command state for a retry
- *
- * Arguments: cmd - command to be restored
- *
- * Returns: Nothing
- *
- * Notes: Immediately prior to retrying a command, we need
- * to restore certain fields that we saved above.
- */
-void scsi_setup_cmd_retry(struct scsi_cmnd *cmd)
-{
- memcpy(cmd->cmnd, cmd->data_cmnd, sizeof(cmd->data_cmnd));
- cmd->request_buffer = cmd->buffer;
- cmd->request_bufflen = cmd->bufflen;
- cmd->use_sg = cmd->old_use_sg;
- cmd->cmd_len = cmd->old_cmd_len;
- cmd->sc_data_direction = cmd->sc_old_data_direction;
- cmd->underflow = cmd->old_underflow;
}
void scsi_device_unbusy(struct scsi_device *sdev)
@@ -807,22 +763,13 @@
*/
static void scsi_release_buffers(struct scsi_cmnd *cmd)
{
- struct request *req = cmd->request;
-
- /*
- * Free up any indirection buffers we allocated for DMA purposes.
- */
if (cmd->use_sg)
scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
- else if (cmd->request_buffer != req->buffer)
- kfree(cmd->request_buffer);
/*
* Zero these out. They now point to freed memory, and it is
* dangerous to hang onto the pointers.
*/
- cmd->buffer = NULL;
- cmd->bufflen = 0;
cmd->request_buffer = NULL;
cmd->request_bufflen = 0;
}
@@ -859,7 +806,7 @@
unsigned int block_bytes)
{
int result = cmd->result;
- int this_count = cmd->bufflen;
+ int this_count = cmd->request_bufflen;
request_queue_t *q = cmd->device->request_queue;
struct request *req = cmd->request;
int clear_errors = 1;
@@ -867,28 +814,14 @@
int sense_valid = 0;
int sense_deferred = 0;
- /*
- * Free up any indirection buffers we allocated for DMA purposes.
- * For the case of a READ, we need to copy the data out of the
- * bounce buffer and into the real buffer.
- */
- if (cmd->use_sg)
- scsi_free_sgtable(cmd->buffer, cmd->sglist_len);
- else if (cmd->buffer != req->buffer) {
- if (rq_data_dir(req) == READ) {
- unsigned long flags;
- char *to = bio_kmap_irq(req->bio, &flags);
- memcpy(to, cmd->buffer, cmd->bufflen);
- bio_kunmap_irq(to, &flags);
- }
- kfree(cmd->buffer);
- }
+ scsi_release_buffers(cmd);
if (result) {
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
if (sense_valid)
sense_deferred = scsi_sense_is_deferred(&sshdr);
}
+
if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
req->errors = result;
if (result) {
@@ -909,15 +842,6 @@
}
/*
- * Zero these out. They now point to freed memory, and it is
- * dangerous to hang onto the pointers.
- */
- cmd->buffer = NULL;
- cmd->bufflen = 0;
- cmd->request_buffer = NULL;
- cmd->request_bufflen = 0;
-
- /*
* Next deal with any sectors which we were able to correctly
* handle.
*/
@@ -1017,7 +941,7 @@
if (!(req->flags & REQ_QUIET)) {
scmd_printk(KERN_INFO, cmd,
"Volume overflow, CDB: ");
- __scsi_print_command(cmd->data_cmnd);
+ __scsi_print_command(cmd->cmnd);
scsi_print_sense("", cmd);
}
scsi_end_request(cmd, 0, block_bytes, 1);
@@ -1156,7 +1080,7 @@
* successfully. Since this is a REQ_BLOCK_PC command the
* caller should check the request's errors value
*/
- scsi_io_completion(cmd, cmd->bufflen, 0);
+ scsi_io_completion(cmd, cmd->request_bufflen, 0);
}
static void scsi_setup_blk_pc_cmnd(struct scsi_cmnd *cmd)
Index: scsi-misc-2.6/drivers/scsi/scsi.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi.c 2006-06-10 17:22:08.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi.c 2006-06-10 17:53:47.000000000 +0200
@@ -346,7 +346,7 @@
if (level > 3) {
printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
" done = 0x%p, queuecommand 0x%p\n",
- cmd->buffer, cmd->bufflen,
+ cmd->request_buffer, cmd->request_bufflen,
cmd->done,
sdev->host->hostt->queuecommand);
@@ -643,11 +643,6 @@
*/
int scsi_retry_command(struct scsi_cmnd *cmd)
{
- /*
- * Restore the SCSI command state.
- */
- scsi_setup_cmd_retry(cmd);
-
/*
* Zero the sense information from the last time we tried
* this command.
@@ -693,10 +688,6 @@
"Notifying upper driver of completion "
"(result %x)\n", cmd->result));
- /*
- * We can get here with use_sg=0, causing a panic in the upper level
- */
- cmd->use_sg = cmd->old_use_sg;
cmd->done(cmd);
}
EXPORT_SYMBOL(scsi_finish_command);
Index: scsi-misc-2.6/include/scsi/scsi_cmnd.h
===================================================================
--- scsi-misc-2.6.orig/include/scsi/scsi_cmnd.h 2006-06-10 17:39:10.000000000 +0200
+++ scsi-misc-2.6/include/scsi/scsi_cmnd.h 2006-06-10 17:53:06.000000000 +0200
@@ -58,9 +58,7 @@
int timeout_per_command;
unsigned char cmd_len;
- unsigned char old_cmd_len;
enum dma_data_direction sc_data_direction;
- enum dma_data_direction sc_old_data_direction;
/* These elements define the operation we are about to perform */
#define MAX_COMMAND_SIZE 16
@@ -71,18 +69,11 @@
void *request_buffer; /* Actual requested buffer */
/* These elements define the operation we ultimately want to perform */
- unsigned char data_cmnd[MAX_COMMAND_SIZE];
- unsigned short old_use_sg; /* We save use_sg here when requesting
- * sense info */
unsigned short use_sg; /* Number of pieces of scatter-gather */
unsigned short sglist_len; /* size of malloc'd scatter-gather list */
- unsigned bufflen; /* Size of data buffer */
- void *buffer; /* Data buffer */
unsigned underflow; /* Return error if less than
this amount is transferred */
- unsigned old_underflow; /* save underflow here when reusing the
- * command for error handling */
unsigned transfersize; /* How much we are guaranteed to
transfer with each SCSI transfer
Index: scsi-misc-2.6/drivers/scsi/scsi_priv.h
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_priv.h 2006-06-10 17:29:58.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_priv.h 2006-06-10 17:53:06.000000000 +0200
@@ -58,7 +58,6 @@
/* scsi_lib.c */
extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
-extern void scsi_setup_cmd_retry(struct scsi_cmnd *cmd);
extern void scsi_device_unbusy(struct scsi_device *sdev);
extern int scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
extern void scsi_next_command(struct scsi_cmnd *cmd);
Index: scsi-misc-2.6/drivers/scsi/sd.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/sd.c 2006-06-10 17:00:39.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/sd.c 2006-06-10 17:53:06.000000000 +0200
@@ -477,8 +477,7 @@
SCpnt->cmnd[4] = (unsigned char) this_count;
SCpnt->cmnd[5] = 0;
}
- SCpnt->request_bufflen = SCpnt->bufflen =
- this_count * sdp->sector_size;
+ SCpnt->request_bufflen = this_count * sdp->sector_size;
/*
* We shouldn't disconnect in the middle of a sector, so with a dumb
Index: scsi-misc-2.6/drivers/scsi/sr.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/sr.c 2006-06-10 17:00:39.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/sr.c 2006-06-10 17:53:06.000000000 +0200
@@ -360,7 +360,7 @@
"mismatch count %d, bytes %d\n",
size, SCpnt->request_bufflen);
if (SCpnt->request_bufflen > size)
- SCpnt->request_bufflen = SCpnt->bufflen = size;
+ SCpnt->request_bufflen = size;
}
}
@@ -387,8 +387,7 @@
if (this_count > 0xffff) {
this_count = 0xffff;
- SCpnt->request_bufflen = SCpnt->bufflen =
- this_count * s_size;
+ SCpnt->request_bufflen = this_count * s_size;
}
SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
Index: scsi-misc-2.6/drivers/scsi/aha152x.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/aha152x.c 2006-06-10 16:59:29.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/aha152x.c 2006-06-10 17:53:06.000000000 +0200
@@ -1165,6 +1165,10 @@
DECLARE_MUTEX_LOCKED(sem);
struct timer_list timer;
int ret, issued, disconnected;
+ unsigned char old_cmd_len = SCpnt->cmd_len;
+ unsigned short old_use_sg = SCpnt->use_sg;
+ void *old_buffer = SCpnt->request_buffer;
+ unsigned old_bufflen = SCpnt->request_bufflen;
unsigned long flags;
#if defined(AHA152X_DEBUG)
@@ -1198,11 +1202,11 @@
add_timer(&timer);
down(&sem);
del_timer(&timer);
-
- SCpnt->cmd_len = SCpnt->old_cmd_len;
- SCpnt->use_sg = SCpnt->old_use_sg;
- SCpnt->request_buffer = SCpnt->buffer;
- SCpnt->request_bufflen = SCpnt->bufflen;
+
+ SCpnt->cmd_len = old_cmd_len;
+ SCpnt->use_sg = old_use_sg;
+ SCpnt->request_buffer = old_buffer;
+ SCpnt->request_bufflen = old_bufflen;
DO_LOCK(flags);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-12 19:19 ` Christoph Hellwig
@ 2006-06-14 2:31 ` James Bottomley
2006-06-14 2:40 ` James Bottomley
2006-06-14 2:43 ` James Bottomley
2 siblings, 0 replies; 13+ messages in thread
From: James Bottomley @ 2006-06-14 2:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: fischer, linux-scsi
On Mon, 2006-06-12 at 21:19 +0200, Christoph Hellwig wrote:
> Note that this breaks the aha152x and 53x700 drivers because they're
> plaing with those fields internally.
Well, this is what I think the changes for the 53c700 need to be ... I
haven't had time to check it with actual hardware yet (particularly as I
need to arrange to trip a check condition in the device to test it) but
it compiles. Presumably the aha drivers also need something similar
doing to them.
James
Index: BUILD-2.6/drivers/scsi/53c700.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/53c700.c 2006-06-13 15:53:08.000000000 -0500
+++ BUILD-2.6/drivers/scsi/53c700.c 2006-06-13 15:53:14.000000000 -0500
@@ -183,6 +183,10 @@
STATIC struct scsi_transport_template *NCR_700_transport_template = NULL;
+struct NCR_700_sense {
+ unsigned char cmnd[MAX_COMMAND_SIZE];
+};
+
static char *NCR_700_phase[] = {
"",
"after selection",
@@ -537,6 +541,7 @@
* finish routine. If we cannot queue the command when it
* is properly build, we then change to NCR_700_SLOT_QUEUED */
slot->state = NCR_700_SLOT_BUSY;
+ slot->flags = 0;
hostdata->command_slot_count++;
return slot;
@@ -586,7 +591,7 @@
if(SCp->sc_data_direction != DMA_NONE &&
SCp->sc_data_direction != DMA_BIDIRECTIONAL) {
if(SCp->use_sg) {
- dma_unmap_sg(hostdata->dev, SCp->buffer,
+ dma_unmap_sg(hostdata->dev, SCp->request_buffer,
SCp->use_sg, SCp->sc_data_direction);
} else {
dma_unmap_single(hostdata->dev, slot->dma_handle,
@@ -608,30 +613,23 @@
(struct NCR_700_command_slot *)SCp->host_scribble;
NCR_700_unmap(hostdata, SCp, slot);
- dma_unmap_single(hostdata->dev, slot->pCmd,
- sizeof(SCp->cmnd), DMA_TO_DEVICE);
- if(SCp->cmnd[0] == REQUEST_SENSE && SCp->cmnd[6] == NCR_700_INTERNAL_SENSE_MAGIC) {
+ if (slot->flags == NCR_700_FLAG_AUTOSENSE) {
+ struct NCR_700_sense *sense = SCp->device->hostdata;
#ifdef NCR_700_DEBUG
printk(" ORIGINAL CMD %p RETURNED %d, new return is %d sense is\n",
SCp, SCp->cmnd[7], result);
scsi_print_sense("53c700", SCp);
#endif
+ dma_unmap_single(hostdata->dev, slot->dma_handle, sizeof(SCp->sense_buffer), DMA_FROM_DEVICE);
/* restore the old result if the request sense was
* successful */
if(result == 0)
- result = SCp->cmnd[7];
- /* now restore the original command */
- memcpy((void *) SCp->cmnd, (void *) SCp->data_cmnd,
- sizeof(SCp->data_cmnd));
- SCp->request_buffer = SCp->buffer;
- SCp->request_bufflen = SCp->bufflen;
- SCp->use_sg = SCp->old_use_sg;
- SCp->cmd_len = SCp->old_cmd_len;
- SCp->sc_data_direction = SCp->sc_old_data_direction;
- SCp->underflow = SCp->old_underflow;
-
- }
+ result = sense->cmnd[7];
+ } else
+ dma_unmap_single(hostdata->dev, slot->pCmd,
+ sizeof(SCp->cmnd), DMA_TO_DEVICE);
+
free_slot(slot, hostdata);
#ifdef NCR_700_DEBUG
if(NCR_700_get_depth(SCp->device) == 0 ||
@@ -979,6 +977,7 @@
"broken device is looping in contingent allegiance: ignoring\n");
NCR_700_scsi_done(hostdata, SCp, hostdata->status[0]);
} else {
+ struct NCR_700_sense *sense = SCp->device->hostdata;
#ifdef NCR_DEBUG
scsi_print_command(SCp);
printk(" cmd %p has status %d, requesting sense\n",
@@ -992,27 +991,25 @@
* data associated with the command
* here */
NCR_700_unmap(hostdata, SCp, slot);
-
- SCp->cmnd[0] = REQUEST_SENSE;
- SCp->cmnd[1] = (SCp->device->lun & 0x7) << 5;
- SCp->cmnd[2] = 0;
- SCp->cmnd[3] = 0;
- SCp->cmnd[4] = sizeof(SCp->sense_buffer);
- SCp->cmnd[5] = 0;
- SCp->cmd_len = 6;
+ dma_unmap_single(hostdata->dev, slot->pCmd,
+ sizeof(SCp->cmnd),
+ DMA_TO_DEVICE);
+
+ sense->cmnd[0] = REQUEST_SENSE;
+ sense->cmnd[1] = (SCp->device->lun & 0x7) << 5;
+ sense->cmnd[2] = 0;
+ sense->cmnd[3] = 0;
+ sense->cmnd[4] = sizeof(SCp->sense_buffer);
+ sense->cmnd[5] = 0;
/* Here's a quiet hack: the
* REQUEST_SENSE command is six bytes,
* so store a flag indicating that
* this was an internal sense request
* and the original status at the end
* of the command */
- SCp->cmnd[6] = NCR_700_INTERNAL_SENSE_MAGIC;
- SCp->cmnd[7] = hostdata->status[0];
- SCp->use_sg = 0;
- SCp->sc_data_direction = DMA_FROM_DEVICE;
- dma_sync_single_for_device(hostdata->dev, slot->pCmd,
- SCp->cmd_len, DMA_TO_DEVICE);
- SCp->request_bufflen = sizeof(SCp->sense_buffer);
+ sense->cmnd[6] = NCR_700_INTERNAL_SENSE_MAGIC;
+ sense->cmnd[7] = hostdata->status[0];
+ slot->pCmd = dma_map_single(hostdata->dev, sense->cmnd, sizeof(sense->cmnd), DMA_TO_DEVICE);
slot->dma_handle = dma_map_single(hostdata->dev, SCp->sense_buffer, sizeof(SCp->sense_buffer), DMA_FROM_DEVICE);
slot->SG[0].ins = bS_to_host(SCRIPT_MOVE_DATA_IN | sizeof(SCp->sense_buffer));
slot->SG[0].pAddr = bS_to_host(slot->dma_handle);
@@ -1024,6 +1021,7 @@
/* queue the command for reissue */
slot->state = NCR_700_SLOT_QUEUED;
+ slot->flags = NCR_700_FLAG_AUTOSENSE;
hostdata->state = NCR_700_HOST_FREE;
hostdata->cmd = NULL;
}
@@ -1244,7 +1242,7 @@
if(SCp->use_sg) {
for(i = 0; i < SCp->use_sg + 1; i++) {
- printk(KERN_INFO " SG[%d].length = %d, move_insn=%08x, addr %08x\n", i, ((struct scatterlist *)SCp->buffer)[i].length, ((struct NCR_700_command_slot *)SCp->host_scribble)->SG[i].ins, ((struct NCR_700_command_slot *)SCp->host_scribble)->SG[i].pAddr);
+ printk(KERN_INFO " SG[%d].length = %d, move_insn=%08x, addr %08x\n", i, ((struct scatterlist *)SCp->request_buffer)[i].length, ((struct NCR_700_command_slot *)SCp->host_scribble)->SG[i].ins, ((struct NCR_700_command_slot *)SCp->host_scribble)->SG[i].pAddr);
}
}
}
@@ -1403,12 +1401,14 @@
/* keep interrupts disabled until we have the command correctly
* set up so we cannot take a selection interrupt */
- hostdata->msgout[0] = NCR_700_identify(SCp->cmnd[0] != REQUEST_SENSE,
+ hostdata->msgout[0] = NCR_700_identify((SCp->cmnd[0] != REQUEST_SENSE &&
+ slot->flags != NCR_700_FLAG_AUTOSENSE),
SCp->device->lun);
/* for INQUIRY or REQUEST_SENSE commands, we cannot be sure
* if the negotiated transfer parameters still hold, so
* always renegotiate them */
- if(SCp->cmnd[0] == INQUIRY || SCp->cmnd[0] == REQUEST_SENSE) {
+ if(SCp->cmnd[0] == INQUIRY || SCp->cmnd[0] == REQUEST_SENSE ||
+ slot->flags == NCR_700_FLAG_AUTOSENSE) {
NCR_700_clear_flag(SCp->device, NCR_700_DEV_NEGOTIATED_SYNC);
}
@@ -1417,7 +1417,8 @@
* will refuse all tags, so send the request sense as untagged
* */
if((hostdata->tag_negotiated & (1<<scmd_id(SCp)))
- && (slot->tag != SCSI_NO_TAG && SCp->cmnd[0] != REQUEST_SENSE)) {
+ && (slot->tag != SCSI_NO_TAG && SCp->cmnd[0] != REQUEST_SENSE &&
+ slot->flags != NCR_700_FLAG_AUTOSENSE)) {
count += scsi_populate_tag_msg(SCp, &hostdata->msgout[count]);
}
@@ -1863,8 +1864,9 @@
__u32 count = 0;
if(SCp->use_sg) {
- sg_count = dma_map_sg(hostdata->dev, SCp->buffer,
- SCp->use_sg, direction);
+ sg_count = dma_map_sg(hostdata->dev,
+ SCp->request_buffer, SCp->use_sg,
+ direction);
} else {
vPtr = dma_map_single(hostdata->dev,
SCp->request_buffer,
@@ -1879,7 +1881,7 @@
for(i = 0; i < sg_count; i++) {
if(SCp->use_sg) {
- struct scatterlist *sg = SCp->buffer;
+ struct scatterlist *sg = SCp->request_buffer;
vPtr = sg_dma_address(&sg[i]);
count = sg_dma_len(&sg[i]);
@@ -2042,6 +2044,11 @@
struct NCR_700_Host_Parameters *hostdata =
(struct NCR_700_Host_Parameters *)SDp->host->hostdata[0];
+ SDp->hostdata = kmalloc(GFP_KERNEL, sizeof(struct NCR_700_sense));
+
+ if (!SDp->hostdata)
+ return -ENOMEM;
+
/* to do here: allocate memory; build a queue_full list */
if(SDp->tagged_supported) {
scsi_set_tag_type(SDp, MSG_ORDERED_TAG);
@@ -2065,7 +2072,8 @@
STATIC void
NCR_700_slave_destroy(struct scsi_device *SDp)
{
- /* to do here: deallocate memory */
+ kfree(SDp->hostdata);
+ SDp->hostdata = NULL;
}
static int
Index: BUILD-2.6/drivers/scsi/53c700.h
===================================================================
--- BUILD-2.6.orig/drivers/scsi/53c700.h 2006-06-13 15:53:10.000000000 -0500
+++ BUILD-2.6/drivers/scsi/53c700.h 2006-06-13 15:53:14.000000000 -0500
@@ -163,6 +163,8 @@
#define NCR_700_SLOT_BUSY (1|NCR_700_SLOT_MAGIC) /* slot has command active on HA */
#define NCR_700_SLOT_QUEUED (2|NCR_700_SLOT_MAGIC) /* slot has command to be made active on HA */
__u8 state;
+ #define NCR_700_FLAG_AUTOSENSE 0x01
+ __u8 flags;
int tag;
__u32 resume_offset;
struct scsi_cmnd *cmnd;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-12 19:19 ` Christoph Hellwig
2006-06-14 2:31 ` James Bottomley
@ 2006-06-14 2:40 ` James Bottomley
2006-06-14 2:43 ` James Bottomley
2 siblings, 0 replies; 13+ messages in thread
From: James Bottomley @ 2006-06-14 2:40 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: jejb, fischer, linux-scsi
On Mon, 2006-06-12 at 21:19 +0200, Christoph Hellwig wrote:
> looks like no one but James looked at it as I attached a completely
> different patch that has been merged long ago. Here's the real one:
Here's a piece that was missed from the recently added highpoint driver
James
Index: BUILD-2.6/drivers/scsi/hptiop.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/hptiop.c 2006-06-13 15:53:53.000000000 -0500
+++ BUILD-2.6/drivers/scsi/hptiop.c 2006-06-13 15:53:57.000000000 -0500
@@ -554,7 +554,7 @@
req->header.context = cpu_to_le32(IOPMU_QUEUE_ADDR_HOST_BIT |
(u32)_req->index);
req->header.context_hi32 = 0;
- req->dataxfer_length = cpu_to_le32(scp->bufflen);
+ req->dataxfer_length = cpu_to_le32(scp->request_bufflen);
req->channel = scp->device->channel;
req->target = scp->device->id;
req->lun = scp->device->lun;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-12 19:19 ` Christoph Hellwig
2006-06-14 2:31 ` James Bottomley
2006-06-14 2:40 ` James Bottomley
@ 2006-06-14 2:43 ` James Bottomley
2006-06-14 18:43 ` FUJITA Tomonori
2 siblings, 1 reply; 13+ messages in thread
From: James Bottomley @ 2006-06-14 2:43 UTC (permalink / raw)
To: FUJITA Tomonori, Christoph Hellwig; +Cc: linux-scsi
On Mon, 2006-06-12 at 21:19 +0200, Christoph Hellwig wrote:
> looks like no one but James looked at it as I attached a completely
> different patch that has been merged long ago. Here's the real one:
And finally, the scsi-target-2.6 tree likewise needs converting. This
one I'm not sure about, since they appear to need the copy from
cmnd_data to cmnd which I just killed. Can someone who knows this tree
look this over?
Thanks,
James
Index: BUILD-2.6/drivers/scsi/libsrp.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/libsrp.c 2006-06-13 15:54:32.000000000 -0500
+++ BUILD-2.6/drivers/scsi/libsrp.c 2006-06-13 15:54:35.000000000 -0500
@@ -434,7 +434,7 @@
scmd = scsi_host_get_command(shost, data_dir, GFP_KERNEL);
BUG_ON(!scmd);
scmd->SCp.ptr = (char *) iue;
- memcpy(scmd->data_cmnd, cmd->cdb, MAX_COMMAND_SIZE);
+ memcpy(scmd->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
scmd->request_bufflen = len;
scmd->tag = tag;
iue->scmd = scmd;
Index: BUILD-2.6/drivers/scsi/scsi_tgt_if.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/scsi_tgt_if.c 2006-06-13 15:54:32.000000000 -0500
+++ BUILD-2.6/drivers/scsi/scsi_tgt_if.c 2006-06-13 15:54:35.000000000 -0500
@@ -65,9 +65,6 @@
struct tgt_event *ev;
int err, len;
- /* FIXME: we need scsi core to do that. */
- memcpy(cmd->cmnd, cmd->data_cmnd, MAX_COMMAND_SIZE);
-
len = NLMSG_SPACE(sizeof(*ev));
/*
* TODO: add MAX_COMMAND_SIZE to ev and add mempool
@@ -122,7 +119,7 @@
ev.k.tsk_mgmt_req.function = function;
ev.k.tsk_mgmt_req.tag = tag;
memcpy(ev.k.tsk_mgmt_req.lun, scsilun, sizeof(ev.k.tsk_mgmt_req.lun));
- ev.k.tsk_mgmt_req.mid = (u64) data;
+ ev.k.tsk_mgmt_req.mid = (u64)(unsigned long)data;
dprintk("%d %x %llx %llx\n", host_no, function, (unsigned long long) tag,
(unsigned long long) ev.k.tsk_mgmt_req.mid);
Index: BUILD-2.6/drivers/scsi/scsi_tgt_lib.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/scsi_tgt_lib.c 2006-06-13 15:54:33.000000000 -0500
+++ BUILD-2.6/drivers/scsi/scsi_tgt_lib.c 2006-06-13 15:54:35.000000000 -0500
@@ -337,7 +337,7 @@
cmd->request_bufflen = rq->data_len;
- dprintk("cmd %p addr %p cnt %d %lu\n", cmd, cmd->buffer, cmd->use_sg,
+ dprintk("cmd %p addr %p cnt %d %lu\n", cmd, cmd->request_buffer, cmd->use_sg,
rq_data_dir(rq));
count = blk_rq_map_sg(rq->q, rq, cmd->request_buffer);
if (likely(count <= cmd->use_sg)) {
@@ -345,7 +345,7 @@
return 0;
}
- eprintk("cmd %p addr %p cnt %d\n", cmd, cmd->buffer, cmd->use_sg);
+ eprintk("cmd %p addr %p cnt %d\n", cmd, cmd->request_buffer, cmd->use_sg);
scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
return -EINVAL;
}
@@ -356,8 +356,8 @@
{
struct request_queue *q = cmd->request->q;
struct request *rq = cmd->request;
- void *uaddr = cmd->buffer;
- unsigned int len = cmd->bufflen;
+ void *uaddr = cmd->request_buffer;
+ unsigned int len = cmd->request_bufflen;
struct bio *bio;
int err;
@@ -425,12 +425,12 @@
}
dprintk("cmd %p request_bufflen %u bufflen %u\n",
- cmd, cmd->request_bufflen, cmd->bufflen);
+ cmd, cmd->request_bufflen, cmd->request_bufflen);
scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
bio_list_add(&tcmd->xfer_done_list, cmd->request->bio);
- cmd->buffer += cmd->request_bufflen;
+ cmd->request_buffer += cmd->request_bufflen;
cmd->offset += cmd->request_bufflen;
if (!tcmd->xfer_list.head) {
@@ -439,7 +439,7 @@
}
dprintk("cmd2 %p request_bufflen %u bufflen %u\n",
- cmd, cmd->request_bufflen, cmd->bufflen);
+ cmd, cmd->request_bufflen, cmd->request_bufflen);
bio = bio_list_pop(&tcmd->xfer_list);
BUG_ON(!bio);
@@ -556,12 +556,12 @@
* store the userspace values here, the working values are
* in the request_* values
*/
- cmd->buffer = (void *)uaddr;
+ cmd->request_buffer = (void *)uaddr;
if (len)
- cmd->bufflen = len;
+ cmd->request_bufflen = len;
cmd->result = result;
- if (!cmd->bufflen) {
+ if (!cmd->request_bufflen) {
err = __scsi_tgt_transfer_response(cmd);
goto done;
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-14 2:43 ` James Bottomley
@ 2006-06-14 18:43 ` FUJITA Tomonori
2006-06-14 19:03 ` Mike Christie
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: FUJITA Tomonori @ 2006-06-14 18:43 UTC (permalink / raw)
To: James.Bottomley; +Cc: fujita.tomonori, hch, linux-scsi
From: James Bottomley <James.Bottomley@SteelEye.com>
Subject: Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
Date: Tue, 13 Jun 2006 21:43:49 -0500
> On Mon, 2006-06-12 at 21:19 +0200, Christoph Hellwig wrote:
> > looks like no one but James looked at it as I attached a completely
> > different patch that has been merged long ago. Here's the real one:
>
> And finally, the scsi-target-2.6 tree likewise needs converting. This
> one I'm not sure about, since they appear to need the copy from
> cmnd_data to cmnd which I just killed. Can someone who knows this tree
> look this over?
Thanks for the patch.
tgt uses request_bufflen, bufflen, request_buffer, buffer to handle
very large requests (that involves multiple bios). We can add
substitutes into tgt specific data structure (scsi_tgt_cmd structure).
I sent a patch related with scsi_tgt_cmd structure last week.
http://marc.theaimsgroup.com/?l=linux-scsi&m=114960851929878&w=2
Will this patch be merged? I like to send a new patch on the top of
it.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-14 18:43 ` FUJITA Tomonori
@ 2006-06-14 19:03 ` Mike Christie
2006-06-16 6:31 ` FUJITA Tomonori
2006-06-20 7:49 ` FUJITA Tomonori
2 siblings, 0 replies; 13+ messages in thread
From: Mike Christie @ 2006-06-14 19:03 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: James.Bottomley, hch, linux-scsi
FUJITA Tomonori wrote:
> From: James Bottomley <James.Bottomley@SteelEye.com>
> Subject: Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
> Date: Tue, 13 Jun 2006 21:43:49 -0500
>
>> On Mon, 2006-06-12 at 21:19 +0200, Christoph Hellwig wrote:
>>> looks like no one but James looked at it as I attached a completely
>>> different patch that has been merged long ago. Here's the real one:
>> And finally, the scsi-target-2.6 tree likewise needs converting. This
>> one I'm not sure about, since they appear to need the copy from
>> cmnd_data to cmnd which I just killed. Can someone who knows this tree
>> look this over?
>
> Thanks for the patch.
>
> tgt uses request_bufflen, bufflen, request_buffer, buffer to handle
> very large requests (that involves multiple bios). We can add
> substitutes into tgt specific data structure (scsi_tgt_cmd structure).
>
I think that is going to be best since we are the only users now.
> I sent a patch related with scsi_tgt_cmd structure last week.
>
> http://marc.theaimsgroup.com/?l=linux-scsi&m=114960851929878&w=2
>
> Will this patch be merged? I like to send a new patch on the top of
> it.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-14 18:43 ` FUJITA Tomonori
2006-06-14 19:03 ` Mike Christie
@ 2006-06-16 6:31 ` FUJITA Tomonori
2006-06-20 7:49 ` FUJITA Tomonori
2 siblings, 0 replies; 13+ messages in thread
From: FUJITA Tomonori @ 2006-06-16 6:31 UTC (permalink / raw)
To: James.Bottomley; +Cc: hch, linux-scsi
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
Date: Thu, 15 Jun 2006 03:43:26 +0900
> From: James Bottomley <James.Bottomley@SteelEye.com>
> Subject: Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
> Date: Tue, 13 Jun 2006 21:43:49 -0500
>
> > On Mon, 2006-06-12 at 21:19 +0200, Christoph Hellwig wrote:
> > > looks like no one but James looked at it as I attached a completely
> > > different patch that has been merged long ago. Here's the real one:
> >
> > And finally, the scsi-target-2.6 tree likewise needs converting. This
> > one I'm not sure about, since they appear to need the copy from
> > cmnd_data to cmnd which I just killed. Can someone who knows this tree
> > look this over?
>
> Thanks for the patch.
>
> tgt uses request_bufflen, bufflen, request_buffer, buffer to handle
> very large requests (that involves multiple bios). We can add
> substitutes into tgt specific data structure (scsi_tgt_cmd structure).
>
> I sent a patch related with scsi_tgt_cmd structure last week.
>
> http://marc.theaimsgroup.com/?l=linux-scsi&m=114960851929878&w=2
>
> Will this patch be merged? I like to send a new patch on the top of
> it.
Kill the tgt code touching data_cmnd, sc->buffer, and sc->bufflen.
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
drivers/scsi/libsrp.c | 2 +-
drivers/scsi/scsi_tgt_if.c | 5 +----
drivers/scsi/scsi_tgt_lib.c | 27 ++++++++++++++++-----------
3 files changed, 18 insertions(+), 16 deletions(-)
35dcdd1d8ccb54502129abae619c58b4b3ab6eaf
diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 8a4534c..b8a7c3d 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -434,7 +434,7 @@ int srp_cmd_perform(struct iu_entry *iue
scmd = scsi_host_get_command(shost, data_dir, GFP_KERNEL);
BUG_ON(!scmd);
scmd->SCp.ptr = (char *) iue;
- memcpy(scmd->data_cmnd, cmd->cdb, MAX_COMMAND_SIZE);
+ memcpy(scmd->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
scmd->request_bufflen = len;
scmd->tag = tag;
iue->scmd = scmd;
diff --git a/drivers/scsi/scsi_tgt_if.c b/drivers/scsi/scsi_tgt_if.c
index ba1b75b..37e0feb 100644
--- a/drivers/scsi/scsi_tgt_if.c
+++ b/drivers/scsi/scsi_tgt_if.c
@@ -65,9 +65,6 @@ int scsi_tgt_uspace_send(struct scsi_cmn
struct tgt_event *ev;
int err, len;
- /* FIXME: we need scsi core to do that. */
- memcpy(cmd->cmnd, cmd->data_cmnd, MAX_COMMAND_SIZE);
-
len = NLMSG_SPACE(sizeof(*ev));
/*
* TODO: add MAX_COMMAND_SIZE to ev and add mempool
@@ -122,7 +119,7 @@ int scsi_tgt_uspace_send_tsk_mgmt(int ho
ev.k.tsk_mgmt_req.function = function;
ev.k.tsk_mgmt_req.tag = tag;
memcpy(ev.k.tsk_mgmt_req.lun, scsilun, sizeof(ev.k.tsk_mgmt_req.lun));
- ev.k.tsk_mgmt_req.mid = (u64) data;
+ ev.k.tsk_mgmt_req.mid = (u64) (unsigned long) data;
dprintk("%d %x %llx %llx\n", host_no, function, (unsigned long long) tag,
(unsigned long long) ev.k.tsk_mgmt_req.mid);
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index 2385db0..e82340c 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -50,6 +50,9 @@ struct scsi_tgt_cmd {
struct list_head hash_list;
struct request *rq;
u64 tag;
+
+ void *buffer;
+ unsigned bufflen;
};
#define TGT_HASH_ORDER 4
@@ -407,6 +410,7 @@ static void scsi_tgt_transfer_response(s
static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
{
struct request *rq = cmd->request;
+ struct scsi_tgt_cmd *tcmd = rq->end_io_data;
int count;
cmd->use_sg = rq->nr_phys_segments;
@@ -416,7 +420,7 @@ static int scsi_tgt_init_cmd(struct scsi
cmd->request_bufflen = rq->data_len;
- dprintk("cmd %p addr %p cnt %d %lu\n", cmd, cmd->buffer, cmd->use_sg,
+ dprintk("cmd %p addr %p cnt %d %lu\n", cmd, tcmd->buffer, cmd->use_sg,
rq_data_dir(rq));
count = blk_rq_map_sg(rq->q, rq, cmd->request_buffer);
if (likely(count <= cmd->use_sg)) {
@@ -424,7 +428,7 @@ static int scsi_tgt_init_cmd(struct scsi
return 0;
}
- eprintk("cmd %p addr %p cnt %d\n", cmd, cmd->buffer, cmd->use_sg);
+ eprintk("cmd %p addr %p cnt %d\n", cmd, tcmd->buffer, cmd->use_sg);
scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
return -EINVAL;
}
@@ -435,8 +439,8 @@ static int scsi_map_user_pages(struct sc
{
struct request_queue *q = cmd->request->q;
struct request *rq = cmd->request;
- void *uaddr = cmd->buffer;
- unsigned int len = cmd->bufflen;
+ void *uaddr = tcmd->buffer;
+ unsigned int len = tcmd->bufflen;
struct bio *bio;
int err;
@@ -504,12 +508,12 @@ send_uspace_err:
}
dprintk("cmd %p request_bufflen %u bufflen %u\n",
- cmd, cmd->request_bufflen, cmd->bufflen);
+ cmd, cmd->request_bufflen, tcmd->bufflen);
scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
bio_list_add(&tcmd->xfer_done_list, cmd->request->bio);
- cmd->buffer += cmd->request_bufflen;
+ tcmd->buffer += cmd->request_bufflen;
cmd->offset += cmd->request_bufflen;
if (!tcmd->xfer_list.head) {
@@ -518,7 +522,7 @@ send_uspace_err:
}
dprintk("cmd2 %p request_bufflen %u bufflen %u\n",
- cmd, cmd->request_bufflen, cmd->bufflen);
+ cmd, cmd->request_bufflen, tcmd->bufflen);
bio = bio_list_pop(&tcmd->xfer_list);
BUG_ON(!bio);
@@ -604,6 +608,7 @@ int scsi_tgt_kspace_exec(int host_no, u3
struct Scsi_Host *shost;
struct scsi_cmnd *cmd;
struct request *rq;
+ struct scsi_tgt_cmd *tcmd;
int err = 0;
dprintk("%d %u %d %u %lx %u\n", host_no, cid, result,
@@ -635,12 +640,12 @@ int scsi_tgt_kspace_exec(int host_no, u3
* store the userspace values here, the working values are
* in the request_* values
*/
- cmd->buffer = (void *)uaddr;
- if (len)
- cmd->bufflen = len;
+ tcmd = cmd->request->end_io_data;
+ tcmd->buffer = (void *)uaddr;
+ tcmd->bufflen = len;
cmd->result = result;
- if (!cmd->bufflen) {
+ if (!tcmd->bufflen) {
err = __scsi_tgt_transfer_response(cmd);
goto done;
}
--
1.1.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-14 18:43 ` FUJITA Tomonori
2006-06-14 19:03 ` Mike Christie
2006-06-16 6:31 ` FUJITA Tomonori
@ 2006-06-20 7:49 ` FUJITA Tomonori
2 siblings, 0 replies; 13+ messages in thread
From: FUJITA Tomonori @ 2006-06-20 7:49 UTC (permalink / raw)
To: fujita.tomonori; +Cc: James.Bottomley, hch, linux-scsi
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
Date: Thu, 15 Jun 2006 03:43:26 +0900
> From: James Bottomley <James.Bottomley@SteelEye.com>
> Subject: Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
> Date: Tue, 13 Jun 2006 21:43:49 -0500
>
> > On Mon, 2006-06-12 at 21:19 +0200, Christoph Hellwig wrote:
> > > looks like no one but James looked at it as I attached a completely
> > > different patch that has been merged long ago. Here's the real one:
> >
> > And finally, the scsi-target-2.6 tree likewise needs converting. This
> > one I'm not sure about, since they appear to need the copy from
> > cmnd_data to cmnd which I just killed. Can someone who knows this tree
> > look this over?
>
> Thanks for the patch.
>
> tgt uses request_bufflen, bufflen, request_buffer, buffer to handle
> very large requests (that involves multiple bios). We can add
> substitutes into tgt specific data structure (scsi_tgt_cmd structure).
>
> I sent a patch related with scsi_tgt_cmd structure last week.
>
> http://marc.theaimsgroup.com/?l=linux-scsi&m=114960851929878&w=2
>
> Will this patch be merged? I like to send a new patch on the top of
> it.
Hi James,
The 'update for cmnd field removal' patch got merged but the patch
refereed to in this mail did not. Did you just forget or you don't
like it?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-06-03 11:26 [PATCH, RFC] hide EH backup data outside the scsi_cmnd Christoph Hellwig
2006-06-03 11:31 ` Christoph Hellwig
2006-06-10 16:08 ` Christoph Hellwig
@ 2006-07-24 17:27 ` Tony Luck
2006-07-24 19:29 ` Christoph Hellwig
2 siblings, 1 reply; 13+ messages in thread
From: Tony Luck @ 2006-07-24 17:27 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: jejb, fischer, linux-scsi
On 6/3/06, Christoph Hellwig <hch@lst.de> wrote:
> Currently struct scsi_cmnd has various fields that are used to backup
> original data after the corresponding fields have been overridden for
> EH commands. This means drivers can easily get at it and misuse it.
> Due to the old_ naming this doesn't happen for most of them, but two
> that have different names have been used wrong a lot (see previous
> patch).
I guess that I'm an abuser :-(
> Index: scsi-misc-2.6/include/scsi/scsi_cmnd.h
> - void *buffer; /* Data buffer */
arch/ia64/hp/sim/simscsi.c is still using this (and so doesn't compile
now that this change has hit mainline). It seems that simscsi.c is
expecting to find a "scatterlist" there ... but looking at the rest of
the patch that removed this element, it isn't obvious where it went.
-Tony
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd
2006-07-24 17:27 ` Tony Luck
@ 2006-07-24 19:29 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2006-07-24 19:29 UTC (permalink / raw)
To: Tony Luck; +Cc: Christoph Hellwig, jejb, fischer, linux-scsi
On Mon, Jul 24, 2006 at 10:27:55AM -0700, Tony Luck wrote:
> On 6/3/06, Christoph Hellwig <hch@lst.de> wrote:
> >Currently struct scsi_cmnd has various fields that are used to backup
> >original data after the corresponding fields have been overridden for
> >EH commands. This means drivers can easily get at it and misuse it.
> >Due to the old_ naming this doesn't happen for most of them, but two
> >that have different names have been used wrong a lot (see previous
> >patch).
>
> I guess that I'm an abuser :-(
>
> >Index: scsi-misc-2.6/include/scsi/scsi_cmnd.h
>
> >- void *buffer; /* Data buffer */
>
> arch/ia64/hp/sim/simscsi.c is still using this (and so doesn't compile
> now that this change has hit mainline). It seems that simscsi.c is
> expecting to find a "scatterlist" there ... but looking at the rest of
> the patch that removed this element, it isn't obvious where it went.
Just change it to access the request_buffer member instead. buffer
and request_buffer have been synonymous 99% of the time, and a driver
never even wants to access buffer.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2006-07-24 19:30 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-06-03 11:26 [PATCH, RFC] hide EH backup data outside the scsi_cmnd Christoph Hellwig
2006-06-03 11:31 ` Christoph Hellwig
2006-06-10 16:08 ` Christoph Hellwig
2006-06-12 19:19 ` Christoph Hellwig
2006-06-14 2:31 ` James Bottomley
2006-06-14 2:40 ` James Bottomley
2006-06-14 2:43 ` James Bottomley
2006-06-14 18:43 ` FUJITA Tomonori
2006-06-14 19:03 ` Mike Christie
2006-06-16 6:31 ` FUJITA Tomonori
2006-06-20 7:49 ` FUJITA Tomonori
2006-07-24 17:27 ` Tony Luck
2006-07-24 19:29 ` Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.