[PATCH 00/11] removing the on-stack struct request

[PATCH 00/11] removing the on-stack struct request

[FUJITA Tomonori]

This patchset converts some of the users of struct request on the
stack to use blk_get_request (as discussed in the large command
support thread [*1]).

IDE has the most users of the on-stack struct request. The on-stack
struct request has the benefit that it'll always work but I think that
using blk_get_request properly make the code clearer.

I've not got rid of all the on-stack struct request yet, but at least
this patchset removes the users of struct request that calls
blk_put_request. That is, we can remove the following hack in
blk_put_request:

/*
 * Gee, IDE calls in w/ NULL q.  Fix IDE and remove the
 * following if (q) test.
 */
if (q) {
	spin_lock_irqsave(q->queue_lock, flags);
	__blk_put_request(q, req);
	spin_unlock_irqrestore(q->queue_lock, flags);
}

This patchset is against Bartlomiej's ide tree. #1-9 patches are for
the IDE subsystem and #10-11 are for the block layer. #10-11 depends
on #1-9 so probably it would be easier to push all the patch via a
single tree.

[*1]
http://marc.info/?l=linux-scsi&m=120817161219068&w=2

[PATCH 01/11] ide: use blk_get_request in the ide_do_drive_cmd path using ide_wait

[FUJITA Tomonori]

This replaces struct request on the stack with blk_get_request in the
ide_do_drive_cmd path using ide_wait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 917c72d..c5b33f5 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -626,7 +626,8 @@ out:
 
 int set_pio_mode(ide_drive_t *drive, int arg)
 {
-	struct request rq;
+	struct request *rq;
+
 
 	if (arg < 0 || arg > 255)
 		return -EINVAL;
@@ -637,12 +638,12 @@ int set_pio_mode(ide_drive_t *drive, int arg)
 	if (drive->special.b.set_tune)
 		return -EBUSY;
 
-	ide_init_drive_cmd(&rq);
-	rq.cmd_type = REQ_TYPE_ATA_TASKFILE;
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
 
 	drive->tune_req = (u8) arg;
 	drive->special.b.set_tune = 1;
-	(void) ide_do_drive_cmd(drive, &rq, ide_wait);
+	ide_do_drive_cmd(drive, rq, ide_wait);
 	return 0;
 }
 
@@ -680,7 +681,7 @@ static int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 {
 	ide_drive_t *drive = dev->driver_data;
 	ide_hwif_t *hwif = HWIF(drive);
-	struct request rq;
+	struct request *rq;
 	struct request_pm_state rqpm;
 	ide_task_t args;
 	int ret;
@@ -689,18 +690,18 @@ static int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 	if (!(drive->dn % 2))
 		ide_acpi_get_timing(hwif);
 
-	memset(&rq, 0, sizeof(rq));
 	memset(&rqpm, 0, sizeof(rqpm));
 	memset(&args, 0, sizeof(args));
-	rq.cmd_type = REQ_TYPE_PM_SUSPEND;
-	rq.special = &args;
-	rq.data = &rqpm;
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->cmd_type = REQ_TYPE_PM_SUSPEND;
+	rq->special = &args;
+	rq->data = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_suspend;
 	if (mesg.event == PM_EVENT_PRETHAW)
 		mesg.event = PM_EVENT_FREEZE;
 	rqpm.pm_state = mesg.event;
 
-	ret = ide_do_drive_cmd(drive, &rq, ide_wait);
+	ret = ide_do_drive_cmd(drive, rq, ide_wait);
 	/* only call ACPI _PS3 after both drivers are suspended */
 	if (!ret && (((drive->dn % 2) && hwif->drives[0].present
 		 && hwif->drives[1].present)
@@ -714,7 +715,7 @@ static int generic_ide_resume(struct device *dev)
 {
 	ide_drive_t *drive = dev->driver_data;
 	ide_hwif_t *hwif = HWIF(drive);
-	struct request rq;
+	struct request *rq;
 	struct request_pm_state rqpm;
 	ide_task_t args;
 	int err;
@@ -727,16 +728,16 @@ static int generic_ide_resume(struct device *dev)
 
 	ide_acpi_exec_tfs(drive);
 
-	memset(&rq, 0, sizeof(rq));
 	memset(&rqpm, 0, sizeof(rqpm));
 	memset(&args, 0, sizeof(args));
-	rq.cmd_type = REQ_TYPE_PM_RESUME;
-	rq.special = &args;
-	rq.data = &rqpm;
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->cmd_type = REQ_TYPE_PM_RESUME;
+	rq->special = &args;
+	rq->data = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_resume;
 	rqpm.pm_state = PM_EVENT_ON;
 
-	err = ide_do_drive_cmd(drive, &rq, ide_head_wait);
+	err = ide_do_drive_cmd(drive, rq, ide_head_wait);
 
 	if (err == 0 && dev->driver) {
 		ide_driver_t *drv = to_ide_driver(dev->driver);
-- 
1.5.4.2

[PATCH 02/11] ide-floppy: use blk_get_request in the ide_do_drive_cmd path

[FUJITA Tomonori]

This replaces struct request on the stack with blk_get_request in the
ide_do_drive_cmd path that uses ide_wait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-floppy.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 5f133df..d75b2aa 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -885,14 +885,14 @@ static ide_startstop_t idefloppy_do_request(ide_drive_t *drive,
 static int idefloppy_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
 {
 	struct ide_floppy_obj *floppy = drive->driver_data;
-	struct request rq;
+	struct request *rq;
 
-	ide_init_drive_cmd(&rq);
-	rq.buffer = (char *) pc;
-	rq.cmd_type = REQ_TYPE_SPECIAL;
-	rq.rq_disk = floppy->disk;
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->buffer = (char *) pc;
+	rq->cmd_type = REQ_TYPE_SPECIAL;
+	rq->rq_disk = floppy->disk;
 
-	return ide_do_drive_cmd(drive, &rq, ide_wait);
+	return ide_do_drive_cmd(drive, rq, ide_wait);
 }
 
 /*
-- 
1.5.4.2

[PATCH 03/11] ide-taskfile: use blk_get_request in the ide_do_drive_cmd path

[FUJITA Tomonori]

This replaces struct request on the stack with blk_get_request in the
ide_do_drive_cmd path that uses ide_wait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-taskfile.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index 155cc90..fac7fad 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -530,12 +530,11 @@ static ide_startstop_t pre_task_out_intr(ide_drive_t *drive, struct request *rq)
 
 int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
 {
-	struct request rq;
+	struct request *rq;
 
-	memset(&rq, 0, sizeof(rq));
-	rq.ref_count = 1;
-	rq.cmd_type = REQ_TYPE_ATA_TASKFILE;
-	rq.buffer = buf;
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
+	rq->buffer = buf;
 
 	/*
 	 * (ks) We transfer currently only whole sectors.
@@ -543,16 +542,16 @@ int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
 	 * if we would find a solution to transfer any size.
 	 * To support special commands like READ LONG.
 	 */
-	rq.hard_nr_sectors = rq.nr_sectors = nsect;
-	rq.hard_cur_sectors = rq.current_nr_sectors = nsect;
+	rq->hard_nr_sectors = rq->nr_sectors = nsect;
+	rq->hard_cur_sectors = rq->current_nr_sectors = nsect;
 
 	if (task->tf_flags & IDE_TFLAG_WRITE)
-		rq.cmd_flags |= REQ_RW;
+		rq->cmd_flags |= REQ_RW;
 
-	rq.special = task;
-	task->rq = &rq;
+	rq->special = task;
+	task->rq = rq;
 
-	return ide_do_drive_cmd(drive, &rq, ide_wait);
+	return ide_do_drive_cmd(drive, rq, ide_wait);
 }
 
 EXPORT_SYMBOL(ide_raw_taskfile);
@@ -778,12 +777,12 @@ int ide_cmd_ioctl (ide_drive_t *drive, unsigned int cmd, unsigned long arg)
 	struct hd_driveid *id = drive->id;
 
 	if (NULL == (void *) arg) {
-		struct request rq;
+		struct request *rq;
 
-		ide_init_drive_cmd(&rq);
-		rq.cmd_type = REQ_TYPE_ATA_TASKFILE;
+		rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+		rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
 
-		return ide_do_drive_cmd(drive, &rq, ide_wait);
+		return ide_do_drive_cmd(drive, rq, ide_wait);
 	}
 
 	if (copy_from_user(args, (void __user *)arg, 4))
-- 
1.5.4.2

[PATCH 04/11] ide-disk: use blk_get_request in the ide_do_drive_cmd path

[FUJITA Tomonori]

This replaces struct request on the stack with blk_get_request in the
ide_do_drive_cmd path that uses ide_wait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-disk.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
index 39501d1..497273a 100644
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -617,7 +617,7 @@ static void idedisk_prepare_flush(struct request_queue *q, struct request *rq)
  */
 static int set_multcount(ide_drive_t *drive, int arg)
 {
-	struct request rq;
+	struct request *rq;
 
 	if (arg < 0 || arg > drive->id->max_multsect)
 		return -EINVAL;
@@ -625,12 +625,12 @@ static int set_multcount(ide_drive_t *drive, int arg)
 	if (drive->special.b.set_multmode)
 		return -EBUSY;
 
-	ide_init_drive_cmd (&rq);
-	rq.cmd_type = REQ_TYPE_ATA_TASKFILE;
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
 
 	drive->mult_req = arg;
 	drive->special.b.set_multmode = 1;
-	(void) ide_do_drive_cmd (drive, &rq, ide_wait);
+	ide_do_drive_cmd(drive, rq, ide_wait);
 	return (drive->mult_count == arg) ? 0 : -EIO;
 }
 
-- 
1.5.4.2

[PATCH 05/11] ide-cd: convert ide_cd_queue_pc to use blk_get_request

[FUJITA Tomonori]

This converts ide_cd_queue_pc to use blk_get_request to remove struct
request on the stack in the users of ide_cd_queue_pc, necessitating
changing the ide_cd_queue_pc prototype into a form that doesn't takes
a pointer to request struct. ide_cd_queue_pc works like scsi_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd.c       |   84 +++++++++++++++++++------------------
 drivers/ide/ide-cd.h       |    3 +-
 drivers/ide/ide-cd_ioctl.c |   99 ++++++++++++++++++++-----------------------
 3 files changed, 92 insertions(+), 94 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 3960002..584c522 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -932,31 +932,46 @@ static void ide_cd_request_sense_fixup(struct request *rq)
 		}
 }
 
-int ide_cd_queue_pc(ide_drive_t *drive, struct request *rq)
+int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
+		    int write, void *buffer, unsigned bufflen,
+		    struct request_sense *sense, int timeout,
+		    unsigned int flags)
 {
-	struct request_sense sense;
+	struct cdrom_info *info = drive->driver_data;
+	struct request_sense local_sense;
 	int retries = 10;
-	unsigned int flags = rq->cmd_flags;
 
-	if (rq->sense == NULL)
-		rq->sense = &sense;
+	if (!sense)
+		sense = &local_sense;
 
 	/* Start of retry loop. */
 	do {
+		struct request *rq;
 		int error;
 		unsigned long time = jiffies;
-		rq->cmd_flags = flags;
+
+		rq = blk_get_request(drive->queue, write, __GFP_WAIT);
+
+		memcpy(rq->cmd, cmd, BLK_MAX_CDB);
+		rq->cmd_type = REQ_TYPE_ATA_PC;
+		rq->rq_disk = info->disk;
+		rq->sense = sense;
+		rq->cmd_flags |= flags;
+		rq->data = buffer;
+		rq->data_len = bufflen;
+		rq->timeout = timeout;
 
 		error = ide_do_drive_cmd(drive, rq, ide_wait);
 		time = jiffies - time;
 
 		/* FIXME: we should probably abort/retry or something 
 		 * in case of failure */
-		if (rq->cmd_flags & REQ_FAILED) {
+		if (flags & REQ_FAILED) {
 			/* The request failed.  Retry if it was due to a unit
 			   attention status
 			   (usually means media was changed). */
-			struct request_sense *reqbuf = rq->sense;
+			struct request_sense *reqbuf =
+				(struct request_sense *)sense;
 
 			if (reqbuf->sense_key == UNIT_ATTENTION)
 				cdrom_saw_media_change(drive);
@@ -974,10 +989,10 @@ int ide_cd_queue_pc(ide_drive_t *drive, struct request *rq)
 		}
 
 		/* End of retry loop. */
-	} while ((rq->cmd_flags & REQ_FAILED) && retries >= 0);
+	} while ((flags & REQ_FAILED) && retries >= 0);
 
 	/* Return an error if the command failed. */
-	return (rq->cmd_flags & REQ_FAILED) ? -EIO : 0;
+	return (flags & REQ_FAILED) ? -EIO : 0;
 }
 
 /*
@@ -1383,23 +1398,20 @@ void msf_from_bcd (struct atapi_msf *msf)
 
 int cdrom_check_status(ide_drive_t *drive, struct request_sense *sense)
 {
-	struct request req;
 	struct cdrom_info *info = drive->driver_data;
 	struct cdrom_device_info *cdi = &info->devinfo;
+	unsigned char cmd[BLK_MAX_CDB];
 
-	ide_cd_init_rq(drive, &req);
-
-	req.sense = sense;
-	req.cmd[0] = GPCMD_TEST_UNIT_READY;
-	req.cmd_flags |= REQ_QUIET;
+	memset(cmd, 0, BLK_MAX_CDB);
+	cmd[0] = GPCMD_TEST_UNIT_READY;
 
 	/*
 	 * Sanyo 3 CD changer uses byte 7 of TEST_UNIT_READY to
 	 * switch CDs instead of supporting the LOAD_UNLOAD opcode.
 	 */
-	req.cmd[7] = cdi->sanyo_slot % 3;
+	cmd[7] = cdi->sanyo_slot % 3;
 
-	return ide_cd_queue_pc(drive, &req);
+	return ide_cd_queue_pc(drive, cmd, 0, NULL, 0, sense, 0, REQ_QUIET);
 }
 
 static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
@@ -1412,17 +1424,13 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
 	} capbuf;
 
 	int stat;
-	struct request req;
-
-	ide_cd_init_rq(drive, &req);
+	unsigned char cmd[BLK_MAX_CDB];
 
-	req.sense = sense;
-	req.cmd[0] = GPCMD_READ_CDVD_CAPACITY;
-	req.data = (char *)&capbuf;
-	req.data_len = sizeof(capbuf);
-	req.cmd_flags |= REQ_QUIET;
+	memset(cmd, 0, BLK_MAX_CDB);
+	cmd[0] = GPCMD_READ_CDVD_CAPACITY;
 
-	stat = ide_cd_queue_pc(drive, &req);
+	stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, sizeof(capbuf),
+			       sense, 0, REQ_QUIET);
 	if (stat == 0) {
 		*capacity = 1 + be32_to_cpu(capbuf.lba);
 		*sectors_per_frame =
@@ -1436,24 +1444,20 @@ static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
 				int format, char *buf, int buflen,
 				struct request_sense *sense)
 {
-	struct request req;
+	unsigned char cmd[BLK_MAX_CDB];
 
-	ide_cd_init_rq(drive, &req);
+	memset(cmd, 0, BLK_MAX_CDB);
 
-	req.sense = sense;
-	req.data =  buf;
-	req.data_len = buflen;
-	req.cmd_flags |= REQ_QUIET;
-	req.cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
-	req.cmd[6] = trackno;
-	req.cmd[7] = (buflen >> 8);
-	req.cmd[8] = (buflen & 0xff);
-	req.cmd[9] = (format << 6);
+	cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
+	cmd[6] = trackno;
+	cmd[7] = (buflen >> 8);
+	cmd[8] = (buflen & 0xff);
+	cmd[9] = (format << 6);
 
 	if (msf_flag)
-		req.cmd[1] = 2;
+		cmd[1] = 2;
 
-	return ide_cd_queue_pc(drive, &req);
+	return ide_cd_queue_pc(drive, cmd, 0, buf, buflen, sense, 0, REQ_QUIET);
 }
 
 /* Try to read the entire TOC for the disk into our internal buffer. */
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index 22e3751..1f2abdd 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -148,7 +148,8 @@ void ide_cd_log_error(const char *, struct request *, struct request_sense *);
 
 /* ide-cd.c functions used by ide-cd_ioctl.c */
 void ide_cd_init_rq(ide_drive_t *, struct request *);
-int ide_cd_queue_pc(ide_drive_t *, struct request *);
+int ide_cd_queue_pc(ide_drive_t *, const unsigned char *, int, void *, unsigned,
+		    struct request_sense *, int, unsigned int);
 int ide_cd_read_toc(ide_drive_t *, struct request_sense *);
 int ide_cdrom_get_capabilities(ide_drive_t *, u8 *);
 void ide_cdrom_update_speed(ide_drive_t *, u8 *);
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 6d147ce..9c044da 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -104,8 +104,8 @@ int cdrom_eject(ide_drive_t *drive, int ejectflag,
 {
 	struct cdrom_info *cd = drive->driver_data;
 	struct cdrom_device_info *cdi = &cd->devinfo;
-	struct request req;
 	char loej = 0x02;
+	unsigned char cmd[BLK_MAX_CDB];
 
 	if ((cd->cd_flags & IDE_CD_FLAG_NO_EJECT) && !ejectflag)
 		return -EDRIVE_CANT_DO_THIS;
@@ -114,17 +114,16 @@ int cdrom_eject(ide_drive_t *drive, int ejectflag,
 	if ((cd->cd_flags & IDE_CD_FLAG_DOOR_LOCKED) && ejectflag)
 		return 0;
 
-	ide_cd_init_rq(drive, &req);
-
 	/* only tell drive to close tray if open, if it can do that */
 	if (ejectflag && (cdi->mask & CDC_CLOSE_TRAY))
 		loej = 0;
 
-	req.sense = sense;
-	req.cmd[0] = GPCMD_START_STOP_UNIT;
-	req.cmd[4] = loej | (ejectflag != 0);
+	memset(cmd, 0, BLK_MAX_CDB);
+
+	cmd[0] = GPCMD_START_STOP_UNIT;
+	cmd[4] = loej | (ejectflag != 0);
 
-	return ide_cd_queue_pc(drive, &req);
+	return ide_cd_queue_pc(drive, cmd, 0, NULL, 0, sense, 0, 0);
 }
 
 /* Lock the door if LOCKFLAG is nonzero; unlock it otherwise. */
@@ -134,7 +133,6 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag,
 {
 	struct cdrom_info *cd = drive->driver_data;
 	struct request_sense my_sense;
-	struct request req;
 	int stat;
 
 	if (sense == NULL)
@@ -144,11 +142,15 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag,
 	if (cd->cd_flags & IDE_CD_FLAG_NO_DOORLOCK) {
 		stat = 0;
 	} else {
-		ide_cd_init_rq(drive, &req);
-		req.sense = sense;
-		req.cmd[0] = GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL;
-		req.cmd[4] = lockflag ? 1 : 0;
-		stat = ide_cd_queue_pc(drive, &req);
+		unsigned char cmd[BLK_MAX_CDB];
+
+		memset(cmd, 0, BLK_MAX_CDB);
+
+		cmd[0] = GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL;
+		cmd[4] = lockflag ? 1 : 0;
+
+		stat = ide_cd_queue_pc(drive, cmd, 0, NULL, 0,
+				       sense, 0, 0);
 	}
 
 	/* If we got an illegal field error, the drive
@@ -206,32 +208,30 @@ int ide_cdrom_select_speed(struct cdrom_device_info *cdi, int speed)
 {
 	ide_drive_t *drive = cdi->handle;
 	struct cdrom_info *cd = drive->driver_data;
-	struct request rq;
 	struct request_sense sense;
 	u8 buf[ATAPI_CAPABILITIES_PAGE_SIZE];
 	int stat;
-
-	ide_cd_init_rq(drive, &rq);
-
-	rq.sense = &sense;
+	unsigned char cmd[BLK_MAX_CDB];
 
 	if (speed == 0)
 		speed = 0xffff; /* set to max */
 	else
 		speed *= 177;   /* Nx to kbytes/s */
 
-	rq.cmd[0] = GPCMD_SET_SPEED;
+	memset(cmd, 0, BLK_MAX_CDB);
+
+	cmd[0] = GPCMD_SET_SPEED;
 	/* Read Drive speed in kbytes/second MSB/LSB */
-	rq.cmd[2] = (speed >> 8) & 0xff;
-	rq.cmd[3] = speed & 0xff;
+	cmd[2] = (speed >> 8) & 0xff;
+	cmd[3] = speed & 0xff;
 	if ((cdi->mask & (CDC_CD_R | CDC_CD_RW | CDC_DVD_R)) !=
 	    (CDC_CD_R | CDC_CD_RW | CDC_DVD_R)) {
 		/* Write Drive speed in kbytes/second MSB/LSB */
-		rq.cmd[4] = (speed >> 8) & 0xff;
-		rq.cmd[5] = speed & 0xff;
+		cmd[4] = (speed >> 8) & 0xff;
+		cmd[5] = speed & 0xff;
 	}
 
-	stat = ide_cd_queue_pc(drive, &rq);
+	stat = ide_cd_queue_pc(drive, cmd, 0, NULL, 0, &sense, 0, 0);
 
 	if (!ide_cdrom_get_capabilities(drive, buf)) {
 		ide_cdrom_update_speed(drive, buf);
@@ -268,21 +268,18 @@ int ide_cdrom_get_mcn(struct cdrom_device_info *cdi,
 {
 	ide_drive_t *drive = cdi->handle;
 	int stat, mcnlen;
-	struct request rq;
 	char buf[24];
+	unsigned char cmd[BLK_MAX_CDB];
 
-	ide_cd_init_rq(drive, &rq);
+	memset(cmd, 0, BLK_MAX_CDB);
 
-	rq.data = buf;
-	rq.data_len = sizeof(buf);
+	cmd[0] = GPCMD_READ_SUBCHANNEL;
+	cmd[1] = 2;		/* MSF addressing */
+	cmd[2] = 0x40;	/* request subQ data */
+	cmd[3] = 2;		/* format */
+	cmd[8] = sizeof(buf);
 
-	rq.cmd[0] = GPCMD_READ_SUBCHANNEL;
-	rq.cmd[1] = 2;		/* MSF addressing */
-	rq.cmd[2] = 0x40;	/* request subQ data */
-	rq.cmd[3] = 2;		/* format */
-	rq.cmd[8] = sizeof(buf);
-
-	stat = ide_cd_queue_pc(drive, &rq);
+	stat = ide_cd_queue_pc(drive, cmd, 0, buf, sizeof(buf), NULL, 0, 0);
 	if (stat)
 		return stat;
 
@@ -351,8 +348,8 @@ static int ide_cd_fake_play_trkind(ide_drive_t *drive, void *arg)
 	struct atapi_toc_entry *first_toc, *last_toc;
 	unsigned long lba_start, lba_end;
 	int stat;
-	struct request rq;
 	struct request_sense sense;
+	unsigned char cmd[BLK_MAX_CDB];
 
 	stat = ide_cd_get_toc_entry(drive, ti->cdti_trk0, &first_toc);
 	if (stat)
@@ -370,14 +367,13 @@ static int ide_cd_fake_play_trkind(ide_drive_t *drive, void *arg)
 	if (lba_end <= lba_start)
 		return -EINVAL;
 
-	ide_cd_init_rq(drive, &rq);
+	memset(cmd, 0, BLK_MAX_CDB);
 
-	rq.sense = &sense;
-	rq.cmd[0] = GPCMD_PLAY_AUDIO_MSF;
-	lba_to_msf(lba_start,   &rq.cmd[3], &rq.cmd[4], &rq.cmd[5]);
-	lba_to_msf(lba_end - 1, &rq.cmd[6], &rq.cmd[7], &rq.cmd[8]);
+	cmd[0] = GPCMD_PLAY_AUDIO_MSF;
+	lba_to_msf(lba_start,   &cmd[3], &cmd[4], &cmd[5]);
+	lba_to_msf(lba_end - 1, &cmd[6], &cmd[7], &cmd[8]);
 
-	return ide_cd_queue_pc(drive, &rq);
+	return ide_cd_queue_pc(drive, cmd, 0, NULL, 0, &sense, 0, 0);
 }
 
 static int ide_cd_read_tochdr(ide_drive_t *drive, void *arg)
@@ -447,8 +443,8 @@ int ide_cdrom_audio_ioctl(struct cdrom_device_info *cdi,
 int ide_cdrom_packet(struct cdrom_device_info *cdi,
 			    struct packet_command *cgc)
 {
-	struct request req;
 	ide_drive_t *drive = cdi->handle;
+	unsigned int flags = 0;
 
 	if (cgc->timeout <= 0)
 		cgc->timeout = ATAPI_WAIT_PC;
@@ -456,24 +452,21 @@ int ide_cdrom_packet(struct cdrom_device_info *cdi,
 	/* here we queue the commands from the uniform CD-ROM
 	   layer. the packet must be complete, as we do not
 	   touch it at all. */
-	ide_cd_init_rq(drive, &req);
 
 	if (cgc->data_direction == CGC_DATA_WRITE)
-		req.cmd_flags |= REQ_RW;
+		flags |= REQ_RW;
 
-	memcpy(req.cmd, cgc->cmd, CDROM_PACKET_SIZE);
 	if (cgc->sense)
 		memset(cgc->sense, 0, sizeof(struct request_sense));
-	req.data = cgc->buffer;
-	req.data_len = cgc->buflen;
-	req.timeout = cgc->timeout;
 
 	if (cgc->quiet)
-		req.cmd_flags |= REQ_QUIET;
+		flags |= REQ_QUIET;
 
-	req.sense = cgc->sense;
-	cgc->stat = ide_cd_queue_pc(drive, &req);
+	cgc->stat = ide_cd_queue_pc(drive, cgc->cmd,
+				    cgc->data_direction == CGC_DATA_WRITE,
+				    cgc->buffer, cgc->buflen,
+				    cgc->sense, cgc->timeout, flags);
 	if (!cgc->stat)
-		cgc->buflen -= req.data_len;
+		cgc->buflen = 0;
 	return cgc->stat;
 }
-- 
1.5.4.2

[PATCH 06/11] ide: make ide_do_drive_cmd return rq->errors

[FUJITA Tomonori]

ide_do_drive_cmd forges an error value (-EIO) instead of
rq->errors. idetape_queue_rw_tail wants rq->errors so this patch makes
ide_do_drive_cmd return rq->errors.

For compatibility, this patch makes the users of ide_do_drive_cmd
return -EIO instead of a return value of ide_do_drive_cmd
(rq->errors).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd_ioctl.c |    3 ++-
 drivers/ide/ide-floppy.c   |    4 +++-
 drivers/ide/ide-io.c       |    2 +-
 drivers/ide/ide-tape.c     |    5 ++++-
 drivers/ide/ide-taskfile.c |    7 +++++--
 drivers/ide/ide.c          |    2 ++
 6 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 9c044da..57dda1f 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -302,7 +302,8 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
 	req.cmd_type = REQ_TYPE_SPECIAL;
 	req.cmd_flags = REQ_QUIET;
 	ret = ide_do_drive_cmd(drive, &req, ide_wait);
-
+	if (ret)
+		ret = -EIO;
 	/*
 	 * A reset will unlock the door. If it was previously locked,
 	 * lock it again.
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index d75b2aa..af6625a 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -886,13 +886,15 @@ static int idefloppy_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
 {
 	struct ide_floppy_obj *floppy = drive->driver_data;
 	struct request *rq;
+	int ret;
 
 	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 	rq->buffer = (char *) pc;
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->rq_disk = floppy->disk;
 
-	return ide_do_drive_cmd(drive, rq, ide_wait);
+	ret = ide_do_drive_cmd(drive, rq, ide_wait);
+	return ret ? -EIO : 0;
 }
 
 /*
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 31e5afa..ce09f70 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1667,7 +1667,7 @@ int ide_do_drive_cmd (ide_drive_t *drive, struct request *rq, ide_action_t actio
 	if (must_wait) {
 		wait_for_completion(&wait);
 		if (rq->errors)
-			err = -EIO;
+			err = rq->errors;
 
 		blk_put_request(rq);
 	}
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index f43fd07..02851b3 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -1911,11 +1911,14 @@ static int __idetape_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
 {
 	struct ide_tape_obj *tape = drive->driver_data;
 	struct request rq;
+	int ret;
 
 	idetape_init_rq(&rq, REQ_IDETAPE_PC1);
 	rq.buffer = (char *) pc;
 	rq.rq_disk = tape->disk;
-	return ide_do_drive_cmd(drive, &rq, ide_wait);
+	ret = ide_do_drive_cmd(drive, &rq, ide_wait);
+	return ret ? -EIO : 0;
+
 }
 
 static void idetape_create_load_unload_cmd(ide_drive_t *drive,
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index fac7fad..6d433c4 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -531,6 +531,7 @@ static ide_startstop_t pre_task_out_intr(ide_drive_t *drive, struct request *rq)
 int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
 {
 	struct request *rq;
+	int ret;
 
 	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
@@ -551,7 +552,8 @@ int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
 	rq->special = task;
 	task->rq = rq;
 
-	return ide_do_drive_cmd(drive, rq, ide_wait);
+	ret = ide_do_drive_cmd(drive, rq, ide_wait);
+	return ret ? -EIO : 0;
 }
 
 EXPORT_SYMBOL(ide_raw_taskfile);
@@ -782,7 +784,8 @@ int ide_cmd_ioctl (ide_drive_t *drive, unsigned int cmd, unsigned long arg)
 		rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 		rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
 
-		return ide_do_drive_cmd(drive, rq, ide_wait);
+		err = ide_do_drive_cmd(drive, rq, ide_wait);
+		return err ? -EIO : 0;
 	}
 
 	if (copy_from_user(args, (void __user *)arg, 4))
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index c5b33f5..8970054 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -738,6 +738,8 @@ static int generic_ide_resume(struct device *dev)
 	rqpm.pm_state = PM_EVENT_ON;
 
 	err = ide_do_drive_cmd(drive, rq, ide_head_wait);
+	if (err)
+		err = -EIO;
 
 	if (err == 0 && dev->driver) {
 		ide_driver_t *drv = to_ide_driver(dev->driver);
-- 
1.5.4.2

[PATCH 07/11] ide-tape: use blk_get_request in the ide_do_drive_cmd path

[FUJITA Tomonori]

This replaces struct request on the stack with blk_get_request in the
ide_do_drive_cmd path that uses ide_wait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-tape.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 02851b3..04c611e 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -1910,13 +1910,15 @@ static void idetape_create_test_unit_ready_cmd(struct ide_atapi_pc *pc)
 static int __idetape_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
 {
 	struct ide_tape_obj *tape = drive->driver_data;
-	struct request rq;
+	struct request *rq;
 	int ret;
 
-	idetape_init_rq(&rq, REQ_IDETAPE_PC1);
-	rq.buffer = (char *) pc;
-	rq.rq_disk = tape->disk;
-	ret = ide_do_drive_cmd(drive, &rq, ide_wait);
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->cmd_type = REQ_TYPE_SPECIAL;
+	rq->cmd[0] = REQ_IDETAPE_PC1;
+	rq->buffer = (char *)pc;
+	rq->rq_disk = tape->disk;
+	ret = ide_do_drive_cmd(drive, rq, ide_wait);
 	return ret ? -EIO : 0;
 
 }
@@ -2130,7 +2132,8 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
 				 struct idetape_bh *bh)
 {
 	idetape_tape_t *tape = drive->driver_data;
-	struct request rq;
+	struct request *rq;
+	int ret;
 
 	debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
 
@@ -2140,22 +2143,24 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
 		return (0);
 	}
 
-	idetape_init_rq(&rq, cmd);
-	rq.rq_disk = tape->disk;
-	rq.special = (void *)bh;
-	rq.sector = tape->first_frame;
-	rq.nr_sectors		= blocks;
-	rq.current_nr_sectors	= blocks;
-	(void) ide_do_drive_cmd(drive, &rq, ide_wait);
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->cmd_type = REQ_TYPE_SPECIAL;
+	rq->cmd[0] = cmd;
+	rq->rq_disk = tape->disk;
+	rq->special = (void *)bh;
+	rq->sector = tape->first_frame;
+	rq->nr_sectors = blocks;
+	rq->current_nr_sectors = blocks;
+	ret = ide_do_drive_cmd(drive, rq, ide_wait);
 
 	if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
 		return 0;
 
 	if (tape->merge_stage)
 		idetape_init_merge_stage(tape);
-	if (rq.errors == IDETAPE_ERROR_GENERAL)
+	if (ret == IDETAPE_ERROR_GENERAL)
 		return -EIO;
-	return (tape->blk_size * (blocks-rq.current_nr_sectors));
+	return (tape->blk_size * (blocks-rq->current_nr_sectors));
 }
 
 /* start servicing the pipeline stages, starting from tape->next_stage. */
-- 
1.5.4.2

[PATCH 08/11] ide-cd: use blk_get_request in the ide_do_drive_cmd path

[FUJITA Tomonori]

This replaces struct request on the stack with blk_get_request in the
ide_do_drive_cmd path that uses ide_wait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd_ioctl.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 57dda1f..8739893 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -295,13 +295,14 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
 	ide_drive_t *drive = cdi->handle;
 	struct cdrom_info *cd = drive->driver_data;
 	struct request_sense sense;
-	struct request req;
+	struct request *rq;
 	int ret;
 
-	ide_cd_init_rq(drive, &req);
-	req.cmd_type = REQ_TYPE_SPECIAL;
-	req.cmd_flags = REQ_QUIET;
-	ret = ide_do_drive_cmd(drive, &req, ide_wait);
+	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
+	rq->rq_disk = cd->disk;
+	rq->cmd_type = REQ_TYPE_SPECIAL;
+	rq->cmd_flags = REQ_QUIET;
+	ret = ide_do_drive_cmd(drive, rq, ide_wait);
 	if (ret)
 		ret = -EIO;
 	/*
-- 
1.5.4.2

[PATCH 09/11] ide: call blk_put_request properly

[FUJITA Tomonori]

Now all the users of ide_do_drive_cmd using ide_wait are converted to
use blk_get_request instead of struct request on the stack. This puts
WARN_ON to catch an overlooked user. We can remove WARN_ON and "if
(rq->q)" part after a while.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-io.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index ce09f70..194e666 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1669,7 +1669,9 @@ int ide_do_drive_cmd (ide_drive_t *drive, struct request *rq, ide_action_t actio
 		if (rq->errors)
 			err = rq->errors;
 
-		blk_put_request(rq);
+		WARN_ON(!rq->q);
+		if (rq->q)
+			blk_put_request(rq);
 	}
 
 	return err;
-- 
1.5.4.2

[PATCH 10/11] block: convert pd to use blk_get_request

[FUJITA Tomonori]

pd.c uses blk_put_request with struct request on the stack. As a
result, blk_put_request needs a hack to catch a NULL request_queue.
This converts pd.c to use blk_get_request.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 drivers/block/paride/pd.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index df819f8..e52babb 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -713,20 +713,19 @@ static int pd_special_command(struct pd_unit *disk,
 		      enum action (*func)(struct pd_unit *disk))
 {
 	DECLARE_COMPLETION_ONSTACK(wait);
-	struct request rq;
+	struct request *rq;
 	int err = 0;
 
-	memset(&rq, 0, sizeof(rq));
-	rq.errors = 0;
-	rq.rq_disk = disk->gd;
-	rq.ref_count = 1;
-	rq.end_io_data = &wait;
-	rq.end_io = blk_end_sync_rq;
-	blk_insert_request(disk->gd->queue, &rq, 0, func);
+	rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT);
+	rq->ref_count++;
+	rq->rq_disk = disk->gd;
+	rq->end_io_data = &wait;
+	rq->end_io = blk_end_sync_rq;
+	blk_insert_request(disk->gd->queue, rq, 0, func);
 	wait_for_completion(&wait);
-	if (rq.errors)
+	if (rq->errors)
 		err = -EIO;
-	blk_put_request(&rq);
+	blk_put_request(rq);
 	return err;
 }
 
-- 
1.5.4.2

[PATCH 11/11] block: remove the checking for NULL queue in blk_put_request

[FUJITA Tomonori]

Some calls blk_put_request with struct request on the stack. As a
result, blk_put_request has a hack to catch a NULL request_queue. Now
such callers are fixed (they use blk_get_request properly). So we can
safely remove the hack in blk_put_request.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-core.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..62f9b12 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1050,15 +1050,9 @@ void blk_put_request(struct request *req)
 	unsigned long flags;
 	struct request_queue *q = req->q;
 
-	/*
-	 * Gee, IDE calls in w/ NULL q.  Fix IDE and remove the
-	 * following if (q) test.
-	 */
-	if (q) {
-		spin_lock_irqsave(q->queue_lock, flags);
-		__blk_put_request(q, req);
-		spin_unlock_irqrestore(q->queue_lock, flags);
-	}
+	spin_lock_irqsave(q->queue_lock, flags);
+	__blk_put_request(q, req);
+	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 EXPORT_SYMBOL(blk_put_request);
 
-- 
1.5.4.2

Re: [PATCH 00/11] removing the on-stack struct request

[Bartlomiej Zolnierkiewicz]

Hi,

On Tue, Apr 22, 2008 at 2:26 AM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
> This patchset converts some of the users of struct request on the
>  stack to use blk_get_request (as discussed in the large command
>  support thread [*1]).
>
>  IDE has the most users of the on-stack struct request. The on-stack
>  struct request has the benefit that it'll always work but I think that
>  using blk_get_request properly make the code clearer.
>
>  I've not got rid of all the on-stack struct request yet, but at least
>  this patchset removes the users of struct request that calls
>  blk_put_request. That is, we can remove the following hack in
>  blk_put_request:
>
>  /*
>   * Gee, IDE calls in w/ NULL q.  Fix IDE and remove the
>   * following if (q) test.
>   */
>  if (q) {
>         spin_lock_irqsave(q->queue_lock, flags);
>         __blk_put_request(q, req);
>         spin_unlock_irqrestore(q->queue_lock, flags);
>  }
>
>  This patchset is against Bartlomiej's ide tree. #1-9 patches are for
>  the IDE subsystem and #10-11 are for the block layer. #10-11 depends
>  on #1-9 so probably it would be easier to push all the patch via a
>  single tree.
>
>  [*1]
>  http://marc.info/?l=linux-scsi&m=120817161219068&w=2

Looks promising but probably I'll not be able to review it properly
this week (I'm busy with my real job) and the next week (I'll be busy
with pushing overdue IDE updates).  OTOH I'm hoping that Borislav
would be able to take a look in the meantime.

Thanks,
Bart

Re: [PATCH 07/11] ide-tape: use blk_get_request in the ide_do_drive_cmd path

[Borislav Petkov]

On Tue, Apr 22, 2008 at 09:26:38AM +0900, FUJITA Tomonori wrote:
> This replaces struct request on the stack with blk_get_request in the
> ide_do_drive_cmd path that uses ide_wait.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-tape.c |   35 ++++++++++++++++++++---------------
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 02851b3..04c611e 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -1910,13 +1910,15 @@ static void idetape_create_test_unit_ready_cmd(struct ide_atapi_pc *pc)
>  static int __idetape_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
>  {
>  	struct ide_tape_obj *tape = drive->driver_data;
> -	struct request rq;
> +	struct request *rq;
>  	int ret;
>  
> -	idetape_init_rq(&rq, REQ_IDETAPE_PC1);
> -	rq.buffer = (char *) pc;
> -	rq.rq_disk = tape->disk;
> -	ret = ide_do_drive_cmd(drive, &rq, ide_wait);
> +	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
> +	rq->cmd_type = REQ_TYPE_SPECIAL;
> +	rq->cmd[0] = REQ_IDETAPE_PC1;
> +	rq->buffer = (char *)pc;
> +	rq->rq_disk = tape->disk;
> +	ret = ide_do_drive_cmd(drive, rq, ide_wait);
>  	return ret ? -EIO : 0;
>  
>  }
> @@ -2130,7 +2132,8 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
>  				 struct idetape_bh *bh)
>  {
>  	idetape_tape_t *tape = drive->driver_data;
> -	struct request rq;
> +	struct request *rq;
> +	int ret;
>  
>  	debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
>  
> @@ -2140,22 +2143,24 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
>  		return (0);
>  	}
>  
> -	idetape_init_rq(&rq, cmd);
> -	rq.rq_disk = tape->disk;
> -	rq.special = (void *)bh;
> -	rq.sector = tape->first_frame;
> -	rq.nr_sectors		= blocks;
> -	rq.current_nr_sectors	= blocks;
> -	(void) ide_do_drive_cmd(drive, &rq, ide_wait);
> +	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
> +	rq->cmd_type = REQ_TYPE_SPECIAL;
> +	rq->cmd[0] = cmd;
> +	rq->rq_disk = tape->disk;
> +	rq->special = (void *)bh;
> +	rq->sector = tape->first_frame;
> +	rq->nr_sectors = blocks;
> +	rq->current_nr_sectors = blocks;
> +	ret = ide_do_drive_cmd(drive, rq, ide_wait);
>  
>  	if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
>  		return 0;
>  
>  	if (tape->merge_stage)
>  		idetape_init_merge_stage(tape);
> -	if (rq.errors == IDETAPE_ERROR_GENERAL)
> +	if (ret == IDETAPE_ERROR_GENERAL)
>  		return -EIO;
> -	return (tape->blk_size * (blocks-rq.current_nr_sectors));
> +	return (tape->blk_size * (blocks-rq->current_nr_sectors));
>  }
>  
>  /* start servicing the pipeline stages, starting from tape->next_stage. */

are you sure you're patching against the right ide tree? See, we removed
pipelining (patches went in around the beginning of April) and the above comment
kinda says the opposite. Just to make sure, the latest quilt tree is at
http://www.kernel.org/pub/linux/kernel/people/bart/pata-2.6/patches/.

> -- 
> 1.5.4.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 06/11] ide: make ide_do_drive_cmd return rq->errors

[Borislav Petkov]

Hi,

On Tue, Apr 22, 2008 at 09:26:37AM +0900, FUJITA Tomonori wrote:
> ide_do_drive_cmd forges an error value (-EIO) instead of
> rq->errors. idetape_queue_rw_tail wants rq->errors so this patch makes
> ide_do_drive_cmd return rq->errors.
> 
> For compatibility, this patch makes the users of ide_do_drive_cmd
> return -EIO instead of a return value of ide_do_drive_cmd
> (rq->errors).

i don't see the reason for it. In the end, the only error type that is being
handed to and fro is -EIO and nobody is interested in rq->errors. So, whether
ide_do_drive_cmd or its callers return -EIO is kinda unimportant. In the second
case, however, you have simply added more code (as per the diffstat below) with
no apparent functionality. It would only make sense, IMHO, if you would
differentiate behaviour based on rq->errors...

> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-cd_ioctl.c |    3 ++-
>  drivers/ide/ide-floppy.c   |    4 +++-
>  drivers/ide/ide-io.c       |    2 +-
>  drivers/ide/ide-tape.c     |    5 ++++-
>  drivers/ide/ide-taskfile.c |    7 +++++--
>  drivers/ide/ide.c          |    2 ++
>  6 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
> index 9c044da..57dda1f 100644
> --- a/drivers/ide/ide-cd_ioctl.c
> +++ b/drivers/ide/ide-cd_ioctl.c
> @@ -302,7 +302,8 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
>  	req.cmd_type = REQ_TYPE_SPECIAL;
>  	req.cmd_flags = REQ_QUIET;
>  	ret = ide_do_drive_cmd(drive, &req, ide_wait);
> -
> +	if (ret)
> +		ret = -EIO;
>  	/*
>  	 * A reset will unlock the door. If it was previously locked,
>  	 * lock it again.
> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> index d75b2aa..af6625a 100644
> --- a/drivers/ide/ide-floppy.c
> +++ b/drivers/ide/ide-floppy.c
> @@ -886,13 +886,15 @@ static int idefloppy_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
>  {
>  	struct ide_floppy_obj *floppy = drive->driver_data;
>  	struct request *rq;
> +	int ret;
>  
>  	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
>  	rq->buffer = (char *) pc;
>  	rq->cmd_type = REQ_TYPE_SPECIAL;
>  	rq->rq_disk = floppy->disk;
>  
> -	return ide_do_drive_cmd(drive, rq, ide_wait);
> +	ret = ide_do_drive_cmd(drive, rq, ide_wait);
> +	return ret ? -EIO : 0;
>  }
>  
>  /*
> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index 31e5afa..ce09f70 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -1667,7 +1667,7 @@ int ide_do_drive_cmd (ide_drive_t *drive, struct request *rq, ide_action_t actio
>  	if (must_wait) {
>  		wait_for_completion(&wait);
>  		if (rq->errors)
> -			err = -EIO;
> +			err = rq->errors;
>  
>  		blk_put_request(rq);
>  	}
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index f43fd07..02851b3 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -1911,11 +1911,14 @@ static int __idetape_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
>  {
>  	struct ide_tape_obj *tape = drive->driver_data;
>  	struct request rq;
> +	int ret;
>  
>  	idetape_init_rq(&rq, REQ_IDETAPE_PC1);
>  	rq.buffer = (char *) pc;
>  	rq.rq_disk = tape->disk;
> -	return ide_do_drive_cmd(drive, &rq, ide_wait);
> +	ret = ide_do_drive_cmd(drive, &rq, ide_wait);
> +	return ret ? -EIO : 0;
> +
>  }
>  
>  static void idetape_create_load_unload_cmd(ide_drive_t *drive,
> diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
> index fac7fad..6d433c4 100644
> --- a/drivers/ide/ide-taskfile.c
> +++ b/drivers/ide/ide-taskfile.c
> @@ -531,6 +531,7 @@ static ide_startstop_t pre_task_out_intr(ide_drive_t *drive, struct request *rq)
>  int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
>  {
>  	struct request *rq;
> +	int ret;
>  
>  	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
>  	rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
> @@ -551,7 +552,8 @@ int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
>  	rq->special = task;
>  	task->rq = rq;
>  
> -	return ide_do_drive_cmd(drive, rq, ide_wait);
> +	ret = ide_do_drive_cmd(drive, rq, ide_wait);
> +	return ret ? -EIO : 0;
>  }
>  
>  EXPORT_SYMBOL(ide_raw_taskfile);
> @@ -782,7 +784,8 @@ int ide_cmd_ioctl (ide_drive_t *drive, unsigned int cmd, unsigned long arg)
>  		rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
>  		rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
>  
> -		return ide_do_drive_cmd(drive, rq, ide_wait);
> +		err = ide_do_drive_cmd(drive, rq, ide_wait);
> +		return err ? -EIO : 0;
>  	}
>  
>  	if (copy_from_user(args, (void __user *)arg, 4))
> diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
> index c5b33f5..8970054 100644
> --- a/drivers/ide/ide.c
> +++ b/drivers/ide/ide.c
> @@ -738,6 +738,8 @@ static int generic_ide_resume(struct device *dev)
>  	rqpm.pm_state = PM_EVENT_ON;
>  
>  	err = ide_do_drive_cmd(drive, rq, ide_head_wait);
> +	if (err)
> +		err = -EIO;
>  
>  	if (err == 0 && dev->driver) {
>  		ide_driver_t *drv = to_ide_driver(dev->driver);
> -- 
> 1.5.4.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 00/11] removing the on-stack struct request

[Borislav Petkov]

On Tue, Apr 22, 2008 at 11:14:28AM +0200, Bartlomiej Zolnierkiewicz wrote:

[..]

> Looks promising but probably I'll not be able to review it properly
> this week (I'm busy with my real job) and the next week (I'll be busy
> with pushing overdue IDE updates).  OTOH I'm hoping that Borislav
> would be able to take a look in the meantime.

Yep, they're quite straight-forward and look fine except some nitpicking i sent
in separate mails. FWIW, this is what we wanted to do initially but Jens didn't
agree to that at the time. I guess, it's because i probably didn't express
myself clearly enough then. See http://kerneltrap.org/mailarchive/linux-kernel/2008/3/1/1037974
for the whole discussion.

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 00/11] removing the on-stack struct request

[Jens Axboe]

On Wed, Apr 23 2008, Borislav Petkov wrote:
> On Tue, Apr 22, 2008 at 11:14:28AM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> [..]
> 
> > Looks promising but probably I'll not be able to review it properly
> > this week (I'm busy with my real job) and the next week (I'll be busy
> > with pushing overdue IDE updates).  OTOH I'm hoping that Borislav
> > would be able to take a look in the meantime.
> 
> Yep, they're quite straight-forward and look fine except some
> nitpicking i sent in separate mails. FWIW, this is what we wanted to
> do initially but Jens didn't agree to that at the time. I guess, it's
> because i probably didn't express myself clearly enough then. See
> http://kerneltrap.org/mailarchive/linux-kernel/2008/3/1/1037974 for
> the whole discussion.

The cases that do ide_wait can clearly use __GFP_WAIT, and then there
are no issues. My worry was with current paths that allocate the request
on stack from interrupt context, where you cannot block for mempool
allocation. Perhaps such paths don't exist in ide (anymore?), if not
then there are no objections from me.

-- 
Jens Axboe

Re: [PATCH 06/11] ide: make ide_do_drive_cmd return rq->errors

[FUJITA Tomonori]

On Wed, 23 Apr 2008 09:32:22 +0200
Borislav Petkov <petkovbb@googlemail.com> wrote:

> Hi,
> 
> On Tue, Apr 22, 2008 at 09:26:37AM +0900, FUJITA Tomonori wrote:
> > ide_do_drive_cmd forges an error value (-EIO) instead of
> > rq->errors. idetape_queue_rw_tail wants rq->errors so this patch makes
> > ide_do_drive_cmd return rq->errors.
> > 
> > For compatibility, this patch makes the users of ide_do_drive_cmd
> > return -EIO instead of a return value of ide_do_drive_cmd
> > (rq->errors).
> 
> i don't see the reason for it. In the end, the only error type that is being
> handed to and fro is -EIO and nobody is interested in rq->errors. So, whether
> ide_do_drive_cmd or its callers return -EIO is kinda unimportant. In the second
> case, however, you have simply added more code (as per the diffstat below) with
> no apparent functionality. It would only make sense, IMHO, if you would
> differentiate behaviour based on rq->errors...

I did this only because idetape_queue_rw_tail seems to have
differentiate behaviour based on rq->errors:


static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
				 struct idetape_bh *bh)
{
...

	(void) ide_do_drive_cmd(drive, &rq, ide_wait);

	if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
		return 0;

	if (tape->merge_stage)
		idetape_init_merge_stage(tape);
	if (rq.errors == IDETAPE_ERROR_GENERAL)
		return -EIO;


If we can do something like:

	ret = ide_do_drive_cmd(drive, &rq, ide_wait);

	if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
		return 0;

	if (tape->merge_stage)
		idetape_init_merge_stage(tape);
	if (ret)
		return -EIO;


Then yeah, we don't need this patch.

Re: [PATCH 07/11] ide-tape: use blk_get_request in the ide_do_drive_cmd path

[FUJITA Tomonori]

On Wed, 23 Apr 2008 09:31:36 +0200
Borislav Petkov <petkovbb@googlemail.com> wrote:

> On Tue, Apr 22, 2008 at 09:26:38AM +0900, FUJITA Tomonori wrote:
> > This replaces struct request on the stack with blk_get_request in the
> > ide_do_drive_cmd path that uses ide_wait.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> >  drivers/ide/ide-tape.c |   35 ++++++++++++++++++++---------------
> >  1 files changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> > index 02851b3..04c611e 100644
> > --- a/drivers/ide/ide-tape.c
> > +++ b/drivers/ide/ide-tape.c
> > @@ -1910,13 +1910,15 @@ static void idetape_create_test_unit_ready_cmd(struct ide_atapi_pc *pc)
> >  static int __idetape_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
> >  {
> >  	struct ide_tape_obj *tape = drive->driver_data;
> > -	struct request rq;
> > +	struct request *rq;
> >  	int ret;
> >  
> > -	idetape_init_rq(&rq, REQ_IDETAPE_PC1);
> > -	rq.buffer = (char *) pc;
> > -	rq.rq_disk = tape->disk;
> > -	ret = ide_do_drive_cmd(drive, &rq, ide_wait);
> > +	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
> > +	rq->cmd_type = REQ_TYPE_SPECIAL;
> > +	rq->cmd[0] = REQ_IDETAPE_PC1;
> > +	rq->buffer = (char *)pc;
> > +	rq->rq_disk = tape->disk;
> > +	ret = ide_do_drive_cmd(drive, rq, ide_wait);
> >  	return ret ? -EIO : 0;
> >  
> >  }
> > @@ -2130,7 +2132,8 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
> >  				 struct idetape_bh *bh)
> >  {
> >  	idetape_tape_t *tape = drive->driver_data;
> > -	struct request rq;
> > +	struct request *rq;
> > +	int ret;
> >  
> >  	debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
> >  
> > @@ -2140,22 +2143,24 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
> >  		return (0);
> >  	}
> >  
> > -	idetape_init_rq(&rq, cmd);
> > -	rq.rq_disk = tape->disk;
> > -	rq.special = (void *)bh;
> > -	rq.sector = tape->first_frame;
> > -	rq.nr_sectors		= blocks;
> > -	rq.current_nr_sectors	= blocks;
> > -	(void) ide_do_drive_cmd(drive, &rq, ide_wait);
> > +	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
> > +	rq->cmd_type = REQ_TYPE_SPECIAL;
> > +	rq->cmd[0] = cmd;
> > +	rq->rq_disk = tape->disk;
> > +	rq->special = (void *)bh;
> > +	rq->sector = tape->first_frame;
> > +	rq->nr_sectors = blocks;
> > +	rq->current_nr_sectors = blocks;
> > +	ret = ide_do_drive_cmd(drive, rq, ide_wait);
> >  
> >  	if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
> >  		return 0;
> >  
> >  	if (tape->merge_stage)
> >  		idetape_init_merge_stage(tape);
> > -	if (rq.errors == IDETAPE_ERROR_GENERAL)
> > +	if (ret == IDETAPE_ERROR_GENERAL)
> >  		return -EIO;
> > -	return (tape->blk_size * (blocks-rq.current_nr_sectors));
> > +	return (tape->blk_size * (blocks-rq->current_nr_sectors));
> >  }
> >  
> >  /* start servicing the pipeline stages, starting from tape->next_stage. */
> 
> are you sure you're patching against the right ide tree? See, we removed
> pipelining (patches went in around the beginning of April) and the above comment
> kinda says the opposite. Just to make sure, the latest quilt tree is at
> http://www.kernel.org/pub/linux/kernel/people/bart/pata-2.6/patches/.

The patchset is against Bart's git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/bart/ide-2.6.git


Doesn't his git tree include the latest ide code?

Re: [PATCH 00/11] removing the on-stack struct request

[FUJITA Tomonori]

On Wed, 23 Apr 2008 09:40:11 +0200
Borislav Petkov <petkovbb@googlemail.com> wrote:

> On Tue, Apr 22, 2008 at 11:14:28AM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> [..]
> 
> > Looks promising but probably I'll not be able to review it properly
> > this week (I'm busy with my real job) and the next week (I'll be busy
> > with pushing overdue IDE updates).  OTOH I'm hoping that Borislav
> > would be able to take a look in the meantime.
> 
> Yep, they're quite straight-forward and look fine except some nitpicking i sent
> in separate mails.

Thanks for reviewing,

Yeah, these patches are pretty straightforward mainly because they are
against the paths where we can use __GFP_WAIT.


> FWIW, this is what we wanted to do initially but Jens didn't
> agree to that at the time. I guess, it's because i probably didn't express
> myself clearly enough then. See http://kerneltrap.org/mailarchive/linux-kernel/2008/3/1/1037974
> for the whole discussion.

I think that IDE uses request struct from interrupt context at some
places. In such places, I don't think that converting requests on the
stack and a pre-allocated fixed number of requests to
kmalloc/blk_get_request with GFP_ATOMIC is an good idea. It's better
to remove such code (allocating requests from interrupt context).

Re: [PATCH 00/11] removing the on-stack struct request

[Jens Axboe]

On Wed, Apr 23 2008, FUJITA Tomonori wrote:
> On Wed, 23 Apr 2008 09:40:11 +0200
> Borislav Petkov <petkovbb@googlemail.com> wrote:
> 
> > On Tue, Apr 22, 2008 at 11:14:28AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > 
> > [..]
> > 
> > > Looks promising but probably I'll not be able to review it properly
> > > this week (I'm busy with my real job) and the next week (I'll be busy
> > > with pushing overdue IDE updates).  OTOH I'm hoping that Borislav
> > > would be able to take a look in the meantime.
> > 
> > Yep, they're quite straight-forward and look fine except some nitpicking i sent
> > in separate mails.
> 
> Thanks for reviewing,
> 
> Yeah, these patches are pretty straightforward mainly because they are
> against the paths where we can use __GFP_WAIT.
> 
> 
> > FWIW, this is what we wanted to do initially but Jens didn't
> > agree to that at the time. I guess, it's because i probably didn't express
> > myself clearly enough then. See http://kerneltrap.org/mailarchive/linux-kernel/2008/3/1/1037974
> > for the whole discussion.
> 
> I think that IDE uses request struct from interrupt context at some
> places. In such places, I don't think that converting requests on the
> stack and a pre-allocated fixed number of requests to
> kmalloc/blk_get_request with GFP_ATOMIC is an good idea. It's better
> to remove such code (allocating requests from interrupt context).

Fully agree, that is what I originally opposed. Then IDE should
pre-alloc a request at a safe time for usage like this.

-- 
Jens Axboe

Re: [PATCH 07/11] ide-tape: use blk_get_request in the ide_do_drive_cmd path

[Boris Petkov]

On Wed, Apr 23, 2008 at 10:25 AM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
>
> On Wed, 23 Apr 2008 09:31:36 +0200
>  Borislav Petkov <petkovbb@googlemail.com> wrote:
>
>  > On Tue, Apr 22, 2008 at 09:26:38AM +0900, FUJITA Tomonori wrote:
>  > > This replaces struct request on the stack with blk_get_request in the
>  > > ide_do_drive_cmd path that uses ide_wait.
>  > >
>  > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>  > > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>  > > ---
>  > >  drivers/ide/ide-tape.c |   35 ++++++++++++++++++++---------------
>  > >  1 files changed, 20 insertions(+), 15 deletions(-)
>  > >
>  > > diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>  > > index 02851b3..04c611e 100644
>  > > --- a/drivers/ide/ide-tape.c
>  > > +++ b/drivers/ide/ide-tape.c
>  > > @@ -1910,13 +1910,15 @@ static void idetape_create_test_unit_ready_cmd(struct ide_atapi_pc *pc)
>  > >  static int __idetape_queue_pc_tail(ide_drive_t *drive, struct ide_atapi_pc *pc)
>  > >  {
>  > >     struct ide_tape_obj *tape = drive->driver_data;
>  > > -   struct request rq;
>  > > +   struct request *rq;
>  > >     int ret;
>  > >
>  > > -   idetape_init_rq(&rq, REQ_IDETAPE_PC1);
>  > > -   rq.buffer = (char *) pc;
>  > > -   rq.rq_disk = tape->disk;
>  > > -   ret = ide_do_drive_cmd(drive, &rq, ide_wait);
>  > > +   rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
>  > > +   rq->cmd_type = REQ_TYPE_SPECIAL;
>  > > +   rq->cmd[0] = REQ_IDETAPE_PC1;
>  > > +   rq->buffer = (char *)pc;
>  > > +   rq->rq_disk = tape->disk;
>  > > +   ret = ide_do_drive_cmd(drive, rq, ide_wait);
>  > >     return ret ? -EIO : 0;
>  > >
>  > >  }
>  > > @@ -2130,7 +2132,8 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
>  > >                              struct idetape_bh *bh)
>  > >  {
>  > >     idetape_tape_t *tape = drive->driver_data;
>  > > -   struct request rq;
>  > > +   struct request *rq;
>  > > +   int ret;
>  > >
>  > >     debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
>  > >
>  > > @@ -2140,22 +2143,24 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
>  > >             return (0);
>  > >     }
>  > >
>  > > -   idetape_init_rq(&rq, cmd);
>  > > -   rq.rq_disk = tape->disk;
>  > > -   rq.special = (void *)bh;
>  > > -   rq.sector = tape->first_frame;
>  > > -   rq.nr_sectors           = blocks;
>  > > -   rq.current_nr_sectors   = blocks;
>  > > -   (void) ide_do_drive_cmd(drive, &rq, ide_wait);
>  > > +   rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
>  > > +   rq->cmd_type = REQ_TYPE_SPECIAL;
>  > > +   rq->cmd[0] = cmd;
>  > > +   rq->rq_disk = tape->disk;
>  > > +   rq->special = (void *)bh;
>  > > +   rq->sector = tape->first_frame;
>  > > +   rq->nr_sectors = blocks;
>  > > +   rq->current_nr_sectors = blocks;
>  > > +   ret = ide_do_drive_cmd(drive, rq, ide_wait);
>  > >
>  > >     if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
>  > >             return 0;
>  > >
>  > >     if (tape->merge_stage)
>  > >             idetape_init_merge_stage(tape);
>  > > -   if (rq.errors == IDETAPE_ERROR_GENERAL)
>  > > +   if (ret == IDETAPE_ERROR_GENERAL)
>  > >             return -EIO;
>  > > -   return (tape->blk_size * (blocks-rq.current_nr_sectors));
>  > > +   return (tape->blk_size * (blocks-rq->current_nr_sectors));
>  > >  }
>  > >
>  > >  /* start servicing the pipeline stages, starting from tape->next_stage. */
>  >
>  > are you sure you're patching against the right ide tree? See, we removed
>  > pipelining (patches went in around the beginning of April) and the above comment
>  > kinda says the opposite. Just to make sure, the latest quilt tree is at
>  > http://www.kernel.org/pub/linux/kernel/people/bart/pata-2.6/patches/.
>
>  The patchset is against Bart's git tree:
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/bart/ide-2.6.git
>
>
>  Doesn't his git tree include the latest ide code?
>

No, this is the git tree that Bart sends to Linus to merge upstream. You should
get the latest patches from the URL i gave you before and quilt push them on
your git tree. That's the way i do it.

-- 
Regards/Gruß,
Boris

Re: [PATCH 06/11] ide: make ide_do_drive_cmd return rq->errors

[Boris Petkov]

On Wed, Apr 23, 2008 at 10:25 AM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Wed, 23 Apr 2008 09:32:22 +0200
>  Borislav Petkov <petkovbb@googlemail.com> wrote:
>
>  > Hi,
>  >
>  > On Tue, Apr 22, 2008 at 09:26:37AM +0900, FUJITA Tomonori wrote:
>  > > ide_do_drive_cmd forges an error value (-EIO) instead of
>  > > rq->errors. idetape_queue_rw_tail wants rq->errors so this patch makes
>  > > ide_do_drive_cmd return rq->errors.
>  > >
>  > > For compatibility, this patch makes the users of ide_do_drive_cmd
>  > > return -EIO instead of a return value of ide_do_drive_cmd
>  > > (rq->errors).
>  >
>  > i don't see the reason for it. In the end, the only error type that is being
>  > handed to and fro is -EIO and nobody is interested in rq->errors. So, whether
>  > ide_do_drive_cmd or its callers return -EIO is kinda unimportant. In the second
>  > case, however, you have simply added more code (as per the diffstat below) with
>  > no apparent functionality. It would only make sense, IMHO, if you would
>  > differentiate behaviour based on rq->errors...
>
>  I did this only because idetape_queue_rw_tail seems to have
>  differentiate behaviour based on rq->errors:
>
>
>  static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
>                                  struct idetape_bh *bh)
>  {
>  ...
>
>         (void) ide_do_drive_cmd(drive, &rq, ide_wait);
>
>         if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
>                 return 0;
>
>         if (tape->merge_stage)
>                 idetape_init_merge_stage(tape);
>         if (rq.errors == IDETAPE_ERROR_GENERAL)
>                 return -EIO;
>
>
>  If we can do something like:
>
>
>         ret = ide_do_drive_cmd(drive, &rq, ide_wait);
>
>         if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
>                 return 0;
>
>         if (tape->merge_stage)
>                 idetape_init_merge_stage(tape);
>         if (ret)
>                 return -EIO;
>
>
>  Then yeah, we don't need this patch.
>

Well, AFAICS, all calls to idetape_queue_rw_tail simply check whether its return
value is negative, and, if so, discontinue further command queueing. So, i think
we're OK if we simply return -EIO. Bart?

-- 
Regards/Gruß,
Boris

Re: [PATCH 00/11] removing the on-stack struct request

[Boris Petkov]

On Wed, Apr 23, 2008 at 10:27 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
>
> On Wed, Apr 23 2008, FUJITA Tomonori wrote:
>  > On Wed, 23 Apr 2008 09:40:11 +0200
>  > Borislav Petkov <petkovbb@googlemail.com> wrote:
>  >
>  > > On Tue, Apr 22, 2008 at 11:14:28AM +0200, Bartlomiej Zolnierkiewicz wrote:
>  > >
>  > > [..]
>  > >
>  > > > Looks promising but probably I'll not be able to review it properly
>  > > > this week (I'm busy with my real job) and the next week (I'll be busy
>  > > > with pushing overdue IDE updates).  OTOH I'm hoping that Borislav
>  > > > would be able to take a look in the meantime.
>  > >
>  > > Yep, they're quite straight-forward and look fine except some nitpicking i sent
>  > > in separate mails.
>  >
>  > Thanks for reviewing,
>  >
>  > Yeah, these patches are pretty straightforward mainly because they are
>  > against the paths where we can use __GFP_WAIT.
>  >
>  >
>  > > FWIW, this is what we wanted to do initially but Jens didn't
>  > > agree to that at the time. I guess, it's because i probably didn't express
>  > > myself clearly enough then. See http://kerneltrap.org/mailarchive/linux-kernel/2008/3/1/1037974
>  > > for the whole discussion.
>  >
>  > I think that IDE uses request struct from interrupt context at some
>  > places. In such places, I don't think that converting requests on the
>  > stack and a pre-allocated fixed number of requests to
>  > kmalloc/blk_get_request with GFP_ATOMIC is an good idea. It's better
>  > to remove such code (allocating requests from interrupt context).

Thanks for clarifying that, i've overlooked that case.

-- 
Regards/Gruß,
Boris