[PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods - Kernel

This is a discussion on [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods - Kernel ; * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to falconide and q40ide host drivers (->ata_* methods are implemented on top of ->atapi_* methods so they also do byte-swapping now). * Cleanup atapi_{in,out}put_bytes(). Cc: Geert Uytterhoeven Cc: Michael Schmitz Signed-off-by: Bartlomiej Zolnierkiewicz --- ...

+ Reply to Thread
Results 1 to 11 of 11

Thread: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

  1. [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to
    falconide and q40ide host drivers (->ata_* methods are implemented on
    top of ->atapi_* methods so they also do byte-swapping now).

    * Cleanup atapi_{in,out}put_bytes().

    Cc: Geert Uytterhoeven
    Cc: Michael Schmitz
    Signed-off-by: Bartlomiej Zolnierkiewicz
    ---
    PS one of future patches will merge ->atapi_ and ->ata_ methods for IDE

    drivers/ide/ide-iops.c | 14 --------------
    drivers/ide/legacy/falconide.c | 30 ++++++++++++++++++++++++++++++
    drivers/ide/legacy/q40ide.c | 28 ++++++++++++++++++++++++++++
    3 files changed, 58 insertions(+), 14 deletions(-)

    Index: b/drivers/ide/ide-iops.c
    ================================================== =================
    --- a/drivers/ide/ide-iops.c
    +++ b/drivers/ide/ide-iops.c
    @@ -248,13 +248,6 @@ static void atapi_input_bytes(ide_drive_
    ide_hwif_t *hwif = HWIF(drive);

    ++bytecount;
    -#if defined(CONFIG_ATARI) || defined(CONFIG_Q40)
    - if (MACH_IS_ATARI || MACH_IS_Q40) {
    - /* Atari has a byte-swapped IDE interface */
    - insw_swapw(hwif->io_ports.data_addr, buffer, bytecount / 2);
    - return;
    - }
    -#endif /* CONFIG_ATARI || CONFIG_Q40 */
    hwif->ata_input_data(drive, buffer, bytecount / 4);
    if ((bytecount & 0x03) >= 2)
    hwif->INSW(hwif->io_ports.data_addr,
    @@ -266,13 +259,6 @@ static void atapi_output_bytes(ide_drive
    ide_hwif_t *hwif = HWIF(drive);

    ++bytecount;
    -#if defined(CONFIG_ATARI) || defined(CONFIG_Q40)
    - if (MACH_IS_ATARI || MACH_IS_Q40) {
    - /* Atari has a byte-swapped IDE interface */
    - outsw_swapw(hwif->io_ports.data_addr, buffer, bytecount / 2);
    - return;
    - }
    -#endif /* CONFIG_ATARI || CONFIG_Q40 */
    hwif->ata_output_data(drive, buffer, bytecount / 4);
    if ((bytecount & 0x03) >= 2)
    hwif->OUTSW(hwif->io_ports.data_addr,
    Index: b/drivers/ide/legacy/falconide.c
    ================================================== =================
    --- a/drivers/ide/legacy/falconide.c
    +++ b/drivers/ide/legacy/falconide.c
    @@ -44,6 +44,30 @@
    int falconide_intr_lock;
    EXPORT_SYMBOL(falconide_intr_lock);

    +static void falconide_atapi_input_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + insw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}
    +
    +static void falconide_atapi_output_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + outsw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}
    +
    +static void falconide_ata_input_data(ide_drive_t *drive, void *buf,
    + unsigned int wcount)
    +{
    + falconide_atapi_input_bytes(drive, buf, wcount * 4);
    +}
    +
    +static void falconide_ata_output_data(ide_drive_t *drive, void *buf,
    + unsigned int wcount)
    +{
    + falconide_atapi_output_bytes(drive, buf, wcount * 4);
    +}
    +
    static void __init falconide_setup_ports(hw_regs_t *hw)
    {
    int i;
    @@ -89,6 +113,12 @@ static int __init falconide_init(void)

    ide_init_port_hw(hwif, &hw);

    + /* Atari has a byte-swapped IDE interface */
    + hwif->atapi_input_bytes = falconide_atapi_input_bytes;
    + hwif->atapi_output_bytes = falconide_atapi_output_bytes;
    + hwif->ata_input_data = falconide_ata_input_data;
    + hwif->ata_output_data = falconide_ata_output_data;
    +
    ide_get_lock(NULL, NULL);
    ide_device_add(idx, NULL);
    ide_release_lock();
    Index: b/drivers/ide/legacy/q40ide.c
    ================================================== =================
    --- a/drivers/ide/legacy/q40ide.c
    +++ b/drivers/ide/legacy/q40ide.c
    @@ -90,7 +90,29 @@ void q40_ide_setup_ports ( hw_regs_t *hw
    hw->ack_intr = ack_intr;
    }

    +static void q40ide_atapi_input_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + insw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}

    +static void q40ide_atapi_output_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + outsw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}
    +
    +static void q40ide_ata_input_data(ide_drive_t *drive, void *buf,
    + unsigned int wcount)
    +{
    + q40ide_atapi_input_bytes(drive, buf, wcount * 4);
    +}
    +
    +static void q40ide_ata_output_data(ide_drive_t *drive, void *buf,
    + unsigned int wcount)
    +{
    + q40ide_atapi_output_bytes(drive, buf, wcount * 4);
    +}

    /*
    * the static array is needed to have the name reported in /proc/ioports,
    @@ -141,6 +163,12 @@ static int __init q40ide_init(void)
    if (hwif) {
    ide_init_port_hw(hwif, &hw);

    + /* Q40 has a byte-swapped IDE interface */
    + hwif->atapi_input_bytes = q40ide_atapi_input_bytes;
    + hwif->atapi_output_bytes = q40ide_atapi_output_bytes;
    + hwif->ata_input_data = q40ide_ata_input_data;
    + hwif->ata_output_data = q40ide_ata_output_data;
    +
    idx[i] = hwif->index;
    }
    }
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    Hi Bart,

    On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to
    > falconide and q40ide host drivers (->ata_* methods are implemented on
    > top of ->atapi_* methods so they also do byte-swapping now).
    >
    > * Cleanup atapi_{in,out}put_bytes().


    Thanks!

    One remaining issue (for which the fix has never been submitted upstream so
    far) with Atari and Q40 is that due to the byteswapped interface, the driveid
    is also byteswapped, so it has to be unswapped again in ide_fix_driveid().

    Here's a very old and unclean but working patch:

    --- a/drivers/ide/ide-iops.c
    +++ b/drivers/ide/ide-iops.c
    @@ -284,6 +284,23 @@ void ide_fix_driveid (struct hd_driveid
    int i;
    u16 *stringcast;

    +#ifdef __mc68000__
    + if (!MACH_IS_AMIGA && !MACH_IS_MAC && !MACH_IS_Q40 && !MACH_IS_ATARI)
    + return;
    +
    +#ifdef M68K_IDE_SWAPW
    + if (M68K_IDE_SWAPW) { /* fix bus byteorder first */
    + u_char *p = (u_char *)id;
    + u_char t;
    + for (i = 0; i < 512; i += 2) {
    + t = p[i];
    + p[i] = p[i+1];
    + p[i+1] = t;
    + }
    + }
    +#endif
    +#endif /* __mc68000__ */
    +
    id->config = __le16_to_cpu(id->config);
    id->cyls = __le16_to_cpu(id->cyls);
    id->reserved2 = __le16_to_cpu(id->reserved2);

    Note that include/asm-m68k/ide.h has

    #define M68K_IDE_SWAPW (MACH_IS_Q40 || MACH_IS_ATARI)

    Gr{oetje,eeting}s,

    Geert

    --
    Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

    In personal conversations with technical people, I call myself a hacker. But
    when I'm talking to journalists I just say "programmer" or something like that.
    -- Linus Torvalds
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods


    Hi,

    On Sunday 30 March 2008, Geert Uytterhoeven wrote:
    > Hi Bart,
    >
    > On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > > * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to
    > > falconide and q40ide host drivers (->ata_* methods are implemented on
    > > top of ->atapi_* methods so they also do byte-swapping now).
    > >
    > > * Cleanup atapi_{in,out}put_bytes().

    >
    > Thanks!
    >
    > One remaining issue (for which the fix has never been submitted upstream so
    > far) with Atari and Q40 is that due to the byteswapped interface, the driveid
    > is also byteswapped, so it has to be unswapped again in ide_fix_driveid().


    My patch causes unswapping for _all_ data coming from the device so I wonder
    whether the ide_fix_driveid() fix is still needed?

    [ I now recall some discussion that we shouldn't un-swap fs requests because
    of how the things were done in the past fs itself is stored byte-swapped on
    the disk - if this is the case I will recast the patch to pass rq to
    ->ata_*put_data in ide_pio_sector() and check rq->cmd_type == REQ_TYPE_FS
    in falconide/q40ide_*put_data() to decide whether to unswap data or not ]

    Thanks,
    Bart

    > Here's a very old and unclean but working patch:
    >
    > --- a/drivers/ide/ide-iops.c
    > +++ b/drivers/ide/ide-iops.c
    > @@ -284,6 +284,23 @@ void ide_fix_driveid (struct hd_driveid
    > int i;
    > u16 *stringcast;
    >
    > +#ifdef __mc68000__
    > + if (!MACH_IS_AMIGA && !MACH_IS_MAC && !MACH_IS_Q40 && !MACH_IS_ATARI)
    > + return;
    > +
    > +#ifdef M68K_IDE_SWAPW
    > + if (M68K_IDE_SWAPW) { /* fix bus byteorder first */
    > + u_char *p = (u_char *)id;
    > + u_char t;
    > + for (i = 0; i < 512; i += 2) {
    > + t = p[i];
    > + p[i] = p[i+1];
    > + p[i+1] = t;
    > + }
    > + }
    > +#endif
    > +#endif /* __mc68000__ */
    > +
    > id->config = __le16_to_cpu(id->config);
    > id->cyls = __le16_to_cpu(id->cyls);
    > id->reserved2 = __le16_to_cpu(id->reserved2);
    >
    > Note that include/asm-m68k/ide.h has
    >
    > #define M68K_IDE_SWAPW (MACH_IS_Q40 || MACH_IS_ATARI)
    >
    > Gr{oetje,eeting}s,
    >
    > Geert
    >
    > --
    > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
    >
    > In personal conversations with technical people, I call myself a hacker. But
    > when I'm talking to journalists I just say "programmer" or something like that.
    > -- Linus Torvalds

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > On Sunday 30 March 2008, Geert Uytterhoeven wrote:
    > > On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > > > * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to
    > > > falconide and q40ide host drivers (->ata_* methods are implemented on
    > > > top of ->atapi_* methods so they also do byte-swapping now).
    > > >
    > > > * Cleanup atapi_{in,out}put_bytes().

    > >
    > > Thanks!
    > >
    > > One remaining issue (for which the fix has never been submitted upstream so
    > > far) with Atari and Q40 is that due to the byteswapped interface, the driveid
    > > is also byteswapped, so it has to be unswapped again in ide_fix_driveid().

    >
    > My patch causes unswapping for _all_ data coming from the device so I wonder
    > whether the ide_fix_driveid() fix is still needed?


    I'll give it a try on Aranym...

    > [ I now recall some discussion that we shouldn't un-swap fs requests because
    > of how the things were done in the past fs itself is stored byte-swapped on
    > the disk - if this is the case I will recast the patch to pass rq to
    > ->ata_*put_data in ide_pio_sector() and check rq->cmd_type == REQ_TYPE_FS
    > in falconide/q40ide_*put_data() to decide whether to unswap data or not ]


    Yes, the data on the disk is stored byte-swapped.
    So it's only the drive ID and packet commands that should be swapped.

    Gr{oetje,eeting}s,

    Geert

    --
    Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

    In personal conversations with technical people, I call myself a hacker. But
    when I'm talking to journalists I just say "programmer" or something like that.
    -- Linus Torvalds
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    On Mon, 31 Mar 2008, Geert Uytterhoeven wrote:
    > On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > > On Sunday 30 March 2008, Geert Uytterhoeven wrote:
    > > > On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > > > > * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to
    > > > > falconide and q40ide host drivers (->ata_* methods are implemented on
    > > > > top of ->atapi_* methods so they also do byte-swapping now).
    > > > >
    > > > > * Cleanup atapi_{in,out}put_bytes().
    > > >
    > > > Thanks!
    > > >
    > > > One remaining issue (for which the fix has never been submitted upstream so
    > > > far) with Atari and Q40 is that due to the byteswapped interface, the driveid
    > > > is also byteswapped, so it has to be unswapped again in ide_fix_driveid().

    > >
    > > My patch causes unswapping for _all_ data coming from the device so I wonder
    > > whether the ide_fix_driveid() fix is still needed?

    >
    > I'll give it a try on Aranym...


    Oops, just applying this series of 5 patches is not sufficient, I get
    lots of rejects...

    Gr{oetje,eeting}s,

    Geert

    --
    Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

    In personal conversations with technical people, I call myself a hacker. But
    when I'm talking to journalists I just say "programmer" or something like that.
    -- Linus Torvalds
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    > Yes, the data on the disk is stored byte-swapped.
    > So it's only the drive ID and packet commands that should be swapped.


    If you are storing the data on disk byte swapped then reverse the logic
    in the driver so you don't need hacks for un-swapping commands and write
    a bytesewap device mapper layer in the right place. Then you can even
    move disks around.

    It's ugly because the solution is currently in the wrong place (for good
    historical reasons this is true)

    Alan
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    On Monday 31 March 2008, Geert Uytterhoeven wrote:
    > On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > > On Sunday 30 March 2008, Geert Uytterhoeven wrote:
    > > > On Sun, 30 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
    > > > > * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to
    > > > > falconide and q40ide host drivers (->ata_* methods are implemented on
    > > > > top of ->atapi_* methods so they also do byte-swapping now).
    > > > >
    > > > > * Cleanup atapi_{in,out}put_bytes().
    > > >
    > > > Thanks!
    > > >
    > > > One remaining issue (for which the fix has never been submitted upstream so
    > > > far) with Atari and Q40 is that due to the byteswapped interface, the driveid
    > > > is also byteswapped, so it has to be unswapped again in ide_fix_driveid().

    > >
    > > My patch causes unswapping for _all_ data coming from the device so I wonder
    > > whether the ide_fix_driveid() fix is still needed?

    >
    > I'll give it a try on Aranym...
    >
    > > [ I now recall some discussion that we shouldn't un-swap fs requests because
    > > of how the things were done in the past fs itself is stored byte-swapped on
    > > the disk - if this is the case I will recast the patch to pass rq to
    > > ->ata_*put_data in ide_pio_sector() and check rq->cmd_type == REQ_TYPE_FS
    > > in falconide/q40ide_*put_data() to decide whether to unswap data or not ]

    >
    > Yes, the data on the disk is stored byte-swapped.
    > So it's only the drive ID and packet commands that should be swapped.


    "take 2" against IDE tree follows

    [ I hope to merge it into IDE tree tomorrow if people are fine with it. ]

    From: Bartlomiej Zolnierkiewicz
    Subject: [PATCH] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods (take 2)

    * Add ->atapi_{in,out}put_bytes and ->ata_{in,out}put_data methods to
    falconide and q40ide host drivers (->ata_* methods are implemented on
    top of ->atapi_* methods so they also do byte-swapping now).

    * Cleanup atapi_{in,out}put_bytes().

    v2:
    * Add 'struct request *rq' argument to ->ata_{in,out}put_data methods
    and don't byte-swap disk fs requests (we shouldn't un-swap fs requests
    because fs itself is stored byte-swapped on the disk) - this is how
    things were done before the patch (ideally device mapper should be
    used instead but it would break existing setups and would have some
    performance impact).

    Cc: Geert Uytterhoeven
    Cc: Michael Schmitz
    Cc: Roman Zippel
    Cc: Alan Cox
    Signed-off-by: Bartlomiej Zolnierkiewicz
    ---
    PS no point in adding rq to ->atapi* methods as the next patch in series
    merges ->ata* and ->atapi* methods

    drivers/ide/cris/ide-cris.c | 14 ++++++++------
    drivers/ide/ide-io.c | 2 +-
    drivers/ide/ide-iops.c | 26 +++++++-------------------
    drivers/ide/ide-probe.c | 2 +-
    drivers/ide/ide-taskfile.c | 16 +++++++++-------
    drivers/ide/legacy/falconide.c | 36 ++++++++++++++++++++++++++++++++++++
    drivers/ide/legacy/q40ide.c | 34 ++++++++++++++++++++++++++++++++++
    include/linux/ide.h | 4 ++--
    8 files changed, 98 insertions(+), 36 deletions(-)

    Index: b/drivers/ide/cris/ide-cris.c
    ================================================== =================
    --- a/drivers/ide/cris/ide-cris.c
    +++ b/drivers/ide/cris/ide-cris.c
    @@ -673,8 +673,10 @@ cris_ide_inb(unsigned long reg)
    return (unsigned char)cris_ide_inw(reg);
    }

    -static void cris_ide_input_data (ide_drive_t *drive, void *, unsigned int);
    -static void cris_ide_output_data (ide_drive_t *drive, void *, unsigned int);
    +static void cris_ide_input_data(ide_drive_t *, struct request *,
    + void *, unsigned int);
    +static void cris_ide_output_data(ide_drive_t *, struct request *,
    + void *, unsigned int);
    static void cris_atapi_input_bytes(ide_drive_t *drive, void *, unsigned int);
    static void cris_atapi_output_bytes(ide_drive_t *drive, void *, unsigned int);

    @@ -900,8 +902,8 @@ cris_atapi_output_bytes (ide_drive_t *dr
    /*
    * This is used for most PIO data transfers *from* the IDE interface
    */
    -static void
    -cris_ide_input_data (ide_drive_t *drive, void *buffer, unsigned int wcount)
    +static void cris_ide_input_data(ide_drive_t *drive, struct request *rq,
    + void *buffer, unsigned int wcount)
    {
    cris_atapi_input_bytes(drive, buffer, wcount << 2);
    }
    @@ -909,8 +911,8 @@ cris_ide_input_data (ide_drive_t *drive,
    /*
    * This is used for most PIO data transfers *to* the IDE interface
    */
    -static void
    -cris_ide_output_data (ide_drive_t *drive, void *buffer, unsigned int wcount)
    +static void cris_ide_output_data(ide_drive_t *drive, struct request *,
    + void *buffer, unsigned int wcount)
    {
    cris_atapi_output_bytes(drive, buffer, wcount << 2);
    }
    Index: b/drivers/ide/ide-io.c
    ================================================== =================
    --- a/drivers/ide/ide-io.c
    +++ b/drivers/ide/ide-io.c
    @@ -422,7 +422,7 @@ static void try_to_flush_leftover_data (
    u32 wcount = (i > 16) ? 16 : i;

    i -= wcount;
    - HWIF(drive)->ata_input_data(drive, buffer, wcount);
    + drive->hwif->ata_input_data(drive, NULL, buffer, wcount);
    }
    }

    Index: b/drivers/ide/ide-iops.c
    ================================================== =================
    --- a/drivers/ide/ide-iops.c
    +++ b/drivers/ide/ide-iops.c
    @@ -192,7 +192,8 @@ static void ata_vlb_sync(ide_drive_t *dr
    /*
    * This is used for most PIO data transfers *from* the IDE interface
    */
    -static void ata_input_data(ide_drive_t *drive, void *buffer, u32 wcount)
    +static void ata_input_data(ide_drive_t *drive, struct request *rq,
    + void *buffer, u32 wcount)
    {
    ide_hwif_t *hwif = drive->hwif;
    struct ide_io_ports *io_ports = &hwif->io_ports;
    @@ -215,7 +216,8 @@ static void ata_input_data(ide_drive_t *
    /*
    * This is used for most PIO data transfers *to* the IDE interface
    */
    -static void ata_output_data(ide_drive_t *drive, void *buffer, u32 wcount)
    +static void ata_output_data(ide_drive_t *drive, struct request *rq,
    + void *buffer, u32 wcount)
    {
    ide_hwif_t *hwif = drive->hwif;
    struct ide_io_ports *io_ports = &hwif->io_ports;
    @@ -248,14 +250,7 @@ static void atapi_input_bytes(ide_drive_
    ide_hwif_t *hwif = HWIF(drive);

    ++bytecount;
    -#if defined(CONFIG_ATARI) || defined(CONFIG_Q40)
    - if (MACH_IS_ATARI || MACH_IS_Q40) {
    - /* Atari has a byte-swapped IDE interface */
    - insw_swapw(hwif->io_ports.data_addr, buffer, bytecount / 2);
    - return;
    - }
    -#endif /* CONFIG_ATARI || CONFIG_Q40 */
    - hwif->ata_input_data(drive, buffer, bytecount / 4);
    + hwif->ata_input_data(drive, NULL, buffer, bytecount / 4);
    if ((bytecount & 0x03) >= 2)
    hwif->INSW(hwif->io_ports.data_addr,
    (u8 *)buffer + (bytecount & ~0x03), 1);
    @@ -266,14 +261,7 @@ static void atapi_output_bytes(ide_drive
    ide_hwif_t *hwif = HWIF(drive);

    ++bytecount;
    -#if defined(CONFIG_ATARI) || defined(CONFIG_Q40)
    - if (MACH_IS_ATARI || MACH_IS_Q40) {
    - /* Atari has a byte-swapped IDE interface */
    - outsw_swapw(hwif->io_ports.data_addr, buffer, bytecount / 2);
    - return;
    - }
    -#endif /* CONFIG_ATARI || CONFIG_Q40 */
    - hwif->ata_output_data(drive, buffer, bytecount / 4);
    + hwif->ata_output_data(drive, NULL, buffer, bytecount / 4);
    if ((bytecount & 0x03) >= 2)
    hwif->OUTSW(hwif->io_ports.data_addr,
    (u8 *)buffer + (bytecount & ~0x03), 1);
    @@ -668,7 +656,7 @@ int ide_driveid_update(ide_drive_t *driv
    local_irq_restore(flags);
    return 0;
    }
    - hwif->ata_input_data(drive, id, SECTOR_WORDS);
    + hwif->ata_input_data(drive, NULL, id, SECTOR_WORDS);
    (void)ide_read_status(drive); /* clear drive IRQ */
    local_irq_enable();
    local_irq_restore(flags);
    Index: b/drivers/ide/ide-probe.c
    ================================================== =================
    --- a/drivers/ide/ide-probe.c
    +++ b/drivers/ide/ide-probe.c
    @@ -126,7 +126,7 @@ static inline void do_identify (ide_driv

    id = drive->id;
    /* read 512 bytes of id info */
    - hwif->ata_input_data(drive, id, SECTOR_WORDS);
    + hwif->ata_input_data(drive, NULL, id, SECTOR_WORDS);

    drive->id_read = 1;
    local_irq_enable();
    Index: b/drivers/ide/ide-taskfile.c
    ================================================== =================
    --- a/drivers/ide/ide-taskfile.c
    +++ b/drivers/ide/ide-taskfile.c
    @@ -283,7 +283,8 @@ static u8 wait_drive_not_busy(ide_drive_
    return stat;
    }

    -static void ide_pio_sector(ide_drive_t *drive, unsigned int write)
    +static void ide_pio_sector(ide_drive_t *drive, struct request *rq,
    + unsigned int write)
    {
    ide_hwif_t *hwif = drive->hwif;
    struct scatterlist *sg = hwif->sg_table;
    @@ -323,9 +324,9 @@ static void ide_pio_sector(ide_drive_t *

    /* do the actual data transfer */
    if (write)
    - hwif->ata_output_data(drive, buf, SECTOR_WORDS);
    + hwif->ata_output_data(drive, rq, buf, SECTOR_WORDS);
    else
    - hwif->ata_input_data(drive, buf, SECTOR_WORDS);
    + hwif->ata_input_data(drive, rq, buf, SECTOR_WORDS);

    kunmap_atomic(buf, KM_BIO_SRC_IRQ);
    #ifdef CONFIG_HIGHMEM
    @@ -333,13 +334,14 @@ static void ide_pio_sector(ide_drive_t *
    #endif
    }

    -static void ide_pio_multi(ide_drive_t *drive, unsigned int write)
    +static void ide_pio_multi(ide_drive_t *drive, struct request *rq,
    + unsigned int write)
    {
    unsigned int nsect;

    nsect = min_t(unsigned int, drive->hwif->nleft, drive->mult_count);
    while (nsect--)
    - ide_pio_sector(drive, write);
    + ide_pio_sector(drive, rq, write);
    }

    static void ide_pio_datablock(ide_drive_t *drive, struct request *rq,
    @@ -362,10 +364,10 @@ static void ide_pio_datablock(ide_drive_
    switch (drive->hwif->data_phase) {
    case TASKFILE_MULTI_IN:
    case TASKFILE_MULTI_OUT:
    - ide_pio_multi(drive, write);
    + ide_pio_multi(drive, rq, write);
    break;
    default:
    - ide_pio_sector(drive, write);
    + ide_pio_sector(drive, rq, write);
    break;
    }

    Index: b/drivers/ide/legacy/falconide.c
    ================================================== =================
    --- a/drivers/ide/legacy/falconide.c
    +++ b/drivers/ide/legacy/falconide.c
    @@ -44,6 +44,36 @@
    int falconide_intr_lock;
    EXPORT_SYMBOL(falconide_intr_lock);

    +static void falconide_atapi_input_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + insw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}
    +
    +static void falconide_atapi_output_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + outsw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}
    +
    +static void falconide_ata_input_data(ide_drive_t *drive, struct request *rq,
    + void *buf, unsigned int wcount)
    +{
    + if (drive->media == ide_disk && rq && rq->cmd_type == REQ_TYPE_FS)
    + return insw(drive->hwif->io_ports.data_addr, buf, wcount * 2);
    +
    + falconide_atapi_input_bytes(drive, buf, wcount * 4);
    +}
    +
    +static void falconide_ata_output_data(ide_drive_t *drive, struct request *rq,
    + void *buf, unsigned int wcount)
    +{
    + if (drive->media == ide_disk && rq && rq->cmd_type == REQ_TYPE_FS)
    + return outsw(drive->hwif->io_ports.data_addr, buf, wcount * 2);
    +
    + falconide_atapi_output_bytes(drive, buf, wcount * 4);
    +}
    +
    static void __init falconide_setup_ports(hw_regs_t *hw)
    {
    int i;
    @@ -89,6 +119,12 @@ static int __init falconide_init(void)

    ide_init_port_hw(hwif, &hw);

    + /* Atari has a byte-swapped IDE interface */
    + hwif->atapi_input_bytes = falconide_atapi_input_bytes;
    + hwif->atapi_output_bytes = falconide_atapi_output_bytes;
    + hwif->ata_input_data = falconide_ata_input_data;
    + hwif->ata_output_data = falconide_ata_output_data;
    +
    ide_get_lock(NULL, NULL);
    ide_device_add(idx, NULL);
    ide_release_lock();
    Index: b/drivers/ide/legacy/q40ide.c
    ================================================== =================
    --- a/drivers/ide/legacy/q40ide.c
    +++ b/drivers/ide/legacy/q40ide.c
    @@ -90,7 +90,35 @@ void q40_ide_setup_ports ( hw_regs_t *hw
    hw->ack_intr = ack_intr;
    }

    +static void q40ide_atapi_input_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + insw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}

    +static void q40ide_atapi_output_bytes(ide_drive_t *drive, void *buf,
    + unsigned int len)
    +{
    + outsw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    +}
    +
    +static void q40ide_ata_input_data(ide_drive_t *drive, struct request *rq,
    + void *buf, unsigned int wcount)
    +{
    + if (drive->media == ide_disk && rq && rq->cmd_type == REQ_TYPE_FS)
    + return insw(drive->hwif->io_ports.data_addr, buf, wcount * 2);
    +
    + q40ide_atapi_input_bytes(drive, buf, wcount * 4);
    +}
    +
    +static void q40ide_ata_output_data(ide_drive_t *drive, struct request *rq,
    + void *buf, unsigned int wcount)
    +{
    + if (drive->media == ide_disk && rq && rq->cmd_type == REQ_TYPE_FS)
    + return outsw(drive->hwif->io_ports.data_addr, buf, wcount * 2);
    +
    + q40ide_atapi_output_bytes(drive, buf, wcount * 4);
    +}

    /*
    * the static array is needed to have the name reported in /proc/ioports,
    @@ -141,6 +169,12 @@ static int __init q40ide_init(void)
    if (hwif) {
    ide_init_port_hw(hwif, &hw);

    + /* Q40 has a byte-swapped IDE interface */
    + hwif->atapi_input_bytes = q40ide_atapi_input_bytes;
    + hwif->atapi_output_bytes = q40ide_atapi_output_bytes;
    + hwif->ata_input_data = q40ide_ata_input_data;
    + hwif->ata_output_data = q40ide_ata_output_data;
    +
    idx[i] = hwif->index;
    }
    }
    Index: b/include/linux/ide.h
    ================================================== =================
    --- a/include/linux/ide.h
    +++ b/include/linux/ide.h
    @@ -469,8 +469,8 @@ typedef struct hwif_s {
    const struct ide_port_ops *port_ops;
    const struct ide_dma_ops *dma_ops;

    - void (*ata_input_data)(ide_drive_t *, void *, u32);
    - void (*ata_output_data)(ide_drive_t *, void *, u32);
    + void (*ata_input_data)(ide_drive_t *, struct request *, void *, u32);
    + void (*ata_output_data)(ide_drive_t *, struct request *, void *, u32);

    void (*atapi_input_bytes)(ide_drive_t *, void *, u32);
    void (*atapi_output_bytes)(ide_drive_t *, void *, u32);
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    On Mon, Apr 07, 2008 at 09:13:42PM +0200, Bartlomiej Zolnierkiewicz wrote:

    >
    > v2:
    > * Add 'struct request *rq' argument to ->ata_{in,out}put_data methods
    > and don't byte-swap disk fs requests (we shouldn't un-swap fs requests
    > because fs itself is stored byte-swapped on the disk) - this is how
    > things were done before the patch (ideally device mapper should be
    > used instead but it would break existing setups and would have some
    > performance impact).


    I like the part about checking 'struct request *rq', will make a clean
    distinction between ide command data and ide disk/medium data which is badly
    needed.

    However, not only FS data is byteswapped, complete disk including partition
    table and everything else is. Will "rq->cmd_type == REQ_TYPE_FS" also catch
    all these cases?



    > Index: b/drivers/ide/legacy/falconide.c
    > ================================================== =================
    > --- a/drivers/ide/legacy/falconide.c
    > +++ b/drivers/ide/legacy/falconide.c
    > @@ -44,6 +44,36 @@
    > int falconide_intr_lock;
    > EXPORT_SYMBOL(falconide_intr_lock);
    >
    > +static void falconide_atapi_input_bytes(ide_drive_t *drive, void *buf,
    > + unsigned int len)
    > +{
    > + insw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    > +}
    > +
    > +static void falconide_atapi_output_bytes(ide_drive_t *drive, void *buf,
    > + unsigned int len)
    > +{
    > + outsw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
    > +}
    > +
    > +static void falconide_ata_input_data(ide_drive_t *drive, struct request *rq,
    > + void *buf, unsigned int wcount)
    > +{
    > + if (drive->media == ide_disk && rq && rq->cmd_type == REQ_TYPE_FS)
    > + return insw(drive->hwif->io_ports.data_addr, buf, wcount * 2);
    > +
    > + falconide_atapi_input_bytes(drive, buf, wcount * 4);
    > +}


    this will still do double swapping for disk access, although this is very
    easier to fix once the distinction between ide and disk data works fine.

    So IMHO despite the little concerns this is the way to go.

    Richard
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    Hi,

    >> v2:
    >> * Add 'struct request *rq' argument to ->ata_{in,out}put_data methods
    >> and don't byte-swap disk fs requests (we shouldn't un-swap fs requests
    >> because fs itself is stored byte-swapped on the disk) - this is how
    >> things were done before the patch (ideally device mapper should be
    >> used instead but it would break existing setups and would have some
    >> performance impact).

    >
    > I like the part about checking 'struct request *rq', will make a clean
    > distinction between ide command data and ide disk/medium data which is badly
    > needed.
    >
    > However, not only FS data is byteswapped, complete disk including partition
    > table and everything else is. Will "rq->cmd_type == REQ_TYPE_FS" also catch
    > all these cases?


    IIRC only identify data (and perhaps ATAPI data) need to be byteswapped
    twice (or not at all), so REQ_TYPE_FS should catch all other cases.

    (Good catch about ATAPI there, BTW - I never thought about that, and ISTR
    that IDE CD-ROMs never worked in Linux. That would be the reason why..)

    Michael
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

    On Wed, Apr 09, 2008 at 03:40:34AM +0200, Michael Schmitz wrote:

    > > However, not only FS data is byteswapped, complete disk including partition
    > > table and everything else is. Will "rq->cmd_type == REQ_TYPE_FS" also catch
    > > all these cases?

    >
    > IIRC only identify data (and perhaps ATAPI data) need to be byteswapped
    > twice (or not at all), so REQ_TYPE_FS should catch all other cases.


    my main worry is whether REQ_TYPE_FS is set even when using raw disk access,
    eg reading partition table or raw partitions.

    The partition table initialisation is one of the reasons why using dmapper
    would be a major pain btw.

    > (Good catch about ATAPI there, BTW - I never thought about that, and ISTR
    > that IDE CD-ROMs never worked in Linux. That would be the reason why..)


    IDE CDROM works perfectly on the Q40?

    Richard
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods


    Hi,

    On Wednesday 09 April 2008, Richard Zidlicky wrote:
    > On Wed, Apr 09, 2008 at 03:40:34AM +0200, Michael Schmitz wrote:
    >
    > > > However, not only FS data is byteswapped, complete disk including partition
    > > > table and everything else is. Will "rq->cmd_type == REQ_TYPE_FS" also catch
    > > > all these cases?

    > >
    > > IIRC only identify data (and perhaps ATAPI data) need to be byteswapped
    > > twice (or not at all), so REQ_TYPE_FS should catch all other cases.

    >
    > my main worry is whether REQ_TYPE_FS is set even when using raw disk access,
    > eg reading partition table or raw partitions.


    AFAIK direct I/O is also handled by REQ_TYPE_FS requests so it should be fine.

    Only special commands (identify, S.M.A.R.T., raw commands passed through
    taskfile ioctl) should be byte-swapped.

    I just merged the patch into IDE tree (together with few other changes)
    so some testing would be greatly appreciated.

    Thanks,
    Bart
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread