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

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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