On Thu, Nov 08 2007 at 5:14 +0200, FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote:
> On Tue, 06 Nov 2007 20:19:32 +0200
> Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
>
<snip>
>
> Hmm, checkpatch.pl complains reasonably:
>
> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28815
> ERROR: use tabs not spaces
> #177: FILE: drivers/scsi/scsi_error.c:629:
> +^I^I scmd->sdb.length);$
>
> ERROR: use tabs not spaces
> #237: FILE: drivers/scsi/scsi_lib.c:741:
> + unsigned short sg_count, gfp_t gfp_mask)$
>
> WARNING: no space between function name and open parenthesis '('
> #487: FILE: drivers/scsi/sr.c:377:
> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
>
> ERROR: "foo* bar" should be "foo *bar"
> #563: FILE: include/scsi/scsi_cmnd.h:20:
> + struct scatterlist* sglist;
>
> total: 3 errors, 1 warnings, 482 lines checked
I think that checkpatch is wrong in two accounts.
1. Tabs are used for "Indents" not "align-to-the-right".
All above have 0 indent and are right aligned for
readability.
2. check patch should be fixed that if a macro is followed
by a "{" it means a control block and not a function-call/
macro-expansion, and a space is recommended.
(Like: for, if, while ...)
But I don't mind. I'll change all in a fixing patch.
>
>
<snip>
>> @@ -821,24 +820,24 @@ enomem:
>>
>> mempool_free(prev, sgp->pool);
>> }
>> - return NULL;
>> + return -1;
>
> I think that -ENOMEM is better. The other functions in scsi_lib.c
> (even static functions) use proper error values.
>
>
will fix, thanks.
<snip>
>> - /*
>> - * If sg table allocation fails, requeue request later.
>> - */
>> - cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
>> - if (unlikely(!cmd->request_buffer)) {
>> + if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
>
> IIRC, preferable style is:
>
> ret = scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask);
> if (ret) {
>
>
It is more readable I agree, but I did not want to allocate one more
stack variable just for the if(), since I am returning a BLKPREP_DEFER
any way. I am not using ret for anything else.
>> scsi_unprep_request(req);
>> return BLKPREP_DEFER;
>> }
>>
>> req->buffer = NULL;
>> if (blk_pc_request(req))
>> - cmd->request_bufflen = req->data_len;
>> + sdb->length = req->data_len;
>> else
>> - cmd->request_bufflen = req->nr_sectors << 9;
>> + sdb->length = req->nr_sectors << 9;
>>
>> /*
>> * Next, walk the list, and fill in the addresses and sizes of
>> * each segment.
>> */
>> - count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
>> - if (likely(count <= cmd->use_sg)) {
>> - cmd->use_sg = count;
>> + count = blk_rq_map_sg(req->q, req, sdb->sglist);
>> + if (likely(count <= sdb->sg_count)) {
>> + sdb->sg_count = count;
>> return BLKPREP_OK;
>> }
>>
>> printk(KERN_ERR "Incorrect number of segments after building list\n");
>> - printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
>> + printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
>> printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
>> req->current_nr_sectors);
>>
>> @@ -1199,9 +1182,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>> BUG_ON(req->data_len);
>> BUG_ON(req->data);
>>
>> - cmd->request_bufflen = 0;
>> - cmd->request_buffer = NULL;
>> - cmd->use_sg = 0;
>> + memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>> req->buffer = NULL;
>> }
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 18343a6..28cf6fe 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>> } else if (rq_data_dir(rq) == READ) {
>> SCpnt->cmnd[0] = READ_6;
>> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> - } else {
>> - scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
>> - goto out;
>
> This should go to the bidi patch?
>
>> }
>>
This is just a dead code cleanup. It is got nothing to do with bidi or scsi_data_buffer
for that matter. It could be in it's own patch, but surly it will not go into the bidi
patch. I will submit a new patch just for that code. Independent of these.
(I was hoping to save the extra effort)
>> SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
>> @@ -510,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>> SCpnt->cmnd[4] = (unsigned char) this_count;
>> SCpnt->cmnd[5] = 0;
>> }
>> - SCpnt->request_bufflen = this_count * sdp->sector_size;
>> + SCpnt->sdb.length = this_count * sdp->sector_size;
>>
>> /*
>> * We shouldn't disconnect in the middle of a sector, so with a dumb
>> @@ -910,7 +907,7 @@ static struct block_device_operations sd_fops = {
>> static int sd_done(struct scsi_cmnd *SCpnt)
>> {
>> int result = SCpnt->result;
>> - unsigned int xfer_size = SCpnt->request_bufflen;
>> + unsigned int xfer_size = scsi_bufflen(SCpnt);
>> unsigned int good_bytes = result ? 0 : xfer_size;
>> u64 start_lba = SCpnt->request->sector;
>> u64 bad_lba;
>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>> index 7702681..6d3bf41 100644
>> --- a/drivers/scsi/sr.c
>> +++ b/drivers/scsi/sr.c
>> @@ -226,7 +226,7 @@ out:
>> static int sr_done(struct scsi_cmnd *SCpnt)
>> {
>> int result = SCpnt->result;
>> - int this_count = SCpnt->request_bufflen;
>> + int this_count = scsi_bufflen(SCpnt);
>> int good_bytes = (result == 0 ? this_count : 0);
>> int block_sectors = 0;
>> long error_sector;
>> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>> } else if (rq_data_dir(rq) == READ) {
>> SCpnt->cmnd[0] = READ_10;
>> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> - } else {
>> - blk_dump_rq_flags(rq, "Unknown sr command");
>> - goto out;
>
> Ditto.
>
Ditto
>
>> }
>>
>> {
>> - struct scatterlist *sg = SCpnt->request_buffer;
>> - int i, size = 0;
>> - for (i = 0; i < SCpnt->use_sg; i++)
>> - size += sg[i].length;
>> + struct scatterlist *sg;
>> + int i, size = 0, sg_count = scsi_sg_count(SCpnt);
>> +
>> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
>> + size += sg->length;
>>
>> - if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
>> + if (size != scsi_bufflen(SCpnt)) {
>> scmd_printk(KERN_ERR, SCpnt,
>> "mismatch count %d, bytes %d\n",
>> - size, SCpnt->request_bufflen);
>> - if (SCpnt->request_bufflen > size)
>> - SCpnt->request_bufflen = size;
>> + size, scsi_bufflen(SCpnt));
>> + if (scsi_bufflen(SCpnt) > size)
>> + SCpnt->sdb.length = size;
>> }
>> }
>>
>> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>> * request doesn't start on hw block boundary, add scatter pads
>> */
>> if (((unsigned int)rq->sector % (s_size >> 9)) ||
>> - (SCpnt->request_bufflen % s_size)) {
>> + (scsi_bufflen(SCpnt) % s_size)) {
>> scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
>> goto out;
>> }
>>
>> - this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
>> + this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>>
>>
>> SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
>> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>
>> if (this_count > 0xffff) {
>> this_count = 0xffff;
>> - SCpnt->request_bufflen = this_count * s_size;
>> + SCpnt->sdb.length = this_count * s_size;
>> }
>>
>> SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
>> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
>> index 178e8c2..2d9a32b 100644
>> --- a/drivers/usb/storage/isd200.c
>> +++ b/drivers/usb/storage/isd200.c
>> @@ -415,14 +415,14 @@ static void isd200_set_srb(struct isd200_info *info,
>> sg_init_one(&info->sg, buff, bufflen);
>>
>> srb->sc_data_direction = dir;
>> - srb->request_buffer = buff ? &info->sg : NULL;
>> - srb->request_bufflen = bufflen;
>> - srb->use_sg = buff ? 1 : 0;
>> + srb->sdb.sglist = buff ? &info->sg : NULL;
>> + srb->sdb.length = bufflen;
>> + srb->sdb.sg_count = buff ? 1 : 0;
>> }
>>
>> static void isd200_srb_set_bufflen(struct scsi_cmnd *srb, unsigned bufflen)
>> {
>> - srb->request_bufflen = bufflen;
>> + srb->sdb.length = bufflen;
>> }
>>
>>
>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>> index a7be605..4e3cf43 100644
>> --- a/include/scsi/scsi_cmnd.h
>> +++ b/include/scsi/scsi_cmnd.h
>> @@ -12,6 +12,13 @@ struct scatterlist;
>> struct Scsi_Host;
>> struct scsi_device;
>>
>> +struct scsi_data_buffer {
>> + unsigned length;
>> + int resid;
>> + unsigned short sg_count;
>> + unsigned short alloc_sg_count;
>
> sg_count and alloc_sg_count are a bit lengthy?
>
> My suggestion is using nr_* like the block layer (nr_sg and
> nr_alloc_sg ?).
will not! sg_count is the name you gave to the accessor and it
must be the same. alloc_sg_count is a private member that must only
be used by scsi_lib.c. If the long name is a discouragement of use
than that much better.
>
>
>> + struct scatterlist* sglist;
>> +};
>>
>> /* embedded in scsi_cmnd */
>> struct scsi_pointer {
>> @@ -62,15 +69,10 @@ struct scsi_cmnd {
>> /* These elements define the operation we are about to perform */
>> #define MAX_COMMAND_SIZE 16
>> unsigned char cmnd[MAX_COMMAND_SIZE];
>> - unsigned request_bufflen; /* Actual request size */
>>
>> struct timer_list eh_timeout; /* Used to time out the command. */
>> - void *request_buffer; /* Actual requested buffer */
>>
>> /* These elements define the operation we ultimately want to perform */
>> - unsigned short use_sg; /* Number of pieces of scatter-gather */
>> - unsigned short __use_sg;
>> -
>> unsigned underflow; /* Return error if less than
>> this amount is transferred */
>>
<snip>
Will send two new patches.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
