Re: [PATCH 14/17] scsi: replace custom rq mapping withblk_rq_map_kern_sgl()

[Borislav Petkov]

Hi,

On Wed, Apr 15, 2009 at 04:48:35PM +0900, FUJITA Tomonori wrote:

[..]

> I have some comments.
> 
> 
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index 35d0973..82c9339 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -206,32 +206,21 @@ static void cdrom_analyze_sense_data(ide_drive_t *drive,
> >  	ide_cd_log_error(drive->name, failed_command, sense);
> >  }
> >  
> > -static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
> > -				      struct request *failed_command)
> > +static struct request *ide_cd_prep_sense(ide_drive_t *drive)
> >  {
> >  	struct cdrom_info *info	= drive->driver_data;
> > -	struct request *rq	= &drive->request_sense_rq;
> > -	struct bio *bio		= &drive->request_sense_bio;
> > -	struct bio_vec *bvec	= drive->request_sense_bvec;
> > -	unsigned int bvec_len	= ARRAY_SIZE(drive->request_sense_bvec);
> > -	unsigned sense_len	= 18;
> > -	int error;
> > +	void *sense		= &info->sense_data;
> > +	unsigned sense_len	= sizeof(struct request_sense);
> > +	struct request *rq;
> >  
> >  	ide_debug_log(IDE_DBG_SENSE, "enter");
> >  
> > -	if (sense == NULL) {
> > -		sense = &info->sense_data;
> > -		sense_len = sizeof(struct request_sense);
> > -	}
> > -
> >  	memset(sense, 0, sense_len);
> >  
> > -	/* stuff the sense request in front of our current request */
> > -	blk_rq_init(NULL, rq);
> > +	rq = blk_get_request(drive->queue, 0, __GFP_WAIT);
> >  
> > -	error = blk_rq_map_kern_prealloc(drive->queue, rq, bio, bvec, bvec_len,
> > -					 sense, sense_len, true);
> > -	BUG_ON(error);
> > +	if (blk_rq_map_kern(drive->queue, rq, sense, sense_len, __GFP_WAIT))
> > +		return NULL;
> >  
> >  	rq->rq_disk = info->disk;
> >  
> > @@ -241,18 +230,17 @@ static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
> >  	rq->cmd_type = REQ_TYPE_SENSE;
> >  	rq->cmd_flags |= REQ_PREEMPT;
> >  
> > -	/* NOTE! Save the failed command in "rq->special" */
> > -	rq->special = (void *)failed_command;
> > -
> > -	if (failed_command)
> > -		ide_debug_log(IDE_DBG_SENSE, "failed_cmd: 0x%x",
> > -					     failed_command->cmd[0]);
> > +	return rq;
> > +}
> >  
> > -	drive->hwif->rq = NULL;
> > +static void ide_cd_queue_rq_sense(ide_drive_t *drive)
> > +{
> > +	BUG_ON(!drive->rq_sense);
> >  
> > -	elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 0);
> > +	elv_add_request(drive->queue, drive->rq_sense, ELEVATOR_INSERT_FRONT, 0);
> >  }
> >  
> > +
> >  static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
> >  {
> >  	/*
> > @@ -440,7 +428,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
> >  
> >  	/* if we got a CHECK_CONDITION status, queue a request sense command */
> >  	if (stat & ATA_ERR)
> > -		cdrom_queue_request_sense(drive, NULL, NULL);
> > +		ide_cd_queue_rq_sense(drive);
> >  	return 1;
> >  
> >  end_request:
> > @@ -454,7 +442,7 @@ end_request:
> >  
> >  		hwif->rq = NULL;
> >  
> > -		cdrom_queue_request_sense(drive, rq->sense, rq);
> > +		ide_cd_queue_rq_sense(drive);
> >  		return 1;
> >  	} else
> >  		return 2;
> > @@ -788,6 +776,10 @@ out_end:
> >  
> >  		ide_complete_rq(drive, uptodate ? 0 : -EIO, nsectors << 9);
> >  
> > +		/* our sense buffer got used, reset it the next time around. */
> > +		if (sense)
> > +			drive->rq_sense = NULL;
> 
> Needs to call blk_put_request() here?

No, because this is done by ide_complete_rq() above. Otherwise I'd be
ending the sense request even it didn't get used. This way, I only use
it if I call ide_cd_queue_rq_sense() from which cdrom_decode_status().

> I guess that we also need to call blk_put_request() when ide_drive_s
> is freed.

That we need to do, thanks. Will update accordingly.

> > +
> >  		if (sense && rc == 2)
> >  			ide_error(drive, "request sense failure", stat);
> >  	}
> > @@ -901,6 +893,25 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
> >  		goto out_end;
> >  	}
> >  
> > +	/*
> > +	 * prepare request sense if it got used with the last rq
> > +	 */
> > +	if (!drive->rq_sense) {
> > +		drive->rq_sense = ide_cd_prep_sense(drive);
> > +		if (!drive->rq_sense) {
> > +			printk(KERN_ERR "%s: error prepping sense request!\n",
> > +					drive->name);
> > +			return ide_stopped;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * save the current request in case we'll be queueing a sense rq
> > +	 * afterwards due to its potential failure.
> > +	 */
> > +	if (!blk_sense_request(rq))
> > +		drive->rq_sense->special = (void *)rq;
> > +
> >  	memset(&cmd, 0, sizeof(cmd));
> >  
> >  	if (rq_data_dir(rq))
> > diff --git a/include/linux/ide.h b/include/linux/ide.h
> > index c942533..4c2d310 100644
> > --- a/include/linux/ide.h
> > +++ b/include/linux/ide.h
> > @@ -605,6 +605,9 @@ struct ide_drive_s {
> >  	struct request request_sense_rq;
> >  	struct bio request_sense_bio;
> >  	struct bio_vec request_sense_bvec[2];
> 
> We can remove the above, right?

id-atapi uses them too, see
http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=commitdiff;h=9ac15840a6e5bf1fa6dce293484cb7aba4d078bb

They can go after I've converted ide-floppy and ide-tape to the same
sense handling as ide-cd. I'm on it...

-- 
Regards/Gruss,
    Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/