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 <[email protected]>
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. <[email protected]>
- */
-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;
}
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 <[email protected]>
>
> 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. <[email protected]>
> - */
> -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
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.
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
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
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
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
Hello,
On Tue, 1 Mar 2005 13:21:16 +0900, Tejun Heo <[email protected]> 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
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 <[email protected]> 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
On Tue, 1 Mar 2005 18:29:15 +0900, Tejun Heo <[email protected]> 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 <[email protected]> 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
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
On Wed, 02 Mar 2005 01:08:56 -0500, Jeff Garzik <[email protected]> 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
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
Bartlomiej Zolnierkiewicz wrote:
> On Wed, 02 Mar 2005 01:08:56 -0500, Jeff Garzik <[email protected]> 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