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

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

[Bartlomiej Zolnierkiewicz]

* 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 <geert@linux-m68k.org>
Cc: Michael Schmitz <schmitz@debian.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
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;
 	}
     }

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

[Geert Uytterhoeven]

	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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Geert Uytterhoeven]

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

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

[Geert Uytterhoeven]

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

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

[Alan Cox]

> 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

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

[Bartlomiej Zolnierkiewicz]

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 <bzolnier@gmail.com>
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 <geert@linux-m68k.org>
Cc: Michael Schmitz <schmitz@debian.org>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
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);

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

[Richard Zidlicky]

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

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

[Michael Schmitz]

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

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

[Richard Zidlicky]

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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Mikael Pettersson]

Alan Cox writes:
 > > Sorry for the mess in core code - ist there any other way for only driveid
 > > and packet data getting swapped? And can we actually use the device mapper
 > > to byte-swap the root partition?
 > 
 > Device Mapper can certainly support that if needed - I don't think it has
 > a swab target currently but byteswapped is just a very bad crypto
 > algorithm so you can use dm crypt target as a reference. Might not be a
 > bad idea just to make the media shareable.
 > 
 > The only other way I can see to do it would be to split the ATA transfer
 > functions into two sets one for media data and one for other stuff - but
 > then how do you handle iso9660 cd ?

I had a brief look at this a little while ago, as I was looking at
the hacks Debian applies to make 2.6.x IDE work on Atari/ARAnym.
As I understood it the problem was some IDENTIFY command which needed
to not byteswap (the hack byteswaps a second time). Can't the driver
inspect the command type and choose whether to swap or not?

I had a grand plan of writing a pata_falconide libata driver, but
ran out of steam :-(

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

[Alan Cox]

> Sorry for the mess in core code - ist there any other way for only driveid
> and packet data getting swapped? And can we actually use the device mapper
> to byte-swap the root partition?

Device Mapper can certainly support that if needed - I don't think it has
a swab target currently but byteswapped is just a very bad crypto
algorithm so you can use dm crypt target as a reference. Might not be a
bad idea just to make the media shareable.

The only other way I can see to do it would be to split the ATA transfer
functions into two sets one for media data and one for other stuff - but
then how do you handle iso9660 cd ?

Alan

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

[Michael Schmitz]

Hi,

> > That would require an additional data copy and a double byteswap on machines
> > which are not that fast in first place...
>
> So use the blitter ;)
>
> At this point the future is not Commode Amiga or Atari TT and we risk
> mess in core code for obscure platforms ?

The Amiga isn't affected by this. Only Falcon and Q40 (Atari) are.

Sorry for the mess in core code - ist there any other way for only driveid
and packet data getting swapped? And can we actually use the device mapper
to byte-swap the root partition?

	Michael

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

[Roman Zippel]

Hi,

On Mon, 31 Mar 2008, Alan Cox wrote:

> > That would require an additional data copy and a double byteswap on machines 
> > which are not that fast in first place...
> 
> So use the blitter ;)
> 
> At this point the future is not Commode Amiga or Atari TT and we risk
> mess in core code for obscure platforms ?

We have worse mess and a _single_ simple hook which helps a small platform 
and doesn't hurt others doesn't sound that bad.

bye, Roman

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

[Alan Cox]

> That would require an additional data copy and a double byteswap on machines 
> which are not that fast in first place...

So use the blitter ;)

At this point the future is not Commode Amiga or Atari TT and we risk
mess in core code for obscure platforms ?

Alan

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

[Roman Zippel]

Hi,

On Monday 31. March 2008, Alan Cox wrote:

> > 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.

That would require an additional data copy and a double byteswap on machines 
which are not that fast in first place...

bye, Roman