Data accessors more patches

Data accessors more patches

[Boaz Harrosh]

As a reply to this email I will send some more files
converted to new data accessors, and attempts of cleanups
of: !use_sg code path's, use of scsi_for_each_sg and use
of scsi_{,un}map_dma().

some of these files are in scsi-ml core but they can be converted
now, and are not at all dependent on implementation.

libata-scsi.c	- code will now work with scsi_sgtable but farther cleanup is needed.
scsi.c 		- some prints might as well use accessors now.
scsi_debug.c	- This is actually a scsi LLD. Normal conversion.

The rest of the converted files I will send in the following days

Boaz

[PATCH] libata-scsi.c: convert to use the data buffer accessors

[Boaz Harrosh]

 This is a minimal patch needed if we want to use the
 new scsi_sgtable implementation, by using of the new
 data accessors. But it is not a complete clean up of
 the !use_sg path.
 Libata-core still has the qc->flags & ATA_QCFLAG_SG
 and !qc->n_elem code paths. Perhaps an ata maintainer
 would have a go at it.

 - use of new data accessors.
 - clean some of the more obvious !use_sg path
 - TODO: farther cleanup of qc->flags & ATA_QCFLAG_SG
   and !qc->n_elem code paths in libata-core
 - TODO: Use of new scsi_dma_{map,unmap} if applicable.
---
 drivers/ata/libata-scsi.c |   39 +++++++++++++--------------------------
 1 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 13603a0..c0af390 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -450,13 +450,8 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
 		qc->scsicmd = cmd;
 		qc->scsidone = done;

-		if (cmd->use_sg) {
-			qc->__sg = (struct scatterlist *) cmd->request_buffer;
-			qc->n_elem = cmd->use_sg;
-		} else if (cmd->request_bufflen) {
-			qc->__sg = &qc->sgent;
-			qc->n_elem = 1;
-		}
+		qc->__sg = scsi_sglist(cmd);
+		qc->n_elem = scsi_sg_count(cmd);
 	} else {
 		cmd->result = (DID_OK << 16) | (QUEUE_FULL << 1);
 		done(cmd);
@@ -1496,17 +1491,13 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
 	/* data is present; dma-map it */
 	if (cmd->sc_data_direction == DMA_FROM_DEVICE ||
 	    cmd->sc_data_direction == DMA_TO_DEVICE) {
-		if (unlikely(cmd->request_bufflen < 1)) {
+		if (unlikely(scsi_bufflen(cmd) < 1)) {
 			ata_dev_printk(dev, KERN_WARNING,
 				       "WARNING: zero len r/w req\n");
 			goto err_did;
 		}

-		if (cmd->use_sg)
-			ata_sg_init(qc, cmd->request_buffer, cmd->use_sg);
-		else
-			ata_sg_init_one(qc, cmd->request_buffer,
-					cmd->request_bufflen);
+		ata_sg_init(qc, scsi_sglist(cmd), scsi_sg_count(cmd));

 		qc->dma_dir = cmd->sc_data_direction;
 	}
@@ -1560,15 +1551,14 @@ static unsigned int ata_scsi_rbuf_get(struct scsi_cmnd *cmd, u8 **buf_out)
 	u8 *buf;
 	unsigned int buflen;

-	if (cmd->use_sg) {
-		struct scatterlist *sg;
+	struct scatterlist *sg = scsi_sglist(cmd);

-		sg = (struct scatterlist *) cmd->request_buffer;
+	if (sg) {
 		buf = kmap_atomic(sg->page, KM_IRQ0) + sg->offset;
 		buflen = sg->length;
 	} else {
-		buf = cmd->request_buffer;
-		buflen = cmd->request_bufflen;
+		buf = NULL;
+		buflen = 0;
 	}

 	*buf_out = buf;
@@ -1588,12 +1578,9 @@ static unsigned int ata_scsi_rbuf_get(struct scsi_cmnd *cmd, u8 **buf_out)

 static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd, u8 *buf)
 {
-	if (cmd->use_sg) {
-		struct scatterlist *sg;
-
-		sg = (struct scatterlist *) cmd->request_buffer;
+	struct scatterlist *sg = scsi_sglist(cmd);
+	if (sg)
 		kunmap_atomic(buf - sg->offset, KM_IRQ0);
-	}
 }

 /**
@@ -2420,7 +2407,7 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 			qc->tf.feature |= ATAPI_DMADIR;
 	}

-	qc->nbytes = scmd->request_bufflen;
+	qc->nbytes = scsi_bufflen(scmd);

 	return 0;
 }
@@ -2633,7 +2620,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	case ATA_CMD_WRITE_LONG_ONCE:
 		if (tf->protocol != ATA_PROT_PIO || tf->nsect != 1)
 			goto invalid_fld;
-		qc->sect_size = scmd->request_bufflen;
+		qc->sect_size = scsi_bufflen(scmd);
 	}

 	/*
@@ -2663,7 +2650,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	 * TODO: find out if we need to do more here to
 	 *       cover scatter/gather case.
 	 */
-	qc->nbytes = scmd->request_bufflen;
+	qc->nbytes = scsi_bufflen(scmd);

 	/* request result TF */
 	qc->flags |= ATA_QCFLAG_RESULT_TF;
-- 
1.5.2.2.249.g45fd

[PATCH] scsi.c: convert to use the data buffer accessors

[Boaz Harrosh]

 - a couple of prints, they can use the accessors

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 4c1e313..1d1aba6 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -368,7 +368,7 @@ void scsi_log_send(struct scsi_cmnd *cmd)
 			if (level > 3) {
 				printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
 				       " done = 0x%p, queuecommand 0x%p\n",
-					cmd->request_buffer, cmd->request_bufflen,
+					scsi_sglist(cmd), scsi_bufflen(cmd),
 					cmd->done,
 					cmd->device->host->hostt->queuecommand);

-- 
1.5.2.2.249.g45fd

[PATCH] scsi_debug: convert to use the data buffer accessors and !use_sg cleanup

[Boaz Harrosh]

 scsi_debug is just a synthetic scsi target that simulate
 a scsi device and can inject errors to test the error
 handling paths in scsi-ml. As any scsi LLD it is converted
 to use the new data accessors.
 - Use of new data accessors
 - Clean of !use_sg code path.

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_debug.c |   46 +++++++++++++++-----------------------------
 1 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 4cd9c58..1e2d6bb 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -328,7 +328,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd * SCpnt, done_funct_t done)
 	if (done == NULL)
 		return 0;	/* assume mid level reprocessing command */

-	SCpnt->resid = 0;
+	scsi_set_resid(SCpnt, 0);
 	if ((SCSI_DEBUG_OPT_NOISE & scsi_debug_opts) && cmd) {
 		printk(KERN_INFO "scsi_debug: cmd ");
 		for (k = 0, len = SCpnt->cmd_len; k < len; ++k)
@@ -597,31 +597,22 @@ static int check_readiness(struct scsi_cmnd * SCpnt, int reset_only,
 static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 				int arr_len)
 {
-	int k, req_len, act_len, len, active;
+	int k, req_len, act_len, len, active, sg_count;
 	void * kaddr;
 	void * kaddr_off;
 	struct scatterlist * sgpnt;

-	if (0 == scp->request_bufflen)
+	if (0 == scsi_bufflen(scp))
 		return 0;
-	if (NULL == scp->request_buffer)
+	if (NULL == scsi_sglist(scp))
 		return (DID_ERROR << 16);
 	if (! ((scp->sc_data_direction == DMA_BIDIRECTIONAL) ||
 	      (scp->sc_data_direction == DMA_FROM_DEVICE)))
 		return (DID_ERROR << 16);
-	if (0 == scp->use_sg) {
-		req_len = scp->request_bufflen;
-		act_len = (req_len < arr_len) ? req_len : arr_len;
-		memcpy(scp->request_buffer, arr, act_len);
-		if (scp->resid)
-			scp->resid -= act_len;
-		else
-			scp->resid = req_len - act_len;
-		return 0;
-	}
-	sgpnt = (struct scatterlist *)scp->request_buffer;
+	sg_count = scsi_sg_count(scp);
 	active = 1;
-	for (k = 0, req_len = 0, act_len = 0; k < scp->use_sg; ++k, ++sgpnt) {
+	req_len = act_len = 0;
+	scsi_for_each_sg(scp, sgpnt, sg_count, k) {
 		if (active) {
 			kaddr = (unsigned char *)
 				kmap_atomic(sgpnt->page, KM_USER0);
@@ -639,10 +630,10 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 		}
 		req_len += sgpnt->length;
 	}
-	if (scp->resid)
-		scp->resid -= act_len;
+	if (scsi_get_resid(scp))
+		scsi_set_resid(scp, scsi_get_resid(scp) - act_len);
 	else
-		scp->resid = req_len - act_len;
+		scsi_set_resid(scp, req_len - act_len);
 	return 0;
 }

@@ -650,26 +641,21 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 			       int max_arr_len)
 {
-	int k, req_len, len, fin;
+	int k, req_len, len, fin, sg_count;
 	void * kaddr;
 	void * kaddr_off;
 	struct scatterlist * sgpnt;

-	if (0 == scp->request_bufflen)
+	if (0 == scsi_bufflen(scp))
 		return 0;
-	if (NULL == scp->request_buffer)
+	if (NULL == scsi_sglist(scp))
 		return -1;
 	if (! ((scp->sc_data_direction == DMA_BIDIRECTIONAL) ||
 	      (scp->sc_data_direction == DMA_TO_DEVICE)))
 		return -1;
-	if (0 == scp->use_sg) {
-		req_len = scp->request_bufflen;
-		len = (req_len < max_arr_len) ? req_len : max_arr_len;
-		memcpy(arr, scp->request_buffer, len);
-		return len;
-	}
-	sgpnt = (struct scatterlist *)scp->request_buffer;
-	for (k = 0, req_len = 0, fin = 0; k < scp->use_sg; ++k, ++sgpnt) {
+	sg_count = scsi_sg_count(scp);
+	req_len = fin = 0;
+	scsi_for_each_sg(scp, sgpnt, sg_count, k) {
 		kaddr = (unsigned char *)kmap_atomic(sgpnt->page, KM_USER0);
 		if (NULL == kaddr)
 			return -1;
-- 
1.5.2.2.249.g45fd

[PATCH] microtek.c - use data accessors and !use_sg cleanup

[Boaz Harrosh]

  - use scsi_cmnd data accessors
  - Clean the !use_sg code paths

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/image/microtek.c |   32 ++++++++++++++------------------
 1 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c
index 51bd80d..0a5c6e8 100644
--- a/drivers/usb/image/microtek.c
+++ b/drivers/usb/image/microtek.c
@@ -447,7 +447,8 @@ static void mts_data_done( struct urb* transfer )
 	MTS_INT_INIT();
 
 	if ( context->data_length != transfer->actual_length ) {
-		context->srb->resid = context->data_length - transfer->actual_length;
+		scsi_set_resid(context->srb, context->data_length -
+		                                     transfer->actual_length);
 	} else if ( unlikely(transfer->status) ) {
 		context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
 	}
@@ -490,7 +491,8 @@ static void mts_command_done( struct urb *transfer )
 					   context->data_pipe,
 					   context->data,
 					   context->data_length,
-					   context->srb->use_sg > 1 ? mts_do_sg : mts_data_done);
+					   scsi_sg_count(context->srb) > 1 ?
+					           mts_do_sg : mts_data_done);
 		} else {
 			mts_get_status(transfer);
 		}
@@ -504,21 +506,23 @@ static void mts_do_sg (struct urb* transfer)
 	struct scatterlist * sg;
 	MTS_INT_INIT();
 
-	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,context->srb->use_sg);
+	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,
+	                                          scsi_sg_count(context->srb));
 
 	if (unlikely(transfer->status)) {
                 context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
 		mts_transfer_cleanup(transfer);
         }
 
-	sg = context->srb->request_buffer;
+	sg = scsi_sglist(context->srb);
 	context->fragment++;
 	mts_int_submit_urb(transfer,
 			   context->data_pipe,
 			   page_address(sg[context->fragment].page) +
 			   sg[context->fragment].offset,
 			   sg[context->fragment].length,
-			   context->fragment + 1 == context->srb->use_sg ? mts_data_done : mts_do_sg);
+			   context->fragment + 1 == scsi_sg_count(context->srb) ?
+			   mts_data_done : mts_do_sg);
 	return;
 }
 
@@ -546,20 +550,12 @@ mts_build_transfer_context(struct scsi_cmnd *srb, struct mts_desc* desc)
 	desc->context.srb = srb;
 	desc->context.fragment = 0;
 
-	if (!srb->use_sg) {
-		if ( !srb->request_bufflen ){
-			desc->context.data = NULL;
-			desc->context.data_length = 0;
-			return;
-		} else {
-			desc->context.data = srb->request_buffer;
-			desc->context.data_length = srb->request_bufflen;
-			MTS_DEBUG("length = %d or %d\n",
-				  srb->request_bufflen, srb->bufflen);
-		}
+	if (!scsi_bufflen(srb)) {
+		desc->context.data = NULL;
+		desc->context.data_length = 0;
+		return;
 	} else {
-		MTS_DEBUG("Using scatter/gather\n");
-		sg = srb->request_buffer;
+		sg = scsi_sglist(srb);
 		desc->context.data = page_address(sg[0].page) + sg[0].offset;
 		desc->context.data_length = sg[0].length;
 	}
-- 
1.5.2.2.249.g45fd

Re: Data accessors more patches

[Boaz Harrosh]

Boaz Harrosh wrote:
> As a reply to this email I will send some more files
> converted to new data accessors, and attempts of cleanups
> of: !use_sg code path's, use of scsi_for_each_sg and use
> of scsi_{,un}map_dma().
> 
> some of these files are in scsi-ml core but they can be converted
> now, and are not at all dependent on implementation.
> 
> libata-scsi.c	- code will now work with scsi_sgtable but farther cleanup is needed.
> scsi.c 		- some prints might as well use accessors now.
> scsi_debug.c	- This is actually a scsi LLD. Normal conversion.
> 
> The rest of the converted files I will send in the following days
> 
> Boaz
> 
> -
> 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
Tomo hi.

I would like your Signed-off-by: on all of these. Since you
have more experience with these cleanups.

Thanks

Re: [PATCH] microtek.c - use data accessors and !use_sg cleanup

[Boaz Harrosh]

Boaz Harrosh wrote:
>   - use scsi_cmnd data accessors
>   - Clean the !use_sg code paths
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/usb/image/microtek.c |   32 ++++++++++++++------------------
>  1 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c
> index 51bd80d..0a5c6e8 100644
> --- a/drivers/usb/image/microtek.c
> +++ b/drivers/usb/image/microtek.c
> @@ -447,7 +447,8 @@ static void mts_data_done( struct urb* transfer )
>  	MTS_INT_INIT();
>  
>  	if ( context->data_length != transfer->actual_length ) {
> -		context->srb->resid = context->data_length - transfer->actual_length;
> +		scsi_set_resid(context->srb, context->data_length -
> +		                                     transfer->actual_length);
>  	} else if ( unlikely(transfer->status) ) {
>  		context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
>  	}
> @@ -490,7 +491,8 @@ static void mts_command_done( struct urb *transfer )
>  					   context->data_pipe,
>  					   context->data,
>  					   context->data_length,
> -					   context->srb->use_sg > 1 ? mts_do_sg : mts_data_done);
> +					   scsi_sg_count(context->srb) > 1 ?
> +					           mts_do_sg : mts_data_done);
>  		} else {
>  			mts_get_status(transfer);
>  		}
> @@ -504,21 +506,23 @@ static void mts_do_sg (struct urb* transfer)
>  	struct scatterlist * sg;
>  	MTS_INT_INIT();
>  
> -	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,context->srb->use_sg);
> +	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,
> +	                                          scsi_sg_count(context->srb));
>  
>  	if (unlikely(transfer->status)) {
>                  context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
>  		mts_transfer_cleanup(transfer);
>          }
>  
> -	sg = context->srb->request_buffer;
> +	sg = scsi_sglist(context->srb);
>  	context->fragment++;
>  	mts_int_submit_urb(transfer,
>  			   context->data_pipe,
>  			   page_address(sg[context->fragment].page) +
>  			   sg[context->fragment].offset,
>  			   sg[context->fragment].length,
> -			   context->fragment + 1 == context->srb->use_sg ? mts_data_done : mts_do_sg);
> +			   context->fragment + 1 == scsi_sg_count(context->srb) ?
> +			   mts_data_done : mts_do_sg);
>  	return;
>  }
>  
> @@ -546,20 +550,12 @@ mts_build_transfer_context(struct scsi_cmnd *srb, struct mts_desc* desc)
>  	desc->context.srb = srb;
>  	desc->context.fragment = 0;
>  
> -	if (!srb->use_sg) {
> -		if ( !srb->request_bufflen ){
> -			desc->context.data = NULL;
> -			desc->context.data_length = 0;
> -			return;
> -		} else {
> -			desc->context.data = srb->request_buffer;
> -			desc->context.data_length = srb->request_bufflen;
> -			MTS_DEBUG("length = %d or %d\n",
> -				  srb->request_bufflen, srb->bufflen);
> -		}
> +	if (!scsi_bufflen(srb)) {
> +		desc->context.data = NULL;
> +		desc->context.data_length = 0;
> +		return;
>  	} else {
> -		MTS_DEBUG("Using scatter/gather\n");
> -		sg = srb->request_buffer;
> +		sg = scsi_sglist(srb);
>  		desc->context.data = page_address(sg[0].page) + sg[0].offset;
>  		desc->context.data_length = sg[0].length;
>  	}

I forgot TO: of maintainers

Oliver Neukum <oliver@neukum.name>
Greg Kroah-Hartman <gregkh@suse.de>

You seem to have signed some of the last patches
on this driver. Please review

Thanks
Boaz Harrosh

Re: [PATCH] microtek.c - use data accessors and !use_sg cleanup

[Oliver Neukum]

Am Donnerstag, 12. Juli 2007 schrieb Boaz Harrosh:
> 
>   - use scsi_cmnd data accessors
>   - Clean the !use_sg code paths

This doesn't compile, at least on x86_64, so NAK.

	Regards
		Oliver

  CC [M]  drivers/usb/image/microtek.o
drivers/usb/image/microtek.c: In function ‘mts_data_done’:
drivers/usb/image/microtek.c:450: warning: implicit declaration of function ‘scsi_set_resid’
drivers/usb/image/microtek.c: In function ‘mts_command_done’:
drivers/usb/image/microtek.c:494: warning: implicit declaration of function ‘scsi_sg_count’
drivers/usb/image/microtek.c: In function ‘mts_do_sg’:
drivers/usb/image/microtek.c:517: warning: implicit declaration of function ‘scsi_sglist’
drivers/usb/image/microtek.c:517: warning: assignment makes pointer from integer without a cast
drivers/usb/image/microtek.c: In function ‘mts_build_transfer_context’:
drivers/usb/image/microtek.c:553: warning: implicit declaration of function ‘scsi_bufflen’
drivers/usb/image/microtek.c:558: warning: assignment makes pointer from integer without a cast
  MODPOST vmlinux
Kernel: arch/x86_64/boot/bzImage is ready  (#4)
  Building modules, stage 2.
  MODPOST 1705 modules
ERROR: "scsi_set_resid" [drivers/usb/image/microtek.ko] undefined!
ERROR: "scsi_sg_count" [drivers/usb/image/microtek.ko] undefined!
ERROR: "scsi_sglist" [drivers/usb/image/microtek.ko] undefined!
ERROR: "scsi_bufflen" [drivers/usb/image/microtek.ko] undefined!
make[1]: *** [__modpost] Fehler 1
make: *** [modules] Fehler 2
-
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] microtek.c - use data accessors and !use_sg cleanup

[James Bottomley]

On Thu, 2007-07-12 at 15:29 +0200, Oliver Neukum wrote:
> Am Donnerstag, 12. Juli 2007 schrieb Boaz Harrosh:
> > 
> >   - use scsi_cmnd data accessors
> >   - Clean the !use_sg code paths
> 
> This doesn't compile, at least on x86_64, so NAK.
> 
> 	Regards
> 		Oliver
> 
>   CC [M]  drivers/usb/image/microtek.o
> drivers/usb/image/microtek.c: In function ‘mts_data_done’:
> drivers/usb/image/microtek.c:450: warning: implicit declaration of function ‘scsi_set_resid’
> drivers/usb/image/microtek.c: In function ‘mts_command_done’:
> drivers/usb/image/microtek.c:494: warning: implicit declaration of function ‘scsi_sg_count’
> drivers/usb/image/microtek.c: In function ‘mts_do_sg’:
> drivers/usb/image/microtek.c:517: warning: implicit declaration of function ‘scsi_sglist’
> drivers/usb/image/microtek.c:517: warning: assignment makes pointer from integer without a cast
> drivers/usb/image/microtek.c: In function ‘mts_build_transfer_context’:
> drivers/usb/image/microtek.c:553: warning: implicit declaration of function ‘scsi_bufflen’
> drivers/usb/image/microtek.c:558: warning: assignment makes pointer from integer without a cast

These would appear to be because you're not building against scsi-misc
rather than because of a failure in the patch.

James


-
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] microtek.c - use data accessors and !use_sg cleanup

[Oliver Neukum]

Am Donnerstag, 12. Juli 2007 schrieb James Bottomley:
> On Thu, 2007-07-12 at 15:29 +0200, Oliver Neukum wrote:
> > Am Donnerstag, 12. Juli 2007 schrieb Boaz Harrosh:
> > > 
> > >   - use scsi_cmnd data accessors
> > >   - Clean the !use_sg code paths
> > 
> > This doesn't compile, at least on x86_64, so NAK.
> > 
> >       Regards
> >               Oliver
> > 
> >   CC [M]  drivers/usb/image/microtek.o
> > drivers/usb/image/microtek.c: In function ‘mts_data_done’:
> > drivers/usb/image/microtek.c:450: warning: implicit declaration of function ‘scsi_set_resid’
> > drivers/usb/image/microtek.c: In function ‘mts_command_done’:
> > drivers/usb/image/microtek.c:494: warning: implicit declaration of function ‘scsi_sg_count’
> > drivers/usb/image/microtek.c: In function ‘mts_do_sg’:
> > drivers/usb/image/microtek.c:517: warning: implicit declaration of function ‘scsi_sglist’
> > drivers/usb/image/microtek.c:517: warning: assignment makes pointer from integer without a cast
> > drivers/usb/image/microtek.c: In function ‘mts_build_transfer_context’:
> > drivers/usb/image/microtek.c:553: warning: implicit declaration of function ‘scsi_bufflen’
> > drivers/usb/image/microtek.c:558: warning: assignment makes pointer from integer without a cast
> 
> These would appear to be because you're not building against scsi-misc
> rather than because of a failure in the patch.

Thanks. Which tree should I test against? Where do I get it?

	Regards
		Oliver

-
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] microtek.c - use data accessors and !use_sg cleanup

[Boaz Harrosh]

Oliver Neukum wrote:
> Am Donnerstag, 12. Juli 2007 schrieb Boaz Harrosh:
>>   - use scsi_cmnd data accessors
>>   - Clean the !use_sg code paths
> 
> This doesn't compile, at least on x86_64, so NAK.
> 
> 	Regards
> 		Oliver
> 
>   CC [M]  drivers/usb/image/microtek.o
> drivers/usb/image/microtek.c: In function ‘mts_data_done’:
> drivers/usb/image/microtek.c:450: warning: implicit declaration of function ‘scsi_set_resid’
> drivers/usb/image/microtek.c: In function ‘mts_command_done’:
> drivers/usb/image/microtek.c:494: warning: implicit declaration of function ‘scsi_sg_count’
> drivers/usb/image/microtek.c: In function ‘mts_do_sg’:
> drivers/usb/image/microtek.c:517: warning: implicit declaration of function ‘scsi_sglist’
> drivers/usb/image/microtek.c:517: warning: assignment makes pointer from integer without a cast
> drivers/usb/image/microtek.c: In function ‘mts_build_transfer_context’:
> drivers/usb/image/microtek.c:553: warning: implicit declaration of function ‘scsi_bufflen’
> drivers/usb/image/microtek.c:558: warning: assignment makes pointer from integer without a cast
>   MODPOST vmlinux
> Kernel: arch/x86_64/boot/bzImage is ready  (#4)
>   Building modules, stage 2.
>   MODPOST 1705 modules
> ERROR: "scsi_set_resid" [drivers/usb/image/microtek.ko] undefined!
> ERROR: "scsi_sg_count" [drivers/usb/image/microtek.ko] undefined!
> ERROR: "scsi_sglist" [drivers/usb/image/microtek.ko] undefined!
> ERROR: "scsi_bufflen" [drivers/usb/image/microtek.ko] undefined!
> make[1]: *** [__modpost] Fehler 1
> make: *** [modules] Fehler 2

This is over current scsi-2.6-misc git tree, and the infrastructure 
needed for this is going into the linux 2.6.23 merging window.
The proposed changes above are for probably 2.6.24 merge window.

It is all a part of a clean sweep of every scsi driver in the 
tree. Most drivers have already been converted and accepted into
current scsi-2.6-misc and are do into the current merge window.

The microtek.c driver is sitting in USB land but it is a scsi
driver and should be maintained through the scsi-2.6-misc tree?
No?

Boaz
-
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] microtek.c - use data accessors and !use_sg cleanup

[Boaz Harrosh]

Oliver Neukum wrote:
> 
> Thanks. Which tree should I test against? Where do I get it?
> 
> 	Regards
> 		Oliver
> 

git-clone git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git scsi-misc-2.6

Boaz

Re: [PATCH] microtek.c - use data accessors and !use_sg cleanup

[Greg KH]

On Thu, Jul 12, 2007 at 04:24:04PM +0300, Boaz Harrosh wrote:
> Boaz Harrosh wrote:
> >   - use scsi_cmnd data accessors
> >   - Clean the !use_sg code paths
> > 
> > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> > ---
> >  drivers/usb/image/microtek.c |   32 ++++++++++++++------------------
> >  1 files changed, 14 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c
> > index 51bd80d..0a5c6e8 100644
> > --- a/drivers/usb/image/microtek.c
> > +++ b/drivers/usb/image/microtek.c
> > @@ -447,7 +447,8 @@ static void mts_data_done( struct urb* transfer )
> >  	MTS_INT_INIT();
> >  
> >  	if ( context->data_length != transfer->actual_length ) {
> > -		context->srb->resid = context->data_length - transfer->actual_length;
> > +		scsi_set_resid(context->srb, context->data_length -
> > +		                                     transfer->actual_length);
> >  	} else if ( unlikely(transfer->status) ) {
> >  		context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
> >  	}
> > @@ -490,7 +491,8 @@ static void mts_command_done( struct urb *transfer )
> >  					   context->data_pipe,
> >  					   context->data,
> >  					   context->data_length,
> > -					   context->srb->use_sg > 1 ? mts_do_sg : mts_data_done);
> > +					   scsi_sg_count(context->srb) > 1 ?
> > +					           mts_do_sg : mts_data_done);
> >  		} else {
> >  			mts_get_status(transfer);
> >  		}
> > @@ -504,21 +506,23 @@ static void mts_do_sg (struct urb* transfer)
> >  	struct scatterlist * sg;
> >  	MTS_INT_INIT();
> >  
> > -	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,context->srb->use_sg);
> > +	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,
> > +	                                          scsi_sg_count(context->srb));
> >  
> >  	if (unlikely(transfer->status)) {
> >                  context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
> >  		mts_transfer_cleanup(transfer);
> >          }
> >  
> > -	sg = context->srb->request_buffer;
> > +	sg = scsi_sglist(context->srb);
> >  	context->fragment++;
> >  	mts_int_submit_urb(transfer,
> >  			   context->data_pipe,
> >  			   page_address(sg[context->fragment].page) +
> >  			   sg[context->fragment].offset,
> >  			   sg[context->fragment].length,
> > -			   context->fragment + 1 == context->srb->use_sg ? mts_data_done : mts_do_sg);
> > +			   context->fragment + 1 == scsi_sg_count(context->srb) ?
> > +			   mts_data_done : mts_do_sg);
> >  	return;
> >  }
> >  
> > @@ -546,20 +550,12 @@ mts_build_transfer_context(struct scsi_cmnd *srb, struct mts_desc* desc)
> >  	desc->context.srb = srb;
> >  	desc->context.fragment = 0;
> >  
> > -	if (!srb->use_sg) {
> > -		if ( !srb->request_bufflen ){
> > -			desc->context.data = NULL;
> > -			desc->context.data_length = 0;
> > -			return;
> > -		} else {
> > -			desc->context.data = srb->request_buffer;
> > -			desc->context.data_length = srb->request_bufflen;
> > -			MTS_DEBUG("length = %d or %d\n",
> > -				  srb->request_bufflen, srb->bufflen);
> > -		}
> > +	if (!scsi_bufflen(srb)) {
> > +		desc->context.data = NULL;
> > +		desc->context.data_length = 0;
> > +		return;
> >  	} else {
> > -		MTS_DEBUG("Using scatter/gather\n");
> > -		sg = srb->request_buffer;
> > +		sg = scsi_sglist(srb);
> >  		desc->context.data = page_address(sg[0].page) + sg[0].offset;
> >  		desc->context.data_length = sg[0].length;
> >  	}
> 
> I forgot TO: of maintainers
> 
> Oliver Neukum <oliver@neukum.name>
> Greg Kroah-Hartman <gregkh@suse.de>
> 
> You seem to have signed some of the last patches
> on this driver. Please review

No objetion from me.  Care to resend it to me, and CC: the
linux-usb-devel list also so that it is in a format that I can apply to
the tree?

thanks,

greg k-h

Re: [PATCH] microtek.c - use data accessors and !use_sg cleanup

[Boaz Harrosh]

Greg KH wrote:
> On Thu, Jul 12, 2007 at 04:24:04PM +0300, Boaz Harrosh wrote:
>> Boaz Harrosh wrote:
>>>   - use scsi_cmnd data accessors
>>>   - Clean the !use_sg code paths
>>>
>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>>> ---
>>>  drivers/usb/image/microtek.c |   32 ++++++++++++++------------------
>>>  1 files changed, 14 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c
>>> index 51bd80d..0a5c6e8 100644
>>> --- a/drivers/usb/image/microtek.c
>>> +++ b/drivers/usb/image/microtek.c
>>> @@ -447,7 +447,8 @@ static void mts_data_done( struct urb* transfer )
>>>  	MTS_INT_INIT();
>>>  
>>>  	if ( context->data_length != transfer->actual_length ) {
>>> -		context->srb->resid = context->data_length - transfer->actual_length;
>>> +		scsi_set_resid(context->srb, context->data_length -
>>> +		                                     transfer->actual_length);
>>>  	} else if ( unlikely(transfer->status) ) {
>>>  		context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
>>>  	}
>>> @@ -490,7 +491,8 @@ static void mts_command_done( struct urb *transfer )
>>>  					   context->data_pipe,
>>>  					   context->data,
>>>  					   context->data_length,
>>> -					   context->srb->use_sg > 1 ? mts_do_sg : mts_data_done);
>>> +					   scsi_sg_count(context->srb) > 1 ?
>>> +					           mts_do_sg : mts_data_done);
>>>  		} else {
>>>  			mts_get_status(transfer);
>>>  		}
>>> @@ -504,21 +506,23 @@ static void mts_do_sg (struct urb* transfer)
>>>  	struct scatterlist * sg;
>>>  	MTS_INT_INIT();
>>>  
>>> -	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,context->srb->use_sg);
>>> +	MTS_DEBUG("Processing fragment %d of %d\n", context->fragment,
>>> +	                                          scsi_sg_count(context->srb));
>>>  
>>>  	if (unlikely(transfer->status)) {
>>>                  context->srb->result = (transfer->status == -ENOENT ? DID_ABORT : DID_ERROR)<<16;
>>>  		mts_transfer_cleanup(transfer);
>>>          }
>>>  
>>> -	sg = context->srb->request_buffer;
>>> +	sg = scsi_sglist(context->srb);
>>>  	context->fragment++;
>>>  	mts_int_submit_urb(transfer,
>>>  			   context->data_pipe,
>>>  			   page_address(sg[context->fragment].page) +
>>>  			   sg[context->fragment].offset,
>>>  			   sg[context->fragment].length,
>>> -			   context->fragment + 1 == context->srb->use_sg ? mts_data_done : mts_do_sg);
>>> +			   context->fragment + 1 == scsi_sg_count(context->srb) ?
>>> +			   mts_data_done : mts_do_sg);
>>>  	return;
>>>  }
>>>  
>>> @@ -546,20 +550,12 @@ mts_build_transfer_context(struct scsi_cmnd *srb, struct mts_desc* desc)
>>>  	desc->context.srb = srb;
>>>  	desc->context.fragment = 0;
>>>  
>>> -	if (!srb->use_sg) {
>>> -		if ( !srb->request_bufflen ){
>>> -			desc->context.data = NULL;
>>> -			desc->context.data_length = 0;
>>> -			return;
>>> -		} else {
>>> -			desc->context.data = srb->request_buffer;
>>> -			desc->context.data_length = srb->request_bufflen;
>>> -			MTS_DEBUG("length = %d or %d\n",
>>> -				  srb->request_bufflen, srb->bufflen);
>>> -		}
>>> +	if (!scsi_bufflen(srb)) {
>>> +		desc->context.data = NULL;
>>> +		desc->context.data_length = 0;
>>> +		return;
>>>  	} else {
>>> -		MTS_DEBUG("Using scatter/gather\n");
>>> -		sg = srb->request_buffer;
>>> +		sg = scsi_sglist(srb);
>>>  		desc->context.data = page_address(sg[0].page) + sg[0].offset;
>>>  		desc->context.data_length = sg[0].length;
>>>  	}
>> I forgot TO: of maintainers
>>
>> Oliver Neukum <oliver@neukum.name>
>> Greg Kroah-Hartman <gregkh@suse.de>
>>
>> You seem to have signed some of the last patches
>> on this driver. Please review
> 
> No objetion from me.  Care to resend it to me, and CC: the
> linux-usb-devel list also so that it is in a format that I can apply to
> the tree?
> 
> thanks,
> 
> greg k-h
Will do. I hope I can post with out signing up.

Re: [PATCH] microtek.c - use data accessors and !use_sg cleanup

[James Bottomley]

On Thu, 2007-07-12 at 09:58 -0700, Greg KH wrote:
> No objetion from me.  Care to resend it to me, and CC: the
> linux-usb-devel list also so that it is in a format that I can apply to
> the tree?

This will give you a cross tree dependency on scsi-misc-2.6 because of
the new data accessors ... if that's what you want to do?

James

Re: [PATCH] microtek.c - use data accessors and !use_sg cleanup

[Greg KH]

On Thu, Jul 12, 2007 at 01:05:55PM -0400, James Bottomley wrote:
> On Thu, 2007-07-12 at 09:58 -0700, Greg KH wrote:
> > No objetion from me.  Care to resend it to me, and CC: the
> > linux-usb-devel list also so that it is in a format that I can apply to
> > the tree?
> 
> This will give you a cross tree dependency on scsi-misc-2.6 because of
> the new data accessors ... if that's what you want to do?

Oops, sorry, I wasn't aware of that.

No, I don't want that at all, feel free to push this through your tree,
I have no objection to it at all:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>


thanks,

greg k-h

Re: Data accessors more patches

[FUJITA Tomonori]

From: Boaz Harrosh <bharrosh@panasas.com>
Subject: Re: Data accessors more patches
Date: Thu, 12 Jul 2007 16:13:52 +0300

> Boaz Harrosh wrote:
> > As a reply to this email I will send some more files
> > converted to new data accessors, and attempts of cleanups
> > of: !use_sg code path's, use of scsi_for_each_sg and use
> > of scsi_{,un}map_dma().
> > 
> > some of these files are in scsi-ml core but they can be converted
> > now, and are not at all dependent on implementation.
> > 
> > libata-scsi.c	- code will now work with scsi_sgtable but farther cleanup is needed.
> > scsi.c 		- some prints might as well use accessors now.
> > scsi_debug.c	- This is actually a scsi LLD. Normal conversion.
> > 
> > The rest of the converted files I will send in the following days
> > 
> > Boaz
> > 
> > -
> > 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
> Tomo hi.
> 
> I would like your Signed-off-by: on all of these. Since you
> have more experience with these cleanups.

aha152x, ide-scsi and libata-scsi?

Well, when I tried to convert them, I thought that it would be better
to leave them to the maintainers. They look broken or too complicated.

I'd like to save 'just do grep and replace' and 'convert without
confidence' options.