[patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

ide_task_ioctl() rewritten to use taskfile transport.
This is the last user of REQ_DRIVE_TASK.

bart: ported to recent IDE changes by me

Signed-off-by: Tejun Heo <htejun@gmail.com>

diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c	2005-02-24 15:30:02 +01:00
+++ b/drivers/ide/ide-taskfile.c	2005-02-24 15:30:02 +01:00
@@ -777,30 +777,42 @@
 	return err;
 }

-static int ide_wait_cmd_task(ide_drive_t *drive, u8 *buf)
-{
-	struct request rq;
-
-	ide_init_drive_cmd(&rq);
-	rq.flags = REQ_DRIVE_TASK;
-	rq.buffer = buf;
-	return ide_do_drive_cmd(drive, &rq, ide_wait);
-}
-
-/*
- * FIXME : this needs to map into at taskfile. <andre@linux-ide.org>
- */
-int ide_task_ioctl (ide_drive_t *drive, unsigned int cmd, unsigned long arg)
+int ide_task_ioctl(ide_drive_t *drive, unsigned int cmd, unsigned long arg)
 {
 	void __user *p = (void __user *)arg;
-	int err = 0;
-	u8 args[7], *argbuf = args;
-	int argsize = 7;
+	int err;
+	u8 args[7];
+	ide_task_t task;
+	struct ata_taskfile *tf = &task.tf;

 	if (copy_from_user(args, p, 7))
 		return -EFAULT;
-	err = ide_wait_cmd_task(drive, argbuf);
-	if (copy_to_user(p, argbuf, argsize))
+
+	memset(&task, 0, sizeof(task));
+
+	tf->command	= args[0];
+	tf->feature	= args[1];
+	tf->nsect	= args[2];
+	tf->lbal	= args[3];
+	tf->lbam	= args[4];
+	tf->lbah	= args[5];
+	tf->device	= args[6];
+
+	task.command_type = IDE_DRIVE_TASK_NO_DATA;
+	task.data_phase = TASKFILE_NO_DATA;
+	task.handler = &task_no_data_intr;
+
+	err = ide_diag_taskfile(drive, &task, 0, NULL);
+
+	args[0] = tf->command;
+	args[1] = tf->feature;
+	args[2] = tf->nsect;
+	args[3] = tf->lbal;
+	args[4] = tf->lbam;
+	args[5] = tf->lbah;
+	args[6] = tf->device;
+
+	if (copy_to_user(p, args, 7))
 		err = -EFAULT;
 	return err;
 }

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Tejun Heo]

 Hello, Bartlomiej.

 This patch should be modified to use flagged taskfile if the
task_end_request_fix patch isn't applied.  As non-flagged taskfile
won't return valid result registers, TASK ioctl users won't get the
correct register output.

 IMHO, this flag-to-get-result-registers thing is way too subtle.  How
about keeping old behavior by just not copying out register outputs in
ide_taskfile_ioctl() in applicable cases instead of not reading
registers when ending commands?  That is, unless there's some
noticeable performance impacts I'm not aware of.

 Thanks.

On Thu, Feb 24, 2005 at 03:48:33PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> ide_task_ioctl() rewritten to use taskfile transport.
> This is the last user of REQ_DRIVE_TASK.
> 
> bart: ported to recent IDE changes by me
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
> --- a/drivers/ide/ide-taskfile.c	2005-02-24 15:30:02 +01:00
> +++ b/drivers/ide/ide-taskfile.c	2005-02-24 15:30:02 +01:00
> @@ -777,30 +777,42 @@
>  	return err;
>  }
> 
> -static int ide_wait_cmd_task(ide_drive_t *drive, u8 *buf)
> -{
> -	struct request rq;
> -
> -	ide_init_drive_cmd(&rq);
> -	rq.flags = REQ_DRIVE_TASK;
> -	rq.buffer = buf;
> -	return ide_do_drive_cmd(drive, &rq, ide_wait);
> -}
> -
> -/*
> - * FIXME : this needs to map into at taskfile. <andre@linux-ide.org>
> - */
> -int ide_task_ioctl (ide_drive_t *drive, unsigned int cmd, unsigned long arg)
> +int ide_task_ioctl(ide_drive_t *drive, unsigned int cmd, unsigned long arg)
>  {
>  	void __user *p = (void __user *)arg;
> -	int err = 0;
> -	u8 args[7], *argbuf = args;
> -	int argsize = 7;
> +	int err;
> +	u8 args[7];
> +	ide_task_t task;
> +	struct ata_taskfile *tf = &task.tf;
> 
>  	if (copy_from_user(args, p, 7))
>  		return -EFAULT;
> -	err = ide_wait_cmd_task(drive, argbuf);
> -	if (copy_to_user(p, argbuf, argsize))
> +
> +	memset(&task, 0, sizeof(task));
> +
> +	tf->command	= args[0];
> +	tf->feature	= args[1];
> +	tf->nsect	= args[2];
> +	tf->lbal	= args[3];
> +	tf->lbam	= args[4];
> +	tf->lbah	= args[5];
> +	tf->device	= args[6];
> +
> +	task.command_type = IDE_DRIVE_TASK_NO_DATA;
> +	task.data_phase = TASKFILE_NO_DATA;
> +	task.handler = &task_no_data_intr;
> +
> +	err = ide_diag_taskfile(drive, &task, 0, NULL);
> +
> +	args[0] = tf->command;
> +	args[1] = tf->feature;
> +	args[2] = tf->nsect;
> +	args[3] = tf->lbal;
> +	args[4] = tf->lbam;
> +	args[5] = tf->lbah;
> +	args[6] = tf->device;
> +
> +	if (copy_to_user(p, args, 7))
>  		err = -EFAULT;
>  	return err;
>  }

-- 
tejun

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

On Sunday 27 February 2005 08:36, Tejun Heo wrote:
>  Hello, Bartlomiej.
> 
>  This patch should be modified to use flagged taskfile if the
> task_end_request_fix patch isn't applied.  As non-flagged taskfile
> won't return valid result registers, TASK ioctl users won't get the
> correct register output.

Nope, it works just fine because REQ_DRIVE_TASK used only
no-data protocol, please check task_no_data_intr().

>  IMHO, this flag-to-get-result-registers thing is way too subtle.  How
> about keeping old behavior by just not copying out register outputs in
> ide_taskfile_ioctl() in applicable cases instead of not reading
> registers when ending commands?  That is, unless there's some
> noticeable performance impacts I'm not aware of.

This would miss whole point of not _reading_ these registers (IO is slow).
IMHO new flags denoting {in,out} registers should be added (to <linux/ata.h>
to share them with libata) so new code can be sane and old flags would map
on new flags when needed.

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Tejun Heo]

  Hi,

Bartlomiej Zolnierkiewicz wrote:
> On Sunday 27 February 2005 08:36, Tejun Heo wrote:
> 
>> Hello, Bartlomiej.
>>
>> This patch should be modified to use flagged taskfile if the
>>task_end_request_fix patch isn't applied.  As non-flagged taskfile
>>won't return valid result registers, TASK ioctl users won't get the
>>correct register output.
> 
> 
> Nope, it works just fine because REQ_DRIVE_TASK used only
> no-data protocol, please check task_no_data_intr().
> 

  Sorry, I missed that.  IDE really has a lot of ways to finish a 
command, doesn't it?  hdio.txt is gonna look ugly. :-)

> 
>> IMHO, this flag-to-get-result-registers thing is way too subtle.  How
>>about keeping old behavior by just not copying out register outputs in
>>ide_taskfile_ioctl() in applicable cases instead of not reading
>>registers when ending commands?  That is, unless there's some
>>noticeable performance impacts I'm not aware of.
> 
> 
> This would miss whole point of not _reading_ these registers (IO is slow).
> IMHO new flags denoting {in,out} registers should be added (to <linux/ata.h>
> to share them with libata) so new code can be sane and old flags would map
> on new flags when needed.

  Please do it.

  Or, let me know what you have in mind (added fields, flag names, 
etc...); then, I'll do it.  I think we also need to hear Jeff's opinion 
as things need to be added to ata.h.

  Thanks.

-- 
tejun

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 28 February 2005 16:24, Tejun Heo wrote:
>   Hi,
> 
> Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 27 February 2005 08:36, Tejun Heo wrote:
> > 
> >> Hello, Bartlomiej.
> >>
> >> This patch should be modified to use flagged taskfile if the
> >>task_end_request_fix patch isn't applied.  As non-flagged taskfile
> >>won't return valid result registers, TASK ioctl users won't get the
> >>correct register output.
> > 
> > 
> > Nope, it works just fine because REQ_DRIVE_TASK used only
> > no-data protocol, please check task_no_data_intr().
> > 
> 
>   Sorry, I missed that.  IDE really has a lot of ways to finish a 
> command, doesn't it?  hdio.txt is gonna look ugly. :-)

Yep but it was a lot more "fun" when there were three PIO codepaths. ;-)

hdio.txt doesn't need to know about driver internals so no problem here.

> > 
> >> IMHO, this flag-to-get-result-registers thing is way too subtle.  How
> >>about keeping old behavior by just not copying out register outputs in
> >>ide_taskfile_ioctl() in applicable cases instead of not reading
> >>registers when ending commands?  That is, unless there's some
> >>noticeable performance impacts I'm not aware of.
> > 
> > 
> > This would miss whole point of not _reading_ these registers (IO is slow).
> > IMHO new flags denoting {in,out} registers should be added (to <linux/ata.h>
> > to share them with libata) so new code can be sane and old flags would map
> > on new flags when needed.
> 
>   Please do it.
> 
>   Or, let me know what you have in mind (added fields, flag names, 
> etc...); then, I'll do it.  I think we also need to hear Jeff's opinion 
> as things need to be added to ata.h.

I was thinking about:

* adding ATA_TFLAG_{IN,OUT}_xxx flags (there is enough free
  place for all flags in ->flags field of struct ata_taskfile)
* teaching flagged_taskfile() about these flags and mapping ->tf_out_flags
  onto ATA_TFLAG_OUT_xxx (simple mapping no need to move ->tf_out_flags
  to ide_taskfile_ioctl() etc. - no risk of breaking something)
* moving flagged taskfile writing to ide_tf_load_discrete() helper
* adding ide_tf_read_discrete() helper for use by ide_end_drive_cmd()
  and mapping ->tf_in_flags onto ATA_TFLAG_IN_xxx (ditto)

If you like this plan feel free to implement it.
I'm also open for better ideas, comments etc.

Bartlomiej

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Tejun Heo]

Hello, Bartlomiej.
Hello, Jeff.

On Mon, Feb 28, 2005 at 05:14:55PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Monday 28 February 2005 16:24, Tejun Heo wrote:
> > Bartlomiej Zolnierkiewicz wrote:
> > > 
> > > Nope, it works just fine because REQ_DRIVE_TASK used only
> > > no-data protocol, please check task_no_data_intr().
> > > 
> > 
> >   Sorry, I missed that.  IDE really has a lot of ways to finish a 
> > command, doesn't it?  hdio.txt is gonna look ugly. :-)
> 
> Yep but it was a lot more "fun" when there were three PIO codepaths. ;-)
> 
> hdio.txt doesn't need to know about driver internals so no problem here.
> 

 I was talking about the TASKFILE ioctl IN register result.

> > > 
> > >> IMHO, this flag-to-get-result-registers thing is way too subtle.  How
> > >>about keeping old behavior by just not copying out register outputs in
> > >>ide_taskfile_ioctl() in applicable cases instead of not reading
> > >>registers when ending commands?  That is, unless there's some
> > >>noticeable performance impacts I'm not aware of.
> > > 
> > > 
> > > This would miss whole point of not _reading_ these registers (IO is slow).
> > > IMHO new flags denoting {in,out} registers should be added (to <linux/ata.h>
> > > to share them with libata) so new code can be sane and old flags would map
> > > on new flags when needed.
> > 
> >   Please do it.
> > 
> >   Or, let me know what you have in mind (added fields, flag names, 
> > etc...); then, I'll do it.  I think we also need to hear Jeff's opinion 
> > as things need to be added to ata.h.
> 
> I was thinking about:
> 
> * adding ATA_TFLAG_{IN,OUT}_xxx flags (there is enough free
>   place for all flags in ->flags field of struct ata_taskfile)
> * teaching flagged_taskfile() about these flags and mapping ->tf_out_flags
>   onto ATA_TFLAG_OUT_xxx (simple mapping no need to move ->tf_out_flags
>   to ide_taskfile_ioctl() etc. - no risk of breaking something)
> * moving flagged taskfile writing to ide_tf_load_discrete() helper
> * adding ide_tf_read_discrete() helper for use by ide_end_drive_cmd()
>   and mapping ->tf_in_flags onto ATA_TFLAG_IN_xxx (ditto)
> 
> If you like this plan feel free to implement it.
> I'm also open for better ideas, comments etc.

 So, how do you like the following set of TFLAG's?

/* struct ata_taskfile flags */

/* The following six flags are used by IDE driver to control register IO. */
ATA_TFLAG_OUT_LBA48		= (1 <<  0), /* enable 48-bit LBA and HOB out */
ATA_TFLAG_OUT_ADDR		= (1 <<  1), /* enable out to nsect/lba regs */
ATA_TFLAG_OUT_DEVICE		= (1 <<  2), /* enable out to device reg */
ATA_TFLAG_IN_LBA48		= (1 <<  3), /* enable 48-bit LBA and HOB in */
ATA_TFLAG_IN_ADDR		= (1 <<  4), /* enable in from nsect/lba regs */
ATA_TFLAG_IN_DEVICE		= (1 <<  5), /* enable in from device reg */

/* These three aggregate flags are used by libata, as it doesn't
   really need to optimize register INs */
ATA_TFLAG_LBA48			= (ATA_TFLAG_OUT_LBA48  | ATA_TFLAG_IN_LBA48),
ATA_TFLAG_ISADDR		= (ATA_TFLAG_OUT_ADDR   | ATA_TFLAG_IN_ADDR),
ATA_TFLAG_DEVICE		= (ATA_TFLAG_OUT_DEVICE | ATA_TFLAG_IN_DEVICE),

ATA_TFLAG_WRITE			= (1 <<  6), /* data dir */

/* The rest of TFLAGs are only for implementing ioctl direct drive
   commands in the IDE driver.  DO NOT use in other places. */
ATA_TFLAG_IO_16BIT		= (1 << 11),

ATA_TFLAG_OUT_FEATURE		= (1 << 12),
ATA_TFLAG_OUT_NSECT		= (1 << 13),
ATA_TFLAG_OUT_LBAL		= (1 << 14),
ATA_TFLAG_OUT_LBAM		= (1 << 15),
ATA_TFLAG_OUT_LBAH		= (1 << 16),
ATA_TFLAG_OUT_HOB_FEATURE	= (1 << 17),
ATA_TFLAG_OUT_HOB_NSECT		= (1 << 18),
ATA_TFLAG_OUT_HOB_LBAL		= (1 << 19),
ATA_TFLAG_OUT_HOB_LBAM		= (1 << 20),
ATA_TFLAG_OUT_HOB_LBAH		= (1 << 21),

ATA_TFLAG_IN_FEATURE		= (1 << 22),
ATA_TFLAG_IN_NSECT		= (1 << 23),
ATA_TFLAG_IN_LBAL		= (1 << 24),
ATA_TFLAG_IN_LBAM		= (1 << 25),
ATA_TFLAG_IN_LBAH		= (1 << 26),
ATA_TFLAG_IN_HOB_FEATURE	= (1 << 27),
ATA_TFLAG_IN_HOB_NSECT		= (1 << 28),
ATA_TFLAG_IN_HOB_LBAL		= (1 << 29),
ATA_TFLAG_IN_HOB_LBAM		= (1 << 30),
ATA_TFLAG_IN_HOB_LBAH		= (1 << 31),

ATA_TFLAG_OUT_MASK		= 0x007ff000,
ATA_TFLAG_IN_MASK		= 0xffc00000,
ATA_TFLAG_OUT_IN_MASK		= (ATA_TFLAG_OUT_MASK | ATA_TFLAG_IN_MASK),


 ATA_TFLAG_{OUT|IN}_{LBA48|ADDR|DEVICE} should provide enough
granuality for fs/internal requests without much hassle, and
individual IO/OUT flags will only be used to implement ioctls in
separate IN/OUT functions, something like ide_{load|read}_ioctl_task().

 Would using more specific prefix for ioctl flags - like
ATA_TFLAG_IOC_{IN|OUT}_* - be better?

 libata will work as it works currently, but if optimizing out
register INs can help, converting to use IN/OUT flags should be easy.

 Please let me know what you guys think.

 Thanks.

-- 
tejun

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Tejun Heo]

  Oh, Bartlomiej, one more thing.

  If it isn't too much trouble, can you please set up a bk repository 
which contains the patches you've posted and whatever you're working on 
but hasn't yet made into ide-dev tree?  So that we don't have to juggle 
patches back and forth.  If you maintain your up-to-date working tree, 
I'll make my patches against that tree and if it's convinient for you, I 
can also set up an export tree you can pull from.

  Thanks.  :-)

-- 
tejun

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

Hello,

On Tue, 1 Mar 2005 13:21:16 +0900, Tejun Heo <htejun@gmail.com> wrote:
> Hello, Bartlomiej.
> Hello, Jeff.
> 
> On Mon, Feb 28, 2005 at 05:14:55PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On Monday 28 February 2005 16:24, Tejun Heo wrote:
> > > Bartlomiej Zolnierkiewicz wrote:
> > > >
> > > > Nope, it works just fine because REQ_DRIVE_TASK used only
> > > > no-data protocol, please check task_no_data_intr().
> > > >
> > >
> > >   Sorry, I missed that.  IDE really has a lot of ways to finish a
> > > command, doesn't it?  hdio.txt is gonna look ugly. :-)
> >
> > Yep but it was a lot more "fun" when there were three PIO codepaths. ;-)
> >
> > hdio.txt doesn't need to know about driver internals so no problem here.
> >
> 
>  I was talking about the TASKFILE ioctl IN register result.
> 
> > > >
> > > >> IMHO, this flag-to-get-result-registers thing is way too subtle.  How
> > > >>about keeping old behavior by just not copying out register outputs in
> > > >>ide_taskfile_ioctl() in applicable cases instead of not reading
> > > >>registers when ending commands?  That is, unless there's some
> > > >>noticeable performance impacts I'm not aware of.
> > > >
> > > >
> > > > This would miss whole point of not _reading_ these registers (IO is slow).
> > > > IMHO new flags denoting {in,out} registers should be added (to <linux/ata.h>
> > > > to share them with libata) so new code can be sane and old flags would map
> > > > on new flags when needed.
> > >
> > >   Please do it.
> > >
> > >   Or, let me know what you have in mind (added fields, flag names,
> > > etc...); then, I'll do it.  I think we also need to hear Jeff's opinion
> > > as things need to be added to ata.h.
> >
> > I was thinking about:
> >
> > * adding ATA_TFLAG_{IN,OUT}_xxx flags (there is enough free
> >   place for all flags in ->flags field of struct ata_taskfile)
> > * teaching flagged_taskfile() about these flags and mapping ->tf_out_flags
> >   onto ATA_TFLAG_OUT_xxx (simple mapping no need to move ->tf_out_flags
> >   to ide_taskfile_ioctl() etc. - no risk of breaking something)
> > * moving flagged taskfile writing to ide_tf_load_discrete() helper
> > * adding ide_tf_read_discrete() helper for use by ide_end_drive_cmd()
> >   and mapping ->tf_in_flags onto ATA_TFLAG_IN_xxx (ditto)
> >
> > If you like this plan feel free to implement it.
> > I'm also open for better ideas, comments etc.
> 
>  So, how do you like the following set of TFLAG's?
> 
> /* struct ata_taskfile flags */
> 
> /* The following six flags are used by IDE driver to control register IO. */
> ATA_TFLAG_OUT_LBA48             = (1 <<  0), /* enable 48-bit LBA and HOB out */

aggregate ATA_TFLAG_OUT_HOB_LBA{L,M,H}?

> ATA_TFLAG_OUT_ADDR              = (1 <<  1), /* enable out to nsect/lba regs */

not needed currently, add it {when,if} it is needed

> ATA_TFLAG_OUT_DEVICE            = (1 <<  2), /* enable out to device reg */
> ATA_TFLAG_IN_LBA48              = (1 <<  3), /* enable 48-bit LBA and HOB in */

aggregate ATA_TFLAG_IN_HOB_LBA_{L,M,H}?

> ATA_TFLAG_IN_ADDR               = (1 <<  4), /* enable in from nsect/lba regs */

not needed currently, add it {when,if} it is needed

> ATA_TFLAG_IN_DEVICE             = (1 <<  5), /* enable in from device reg */
> 
> /* These three aggregate flags are used by libata, as it doesn't
>    really need to optimize register INs */
> ATA_TFLAG_LBA48                 = (ATA_TFLAG_OUT_LBA48  | ATA_TFLAG_IN_LBA48),
> ATA_TFLAG_ISADDR                = (ATA_TFLAG_OUT_ADDR   | ATA_TFLAG_IN_ADDR),
> ATA_TFLAG_DEVICE                = (ATA_TFLAG_OUT_DEVICE | ATA_TFLAG_IN_DEVICE),
> 
> ATA_TFLAG_WRITE                 = (1 <<  6), /* data dir */
> 
> /* The rest of TFLAGs are only for implementing ioctl direct drive
>    commands in the IDE driver.  DO NOT use in other places. */
> ATA_TFLAG_IO_16BIT              = (1 << 11),
> 
> ATA_TFLAG_OUT_FEATURE           = (1 << 12),
> ATA_TFLAG_OUT_NSECT             = (1 << 13),
> ATA_TFLAG_OUT_LBAL              = (1 << 14),
> ATA_TFLAG_OUT_LBAM              = (1 << 15),
> ATA_TFLAG_OUT_LBAH              = (1 << 16),
> ATA_TFLAG_OUT_HOB_FEATURE       = (1 << 17),
> ATA_TFLAG_OUT_HOB_NSECT         = (1 << 18),
> ATA_TFLAG_OUT_HOB_LBAL          = (1 << 19),
> ATA_TFLAG_OUT_HOB_LBAM          = (1 << 20),
> ATA_TFLAG_OUT_HOB_LBAH          = (1 << 21),
> 
> ATA_TFLAG_IN_FEATURE            = (1 << 22),
> ATA_TFLAG_IN_NSECT              = (1 << 23),
> ATA_TFLAG_IN_LBAL               = (1 << 24),
> ATA_TFLAG_IN_LBAM               = (1 << 25),
> ATA_TFLAG_IN_LBAH               = (1 << 26),
> ATA_TFLAG_IN_HOB_FEATURE        = (1 << 27),
> ATA_TFLAG_IN_HOB_NSECT          = (1 << 28),
> ATA_TFLAG_IN_HOB_LBAL           = (1 << 29),
> ATA_TFLAG_IN_HOB_LBAM           = (1 << 30),
> ATA_TFLAG_IN_HOB_LBAH           = (1 << 31),

proposed changes will get rid of 4 bits

> ATA_TFLAG_OUT_MASK              = 0x007ff000,
> ATA_TFLAG_IN_MASK               = 0xffc00000,
> ATA_TFLAG_OUT_IN_MASK           = (ATA_TFLAG_OUT_MASK | ATA_TFLAG_IN_MASK),
> 
>  ATA_TFLAG_{OUT|IN}_{LBA48|ADDR|DEVICE} should provide enough
> granuality for fs/internal requests without much hassle, and
> individual IO/OUT flags will only be used to implement ioctls in
> separate IN/OUT functions, something like ide_{load|read}_ioctl_task().

They would be later used by IDE driver itself so names
like ide_discrete_tf_{load,read}() suits it better IMHO.

>  Would using more specific prefix for ioctl flags - like
> ATA_TFLAG_IOC_{IN|OUT}_* - be better?

Nope, they are not limited to ioctls by design.
 
>  libata will work as it works currently, but if optimizing out
> register INs can help, converting to use IN/OUT flags should be easy.
> 
>  Please let me know what you guys think.

It is fine with me.

Thanks,
Bartlomiej

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Tejun Heo]

 Hello,

On Tue, Mar 01, 2005 at 09:42:18AM +0100, Bartlomiej Zolnierkiewicz wrote:
> Hello,
> 
> On Tue, 1 Mar 2005 13:21:16 +0900, Tejun Heo <htejun@gmail.com> wrote:
> > 
> >  So, how do you like the following set of TFLAG's?
> > 
> > /* struct ata_taskfile flags */
> > 
> > /* The following six flags are used by IDE driver to control register IO. */
> > ATA_TFLAG_OUT_LBA48             = (1 <<  0), /* enable 48-bit LBA and HOB out */
> 
> aggregate ATA_TFLAG_OUT_HOB_LBA{L,M,H}?
> 
> > ATA_TFLAG_OUT_ADDR              = (1 <<  1), /* enable out to nsect/lba regs */
> 
> not needed currently, add it {when,if} it is needed
> 

 Sure, I'll add flags on as-needed basis.  I was trying to show where
I'm heading.

> > ATA_TFLAG_OUT_DEVICE            = (1 <<  2), /* enable out to device reg */
> > ATA_TFLAG_IN_LBA48              = (1 <<  3), /* enable 48-bit LBA and HOB in */
> 
> aggregate ATA_TFLAG_IN_HOB_LBA_{L,M,H}?
> 
> > ATA_TFLAG_IN_ADDR               = (1 <<  4), /* enable in from nsect/lba regs */
> 
> not needed currently, add it {when,if} it is needed
> 
> > ATA_TFLAG_IN_DEVICE             = (1 <<  5), /* enable in from device reg */
> > 
> > /* These three aggregate flags are used by libata, as it doesn't
> >    really need to optimize register INs */
> > ATA_TFLAG_LBA48                 = (ATA_TFLAG_OUT_LBA48  | ATA_TFLAG_IN_LBA48),
> > ATA_TFLAG_ISADDR                = (ATA_TFLAG_OUT_ADDR   | ATA_TFLAG_IN_ADDR),
> > ATA_TFLAG_DEVICE                = (ATA_TFLAG_OUT_DEVICE | ATA_TFLAG_IN_DEVICE),
> > 
> > ATA_TFLAG_WRITE                 = (1 <<  6), /* data dir */
> > 
> > /* The rest of TFLAGs are only for implementing ioctl direct drive
> >    commands in the IDE driver.  DO NOT use in other places. */
> > ATA_TFLAG_IO_16BIT              = (1 << 11),
> > 
> > ATA_TFLAG_OUT_FEATURE           = (1 << 12),
> > ATA_TFLAG_OUT_NSECT             = (1 << 13),
> > ATA_TFLAG_OUT_LBAL              = (1 << 14),
> > ATA_TFLAG_OUT_LBAM              = (1 << 15),
> > ATA_TFLAG_OUT_LBAH              = (1 << 16),
> > ATA_TFLAG_OUT_HOB_FEATURE       = (1 << 17),
> > ATA_TFLAG_OUT_HOB_NSECT         = (1 << 18),
> > ATA_TFLAG_OUT_HOB_LBAL          = (1 << 19),
> > ATA_TFLAG_OUT_HOB_LBAM          = (1 << 20),
> > ATA_TFLAG_OUT_HOB_LBAH          = (1 << 21),
> > 
> > ATA_TFLAG_IN_FEATURE            = (1 << 22),
> > ATA_TFLAG_IN_NSECT              = (1 << 23),
> > ATA_TFLAG_IN_LBAL               = (1 << 24),
> > ATA_TFLAG_IN_LBAM               = (1 << 25),
> > ATA_TFLAG_IN_LBAH               = (1 << 26),
> > ATA_TFLAG_IN_HOB_FEATURE        = (1 << 27),
> > ATA_TFLAG_IN_HOB_NSECT          = (1 << 28),
> > ATA_TFLAG_IN_HOB_LBAL           = (1 << 29),
> > ATA_TFLAG_IN_HOB_LBAM           = (1 << 30),
> > ATA_TFLAG_IN_HOB_LBAH           = (1 << 31),
> 
> proposed changes will get rid of 4 bits
> 
> > ATA_TFLAG_OUT_MASK              = 0x007ff000,
> > ATA_TFLAG_IN_MASK               = 0xffc00000,
> > ATA_TFLAG_OUT_IN_MASK           = (ATA_TFLAG_OUT_MASK | ATA_TFLAG_IN_MASK),
> > 
> >  ATA_TFLAG_{OUT|IN}_{LBA48|ADDR|DEVICE} should provide enough
> > granuality for fs/internal requests without much hassle, and
> > individual IO/OUT flags will only be used to implement ioctls in
> > separate IN/OUT functions, something like ide_{load|read}_ioctl_task().
> 
> They would be later used by IDE driver itself so names
> like ide_discrete_tf_{load,read}() suits it better IMHO.
> 
> >  Would using more specific prefix for ioctl flags - like
> > ATA_TFLAG_IOC_{IN|OUT}_* - be better?
> 
> Nope, they are not limited to ioctls by design.
>  

 The reason why I separated the flags into two sets is that if we
define IO flags as aggregate of separate flags, we'll end up with
do_rw_taskfile() or something equivalent which handle both normal
(fs/kernel-internal) and ioctl taskfiles, by design.  From previous
discussions, I kind of have the impression that you wanna separate the
fully-flagged taskfile handling from the normal, supposedly simpler,
taskfile handling.  So, I was aiming for IO control flags with similar
granuality w/ the current libata implementation.

 IMHO, handling both in the same path is better.  It would be
simpler/cleaner and, with simple optimizations, there would be
virtually no overhead.

 So, here's the second proposal of the flags.

/* struct ata_taskfile flags */

/* Individual register IO control flags */
ATA_TFLAG_OUT_FEATURE		= (1 <<  0),
ATA_TFLAG_OUT_NSECT		= (1 <<  1),
ATA_TFLAG_OUT_LBAL		= (1 <<  2),
ATA_TFLAG_OUT_LBAM		= (1 <<  3),
ATA_TFLAG_OUT_LBAH		= (1 <<  4),
ATA_TFLAG_OUT_HOB_FEATURE	= (1 <<  5),
ATA_TFLAG_OUT_HOB_NSECT		= (1 <<  6),
ATA_TFLAG_OUT_HOB_LBAL		= (1 <<  7),
ATA_TFLAG_OUT_HOB_LBAM		= (1 <<  8),
ATA_TFLAG_OUT_HOB_LBAH		= (1 <<  9),
ATA_TFLAG_OUT_DEVICE		= (1 << 10),

ATA_TFLAG_IN_FEATURE		= (1 << 11),
ATA_TFLAG_IN_NSECT		= (1 << 12),
ATA_TFLAG_IN_LBAL		= (1 << 13),
ATA_TFLAG_IN_LBAM		= (1 << 14),
ATA_TFLAG_IN_LBAH		= (1 << 15),
ATA_TFLAG_IN_HOB_FEATURE	= (1 << 16),
ATA_TFLAG_IN_HOB_NSECT		= (1 << 17),
ATA_TFLAG_IN_HOB_LBAL		= (1 << 18),
ATA_TFLAG_IN_HOB_LBAM		= (1 << 19),
ATA_TFLAG_IN_HOB_LBAH		= (1 << 20),
ATA_TFLAG_IN_DEVICE		= (1 << 21),

/* The following four aggreate flags are used by IDE to control register IO. */
ATA_TFLAG_OUT_LBA48		= (ATA_TFLAG_OUT_HOB_FEATURE	|
				   ATA_TFLAG_OUT_HOB_NSECT	|
				   ATA_TFLAG_OUT_HOB_LBAL	|
				   ATA_TFLAG_OUT_HOB_LBAM	|
				   ATA_TFLAG_OUT_HOB_LBAH	),
ATA_TFLAG_OUT_ADDR		= (ATA_TFLAG_OUT_FEATURE	|
				   ATA_TFLAG_OUT_NSECT		|
				   ATA_TFLAG_OUT_LBAL		|
				   ATA_TFLAG_OUT_LBAM		|
				   ATA_TFLAG_OUT_LBAH		),
ATA_TFLAG_IN_LBA48		= (ATA_TFLAG_IN_HOB_FEATURE	|
				   ATA_TFLAG_IN_HOB_NSECT	|
				   ATA_TFLAG_IN_HOB_LBAL	|
				   ATA_TFLAG_IN_HOB_LBAM	|
				   ATA_TFLAG_IN_HOB_LBAH	),
ATA_TFLAG_IN_ADDR		= (ATA_TFLAG_IN_FEATURE		|
				   ATA_TFLAG_IN_NSECT		|
				   ATA_TFLAG_IN_LBAL		|
				   ATA_TFLAG_IN_LBAM		|
				   ATA_TFLAG_IN_LBAH		),

/* These three aggregate flags are used by libata, as it doesn't
   really need to optimize register INs */
ATA_TFLAG_LBA48			= (ATA_TFLAG_OUT_LBA48  | ATA_TFLAG_IN_LBA48 ),
ATA_TFLAG_ISADDR		= (ATA_TFLAG_OUT_ADDR   | ATA_TFLAG_IN_ADDR  ),
ATA_TFLAG_DEVICE		= (ATA_TFLAG_OUT_DEVICE | ATA_TFLAG_IN_DEVICE),

ATA_TFLAG_WRITE			= (1 << 22), /* data dir */
ATA_TFLAG_IO_16BIT		= (1 << 23), /* force 16bit pio (IDE) */

 Thanks a lot.

-- 
tejun

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

On Tue, 1 Mar 2005 18:29:15 +0900, Tejun Heo <htejun@gmail.com> wrote:
>  Hello,
> 
> On Tue, Mar 01, 2005 at 09:42:18AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > Hello,
> >
> > On Tue, 1 Mar 2005 13:21:16 +0900, Tejun Heo <htejun@gmail.com> wrote:
> > >
> > >  So, how do you like the following set of TFLAG's?
> > >
> > > /* struct ata_taskfile flags */
> > >
> > > /* The following six flags are used by IDE driver to control register IO. */
> > > ATA_TFLAG_OUT_LBA48             = (1 <<  0), /* enable 48-bit LBA and HOB out */
> >
> > aggregate ATA_TFLAG_OUT_HOB_LBA{L,M,H}?
> >
> > > ATA_TFLAG_OUT_ADDR              = (1 <<  1), /* enable out to nsect/lba regs */
> >
> > not needed currently, add it {when,if} it is needed
> >
> 
>  Sure, I'll add flags on as-needed basis.  I was trying to show where
> I'm heading.
> 
> > > ATA_TFLAG_OUT_DEVICE            = (1 <<  2), /* enable out to device reg */
> > > ATA_TFLAG_IN_LBA48              = (1 <<  3), /* enable 48-bit LBA and HOB in */
> >
> > aggregate ATA_TFLAG_IN_HOB_LBA_{L,M,H}?
> >
> > > ATA_TFLAG_IN_ADDR               = (1 <<  4), /* enable in from nsect/lba regs */
> >
> > not needed currently, add it {when,if} it is needed
> >
> > > ATA_TFLAG_IN_DEVICE             = (1 <<  5), /* enable in from device reg */
> > >
> > > /* These three aggregate flags are used by libata, as it doesn't
> > >    really need to optimize register INs */
> > > ATA_TFLAG_LBA48                 = (ATA_TFLAG_OUT_LBA48  | ATA_TFLAG_IN_LBA48),
> > > ATA_TFLAG_ISADDR                = (ATA_TFLAG_OUT_ADDR   | ATA_TFLAG_IN_ADDR),
> > > ATA_TFLAG_DEVICE                = (ATA_TFLAG_OUT_DEVICE | ATA_TFLAG_IN_DEVICE),
> > >
> > > ATA_TFLAG_WRITE                 = (1 <<  6), /* data dir */
> > >
> > > /* The rest of TFLAGs are only for implementing ioctl direct drive
> > >    commands in the IDE driver.  DO NOT use in other places. */
> > > ATA_TFLAG_IO_16BIT              = (1 << 11),
> > >
> > > ATA_TFLAG_OUT_FEATURE           = (1 << 12),
> > > ATA_TFLAG_OUT_NSECT             = (1 << 13),
> > > ATA_TFLAG_OUT_LBAL              = (1 << 14),
> > > ATA_TFLAG_OUT_LBAM              = (1 << 15),
> > > ATA_TFLAG_OUT_LBAH              = (1 << 16),
> > > ATA_TFLAG_OUT_HOB_FEATURE       = (1 << 17),
> > > ATA_TFLAG_OUT_HOB_NSECT         = (1 << 18),
> > > ATA_TFLAG_OUT_HOB_LBAL          = (1 << 19),
> > > ATA_TFLAG_OUT_HOB_LBAM          = (1 << 20),
> > > ATA_TFLAG_OUT_HOB_LBAH          = (1 << 21),
> > >
> > > ATA_TFLAG_IN_FEATURE            = (1 << 22),
> > > ATA_TFLAG_IN_NSECT              = (1 << 23),
> > > ATA_TFLAG_IN_LBAL               = (1 << 24),
> > > ATA_TFLAG_IN_LBAM               = (1 << 25),
> > > ATA_TFLAG_IN_LBAH               = (1 << 26),
> > > ATA_TFLAG_IN_HOB_FEATURE        = (1 << 27),
> > > ATA_TFLAG_IN_HOB_NSECT          = (1 << 28),
> > > ATA_TFLAG_IN_HOB_LBAL           = (1 << 29),
> > > ATA_TFLAG_IN_HOB_LBAM           = (1 << 30),
> > > ATA_TFLAG_IN_HOB_LBAH           = (1 << 31),
> >
> > proposed changes will get rid of 4 bits
> >
> > > ATA_TFLAG_OUT_MASK              = 0x007ff000,
> > > ATA_TFLAG_IN_MASK               = 0xffc00000,
> > > ATA_TFLAG_OUT_IN_MASK           = (ATA_TFLAG_OUT_MASK | ATA_TFLAG_IN_MASK),
> > >
> > >  ATA_TFLAG_{OUT|IN}_{LBA48|ADDR|DEVICE} should provide enough
> > > granuality for fs/internal requests without much hassle, and
> > > individual IO/OUT flags will only be used to implement ioctls in
> > > separate IN/OUT functions, something like ide_{load|read}_ioctl_task().
> >
> > They would be later used by IDE driver itself so names
> > like ide_discrete_tf_{load,read}() suits it better IMHO.
> >
> > >  Would using more specific prefix for ioctl flags - like
> > > ATA_TFLAG_IOC_{IN|OUT}_* - be better?
> >
> > Nope, they are not limited to ioctls by design.
> >
> 
>  The reason why I separated the flags into two sets is that if we
> define IO flags as aggregate of separate flags, we'll end up with
> do_rw_taskfile() or something equivalent which handle both normal
> (fs/kernel-internal) and ioctl taskfiles, by design.  From previous
> discussions, I kind of have the impression that you wanna separate the
> fully-flagged taskfile handling from the normal, supposedly simpler,
> taskfile handling.  So, I was aiming for IO control flags with similar
> granuality w/ the current libata implementation.

Yes but it seems that you've assumed that ioctl == flagged taskfile
and fs/internal == normal taskfile which is _not_ what I aim for.

I want fully-flagged taskfile handling like flagged_taskfile() and "hot path"
simpler taskfile handling like do_rw_taskfile() (at least for now - we can
remove "hot path" later) where both can be used for fs/internal/ioctl requests
(depending on the flags).

>  IMHO, handling both in the same path is better.  It would be
> simpler/cleaner and, with simple optimizations, there would be
> virtually no overhead.

We can check this later.

>  So, here's the second proposal of the flags.
> 
> /* struct ata_taskfile flags */
> 
> /* Individual register IO control flags */
> ATA_TFLAG_OUT_FEATURE           = (1 <<  0),
> ATA_TFLAG_OUT_NSECT             = (1 <<  1),
> ATA_TFLAG_OUT_LBAL              = (1 <<  2),
> ATA_TFLAG_OUT_LBAM              = (1 <<  3),
> ATA_TFLAG_OUT_LBAH              = (1 <<  4),
> ATA_TFLAG_OUT_HOB_FEATURE       = (1 <<  5),
> ATA_TFLAG_OUT_HOB_NSECT         = (1 <<  6),
> ATA_TFLAG_OUT_HOB_LBAL          = (1 <<  7),
> ATA_TFLAG_OUT_HOB_LBAM          = (1 <<  8),
> ATA_TFLAG_OUT_HOB_LBAH          = (1 <<  9),
> ATA_TFLAG_OUT_DEVICE            = (1 << 10),
> 
> ATA_TFLAG_IN_FEATURE            = (1 << 11),
> ATA_TFLAG_IN_NSECT              = (1 << 12),
> ATA_TFLAG_IN_LBAL               = (1 << 13),
> ATA_TFLAG_IN_LBAM               = (1 << 14),
> ATA_TFLAG_IN_LBAH               = (1 << 15),
> ATA_TFLAG_IN_HOB_FEATURE        = (1 << 16),
> ATA_TFLAG_IN_HOB_NSECT          = (1 << 17),
> ATA_TFLAG_IN_HOB_LBAL           = (1 << 18),
> ATA_TFLAG_IN_HOB_LBAM           = (1 << 19),
> ATA_TFLAG_IN_HOB_LBAH           = (1 << 20),
> ATA_TFLAG_IN_DEVICE             = (1 << 21),
> 
> /* The following four aggreate flags are used by IDE to control register IO. */
> ATA_TFLAG_OUT_LBA48             = (ATA_TFLAG_OUT_HOB_FEATURE    |
>                                    ATA_TFLAG_OUT_HOB_NSECT      |
>                                    ATA_TFLAG_OUT_HOB_LBAL       |
>                                    ATA_TFLAG_OUT_HOB_LBAM       |
>                                    ATA_TFLAG_OUT_HOB_LBAH       ),
> ATA_TFLAG_OUT_ADDR              = (ATA_TFLAG_OUT_FEATURE        |
>                                    ATA_TFLAG_OUT_NSECT          |
>                                    ATA_TFLAG_OUT_LBAL           |
>                                    ATA_TFLAG_OUT_LBAM           |
>                                    ATA_TFLAG_OUT_LBAH           ),
> ATA_TFLAG_IN_LBA48              = (ATA_TFLAG_IN_HOB_FEATURE     |
>                                    ATA_TFLAG_IN_HOB_NSECT       |
>                                    ATA_TFLAG_IN_HOB_LBAL        |
>                                    ATA_TFLAG_IN_HOB_LBAM        |
>                                    ATA_TFLAG_IN_HOB_LBAH        ),
> ATA_TFLAG_IN_ADDR               = (ATA_TFLAG_IN_FEATURE         |
>                                    ATA_TFLAG_IN_NSECT           |
>                                    ATA_TFLAG_IN_LBAL            |
>                                    ATA_TFLAG_IN_LBAM            |
>                                    ATA_TFLAG_IN_LBAH            ),
> 
> /* These three aggregate flags are used by libata, as it doesn't
>    really need to optimize register INs */
> ATA_TFLAG_LBA48                 = (ATA_TFLAG_OUT_LBA48  | ATA_TFLAG_IN_LBA48 ),
> ATA_TFLAG_ISADDR                = (ATA_TFLAG_OUT_ADDR   | ATA_TFLAG_IN_ADDR  ),
> ATA_TFLAG_DEVICE                = (ATA_TFLAG_OUT_DEVICE | ATA_TFLAG_IN_DEVICE),
> 
> ATA_TFLAG_WRITE                 = (1 << 22), /* data dir */
> ATA_TFLAG_IO_16BIT              = (1 << 23), /* force 16bit pio (IDE) */

s/pio/PIO/

This version look perfect, at least from IDE driver POV. :-)
 
Thanks,
Bartlomiej

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> Yes but it seems that you've assumed that ioctl == flagged taskfile
> and fs/internal == normal taskfile which is _not_ what I aim for.
> 
> I want fully-flagged taskfile handling like flagged_taskfile() and "hot path"
> simpler taskfile handling like do_rw_taskfile() (at least for now - we can
> remove "hot path" later) where both can be used for fs/internal/ioctl requests
> (depending on the flags).


There is no effective difference in performance between

	writeb()
	writeb()
	writeb()
	writeb()

and

	if (bit 1)
		writeb()
	if (bit 2)
		writeb()
	if (bit 3)
		writeb()
	if (bit 4)
		writeb()

The cost of a repeated bit test on the same unsigned long is _zero_. 
It's already in L1 cache.  The I/Os are slow, and adding bit tests will 
not measurably decrease performance.  (this is the reason why I do not 
object to using ioread32() and iowrite32()...  it just adds a simple test)

Plus, it is better to have a single path for all taskfiles, to ensure 
that the path is well-tested.

libata's ->tf_load() and ->tf_read() hooks should be updated to use the 
more fine-grained flags that Tejun is proposing.

Note that on SATA, this is largely irrelevant.  The functions 
ata_tf_read() and ata_tf_load() should be updated for flagged taskfiles, 
because these will be used with PATA drivers.

The hooks implemented in individual SATA drivers will not be updated. 
The reason is that SATA transmits an entire copy of the taskfile to/from 
the device all at once, in the form of a Frame Information Structure 
(FIS) -- essentially a SATA packet.

	Jeff

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Bartlomiej Zolnierkiewicz]

On Wed, 02 Mar 2005 01:08:56 -0500, Jeff Garzik <jgarzik@pobox.com> wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > Yes but it seems that you've assumed that ioctl == flagged taskfile
> > and fs/internal == normal taskfile which is _not_ what I aim for.
> >
> > I want fully-flagged taskfile handling like flagged_taskfile() and "hot path"
> > simpler taskfile handling like do_rw_taskfile() (at least for now - we can
> > remove "hot path" later) where both can be used for fs/internal/ioctl requests
> > (depending on the flags).
> 
> There is no effective difference in performance between
> 
>         writeb()
>         writeb()
>         writeb()
>         writeb()
> 
> and
> 
>         if (bit 1)
>                 writeb()
>         if (bit 2)
>                 writeb()
>         if (bit 3)
>                 writeb()
>         if (bit 4)
>                 writeb()
> 
> The cost of a repeated bit test on the same unsigned long is _zero_.
> It's already in L1 cache.  The I/Os are slow, and adding bit tests will

certainly it is not _zero_ ;-)

I agree that it is negligible compared to the cost of I/O

> not measurably decrease performance.  (this is the reason why I do not
> object to using ioread32() and iowrite32()...  it just adds a simple test)
> 
> Plus, it is better to have a single path for all taskfiles, to ensure
> that the path is well-tested.
> 
> libata's ->tf_load() and ->tf_read() hooks should be updated to use the
> more fine-grained flags that Tejun is proposing.
> 
> Note that on SATA, this is largely irrelevant.  The functions
> ata_tf_read() and ata_tf_load() should be updated for flagged taskfiles,
> because these will be used with PATA drivers.
> 
> The hooks implemented in individual SATA drivers will not be updated.
> The reason is that SATA transmits an entire copy of the taskfile to/from
> the device all at once, in the form of a Frame Information Structure
> (FIS) -- essentially a SATA packet.

agreed

Tejun, one-path approach for IDE driver is fine with me

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Wed, 02 Mar 2005 01:08:56 -0500, Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>>Bartlomiej Zolnierkiewicz wrote:
>>
>>>Yes but it seems that you've assumed that ioctl == flagged taskfile
>>>and fs/internal == normal taskfile which is _not_ what I aim for.
>>>
>>>I want fully-flagged taskfile handling like flagged_taskfile() and "hot path"
>>>simpler taskfile handling like do_rw_taskfile() (at least for now - we can
>>>remove "hot path" later) where both can be used for fs/internal/ioctl requests
>>>(depending on the flags).
>>
>>There is no effective difference in performance between
>>
>>        writeb()
>>        writeb()
>>        writeb()
>>        writeb()
>>
>>and
>>
>>        if (bit 1)
>>                writeb()
>>        if (bit 2)
>>                writeb()
>>        if (bit 3)
>>                writeb()
>>        if (bit 4)
>>                writeb()
>>
>>The cost of a repeated bit test on the same unsigned long is _zero_.
>>It's already in L1 cache.  The I/Os are slow, and adding bit tests will
> 
> 
> certainly it is not _zero_ ;-)
> 
> I agree that it is negligible compared to the cost of I/O

True :)

	Jeff

Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE

[Mark Lord]

SATA, PATA, or anything else:  if it has to cross the PCI bus,
a simple readX()/writeX() can stall the CPU for the equivalent
of hundreds of instructions.  I agree with Jeff, it is always
worth even moderately complex logic to avoid I/O.

Note that an isolated write{bwl}() *may* be almost free in most
cases, due to write buffers between the CPU and the bus.
But those buffers are of limited depth (typically 3/4 entries),
and a stall there often causes a 0.5us (or more) delay.

When measuring PATA hardware, I found the delay was often between
600ns and 1200ns (0.6us to 1.2us), per readX()/writeX().
With SATA, it will likely be around 11 PCI clocks, or say 363ns
on most current platforms.

Cheers