Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[Christoph Hellwig]

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---

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[Christoph Hellwig]

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;

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[Christoph Hellwig]

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);
 

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[James Bottomley]

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;

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[James Bottomley]

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;

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[James Bottomley]

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;
 	}

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[FUJITA Tomonori]

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.

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[Mike Christie]

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

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[FUJITA Tomonori]

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

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[FUJITA Tomonori]

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?

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[Tony Luck]

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

Re: [PATCH, RFC] hide EH backup data outside the scsi_cmnd

[Christoph Hellwig]

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.