[ 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;
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
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.
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
>
>
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
Hello Tejun,
On Thu, 10 Feb 2005 09:01:58 +0900, Tejun Heo <[email protected]> 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
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
On Thu, 10 Feb 2005 09:52:25 +0900, Tejun Heo <[email protected]> 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
Bartlomiej Zolnierkiewicz wrote:
> On Thu, 10 Feb 2005 09:52:25 +0900, Tejun Heo <[email protected]> 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