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

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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