* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd [not found] <20060603112610.GB17018@lst.de> @ 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; 12+ 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] 12+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd [not found] <20060603112610.GB17018@lst.de> 2006-06-03 11:31 ` [PATCH, RFC] hide EH backup data outside the scsi_cmnd 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ messages in thread
* Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd [not found] <20060603112610.GB17018@lst.de> 2006-06-03 11:31 ` [PATCH, RFC] hide EH backup data outside the scsi_cmnd 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; 12+ 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] 12+ 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; 12+ 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] 12+ messages in thread
end of thread, other threads:[~2006-07-24 19:30 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20060603112610.GB17018@lst.de>
2006-06-03 11:31 ` [PATCH, RFC] hide EH backup data outside the scsi_cmnd 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox