[rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Bartlomiej Zolnierkiewicz]

[ against ide-dev-2.6 tree, boot tested on LBA48 drive ]

This small patch fixes unneeded writes/reads to LBA48 taskfile registers
on LBA48 capable disks for following cases:

* Power Management requests
  (WIN_FLUSH_CACHE[_EXT], WIN_STANDBYNOW1, WIN_IDLEIMMEDIATE commands)
* special commands (WIN_SPECIFY, WIN_RESTORE, WIN_SETMULT)
* Host Protected Area support (WIN_READ_NATIVE_MAX, WIN_SET_MAX)
* /proc/ide/ SMART support (WIN_SMART with SMART_ENABLE,
  SMART_READ_VALUES and SMART_READ_THRESHOLDS subcommands)
* write cache enabling/disabling in ide-disk
  (WIN_SETFEATURES with SETFEATURES_{EN,DIS}_WCACHE)
* write cache flushing in ide-disk (WIN_FLUSH_CACHE[_EXT])
* acoustic management in ide-disk
  (WIN_SETFEATURES with SETFEATURES_{EN,DIS}_AAM)
* door (un)locking in ide-disk (WIN_DOORLOCK, WIN_DOORUNLOCK)
* /proc/ide/hd?/identify support (WIN_IDENTIFY)

Patch adds 'unsinged long flags' to ide_task_t and uses ATA_TFLAG_LBA48
flag (from <linux/ata.h>) to indicate need of accessing LBA48 taskfile
registers.

diff -Nru a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c	2005-02-06 23:47:44 +01:00
+++ b/drivers/ide/ide-disk.c	2005-02-06 23:47:44 +01:00
@@ -362,6 +362,9 @@
 	args.tfRegister[IDE_COMMAND_OFFSET]	= WIN_READ_NATIVE_MAX_EXT;
 	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
 	args.handler				= &task_no_data_intr;
+
+	args.flags |= ATA_TFLAG_LBA48;
+
         /* submit command request */
         ide_raw_taskfile(drive, &args, NULL);

@@ -431,6 +434,9 @@
 	args.hobRegister[IDE_CONTROL_OFFSET_HOB]= (drive->ctl|0x80);
 	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
 	args.handler				= &task_no_data_intr;
+
+	args.flags |= ATA_TFLAG_LBA48;
+
 	/* submit command request */
 	ide_raw_taskfile(drive, &args, NULL);
 	/* if OK, compute maximum address value */
diff -Nru a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c	2005-02-06 23:47:44 +01:00
+++ b/drivers/ide/ide-io.c	2005-02-06 23:47:44 +01:00
@@ -487,7 +487,7 @@
 			args->tfRegister[IDE_SELECT_OFFSET]  = hwif->INB(IDE_SELECT_REG);
 			args->tfRegister[IDE_STATUS_OFFSET]  = stat;

-			if (drive->addressing == 1) {
+			if (args->flags & ATA_TFLAG_LBA48) {
 				hwif->OUTB(drive->ctl|0x80, IDE_CONTROL_REG);
 				args->hobRegister[IDE_FEATURE_OFFSET]	= hwif->INB(IDE_FEATURE_REG);
 				args->hobRegister[IDE_NSECTOR_OFFSET]	= hwif->INB(IDE_NSECTOR_REG);
diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c	2005-02-06 23:47:44 +01:00
+++ b/drivers/ide/ide-taskfile.c	2005-02-06 23:47:44 +01:00
@@ -101,7 +101,7 @@
 	ide_hwif_t *hwif	= HWIF(drive);
 	task_struct_t *taskfile	= (task_struct_t *) task->tfRegister;
 	hob_struct_t *hobfile	= (hob_struct_t *) task->hobRegister;
-	u8 HIHI			= (drive->addressing == 1) ? 0xE0 : 0xEF;
+	u8 HIHI			= (task->flags & ATA_TFLAG_LBA48) ? 0xE0 : 0xEF;

 	/* ALL Command Block Executions SHALL clear nIEN, unless otherwise */
 	if (IDE_CONTROL_REG) {
@@ -110,7 +110,7 @@
 	}
 	SELECT_MASK(drive, 0);

-	if (drive->addressing == 1) {
+	if (task->flags & ATA_TFLAG_LBA48) {
 		hwif->OUTB(hobfile->feature, IDE_FEATURE_REG);
 		hwif->OUTB(hobfile->sector_count, IDE_NSECTOR_REG);
 		hwif->OUTB(hobfile->sector_number, IDE_SECTOR_REG);
@@ -577,6 +577,9 @@
 	args.tf_out_flags = req_task->out_flags;
 	args.data_phase   = req_task->data_phase;
 	args.command_type = req_task->req_cmd;
+
+	if (drive->addressing == 1)
+		args.flags |= ATA_TFLAG_LBA48;

 	drive->io_32bit = 0;
 	switch(req_task->data_phase) {
diff -Nru a/include/linux/ide.h b/include/linux/ide.h
--- a/include/linux/ide.h	2005-02-06 23:47:44 +01:00
+++ b/include/linux/ide.h	2005-02-06 23:47:44 +01:00
@@ -1258,6 +1258,7 @@
  *	struct hd_drive_hob_hdr		hobf;
  *	hob_struct_t		hobf;
  */
+	unsigned long		flags;		/* ATA_TFLAG_xxx */
 	task_ioreg_t		tfRegister[8];
 	task_ioreg_t		hobRegister[8];
 	ide_reg_valid_t		tf_out_flags;

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Tejun Heo]

Hello, Bartlomiej.

Bartlomiej Zolnierkiewicz wrote:
> [ against ide-dev-2.6 tree, boot tested on LBA48 drive ]
> 
> This small patch fixes unneeded writes/reads to LBA48 taskfile registers
> on LBA48 capable disks for following cases:
> 
> * Power Management requests
>   (WIN_FLUSH_CACHE[_EXT], WIN_STANDBYNOW1, WIN_IDLEIMMEDIATE commands)
> * special commands (WIN_SPECIFY, WIN_RESTORE, WIN_SETMULT)
> * Host Protected Area support (WIN_READ_NATIVE_MAX, WIN_SET_MAX)
> * /proc/ide/ SMART support (WIN_SMART with SMART_ENABLE,
>   SMART_READ_VALUES and SMART_READ_THRESHOLDS subcommands)
> * write cache enabling/disabling in ide-disk
>   (WIN_SETFEATURES with SETFEATURES_{EN,DIS}_WCACHE)
> * write cache flushing in ide-disk (WIN_FLUSH_CACHE[_EXT])
> * acoustic management in ide-disk
>   (WIN_SETFEATURES with SETFEATURES_{EN,DIS}_AAM)
> * door (un)locking in ide-disk (WIN_DOORLOCK, WIN_DOORUNLOCK)
> * /proc/ide/hd?/identify support (WIN_IDENTIFY)
> 
> Patch adds 'unsinged long flags' to ide_task_t and uses ATA_TFLAG_LBA48
> flag (from <linux/ata.h>) to indicate need of accessing LBA48 taskfile
> registers.

  ide_task_t already has fields to enable/disable writing/reading of 
taskfile and hob registers (tf_in_flags and tf_out_flags).  How about 
adding the following two helpers.

static inline ide_init_task[_std](ide_task_t *task)
{
	memset(task, 0, sizeof(*task));
	task->tf_out_flags = IDE_TASKFILE_STD_OUT_FLAGS;
	task->tf_in_flags = IDE_TASKFILE_STD_IN_FLAGS;
}

static inline ide_init_task_lba48(ide_task_t *task)
{
	memset(task, 0, sizeof(*task));
	task->tf_out_flags = IDE_TASKFILE_STD_OUT_FLAGS
			| (IDE_HOB_STD_OUT_FLAGS << 8);
	task->tf_in_flags = IDE_TASKFILE_STD_IN_FLAGS
			| (IDE_HOB_STD_IN_FLAGS << 8);
}

  And, when writing taskfile,

if (task->tf_out_flags & 0xf == IDE_TASKFILE_STD_OUT_FLAGS) {
	/* Write taskfile regs without testing flags */
} else {
	/* Flagged taskfile */
}

if (task->tf_out_flags & 0xf0 == IDE_TASKFILE_HOB_OUT_FLAGS) {
	/* Write hob regs without testing flags */
} else {
	/* Flagged hob */
}

  And, when reading taskfile back,

if (task->tf_in_flags & 0xf0) {
	/* Read all hob regs */
}

/* Read all taskfile regs */

  And, we need to check if any of in/out flags is zero in ioctl 
functions and set STD flags appropriately.  So, basically, in kernel, 
tf_{in|out}_flags == 0 now means 0, not default.

  By doing like above,

  1. Virtually the same effect
  2. No need to add new field to ide_task_t
  3. Cleaner semantics (ATA_TFLAG_LBA48 and tf_{in|out}_flags carry
     duplicate information.)

  Thanks.

-- 
tejun

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Jeff Garzik]

FWIW, there are two limitations of libata in this area:

1) ISTR some PATA vendor-specific commands have a very specific set of 
input and output registers to use, and input/output sets of registers 
may differ from each other.

2) libata is lazy, and just reads registers in "groups":  the 
lbaH/M/L/nsect registers, the device register, etc.

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Bartlomiej Zolnierkiewicz]

On Monday 07 February 2005 05:47, Tejun Heo wrote:
> Hello, Bartlomiej.
> 
> Bartlomiej Zolnierkiewicz wrote:
> > [ against ide-dev-2.6 tree, boot tested on LBA48 drive ]
> > 
> > This small patch fixes unneeded writes/reads to LBA48 taskfile registers
> > on LBA48 capable disks for following cases:
> > 
> > * Power Management requests
> >   (WIN_FLUSH_CACHE[_EXT], WIN_STANDBYNOW1, WIN_IDLEIMMEDIATE commands)
> > * special commands (WIN_SPECIFY, WIN_RESTORE, WIN_SETMULT)
> > * Host Protected Area support (WIN_READ_NATIVE_MAX, WIN_SET_MAX)
> > * /proc/ide/ SMART support (WIN_SMART with SMART_ENABLE,
> >   SMART_READ_VALUES and SMART_READ_THRESHOLDS subcommands)
> > * write cache enabling/disabling in ide-disk
> >   (WIN_SETFEATURES with SETFEATURES_{EN,DIS}_WCACHE)
> > * write cache flushing in ide-disk (WIN_FLUSH_CACHE[_EXT])
> > * acoustic management in ide-disk
> >   (WIN_SETFEATURES with SETFEATURES_{EN,DIS}_AAM)
> > * door (un)locking in ide-disk (WIN_DOORLOCK, WIN_DOORUNLOCK)
> > * /proc/ide/hd?/identify support (WIN_IDENTIFY)
> > 
> > Patch adds 'unsinged long flags' to ide_task_t and uses ATA_TFLAG_LBA48
> > flag (from <linux/ata.h>) to indicate need of accessing LBA48 taskfile
> > registers.
> 
>   ide_task_t already has fields to enable/disable writing/reading of 
> taskfile and hob registers (tf_in_flags and tf_out_flags).  How about 
> adding the following two helpers.
> 
> static inline ide_init_task[_std](ide_task_t *task)
> {
>  memset(task, 0, sizeof(*task));
>  task->tf_out_flags = IDE_TASKFILE_STD_OUT_FLAGS;
>  task->tf_in_flags = IDE_TASKFILE_STD_IN_FLAGS;
> }
> 
> static inline ide_init_task_lba48(ide_task_t *task)
> {
>  memset(task, 0, sizeof(*task));
>  task->tf_out_flags = IDE_TASKFILE_STD_OUT_FLAGS
>    | (IDE_HOB_STD_OUT_FLAGS << 8);
>  task->tf_in_flags = IDE_TASKFILE_STD_IN_FLAGS
>    | (IDE_HOB_STD_IN_FLAGS << 8);
> }
> 
>   And, when writing taskfile,
> 
> if (task->tf_out_flags & 0xf == IDE_TASKFILE_STD_OUT_FLAGS) {
>  /* Write taskfile regs without testing flags */
> } else {
>  /* Flagged taskfile */
> }
> 
> if (task->tf_out_flags & 0xf0 == IDE_TASKFILE_HOB_OUT_FLAGS) {
>  /* Write hob regs without testing flags */
> } else {
>  /* Flagged hob */
> }
> 
>   And, when reading taskfile back,
> 
> if (task->tf_in_flags & 0xf0) {
>  /* Read all hob regs */
> }
> 
> /* Read all taskfile regs */
> 
>   And, we need to check if any of in/out flags is zero in ioctl 
> functions and set STD flags appropriately.  So, basically, in kernel, 
> tf_{in|out}_flags == 0 now means 0, not default.
> 
>   By doing like above,
> 
>   1. Virtually the same effect
>   2. No need to add new field to ide_task_t
>   3. Cleaner semantics (ATA_TFLAG_LBA48 and tf_{in|out}_flags carry
>      duplicate information.)

I would prefer to not teach do_rw_taskfile() about ->tf_{in,out}_flags
(and convert all users to use helpers) - it is much simpler this way,

->flags field in ide_task_t is needed anyway (32-bit I/O flag).

>   Thanks.
> 
> -- 
> tejun
> 
> 

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Tejun Heo]

  Hello, Bartlomiej.  Happy new lunar year.

Bartlomiej Zolnierkiewicz wrote:
> 
> I would prefer to not teach do_rw_taskfile() about ->tf_{in,out}_flags
> (and convert all users to use helpers) - it is much simpler this way,
> 
> ->flags field in ide_task_t is needed anyway (32-bit I/O flag).
> 

  New lunar year day is one of the biggest holidays here, so I haven't 
got time to work for a few days.  As it's over now, I began to work on 
ide drivers again.  I applied your task->flags patch and am moving my 
patches over it.

  One problem is that, with ATA_TFLAG_LBA48, whether to use HOB 
registers or not cannot be determined separately for writing and 
reading.  So, when initializing flush tasks, if WIN_FLUSH_CACHE_EXT is 
used, we need to turn on ATA_TFLAG_LBA48 to read error location 
properly, and we end up unnecessarily writing HOB registers.

  I think we can...

  1. Just leave it as it is.  It's not that big a deal.
  2. Use another flag(s) to control LBA48 reading/writing separately.
  3. do my proposal. :-)

  I'm currently sticking to #1.  Please let me know what you think.

  Thanks.

-- 
tejun

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Bartlomiej Zolnierkiewicz]

Hello Tejun,

On Thu, 10 Feb 2005 09:01:58 +0900, Tejun Heo <htejun@gmail.com> wrote:
>   Hello, Bartlomiej.  Happy new lunar year.
> 
> Bartlomiej Zolnierkiewicz wrote:
> >
> > I would prefer to not teach do_rw_taskfile() about ->tf_{in,out}_flags
> > (and convert all users to use helpers) - it is much simpler this way,
> >
> > ->flags field in ide_task_t is needed anyway (32-bit I/O flag).
> >
> 
>   New lunar year day is one of the biggest holidays here, so I haven't
> got time to work for a few days.  As it's over now, I began to work on
> ide drivers again.  I applied your task->flags patch and am moving my
> patches over it.
> 
>   One problem is that, with ATA_TFLAG_LBA48, whether to use HOB
> registers or not cannot be determined separately for writing and
> reading.  So, when initializing flush tasks, if WIN_FLUSH_CACHE_EXT is
> used, we need to turn on ATA_TFLAG_LBA48 to read error location
> properly, and we end up unnecessarily writing HOB registers.

Yep, good catch.

>   I think we can...
> 
>   1. Just leave it as it is.  It's not that big a deal.
>   2. Use another flag(s) to control LBA48 reading/writing separately.
>   3. do my proposal. :-)
> 
>   I'm currently sticking to #1.  Please let me know what you think.

agreed, #1 is a good choice, it is not that important to make things
more complicated

Thanks,
Bartlomiej

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Tejun Heo]

  Okay, another quick question.

  To fix the io_32bit race problem in ide_taskfile_ioctl() (and later 
ide_cmd_ioctl() too), it seems simplest to mark the taskfile with 
something like ATA_TFLAG_IO_16BIT flag and use the flag in task_in_intr().

  However, ATA_TFLAG_* are used by libata, and I think that, although 
sharing hardware constants is good if the hardware is similar, sharing 
driver-specific flags isn't such a good idea.  So, what do you think?

  1. Add ATA_TFLAG_IO_16BIT to ata.h
  2. Make ide's own set of task flags, maybe IDE_TFLAG_* (including
     IDE_TFLAG_LBA48)

-- 
tejun

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Bartlomiej Zolnierkiewicz]

On Thu, 10 Feb 2005 09:52:25 +0900, Tejun Heo <htejun@gmail.com> wrote:
> 
>   Okay, another quick question.
> 
>   To fix the io_32bit race problem in ide_taskfile_ioctl() (and later
> ide_cmd_ioctl() too), it seems simplest to mark the taskfile with
> something like ATA_TFLAG_IO_16BIT flag and use the flag in task_in_intr().
> 
>   However, ATA_TFLAG_* are used by libata, and I think that, although
> sharing hardware constants is good if the hardware is similar, sharing
> driver-specific flags isn't such a good idea.  So, what do you think?
> 
>   1. Add ATA_TFLAG_IO_16BIT to ata.h
>   2. Make ide's own set of task flags, maybe IDE_TFLAG_* (including
>      IDE_TFLAG_LBA48)

Please add it to <ata.h> (unless Jeff complains loudly),
I have a patch converting ide_task_t to use struct ata_taskfile
(except ->protocol for now).

Bartlomiej

Re: [rfc][patch] ide: fix unneeded LBA48 taskfile registers access

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Thu, 10 Feb 2005 09:52:25 +0900, Tejun Heo <htejun@gmail.com> wrote:
> 
>>  Okay, another quick question.
>>
>>  To fix the io_32bit race problem in ide_taskfile_ioctl() (and later
>>ide_cmd_ioctl() too), it seems simplest to mark the taskfile with
>>something like ATA_TFLAG_IO_16BIT flag and use the flag in task_in_intr().
>>
>>  However, ATA_TFLAG_* are used by libata, and I think that, although
>>sharing hardware constants is good if the hardware is similar, sharing
>>driver-specific flags isn't such a good idea.  So, what do you think?
>>
>>  1. Add ATA_TFLAG_IO_16BIT to ata.h
>>  2. Make ide's own set of task flags, maybe IDE_TFLAG_* (including
>>     IDE_TFLAG_LBA48)
> 
> 
> Please add it to <ata.h> (unless Jeff complains loudly),
> I have a patch converting ide_task_t to use struct ata_taskfile
> (except ->protocol for now).

ACK