Re: [PATCH 24/24] ide-scsi.c: convert to data accessors and !use_sg cleanup

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Wednesday 12 September 2007, Boaz Harrosh wrote:
> 
>  - Convert ide-scsi to the new data accessors and cleanup
>    the !use_sg code paths.
> 
>   Inspecting old code I can see places that still assume
>   scsi_cmnd->request_buffer is a linear char pointer. Though I
>   admit this assumption is hidden behind a flag:
>   test_bit(PC_TRANSFORM, &pc->flags).

Sigh... somebody forgot to update ide-scsi.c while making SCSI use
only sg for REQ_TYPE_BLOCK_PC.

>   I have commented out the offending code and put a
>   WARN_ON(1) in it's place. So if there is missing functionality

in idescsi_transform_pc2():

        if (drive->media == ide_cdrom || drive->media == ide_optical) {
...
		WARN_ON(1) + #if/#endif 0 should be here
...
	}

rest of the changes look OK and you can add my ACK after fixing
idescsi_transform_pc2()

>   it will complain but will not crash the kernel.

sr is likely to complain due to scsi_mode_sense() usage :(

>   Maintainer of this driver please inspect if this functionality
>   is still needed, if not then please remove the commented code.
> 
>   Thanks
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/ide-scsi.c |   53 ++++++++++++++++++++++++++++++----------------
>  1 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/ide-scsi.c b/drivers/scsi/ide-scsi.c
> index 1cc01ac..e3e7d65 100644
> --- a/drivers/scsi/ide-scsi.c
> +++ b/drivers/scsi/ide-scsi.c
> @@ -175,7 +175,8 @@ static void idescsi_input_buffers (ide_drive_t *drive, idescsi_pc_t *pc, unsigne
>  	char *buf;
>  
>  	while (bcount) {
> -		if (pc->sg - (struct scatterlist *) pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
> +		if (pc->sg - scsi_sglist(pc->scsi_cmd) >
> +		                                 scsi_sg_count(pc->scsi_cmd)) {
>  			printk (KERN_ERR "ide-scsi: scatter gather table too small, discarding data\n");
>  			idescsi_discard_data (drive, bcount);
>  			return;
> @@ -210,7 +211,8 @@ static void idescsi_output_buffers (ide_drive_t *drive, idescsi_pc_t *pc, unsign
>  	char *buf;
>  
>  	while (bcount) {
> -		if (pc->sg - (struct scatterlist *) pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
> +		if (pc->sg - scsi_sglist(pc->scsi_cmd) >
> +		                                 scsi_sg_count(pc->scsi_cmd)) {
>  			printk (KERN_ERR "ide-scsi: scatter gather table too small, padding with zeros\n");
>  			idescsi_output_zeros (drive, bcount);
>  			return;
> @@ -260,6 +262,10 @@ static inline void idescsi_transform_pc1 (ide_drive_t *drive, idescsi_pc_t *pc)
>  			unsigned short new_len;
>  			if (!scsi_buf)
>  				return;
> +			/* FIXME: see comment below at idescsi_transform_pc2 */
> +			WARN_ON(1);
> +
> +#if 0
>  			if ((atapi_buf = kmalloc(pc->buffer_size + 4, GFP_ATOMIC)) == NULL)
>  				return;
>  			memset(atapi_buf, 0, pc->buffer_size + 4);
> @@ -281,12 +287,28 @@ static inline void idescsi_transform_pc1 (ide_drive_t *drive, idescsi_pc_t *pc)
>  			pc->buffer = atapi_buf;
>  			pc->request_transfer += 4;
>  			pc->buffer_size += 4;
> +#endif
>  		}
>  	}
>  }
>  
>  static inline void idescsi_transform_pc2 (ide_drive_t *drive, idescsi_pc_t *pc)
>  {
> +	if (!test_bit(PC_TRANSFORM, &pc->flags))
> +		return;
> +
> +	/*
> +	 * FIXME: if code reached here then this driver is not
> +	 * working for sure. If it is dead code then it can
> +	 * surly be properly removed.
> +	 *
> +	 * Same is with above idescsi_transform_pc1() which
> +	 * assumes pc->buffer must have something which surly
> +	 * does not.
> +	 */
> +        WARN_ON(1);
> +
> +#if 0
>  	u8 *atapi_buf = pc->buffer;
>  	u8 *sc = pc->scsi_cmd->cmnd;
>  	u8 *scsi_buf = pc->scsi_cmd->request_buffer;
> @@ -308,6 +330,7 @@ static inline void idescsi_transform_pc2 (ide_drive_t *drive, idescsi_pc_t *pc)
>  	}
>  	if (atapi_buf && atapi_buf != scsi_buf)
>  		kfree(atapi_buf);
> +#endif
>  }
>  
>  static void hexdump(u8 *x, int len)
> @@ -435,6 +458,7 @@ static int idescsi_end_request (ide_drive_t *drive, int uptodate, int nrsecs)
>  	} else {
>  		pc->scsi_cmd->result = DID_OK << 16;
>  		idescsi_transform_pc2 (drive, pc);
> +#if 0
>  		if (log) {
>  			printk ("ide-scsi: %s: suc %lu", drive->name, pc->scsi_cmd->serial_number);
>  			if (!test_bit(PC_WRITING, &pc->flags) && pc->actually_transferred && pc->actually_transferred <= 1024 && pc->buffer) {
> @@ -443,6 +467,7 @@ static int idescsi_end_request (ide_drive_t *drive, int uptodate, int nrsecs)
>  				hexdump(scsi_buf, min_t(unsigned, 16, pc->scsi_cmd->request_bufflen));
>  			} else printk("\n");
>  		}
> +#endif
>  	}
>  	host = pc->scsi_cmd->device->host;
>  	spin_lock_irqsave(host->host_lock, flags);
> @@ -637,19 +662,14 @@ static int idescsi_map_sg(ide_drive_t *drive, idescsi_pc_t *pc)
>  		return 1;
>  
>  	sg = hwif->sg_table;
> -	scsi_sg = pc->scsi_cmd->request_buffer;
> -	segments = pc->scsi_cmd->use_sg;
> +	scsi_sg = scsi_sglist(pc->scsi_cmd);
> +	segments = scsi_sg_count(pc->scsi_cmd);
>  
>  	if (segments > hwif->sg_max_nents)
>  		return 1;
>  
> -	if (!segments) {
> -		hwif->sg_nents = 1;
> -		sg_init_one(sg, pc->scsi_cmd->request_buffer, pc->request_transfer);
> -	} else {
> -		hwif->sg_nents = segments;
> -		memcpy(sg, scsi_sg, sizeof(*sg) * segments);
> -	}
> +	hwif->sg_nents = segments;
> +	memcpy(sg, scsi_sg, sizeof(*sg) * segments);
>  
>  	return 0;
>  }
> @@ -905,15 +925,10 @@ static int idescsi_queue (struct scsi_cmnd *cmd,
>  	pc->flags = 0;
>  	pc->rq = rq;
>  	memcpy (pc->c, cmd->cmnd, cmd->cmd_len);
> -	if (cmd->use_sg) {
> -		pc->buffer = NULL;
> -		pc->sg = cmd->request_buffer;
> -	} else {
> -		pc->buffer = cmd->request_buffer;
> -		pc->sg = NULL;
> -	}
> +	pc->buffer = NULL;
> +	pc->sg = scsi_sglist(cmd);
>  	pc->b_count = 0;
> -	pc->request_transfer = pc->buffer_size = cmd->request_bufflen;
> +	pc->request_transfer = pc->buffer_size = scsi_bufflen(cmd);
>  	pc->scsi_cmd = cmd;
>  	pc->done = done;
>  	pc->timeout = jiffies + cmd->timeout_per_command;