[PATCH 2.6.11-rc2] ide: driver updates (phase 2)

[PATCH 2.6.11-rc2] ide: driver updates (phase 2)

[Tejun Heo]

 Hello, Bartlomiej.

 These are reordered/modified/(hopefully)appliable nine patches from
the previous series of patches.  #01 is the only new one.  It kills
the unused pkt_task_t in ide.h.  #02/#03 are moved upward and #04 is
modified as you've requested.  #05/#08 now directly use taskfile
transport instead of calling ide_taskfile_ioctl().  #08 now also
properly handles WIN_SMART case.

 If these patches go in, I have eight patches left.  One of them is
ide_explicit_TASKFILE_NO_DATA.  As we've discussed, I'm going to add
default initialization of task->[pre]handler.  That patch wasn't
included in this series because I didn't know if do_rw_taskfile() and
flagged_taskfile() are going to be merged or not.  So, please tell
me what you think. :-)

 I'll talk about other patches in their reply threads.


[ Start of patch descriptions ]

01_ide_kill_pkt_task_t.patch
	: kill unused pkt_task_t

	Remove unused pkt_task_t definition from ide.h.

02_ide_taskfile_init_drive_cmd.patch
	: ide_init_drive_cmd() now defaults to REQ_DRIVE_TASKFILE

	ide_init_drive_cmd() now initializes rq->flags to
	REQ_DRIVE_TASKFILE.

03_ide_diag_taskfile_use_init_drive_cmd.patch
	: ide_diag_taskfile() rq initialization fix

	In ide_diag_taskfile(), when initializing taskfile rq,
	ref_count wasn't initialized properly.  Modified to use
	ide_init_drive_cmd().  This doesn't really change any behavior
	as the request isn't managed via the block layer.

04_ide_taskfile_flush.patch
	: convert REQ_DRIVE_TASK to REQ_DRIVE_TASKFILE

	All REQ_DRIVE_TASK users except ide_task_ioctl() converted
	to use REQ_DRIVE_TASKFILE.
	1. idedisk_issue_flush() converted to use REQ_DRIVE_TASKFILE.
	   This and the changes in ide_get_error_location() remove a
	   possible race condition between ide_get_error_location()
	   and other requests.
	2. ide_queue_flush_cmd() converted to use REQ_DRIVE_TASKFILE.

05_ide_taskfile_task_ioctl.patch
	: map ide_task_ioctl() to ide_taskfile_ioctl()

	ide_task_ioctl() modified to map to ide_taskfile_ioctl().
	This is the last user of REQ_DRIVE_TASK.

06_ide_remove_task.patch
	: remove REQ_DRIVE_TASK handling

	Unused REQ_DRIVE_TASK handling removed.

07_ide_taskfile_cmd.patch
	: convert REQ_DRIVE_CMD to REQ_DRIVE_TASKFILE

	All in-kernel REQ_DRIVE_CMD users except for ide_cmd_ioctl()
	converted to use REQ_DRIVE_TASKFILE.

08_ide_taskfile_cmd_ioctl.patch
	: map ide_cmd_ioctl() to ide_taskfile_ioctl()

	ide_cmd_ioctl() converted to use ide_taskfile_ioctl().  This
	is the last user of REQ_DRIVE_CMD.

09_ide_remove_cmd.patch
	: remove REQ_DRIVE_CMD handling

	Removed unused REQ_DRIVE_CMD handling.

[ End of patch descriptions ]

Thanks.

-- 
tejun

Re: [PATCH 2.6.11-rc2] ide: driver updates (phase 2)

[Tejun Heo]

  Oops, I accidentally put ", ," in the To: line of all the mails which 
contain patches, and they don't show up on the lkml.  I'll post the 
patches again.

  Bartlomiej, if you got the first set of patches, please ignore them. 
The contents are identical, so if you've already reviewed them, you 
don't have to verify the new set.  I'm really sorry.  As I've been 
making quite a few mistakes during mailing patches manually, I've 
written a script to do it, but I still manage to make mistakes. :-(

-- 
tejun

Re: [PATCH 2.6.11-rc2] ide: driver updates (phase 2)

[Tejun Heo]

Tejun Heo wrote:
> 
>  Oops, I accidentally put ", ," in the To: line of all the mails which 
> contain patches, and they don't show up on the lkml.  I'll post the 
> patches again.
> 
>  Bartlomiej, if you got the first set of patches, please ignore them. 
> The contents are identical, so if you've already reviewed them, you 
> don't have to verify the new set.  I'm really sorry.  As I've been 
> making quite a few mistakes during mailing patches manually, I've 
> written a script to do it, but I still manage to make mistakes. :-(
> 

  I resent the patches and the same thing happened.  I digged the 
problem and found out that my script was faulty.  The reason was that 
the mail program didn't handle commas appropriately in to-addr list. 
Bartlomiej, I sincerely apologize for polluting your mailbox yet again. 
  Please forgive me, it won't happen again (really!). :-)

  I'll repost the patches in another thread.  Sorry again about the mess.

-- 
tejun

Re: [PATCH 2.6.11-rc2 04/09] ide: convert REQ_DRIVE_TASK to REQ_DRIVE_TASKFILE

[Tejun Heo]

04_ide_taskfile_flush.patch

	All REQ_DRIVE_TASK users except ide_task_ioctl() converted
	to use REQ_DRIVE_TASKFILE.
	1. idedisk_issue_flush() converted to use REQ_DRIVE_TASKFILE.
	   This and the changes in ide_get_error_location() remove a
	   possible race condition between ide_get_error_location()
	   and other requests.
	2. ide_queue_flush_cmd() converted to use REQ_DRIVE_TASKFILE.


Signed-off-by: Tejun Heo <tj@home-tj.org>


Index: linux-ide-series2-export/drivers/ide/ide-disk.c
===================================================================
--- linux-ide-series2-export.orig/drivers/ide/ide-disk.c	2005-02-05 19:26:51.166394831 +0900
+++ linux-ide-series2-export/drivers/ide/ide-disk.c	2005-02-05 19:27:08.735540988 +0900
@@ -705,24 +705,17 @@ static int idedisk_issue_flush(request_q
 {
 	ide_drive_t *drive = q->queuedata;
 	struct request *rq;
+	ide_task_t args;
 	int ret;
 
 	if (!drive->wcache)
 		return 0;
 
-	rq = blk_get_request(q, WRITE, __GFP_WAIT);
-
-	memset(rq->cmd, 0, sizeof(rq->cmd));
+	ide_init_flush_task(drive, &args);
 
-	if (ide_id_has_flush_cache_ext(drive->id) &&
-	    (drive->capacity64 >= (1UL << 28)))
-		rq->cmd[0] = WIN_FLUSH_CACHE_EXT;
-	else
-		rq->cmd[0] = WIN_FLUSH_CACHE;
-
-
-	rq->flags |= REQ_DRIVE_TASK | REQ_SOFTBARRIER;
-	rq->buffer = rq->cmd;
+	rq = blk_get_request(q, WRITE, __GFP_WAIT);
+	rq->flags |= REQ_DRIVE_TASKFILE | REQ_SOFTBARRIER;
+	rq->special = &args;
 
 	ret = blk_execute_rq(q, disk, rq);
 
@@ -730,8 +723,9 @@ static int idedisk_issue_flush(request_q
 	 * if we failed and caller wants error offset, get it
 	 */
 	if (ret && error_sector)
-		*error_sector = ide_get_error_location(drive, rq->cmd);
+		*error_sector = ide_get_error_location(drive, &args);
 
+	rq->special = NULL;	/* just in case */
 	blk_put_request(rq);
 	return ret;
 }
Index: linux-ide-series2-export/drivers/ide/ide-io.c
===================================================================
--- linux-ide-series2-export.orig/drivers/ide/ide-io.c	2005-02-05 19:27:08.308610336 +0900
+++ linux-ide-series2-export/drivers/ide/ide-io.c	2005-02-05 19:27:08.736540825 +0900
@@ -55,22 +55,19 @@
 #include <asm/io.h>
 #include <asm/bitops.h>
 
-static void ide_fill_flush_cmd(ide_drive_t *drive, struct request *rq)
+void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args)
 {
-	char *buf = rq->cmd;
-
-	/*
-	 * reuse cdb space for ata command
-	 */
-	memset(buf, 0, sizeof(rq->cmd));
-
-	rq->flags = REQ_DRIVE_TASK | REQ_STARTED;
-	rq->buffer = buf;
-	rq->buffer[0] = WIN_FLUSH_CACHE;
+	memset(args, 0, sizeof(*args));
 
 	if (ide_id_has_flush_cache_ext(drive->id) &&
 	    (drive->capacity64 >= (1UL << 28)))
-		rq->buffer[0] = WIN_FLUSH_CACHE_EXT;
+		args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
+	else
+		args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
+
+	args->command_type = IDE_DRIVE_TASK_NO_DATA;
+	args->data_phase = TASKFILE_NO_DATA;
+	args->handler = task_no_data_intr;
 }
 
 /*
@@ -80,7 +77,9 @@ static void ide_fill_flush_cmd(ide_drive
 static struct request *ide_queue_flush_cmd(ide_drive_t *drive,
 					   struct request *rq, int post)
 {
-	struct request *flush_rq = &HWGROUP(drive)->wrq;
+	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
+	struct request *flush_rq = &hwgroup->flush_rq;
+	ide_task_t *args = &hwgroup->flush_args;
 
 	/*
 	 * write cache disabled, clear the barrier bit and treat it like
@@ -92,10 +91,12 @@ static struct request *ide_queue_flush_c
 	}
 
 	ide_init_drive_cmd(flush_rq);
-	ide_fill_flush_cmd(drive, flush_rq);
-
-	flush_rq->special = rq;
+	flush_rq->flags |= REQ_STARTED;
 	flush_rq->nr_sectors = rq->nr_sectors;
+	flush_rq->special = args;
+	hwgroup->flush_real_rq = rq;
+
+	ide_init_flush_task(drive, args);
 
 	if (!post) {
 		drive->doing_barrier = 1;
@@ -105,7 +106,7 @@ static struct request *ide_queue_flush_c
 		flush_rq->flags |= REQ_BAR_POSTFLUSH;
 
 	__elv_add_request(drive->queue, flush_rq, ELEVATOR_INSERT_FRONT, 0);
-	HWGROUP(drive)->rq = NULL;
+	hwgroup->rq = NULL;
 	return flush_rq;
 }
 
@@ -162,12 +163,13 @@ static int __ide_end_request(ide_drive_t
 
 int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 {
+	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
 	struct request *rq;
 	unsigned long flags;
 	int ret = 1;
 
 	spin_lock_irqsave(&ide_lock, flags);
-	rq = HWGROUP(drive)->rq;
+	rq = hwgroup->rq;
 
 	if (!nr_sectors)
 		nr_sectors = rq->hard_cur_sectors;
@@ -175,7 +177,7 @@ int ide_end_request (ide_drive_t *drive,
 	if (!blk_barrier_rq(rq) || !drive->wcache)
 		ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
 	else {
-		struct request *flush_rq = &HWGROUP(drive)->wrq;
+		struct request *flush_rq = &hwgroup->flush_rq;
 
 		flush_rq->nr_sectors -= nr_sectors;
 		if (!flush_rq->nr_sectors) {
@@ -221,41 +223,37 @@ static void ide_complete_pm_request (ide
 /*
  * FIXME: probably move this somewhere else, name is bad too :)
  */
-u64 ide_get_error_location(ide_drive_t *drive, char *args)
+u64 ide_get_error_location(ide_drive_t *drive, ide_task_t *args)
 {
 	u32 high, low;
 	u8 hcyl, lcyl, sect;
-	u64 sector;
 
-	high = 0;
-	hcyl = args[5];
-	lcyl = args[4];
-	sect = args[3];
-
-	if (ide_id_has_flush_cache_ext(drive->id)) {
-		low = (hcyl << 16) | (lcyl << 8) | sect;
-		HWIF(drive)->OUTB(drive->ctl|0x80, IDE_CONTROL_REG);
-		high = ide_read_24(drive);
-	} else {
-		u8 cur = HWIF(drive)->INB(IDE_SELECT_REG);
-		if (cur & 0x40)
-			low = (hcyl << 16) | (lcyl << 8) | sect;
-		else {
-			low = hcyl * drive->head * drive->sect;
-			low += lcyl * drive->sect;
-			low += sect - 1;
-		}
-	}
+	if (ide_id_has_flush_cache_ext(drive->id) &&
+	    (drive->capacity64 >= (1UL << 28)))
+		high = (args->hobRegister[IDE_HCYL_OFFSET] << 16) |
+		       (args->hobRegister[IDE_LCYL_OFFSET] << 8 ) |
+			args->hobRegister[IDE_SECTOR_OFFSET];
+	else
+		high = 0;
+
+	hcyl = args->tfRegister[IDE_HCYL_OFFSET];
+	lcyl = args->tfRegister[IDE_LCYL_OFFSET];
+	sect = args->tfRegister[IDE_SECTOR_OFFSET];
+
+	if (args->tfRegister[IDE_SELECT_OFFSET] & 0x40)
+		low = (hcyl << 16) | (lcyl << 8 ) | sect;
+	else
+		low = hcyl * drive->head * drive->sect +
+			lcyl * drive->sect + sect - 1;
 
-	sector = ((u64) high << 24) | low;
-	return sector;
+	return ((u64)high << 24) | low;
 }
 EXPORT_SYMBOL(ide_get_error_location);
 
 static void ide_complete_barrier(ide_drive_t *drive, struct request *rq,
 				 int error)
 {
-	struct request *real_rq = rq->special;
+	struct request *real_rq = drive->hwif->hwgroup->flush_real_rq;
 	int good_sectors, bad_sectors;
 	sector_t sector;
 
@@ -302,7 +300,7 @@ static void ide_complete_barrier(ide_dri
 		 */
 		good_sectors = 0;
 		if (blk_barrier_postflush(rq)) {
-			sector = ide_get_error_location(drive, rq->buffer);
+			sector = ide_get_error_location(drive, rq->special);
 
 			if ((sector >= real_rq->hard_sector) &&
 			    (sector < real_rq->hard_sector + real_rq->hard_nr_sectors))
Index: linux-ide-series2-export/include/linux/ide.h
===================================================================
--- linux-ide-series2-export.orig/include/linux/ide.h	2005-02-05 19:27:08.104643467 +0900
+++ linux-ide-series2-export/include/linux/ide.h	2005-02-05 19:27:08.737540663 +0900
@@ -930,6 +930,25 @@ typedef ide_startstop_t (ide_pre_handler
 typedef ide_startstop_t (ide_handler_t)(ide_drive_t *);
 typedef int (ide_expiry_t)(ide_drive_t *);
 
+typedef struct ide_task_s {
+/*
+ *	struct hd_drive_task_hdr	tf;
+ *	task_struct_t		tf;
+ *	struct hd_drive_hob_hdr		hobf;
+ *	hob_struct_t		hobf;
+ */
+	task_ioreg_t		tfRegister[8];
+	task_ioreg_t		hobRegister[8];
+	ide_reg_valid_t		tf_out_flags;
+	ide_reg_valid_t		tf_in_flags;
+	int			data_phase;
+	int			command_type;
+	ide_pre_handler_t	*prehandler;
+	ide_handler_t		*handler;
+	struct request		*rq;		/* copy of request */
+	void			*special;	/* valid_t generally */
+} ide_task_t;
+
 typedef struct hwgroup_s {
 		/* irq handler, if active */
 	ide_startstop_t	(*handler)(ide_drive_t *);
@@ -955,8 +974,12 @@ typedef struct hwgroup_s {
 	struct request *rq;
 		/* failsafe timer */
 	struct timer_list timer;
-		/* local copy of current write rq */
-	struct request wrq;
+		/* rq used for flushing */
+	struct request flush_rq;
+		/* task used for flushing */
+	ide_task_t flush_args;
+		/* real_rq for flushing */
+	struct request *flush_real_rq;
 		/* timeout value during long polls */
 	unsigned long poll_timeout;
 		/* queried upon timeouts */
@@ -1201,9 +1224,14 @@ extern ide_startstop_t ide_do_reset (ide
 extern void ide_init_drive_cmd (struct request *rq);
 
 /*
+ * This function initializes @task to WIN_FLUSH_CACHE[_EXT] command.
+ */
+void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args);
+
+/*
  * this function returns error location sector offset in case of a write error
  */
-extern u64 ide_get_error_location(ide_drive_t *, char *);
+u64 ide_get_error_location(ide_drive_t *, ide_task_t *);
 
 /*
  * "action" parameter type for ide_do_drive_cmd() below.
@@ -1260,25 +1288,6 @@ extern void ide_end_drive_cmd(ide_drive_
  */
 extern int ide_wait_cmd(ide_drive_t *, u8, u8, u8, u8, u8 *);
 
-typedef struct ide_task_s {
-/*
- *	struct hd_drive_task_hdr	tf;
- *	task_struct_t		tf;
- *	struct hd_drive_hob_hdr		hobf;
- *	hob_struct_t		hobf;
- */
-	task_ioreg_t		tfRegister[8];
-	task_ioreg_t		hobRegister[8];
-	ide_reg_valid_t		tf_out_flags;
-	ide_reg_valid_t		tf_in_flags;
-	int			data_phase;
-	int			command_type;
-	ide_pre_handler_t	*prehandler;
-	ide_handler_t		*handler;
-	struct request		*rq;		/* copy of request */
-	void			*special;	/* valid_t generally */
-} ide_task_t;
-
 extern u32 ide_read_24(ide_drive_t *);
 
 extern void SELECT_DRIVE(ide_drive_t *);

Re: [PATCH 2.6.11-rc2 01/09] ide: kill unused pkt_task_t

[Bartlomiej Zolnierkiewicz]

applied, thanks

Re: [PATCH 2.6.11-rc2 04/09] ide: convert REQ_DRIVE_TASK to REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

I put some more thought into this change... details below...

On Sat,  5 Feb 2005 11:15:56 +0900 (KST), Tejun Heo <tj@home-tj.org> wrote:

> @@ -705,24 +705,17 @@ static int idedisk_issue_flush(request_q
>  {
>         ide_drive_t *drive = q->queuedata;
>         struct request *rq;
> +       ide_task_t args;
>         int ret;

ide_task_t task please

> @@ -730,8 +723,9 @@ static int idedisk_issue_flush(request_q
>          * if we failed and caller wants error offset, get it
>          */
>         if (ret && error_sector)
> -               *error_sector = ide_get_error_location(drive, rq->cmd);
> +               *error_sector = ide_get_error_location(drive, &args);
> 
> +       rq->special = NULL;     /* just in case */

In what case?

> @@ -55,22 +55,19 @@
>  #include <asm/io.h>
>  #include <asm/bitops.h>
> 
> -static void ide_fill_flush_cmd(ide_drive_t *drive, struct request *rq)
> +void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args)

ide_task_t *task

> @@ -80,7 +77,9 @@ static void ide_fill_flush_cmd(ide_drive
>  static struct request *ide_queue_flush_cmd(ide_drive_t *drive,
>                                            struct request *rq, int post)
>  {
> -       struct request *flush_rq = &HWGROUP(drive)->wrq;
> +       ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
> +       struct request *flush_rq = &hwgroup->flush_rq;
> +       ide_task_t *args = &hwgroup->flush_args;

ide_task_t *task

> @@ -221,41 +223,37 @@ static void ide_complete_pm_request (ide
>  /*
>   * FIXME: probably move this somewhere else, name is bad too :)
>   */
> -u64 ide_get_error_location(ide_drive_t *drive, char *args)
> +u64 ide_get_error_location(ide_drive_t *drive, ide_task_t *args)

ide_task_t *task

>  {
>         u32 high, low;
>         u8 hcyl, lcyl, sect;
> -       u64 sector;
> 
> -       high = 0;
> -       hcyl = args[5];
> -       lcyl = args[4];
> -       sect = args[3];
> -
> -       if (ide_id_has_flush_cache_ext(drive->id)) {
> -               low = (hcyl << 16) | (lcyl << 8) | sect;
> -               HWIF(drive)->OUTB(drive->ctl|0x80, IDE_CONTROL_REG);
> -               high = ide_read_24(drive);
> -       } else {
> -               u8 cur = HWIF(drive)->INB(IDE_SELECT_REG);
> -               if (cur & 0x40)
> -                       low = (hcyl << 16) | (lcyl << 8) | sect;
> -               else {
> -                       low = hcyl * drive->head * drive->sect;
> -                       low += lcyl * drive->sect;
> -                       low += sect - 1;
> -               }
> -       }
> +       if (ide_id_has_flush_cache_ext(drive->id) &&
> +           (drive->capacity64 >= (1UL << 28)))

Please just use if (drive->addressing), it is simpler and still correct.
Since we are now using ide_task_t 'high' will be 0 when
ide_id_has_flush_cache() == 0 and drive->addressing == 1
(such combination is unlikely but...).  Also thanks to this change
ide_get_error_location() becomes a really *generic* helper and
can be later used by other code.

> @@ -1201,9 +1224,14 @@ extern ide_startstop_t ide_do_reset (ide
>  extern void ide_init_drive_cmd (struct request *rq);
> 
>  /*
> + * This function initializes @task to WIN_FLUSH_CACHE[_EXT] command.
> + */
> +void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args);

comment is wrong and not needed,

void ide_init_flush_task(ide_drive_t *, ide_task_t *);

should be enough


There is one problem left with this change - FLUSH_CACHE_{EXT}
command handling becomes slower for drive's supporting LBA48
(also next patches make ide_{task,cmd}_ioctl() slower).  Why is so?
See do_rw_taskfile(), HOB registers are written/read unconditionally
if (drive->addressing == 1).  This can be fixed by i.e. adding
'unsigned long flags' to ide_task_t and IDE_TASK_LBA48 flag.
BTW this fix is needed also to implement LBA48 optimization for
read/write requests (not writing HOB registers when not needed).

IMHO there are some things worth mentioning in the patch description,
do_rw_taskfile() vs execute_drive_cmd()+ide_cmd() details:
some registers are written now in different order and timeout is bumped
(these changes shouldn't make any harm but I'm paranoid :).

Thanks,
Bartlomiej

Re: [PATCH 2.6.11-rc2 05/09] ide: map ide_task_ioctl() to ide_taskfile_ioctl()

[Bartlomiej Zolnierkiewicz]

On Sat,  5 Feb 2005 11:15:56 +0900 (KST), Tejun Heo <tj@home-tj.org> wrote:

> -       int err = 0;
> -       u8 args[7], *argbuf = args;
> -       int argsize = 7;
> +       u8 args[7];
> +       ide_task_t task;
> +       task_ioreg_t *regs = task.tfRegister;

u8 *regs please

> +       int ret;

What is the reason for changing the name of variable
(other than making the patch bigger ;) ?

Please also read comments to patch #4.

Thanks,
Bartlomiej

Re: [PATCH 2.6.11-rc2 08/09] ide: map ide_cmd_ioctl() to ide_taskfile_ioctl()

[Bartlomiej Zolnierkiewicz]

I have not fully reviewed it yet but I've noticed two things...

> @@ -648,11 +648,11 @@ u8 eighty_ninty_three (ide_drive_t *driv
> 
>  EXPORT_SYMBOL(eighty_ninty_three);
> 
> -int ide_ata66_check (ide_drive_t *drive, ide_task_t *args)
> +int ide_ata66_check(ide_drive_t *drive, task_ioreg_t *regs)
>  {
> -       if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
> -           (args->tfRegister[IDE_SECTOR_OFFSET] > XFER_UDMA_2) &&
> -           (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER)) {
> +       if (regs[IDE_COMMAND_OFFSET] == WIN_SETFEATURES &&
> +           regs[IDE_SECTOR_OFFSET] > XFER_UDMA_2 &&
> +           regs[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) {
>  #ifndef CONFIG_IDEDMA_IVB
>                 if ((drive->id->hw_config & 0x6000) == 0) {
>  #else /* !CONFIG_IDEDMA_IVB */

What is the rationale for this change?
ide_task_t is available in ide_cmd_ioctl().

> @@ -678,11 +678,11 @@ int ide_ata66_check (ide_drive_t *drive,
>   * 1 : Safe to update drive->id DMA registers.
>   * 0 : OOPs not allowed.
>   */
> -int set_transfer (ide_drive_t *drive, ide_task_t *args)
> +int set_transfer(ide_drive_t *drive, task_ioreg_t *regs)
>  {
> -       if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
> -           (args->tfRegister[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0) &&
> -           (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) &&
> +       if (regs[IDE_COMMAND_OFFSET] == WIN_SETFEATURES &&
> +           regs[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0 &&
> +           regs[IDE_FEATURE_OFFSET] == SETFEATURES_XFER &&
>             (drive->id->dma_ultra ||
>              drive->id->dma_mword ||
>              drive->id->dma_1word))

ditto

> +       io_32bit = drive->io_32bit;
> +       drive->io_32bit = 0;    /* racy */
> +       ret = ide_diag_taskfile(drive, &task, in_size, buf);
> +       drive->io_32bit = io_32bit;

It wasn't racy before this patch.  I know that it is like that
in ide_taskfile_ioctl() but it is not an excuse for propagating
the bug.  It can be easily fixed if you use drive_cmd_intr()
instead of task_in_intr() as task->handler.

Thanks,
Bartlomiej

Re: [PATCH 2.6.11-rc2 04/09] ide: convert REQ_DRIVE_TASK to REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

I forgot about this one...

> @@ -55,22 +55,19 @@
>  #include <asm/io.h>
>  #include <asm/bitops.h>
> 
> -static void ide_fill_flush_cmd(ide_drive_t *drive, struct request *rq)
> +void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args)
>  {
> -       char *buf = rq->cmd;
> -
> -       /*
> -        * reuse cdb space for ata command
> -        */
> -       memset(buf, 0, sizeof(rq->cmd));
> -
> -       rq->flags = REQ_DRIVE_TASK | REQ_STARTED;
> -       rq->buffer = buf;
> -       rq->buffer[0] = WIN_FLUSH_CACHE;
> +       memset(args, 0, sizeof(*args));
> 
>         if (ide_id_has_flush_cache_ext(drive->id) &&
>             (drive->capacity64 >= (1UL << 28)))
> -               rq->buffer[0] = WIN_FLUSH_CACHE_EXT;
> +               args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
> +       else
> +               args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
> +
> +       args->command_type = IDE_DRIVE_TASK_NO_DATA;
> +       args->data_phase = TASKFILE_NO_DATA;
> +       args->handler = task_no_data_intr;
>  }

Isn't EXPORT_SYMBOL_{GPL} needed for ide_init_flush_task()?

Re: [PATCH 2.6.11-rc2 08/09] ide: map ide_cmd_ioctl() to ide_taskfile_ioctl()

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> I have not fully reviewed it yet but I've noticed two things...
> 
> 
>>@@ -648,11 +648,11 @@ u8 eighty_ninty_three (ide_drive_t *driv
>>
>> EXPORT_SYMBOL(eighty_ninty_three);
>>
>>-int ide_ata66_check (ide_drive_t *drive, ide_task_t *args)
>>+int ide_ata66_check(ide_drive_t *drive, task_ioreg_t *regs)
>> {
>>-       if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
>>-           (args->tfRegister[IDE_SECTOR_OFFSET] > XFER_UDMA_2) &&
>>-           (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER)) {
>>+       if (regs[IDE_COMMAND_OFFSET] == WIN_SETFEATURES &&
>>+           regs[IDE_SECTOR_OFFSET] > XFER_UDMA_2 &&
>>+           regs[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) {
>> #ifndef CONFIG_IDEDMA_IVB
>>                if ((drive->id->hw_config & 0x6000) == 0) {
>> #else /* !CONFIG_IDEDMA_IVB */
> 
> 
> What is the rationale for this change?
> ide_task_t is available in ide_cmd_ioctl().
> 

  Sorry, it was for the previous map to ide_taskfile_ioctl() 
implementation, where ide_cmd_ioctl() didn't have ide_task_t.  I'll 
restore it.  Also, I've forgot to update the description of this patch. 
  I'll fix that too.

> 
>>@@ -678,11 +678,11 @@ int ide_ata66_check (ide_drive_t *drive,
>>  * 1 : Safe to update drive->id DMA registers.
>>  * 0 : OOPs not allowed.
>>  */
>>-int set_transfer (ide_drive_t *drive, ide_task_t *args)
>>+int set_transfer(ide_drive_t *drive, task_ioreg_t *regs)
>> {
>>-       if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
>>-           (args->tfRegister[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0) &&
>>-           (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) &&
>>+       if (regs[IDE_COMMAND_OFFSET] == WIN_SETFEATURES &&
>>+           regs[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0 &&
>>+           regs[IDE_FEATURE_OFFSET] == SETFEATURES_XFER &&
>>            (drive->id->dma_ultra ||
>>             drive->id->dma_mword ||
>>             drive->id->dma_1word))
> 
> 
> ditto
> 

  ditto. :-)

> 
>>+       io_32bit = drive->io_32bit;
>>+       drive->io_32bit = 0;    /* racy */
>>+       ret = ide_diag_taskfile(drive, &task, in_size, buf);
>>+       drive->io_32bit = io_32bit;
> 
> 
> It wasn't racy before this patch.  I know that it is like that
> in ide_taskfile_ioctl() but it is not an excuse for propagating
> the bug.  It can be easily fixed if you use drive_cmd_intr()
> instead of task_in_intr() as task->handler.

  Fixing the race condition in taskfile ioctl was on my to-do list, so I 
was thinking unifying the problem wouldn't be bad such that it can be 
fixed together later (soon).  I think I can make a patch to fix taskfile 
ioctl first and then submit this patch again without the race.

  Thanks.

-- 
tejun

Re: [PATCH 2.6.11-rc2 05/09] ide: map ide_task_ioctl() to ide_taskfile_ioctl()

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> On Sat,  5 Feb 2005 11:15:56 +0900 (KST), Tejun Heo <tj@home-tj.org> wrote:
> 
> 
>>-       int err = 0;
>>-       u8 args[7], *argbuf = args;
>>-       int argsize = 7;
>>+       u8 args[7];
>>+       ide_task_t task;
>>+       task_ioreg_t *regs = task.tfRegister;
> 
> 
> u8 *regs please
> 

  Sure.

> 
>>+       int ret;
> 
> 
> What is the reason for changing the name of variable
> (other than making the patch bigger ;) ?

  Heh, Heh, I thought I could get away with it.

  I'll restore it. :-)

> Please also read comments to patch #4.

  Thanks.

-- 
tejun

Re: [PATCH 2.6.11-rc2 04/09] ide: convert REQ_DRIVE_TASK to REQ_DRIVE_TASKFILE

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> I put some more thought into this change... details below...
> 
> On Sat,  5 Feb 2005 11:15:56 +0900 (KST), Tejun Heo <tj@home-tj.org> wrote:
> 
> 
>>@@ -705,24 +705,17 @@ static int idedisk_issue_flush(request_q
>> {
>>        ide_drive_t *drive = q->queuedata;
>>        struct request *rq;
>>+       ide_task_t args;
>>        int ret;
> 
> 
> ide_task_t task please
> 

  Okay.

> 
>>@@ -730,8 +723,9 @@ static int idedisk_issue_flush(request_q
>>         * if we failed and caller wants error offset, get it
>>         */
>>        if (ret && error_sector)
>>-               *error_sector = ide_get_error_location(drive, rq->cmd);
>>+               *error_sector = ide_get_error_location(drive, &args);
>>
>>+       rq->special = NULL;     /* just in case */
> 
> 
> In what case?
> 

  As the request is allocated and freed by the generic block layer, I 
was kinda worrying about cases where the request outlives 
blk_put_request() and somehow somebody accesses ->special.  Probably 
unnecessary but dangling pointers pointing stack area really scares me.

> 
>>@@ -55,22 +55,19 @@
>> #include <asm/io.h>
>> #include <asm/bitops.h>
>>
>>-static void ide_fill_flush_cmd(ide_drive_t *drive, struct request *rq)
>>+void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args)
> 
> 
> ide_task_t *task
> 
> 
>>@@ -80,7 +77,9 @@ static void ide_fill_flush_cmd(ide_drive
>> static struct request *ide_queue_flush_cmd(ide_drive_t *drive,
>>                                           struct request *rq, int post)
>> {
>>-       struct request *flush_rq = &HWGROUP(drive)->wrq;
>>+       ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
>>+       struct request *flush_rq = &hwgroup->flush_rq;
>>+       ide_task_t *args = &hwgroup->flush_args;
> 
> 
> ide_task_t *task
> 
> 
>>@@ -221,41 +223,37 @@ static void ide_complete_pm_request (ide
>> /*
>>  * FIXME: probably move this somewhere else, name is bad too :)
>>  */
>>-u64 ide_get_error_location(ide_drive_t *drive, char *args)
>>+u64 ide_get_error_location(ide_drive_t *drive, ide_task_t *args)
> 
> 
> ide_task_t *task
> 
> 
>> {
>>        u32 high, low;
>>        u8 hcyl, lcyl, sect;
>>-       u64 sector;
>>
>>-       high = 0;
>>-       hcyl = args[5];
>>-       lcyl = args[4];
>>-       sect = args[3];
>>-
>>-       if (ide_id_has_flush_cache_ext(drive->id)) {
>>-               low = (hcyl << 16) | (lcyl << 8) | sect;
>>-               HWIF(drive)->OUTB(drive->ctl|0x80, IDE_CONTROL_REG);
>>-               high = ide_read_24(drive);
>>-       } else {
>>-               u8 cur = HWIF(drive)->INB(IDE_SELECT_REG);
>>-               if (cur & 0x40)
>>-                       low = (hcyl << 16) | (lcyl << 8) | sect;
>>-               else {
>>-                       low = hcyl * drive->head * drive->sect;
>>-                       low += lcyl * drive->sect;
>>-                       low += sect - 1;
>>-               }
>>-       }
>>+       if (ide_id_has_flush_cache_ext(drive->id) &&
>>+           (drive->capacity64 >= (1UL << 28)))
> 
> 
> Please just use if (drive->addressing), it is simpler and still correct.
> Since we are now using ide_task_t 'high' will be 0 when
> ide_id_has_flush_cache() == 0 and drive->addressing == 1
> (such combination is unlikely but...).  Also thanks to this change
> ide_get_error_location() becomes a really *generic* helper and
> can be later used by other code.
> 

  Sure.

> 
>>@@ -1201,9 +1224,14 @@ extern ide_startstop_t ide_do_reset (ide
>> extern void ide_init_drive_cmd (struct request *rq);
>>
>> /*
>>+ * This function initializes @task to WIN_FLUSH_CACHE[_EXT] command.
>>+ */
>>+void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args);
> 
> 
> comment is wrong and not needed,
> 
> void ide_init_flush_task(ide_drive_t *, ide_task_t *);
> 
> should be enough
> 

  Okay.

> 
> There is one problem left with this change - FLUSH_CACHE_{EXT}
> command handling becomes slower for drive's supporting LBA48
> (also next patches make ide_{task,cmd}_ioctl() slower).  Why is so?
> See do_rw_taskfile(), HOB registers are written/read unconditionally
> if (drive->addressing == 1).  This can be fixed by i.e. adding
> 'unsigned long flags' to ide_task_t and IDE_TASK_LBA48 flag.
> BTW this fix is needed also to implement LBA48 optimization for
> read/write requests (not writing HOB registers when not needed).
> 
> IMHO there are some things worth mentioning in the patch description,
> do_rw_taskfile() vs execute_drive_cmd()+ide_cmd() details:
> some registers are written now in different order and timeout is bumped
> (these changes shouldn't make any harm but I'm paranoid :).
> 

  Yeah, sure.

  Thanks.

-- 
tejun