Re: [PATCH] 2.5.8 IDE 36

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

Norbert Kiesel wrote:
> Hi,
> 
> while trying to understand recent kernel changes I stumbled over
> the following patch to
>  
> diff -urN linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
> --- linux-2.5.8/drivers/ide/ide.c	Tue Apr 16 06:01:07 2002
> +++ linux/drivers/ide/ide.c	Tue Apr 16 05:38:37 2002
> 
> ...
>  while (i > 0) {
> -		u32 buffer[16];
> -		unsigned int wcount = (i > 16) ? 16 : i;
> -		i -= wcount;
> -		ata_input_data (drive, buffer, wcount);
> +		u32 buffer[SECTOR_WORDS];
> +		unsigned int count = (i > 1) ? 1 : i;
> +
> +		ata_read(drive, buffer, count * SECTOR_WORDS);
> +		i -= count;
>  	}
>  }
> ...
> 
> While the old code called ata_input_read() with [0:16] as last param,
> the new code calls the (renamed) ata_read() with either 0 or 16. Also,
> the new code loops "i" times while the old code looped "i/16+1" times.
> Was this intended or should the patch better read like:
> 
> ...
>  while (i > 0) {
> -               u32 buffer[16];
> -               unsigned int wcount = (i > 16) ? 16 : i;
> -               i -= wcount;
> -               ata_input_data (drive, buffer, wcount);
> +               u32 buffer[SECTOR_WORDS];
> +               unsigned int count = max(i, SECTOR_WORDS);
> +
> +               ata_read(drive, buffer, count);
> +               i -= count;
>         }
>  }
> ...
> 
> so long

It's fine as it is I think. Please look up at the initialization of i.
I have just divded the SECTROT_WORDS (== 16) factor out
of all the places above ata_read.

Re: [PATCH] 2.5.8 IDE 36

[Norbert Kiesel]

Hi,

while trying to understand recent kernel changes I stumbled over
the following patch to
 
diff -urN linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
--- linux-2.5.8/drivers/ide/ide.c	Tue Apr 16 06:01:07 2002
+++ linux/drivers/ide/ide.c	Tue Apr 16 05:38:37 2002

...
 while (i > 0) {
-		u32 buffer[16];
-		unsigned int wcount = (i > 16) ? 16 : i;
-		i -= wcount;
-		ata_input_data (drive, buffer, wcount);
+		u32 buffer[SECTOR_WORDS];
+		unsigned int count = (i > 1) ? 1 : i;
+
+		ata_read(drive, buffer, count * SECTOR_WORDS);
+		i -= count;
 	}
 }
...

While the old code called ata_input_read() with [0:16] as last param,
the new code calls the (renamed) ata_read() with either 0 or 16. Also,
the new code loops "i" times while the old code looped "i/16+1" times.
Was this intended or should the patch better read like:

...
 while (i > 0) {
-               u32 buffer[16];
-               unsigned int wcount = (i > 16) ? 16 : i;
-               i -= wcount;
-               ata_input_data (drive, buffer, wcount);
+               u32 buffer[SECTOR_WORDS];
+               unsigned int count = max(i, SECTOR_WORDS);
+
+               ata_read(drive, buffer, count);
+               i -= count;
        }
 }
...

so long
  Norbert

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

Norbert Kiesel wrote:
> On Tue, 2002-04-16 at 01:21, Martin Dalecki wrote:
> 
>>Norbert Kiesel wrote:
>>
>>>Hi,
>>>
>>>while trying to understand recent kernel changes I stumbled over
>>>the following patch to
>>> 
>>>diff -urN linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
>>>--- linux-2.5.8/drivers/ide/ide.c	Tue Apr 16 06:01:07 2002
>>>+++ linux/drivers/ide/ide.c	Tue Apr 16 05:38:37 2002
>>>
>>>...
>>> while (i > 0) {
>>>-		u32 buffer[16];
>>>-		unsigned int wcount = (i > 16) ? 16 : i;
>>>-		i -= wcount;
>>>-		ata_input_data (drive, buffer, wcount);
>>>+		u32 buffer[SECTOR_WORDS];
>>>+		unsigned int count = (i > 1) ? 1 : i;
>>>+
>>>+		ata_read(drive, buffer, count * SECTOR_WORDS);
>>>+		i -= count;
>>> 	}
>>> }
>>>...
>>>
>>>While the old code called ata_input_read() with [0:16] as last param,
>>>the new code calls the (renamed) ata_read() with either 0 or 16. Also,
>>>the new code loops "i" times while the old code looped "i/16+1" times.
>>>Was this intended or should the patch better read like:
>>>
>>>...
>>> while (i > 0) {
>>>-               u32 buffer[16];
>>>-               unsigned int wcount = (i > 16) ? 16 : i;
>>>-               i -= wcount;
>>>-               ata_input_data (drive, buffer, wcount);
>>>+               u32 buffer[SECTOR_WORDS];
>>>+               unsigned int count = max(i, SECTOR_WORDS);
>>>+
>>>+               ata_read(drive, buffer, count);
>>>+               i -= count;
>>>        }
>>> }
>>>...
>>>
>>>so long
>>
>>It's fine as it is I think. Please look up at the initialization of i.
>>I have just divded the SECTROT_WORDS (== 16) factor out
>>of all the places above ata_read.
>>
> 
> 
> You are right (assuming SECTOR_WORDS == 16. I was looking it up in
> 2.4.18 where SECTOR_WORDS is 512/4 == 128).  However, the new code looks
> overly complicated (at least for me, easily proven by my wrong first
> email :-), given that count is now always == 1.  Would the following not
> be nicer?
> 
> 	int i;
> 
> 	if (drive->type != ATA_DISK)
> 		return;
> 
> 	for (i = min(drive->mult_count, 1); i > 0; i--) {
> 		u32 buffer[SECTOR_WORDS];
> 
> 		ata_read(drive, buffer, SECTOR_WORDS);
> 	}
> 
> (This of course assumes that drive->mult_count is always non-negative)

Yes this looks nicer. Would you mind to test it and drop me
a patch?

Re: [PATCH] 2.5.8 IDE 36

[Norbert Kiesel]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]

On Tue, 2002-04-16 at 01:21, Martin Dalecki wrote:
> Norbert Kiesel wrote:
> > Hi,
> > 
> > while trying to understand recent kernel changes I stumbled over
> > the following patch to
> >  
> > diff -urN linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
> > --- linux-2.5.8/drivers/ide/ide.c	Tue Apr 16 06:01:07 2002
> > +++ linux/drivers/ide/ide.c	Tue Apr 16 05:38:37 2002
> > 
> > ...
> >  while (i > 0) {
> > -		u32 buffer[16];
> > -		unsigned int wcount = (i > 16) ? 16 : i;
> > -		i -= wcount;
> > -		ata_input_data (drive, buffer, wcount);
> > +		u32 buffer[SECTOR_WORDS];
> > +		unsigned int count = (i > 1) ? 1 : i;
> > +
> > +		ata_read(drive, buffer, count * SECTOR_WORDS);
> > +		i -= count;
> >  	}
> >  }
> > ...
> > 
> > While the old code called ata_input_read() with [0:16] as last param,
> > the new code calls the (renamed) ata_read() with either 0 or 16. Also,
> > the new code loops "i" times while the old code looped "i/16+1" times.
> > Was this intended or should the patch better read like:
> > 
> > ...
> >  while (i > 0) {
> > -               u32 buffer[16];
> > -               unsigned int wcount = (i > 16) ? 16 : i;
> > -               i -= wcount;
> > -               ata_input_data (drive, buffer, wcount);
> > +               u32 buffer[SECTOR_WORDS];
> > +               unsigned int count = max(i, SECTOR_WORDS);
> > +
> > +               ata_read(drive, buffer, count);
> > +               i -= count;
> >         }
> >  }
> > ...
> > 
> > so long
> 
> It's fine as it is I think. Please look up at the initialization of i.
> I have just divded the SECTROT_WORDS (== 16) factor out
> of all the places above ata_read.
>

You are right (assuming SECTOR_WORDS == 16. I was looking it up in
2.4.18 where SECTOR_WORDS is 512/4 == 128).  However, the new code looks
overly complicated (at least for me, easily proven by my wrong first
email :-), given that count is now always == 1.  Would the following not
be nicer?

	int i;

	if (drive->type != ATA_DISK)
		return;

	for (i = min(drive->mult_count, 1); i > 0; i--) {
		u32 buffer[SECTOR_WORDS];

		ata_read(drive, buffer, SECTOR_WORDS);
	}

(This of course assumes that drive->mult_count is always non-negative)

--nk

 
-- 
Key fingerprint = 6C58 F18D 4747 3295 F2DB  15C1 3882 4302 F8B4 C11C

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] 2.5.8 IDE 36

[Norbert Kiesel]

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

I can for sure provide a patch, testing will take a bit longer because I
currently only use 2.4.x. Give me 24h...

--nk

On Tue, 2002-04-16 at 02:20, Martin Dalecki wrote:
> Norbert Kiesel wrote:
> > On Tue, 2002-04-16 at 01:21, Martin Dalecki wrote:
> > 
> >>Norbert Kiesel wrote:
> >>
> >>>Hi,
> >>>
> >>>while trying to understand recent kernel changes I stumbled over
> >>>the following patch to
> >>> 
> >>>diff -urN linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
> >>>--- linux-2.5.8/drivers/ide/ide.c	Tue Apr 16 06:01:07 2002
> >>>+++ linux/drivers/ide/ide.c	Tue Apr 16 05:38:37 2002
> >>>
> >>>...
> >>> while (i > 0) {
> >>>-		u32 buffer[16];
> >>>-		unsigned int wcount = (i > 16) ? 16 : i;
> >>>-		i -= wcount;
> >>>-		ata_input_data (drive, buffer, wcount);
> >>>+		u32 buffer[SECTOR_WORDS];
> >>>+		unsigned int count = (i > 1) ? 1 : i;
> >>>+
> >>>+		ata_read(drive, buffer, count * SECTOR_WORDS);
> >>>+		i -= count;
> >>> 	}
> >>> }
> >>>...
> >>>
> >>>While the old code called ata_input_read() with [0:16] as last param,
> >>>the new code calls the (renamed) ata_read() with either 0 or 16. Also,
> >>>the new code loops "i" times while the old code looped "i/16+1" times.
> >>>Was this intended or should the patch better read like:
> >>>
> >>>...
> >>> while (i > 0) {
> >>>-               u32 buffer[16];
> >>>-               unsigned int wcount = (i > 16) ? 16 : i;
> >>>-               i -= wcount;
> >>>-               ata_input_data (drive, buffer, wcount);
> >>>+               u32 buffer[SECTOR_WORDS];
> >>>+               unsigned int count = max(i, SECTOR_WORDS);
> >>>+
> >>>+               ata_read(drive, buffer, count);
> >>>+               i -= count;
> >>>        }
> >>> }
> >>>...
> >>>
> >>>so long
> >>
> >>It's fine as it is I think. Please look up at the initialization of i.
> >>I have just divded the SECTROT_WORDS (== 16) factor out
> >>of all the places above ata_read.
> >>
> > 
> > 
> > You are right (assuming SECTOR_WORDS == 16. I was looking it up in
> > 2.4.18 where SECTOR_WORDS is 512/4 == 128).  However, the new code looks
> > overly complicated (at least for me, easily proven by my wrong first
> > email :-), given that count is now always == 1.  Would the following not
> > be nicer?
> > 
> > 	int i;
> > 
> > 	if (drive->type != ATA_DISK)
> > 		return;
> > 
> > 	for (i = min(drive->mult_count, 1); i > 0; i--) {
> > 		u32 buffer[SECTOR_WORDS];
> > 
> > 		ata_read(drive, buffer, SECTOR_WORDS);
> > 	}
> > 
> > (This of course assumes that drive->mult_count is always non-negative)
> 
> Yes this looks nicer. Would you mind to test it and drop me
> a patch?
> 
-- 
Key fingerprint = 6C58 F18D 4747 3295 F2DB  15C1 3882 4302 F8B4 C11C

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] 2.5.8 IDE 36

[Peter T. Breuer]

(mutters under breath .. let me put this back in the thread it came
from. Sorry. Double post for the records, with the right subject line
this time ...)

> On Tue, 16 Apr 2002, Alan Cox wrote:
> > We need to support partitioning on loopback devices in that case.
> 
> No, you just need to do the loopback over nbd - you need something to do
> the byte swapping anyway (ie you can't really use the normal "loop"
> device: I really just meant the more generic "loop the data back"
> approach).
> 
> nbd devices already do partitioning, I'm fairly certain.

Well, sort of. They make the right motions in the code, waving
their arms about in the right directions. It's doubtful if what's
there is enough.

> (But no, I've never tested it, obviously).

Neither have the authors. 

> Btw, while I'm at it - who out there actually uses the new "enbd"
> (Enhanced NBD)? I have this feeling that that would be the better choice,
> since unlike plain nbd it should be deadlock-free on localhost (ie you
> don't need a remote machine).

No - it will deadlock the same way. The deadlock is not related to the
device driver itself. It goes like this.

     kernel runs low on memory
     kernel flushes buffers to device drivers under pressure
     nbd client daemon sends to the net (potential conflict with tcp buffers,
        but not the mechanism that hurts)
     server on the _same machine_ receives request over the net
     server tries to write request to disk
     server process needs buffers to write to
     bang (or whatever deadlock sounds like)

The enbd client has a special "-a" flag which is meant to be used in
cases where we trust the transport medium. Localhost is such a case. 

With it, the protocol in the client daemon will ack the kernel _before_
it gets an ack from the remote server. That should help relieve the
deadlock. Things then go like this:

     kernel runs low on memory
     kernel flushes buffers to device drivers under pressure
     nbd client daemon sends to the net
        * client acks kernel and releases buffers in kernel
     server on the _same machine_ receives request over the net
     server tries to write request to disk
     server process needs buffers to write to
        * server gets buffers released by client in kernel

At least, potentially. If I recall right, there's still a deadlock
window in-kernel, but it's small. I don't recall the details. Oh -
well, the request still hangs around in the driver until the client
daemon acks .. maybe the client daemon can't ack without being swapped
in first, and that'd be deadlock.

Well, there's a "-s" flag (for swap devices) that does an mlockall()
that might take care of that. So "-a -s" might do it. Really needs
a "-aa" option (release write request in kernel asap).

However, a real cure would be not to use the network transport at all in
enbd.  Enbd can use different transports.  Instead of using the stub for
the server in the client daemon code, one could use the "fileserver"
object directly.  It'd just be a question of changing the
initialization.  Both have the same generic interface.

Incidentally, the newer VM code in 2.4 is a real problem for enbd.
I would ideally like _no_ _buffering_ whatsoever on the clientside.

As it is, it seems almost mandatory to raise the bdflush sync
boundary as high as possible, and to drop the async boundary really
low. One wants to start sending buffers out across the net
asynchronously as early as possible, and one never wants to go
sync. Going sync makes for a deadlock where we need to send
a request across the net in order to free a buffer, but we can't,
because we need some buffers to do that with ...

 * Is there any way of turning off the VM for a single device? *

The 2.5 i/o changes, otoh, seem immensely favourable to enbd, since
it's multithreaded and asyncronous in-kernel.


Peter

Re: [PATCH] 2.5.8 IDE 36

[Mike Fedyk]

On Wed, Apr 17, 2002 at 09:46:15AM +0200, Martin Dalecki wrote:
> Benjamin Herrenschmidt wrote:
> >something scary. I beleive the sanest solution that won't please
> >affected people is to _not_ support DMA on these broken HW ;)
> 
> No: the sane sollution would be to not support swapping disks between
> those systems and other systems.

Martin,

Go ahead and remove the byte swaping code for now, since it is a development
kernel after all...

*But*, make sure you put that on your to do list to add it back in a "sane"
way.

Re: [PATCH] 2.5.8 IDE 36

[David Lang]

My understanding of the old code was that it worked in PIO mode, but was
unsafe to use with DMA. I don't think anyone asking to have it upgraded to
work in DMA mode (not that anyone would object to that if it happened),
just don't remove the working PIO mode byteswap capability without some
thought as to how to achieve the same result.

also if another way becomes available to do the same job (i.e. loopback
with partition support) that will also solve my problem and I won't care
if direct byteswap support is removed.

what I do object to is the statement that "the sane solution would be to
not support swapping disks between these systems and other systems"

declaring that the capability that is currently in place and in use by
people should be eliminated with no replacement is wrong.

David Lang

On Wed, 17 Apr 2002, Petr Vandrovec wrote:

> On 17 Apr 02 at 2:39, David Lang wrote:
> > On Wed, 17 Apr 2002, Martin Dalecki wrote:
> >
> > > > Now, the problem of dealing with DMA along with the swapping is
> > > > something scary. I beleive the sanest solution that won't please
> > > > affected people is to _not_ support DMA on these broken HW ;)
> > >
> > > No: the sane sollution would be to not support swapping disks between
> > > those systems and other systems.
> >
> > in this case please send me a system compatable with my tivo so that I can
> > hack on it since you are telling me I'm not going to be able to swap disks
> > between it and any sane system.
> >
> > doing without DMA is very reasonable and not a significant problem (yes it
> > slows me down if I am duplicating drives, but if I am mounting the drive
> > so that I can go in and vi the startup files the speed difference doesn't
> > matter)
>
> I believe that if you'll create patch which will not byteswap data in
> place, and which will not slow system down, he'll accept it.
>
> As there are only three places where bswap should be checked
> (taskfile_input_data, taskfile_output_data, enabling DMA),
> it is trivial - just fork ata_{input,output}_data, and use insw_swapw/
> outsw_swapw in new variant. It will work long as driver properly
> diferentiates that taskfile_*_data is for data read to/from disk plates,
> while ata_*_data is for data produced by disk itself (identify & co.),
> and without speed difference, as PCI/VLB/ISA/disk/whatever is limiting
> factor for speed of ata_*_data function.
>                                                 Petr Vandrovec
>                                                 vandrove@vc.cvut.cz
>
>

Re: [PATCH] 2.5.8 IDE 36

[Petr Vandrovec]

On 17 Apr 02 at 2:39, David Lang wrote:
> On Wed, 17 Apr 2002, Martin Dalecki wrote:
> 
> > > Now, the problem of dealing with DMA along with the swapping is
> > > something scary. I beleive the sanest solution that won't please
> > > affected people is to _not_ support DMA on these broken HW ;)
> >
> > No: the sane sollution would be to not support swapping disks between
> > those systems and other systems.
> 
> in this case please send me a system compatable with my tivo so that I can
> hack on it since you are telling me I'm not going to be able to swap disks
> between it and any sane system.
> 
> doing without DMA is very reasonable and not a significant problem (yes it
> slows me down if I am duplicating drives, but if I am mounting the drive
> so that I can go in and vi the startup files the speed difference doesn't
> matter)

I believe that if you'll create patch which will not byteswap data in
place, and which will not slow system down, he'll accept it.

As there are only three places where bswap should be checked
(taskfile_input_data, taskfile_output_data, enabling DMA),
it is trivial - just fork ata_{input,output}_data, and use insw_swapw/
outsw_swapw in new variant. It will work long as driver properly 
diferentiates that taskfile_*_data is for data read to/from disk plates, 
while ata_*_data is for data produced by disk itself (identify & co.),
and without speed difference, as PCI/VLB/ISA/disk/whatever is limiting 
factor for speed of ata_*_data function.
                                                Petr Vandrovec
                                                vandrove@vc.cvut.cz
                                                

Re: [PATCH] 2.5.8 IDE 36

[David Lang]

On Wed, 17 Apr 2002, Martin Dalecki wrote:

> > Now, the problem of dealing with DMA along with the swapping is
> > something scary. I beleive the sanest solution that won't please
> > affected people is to _not_ support DMA on these broken HW ;)
>
> No: the sane sollution would be to not support swapping disks between
> those systems and other systems.

in this case please send me a system compatable with my tivo so that I can
hack on it since you are telling me I'm not going to be able to swap disks
between it and any sane system.

doing without DMA is very reasonable and not a significant problem (yes it
slows me down if I am duplicating drives, but if I am mounting the drive
so that I can go in and vi the startup files the speed difference doesn't
matter)

David Lang

Re: [PATCH] 2.5.8 IDE 36

[David Lang]

I use the byteswap feature to access these disks. It appears I was wrong
about the reason that I needed to use it, but the need is correct.

the fact is that for whatever reason the drives the tivo uses are
byteswapped compared to what they need to be for me to read them on an x86
system.

if another way is available I will use it, in the meantime I will not be
able to use 2.5 kernels to access those drives (since connecting and
disconnecting the drives requires a reboot this isn't a fatal flaw)

David Lang

On Wed, 17 Apr 2002, Martin Dalecki wrote:

> Date: Wed, 17 Apr 2002 09:44:54 +0200
> From: Martin Dalecki <dalecki@evision-ventures.com>
> To: David Lang <david.lang@digitalinsight.com>
> Cc: David S. Miller <davem@redhat.com>, vojtech@suse.cz,
>      rgooch@ras.ucalgary.ca, torvalds@transmeta.com,
>      linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] 2.5.8 IDE 36
>
> David Lang wrote:
> > Ok, in that case it must be a wierd wiring of the IDE or something.
> >
> > I thought it was something more logical (silly me :-)
> >
> > David Lang
>
> Please do me a favour - next time don't proclaim using some feature which you
> actually don't. I know that the grammatical conjunktiv form is nearly dead in
> english - but please intersperese the next time words like "could" "may be"
> OK?
>

Re: [PATCH] 2.5.8 IDE 36

[Anton Altaparmakov]

At 08:46 17/04/02, Martin Dalecki wrote:
>Benjamin Herrenschmidt wrote:
>>My understanding it that Tivo behaves like some Amiga's here
>>and has broken swapping of the IDE bus itself, not the ext2
>>filesystem.
>>On PPC, we still have some historical horrible macros redefinitions
>>in asm/ide.h to let APUS (PPC Amiga) deal with these.
>>Now, the problem of dealing with DMA along with the swapping is
>>something scary. I beleive the sanest solution that won't please
>>affected people is to _not_ support DMA on these broken HW ;)
>
>No: the sane sollution would be to not support swapping disks between
>those systems and other systems.

<disclaimer: rant>

No it isn't. You can't just go removing features people use. Your attitude 
as the new IDE maintainer is a bit distressing.

In the sake of a cleanup you start throwing away one feature after the 
other. IMHO cleanups are not worth feature removal, obviously your opinion 
differs. Hopefully, we will see the features come back but still the 
interim is annoying for some people...

Also, even more distressing is that you seem to be almost completely 
unresponsive to bug reports about your IDE changes completely breaking IDE. 
My email reporting in detail how any post 2.5.7 kernel fails to boot due to 
hanging during IDE device discovery was left unanswered. Off-line I am told 
you responded to another similar bug report trying to shift the blame to 
someone else's code and when it became apparent it was the IDE code you 
just stopped responding. Do you expect everyone to understand IDE and find 
and fix your bugs? A maintainer of a subsystem should work with people to 
find bugs and fix them, not expect users to do that... Your IDE patches 
keep flowing in one after the other but you completely ignore the fact that 
you broke IDE for some people along the way and the chances of it fixing 
itself by accident are minute... and finding the bug is probably getting 
harder with every patch you submit...

This is _not_ what I would expect from a maintainer of such an important 
subsystem!

</rant>

Apologies for the rant but I feel a lot better now.

Best regards,

Anton


-- 
   "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] 2.5.8 IDE 36

[Alan Cox]

> > A small number of other setups people wired the IDE the quick and easy
> > way and their native format is indeed ass backwards - some M68K disks and
> > the Tivo are examples of that. Interworking requires byteswapping and the
> > ability to handle byteswapped partition tables.
> 
> I said it already multiple times Alan - please note that the byte-swapping code
> for *physically* crosswired systems is *still there*. OK?

Thats not relevant to the discussion. I don't see why you brought it up. The
loopback stuff is about handling backward disks for interchange between 
systems or dealing with historical weirdnesses much more than physical layer

Re: [PATCH] 2.5.8 IDE 36

[Geert Uytterhoeven]

On Tue, 16 Apr 2002, Benjamin Herrenschmidt wrote:

> >   I could be wrong, it's a 2.1.x kernel that they started with. I thought
> >   that was around the time the fix went in.
> >   
> >Again, I did the fix 6 years ago, thats pre-2.0.x days
> >
> >EXT2 has been little-endian only with proper byte-swapping support
> >across all architectures, since that time.
> 
> My understanding it that Tivo behaves like some Amiga's here
> and has broken swapping of the IDE bus itself, not the ext2
> filesystem.

You mean Ataris and Q40/Q60s?

I'm not aware if any Amiga IDE interface with byteswapped IDE interface. Or it
must be a very rare interface not supported by Linux anyway ;-)

> On PPC, we still have some historical horrible macros redefinitions
> in asm/ide.h to let APUS (PPC Amiga) deal with these.

In asm-ppc/ide.h? Didn't see them there.

The main problem is that IDE use[sd] `inb' et al. for accesses, which is not
valid for I/O on something other than ISA/PCI I/O space. So for m68k and APUS
we have to do weird things. The new IN_BYTE() etc. should help a bit there,
though.

> Now, the problem of dealing with DMA along with the swapping is
> something scary. I beleive the sanest solution that won't please
> affected people is to _not_ support DMA on these broken HW ;)

Agreed. And you have to disable DMA when accessing a disk that originates from
such a system on a sane box.

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] 2.5.8 IDE 36

[Martin Dalecki]

Benjamin Herrenschmidt wrote:

> We tweak on pmac by feeding the IDE layer with our controller virtual address
> minus _IO_BASE (for non-PPC people, _IO_BASE is the virtual address of the
> main PCI IO space, all inx/outx are relative to this). The pointer arithmetic
> does the magic. It sucks, but works without redefining everything around.
> I haven't looked at the new IN_BYTE stuff, though if it is IDE specific,
> I'd rather see it called IDE_IN_BYTE. The current scheme sucks also because
> inx/outx, at least on PPC, are a lot slower than normal MMIO access (one
> reason beeing their ability to recovert from machine checks). It would be
> nice for IDE to use it's own accessors on MMIO platforms. This has to be
> a per-controller things though. A global macro is no good. You can (and on
> some configs, you do have on the motherboard) both MMIO mapped controllers
> and old-style IO mapped ones. One example is the B&W mac G3 which has both
> the Apple MMIO mapped mac-io IDE controller and the CMD646 on the PCI bus.
> 
> Also, when applying the taskfile, I suspect we don't need strong barriers as
> we do currently have, only on IO write barrier before actually writing the
> command byte. But I would gladly leave the whole issue of redefining barriers
> especially regarding IOs to Anton Blanchard ;)
> 
> Maybe the entire function for writing a taskfile register state to the
> controller should be made a hwif indirect call. (On Darwin, they more or
> less do that, along with a bitmask indicating which register has to be
> applied, though I suspect the tests against this bitmask would eats pretty
> much all of the benefit of removing the useless barriers).

Thank you for the elaborated explanation. I think that some
of your ideas presented here could be well pursued becouse they are
indeed good. :-).

Re: [PATCH] 2.5.8 IDE 36

[Geert Uytterhoeven]

On Tue, 16 Apr 2002, David S. Miller wrote:
>    From: David Lang <david.lang@digitalinsight.com>
>    Date: Tue, 16 Apr 2002 10:09:38 -0700 (PDT)
> 
>    I could be wrong, it's a 2.1.x kernel that they started with. I thought
>    that was around the time the fix went in.
>    
> Again, I did the fix 6 years ago, thats pre-2.0.x days
> 
> EXT2 has been little-endian only with proper byte-swapping support
> across all architectures, since that time.

On SPARC. M68k followed a bit later. But when I got my CHRP board, ext2 was
still big endian on (at least some) PPC boxes, so PPC must have been switched
over in 1997/1998.

I tried to find the exact date of the appearance of the `-s' option of e2fsck
in the changelog og e2fsprogs, but apparently not all changes are mentioned
there.

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] 2.5.8 IDE 36

[Martin Dalecki]

Brian Gerst wrote:

> 
> There is a typo in the cris ide driver ata_write value.  Also,
> e100_ideproc is now dead and can be removed.  Patch attached (untested, 
> but obvious).
>

You are right. And your patch does the proper thing.
I thank you very much for looking in to this!

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

Benjamin Herrenschmidt wrote:
>>  I could be wrong, it's a 2.1.x kernel that they started with. I thought
>>  that was around the time the fix went in.
>>  
>>Again, I did the fix 6 years ago, thats pre-2.0.x days
>>
>>EXT2 has been little-endian only with proper byte-swapping support
>>across all architectures, since that time.
> 
> 
> My understanding it that Tivo behaves like some Amiga's here
> and has broken swapping of the IDE bus itself, not the ext2
> filesystem.
> 
> On PPC, we still have some historical horrible macros redefinitions
> in asm/ide.h to let APUS (PPC Amiga) deal with these.
> 
> Now, the problem of dealing with DMA along with the swapping is
> something scary. I beleive the sanest solution that won't please
> affected people is to _not_ support DMA on these broken HW ;)

No: the sane sollution would be to not support swapping disks between
those systems and other systems.

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

David Lang wrote:
> Ok, in that case it must be a wierd wiring of the IDE or something.
> 
> I thought it was something more logical (silly me :-)
> 
> David Lang

Please do me a favour - next time don't proclaim using some feature which you
actually don't. I know that the grammatical conjunktiv form is nearly dead in
english - but please intersperese the next time words like "could" "may be"
OK?

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

Linus Torvalds wrote:
> 
> On Tue, 16 Apr 2002, Richard Gooch wrote:
> 
>>What I object to is the removal of a feature that people depend on,
>>*without a replacement being made available prior to removal*. If you
>>want to remove a feature, build the replacement *first*. Don't remove
>>the feature and say "the rest of you can pick up the pieces".
> 
> 
> Hey, that happens all the time - look how many times VFS changes have made
> various filesystems unusable (including yours ;)

Another example from the real world: Do you always have to elect democratically
a new president, before you assult the bad dictator?

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

Alan Cox wrote:
>>Doing it with a loopback like interface at a higher level is the much 
>>saner operation - I understand why Martin removed the byteswap support, 
>>and agree with it 100%. It just didn't make any sense from a driver 
>>standpoint.
> 
> 
> We need to support partitioning on loopback devices in that case.
> 
> 
>>The only reason byteswapping exists is a rather historical one: Linux did 
>>the wrong thing for "insw/outsw" on big-endian architectures at one point 
>>(it byteswapped the data).
> 
> 
> A small number of other setups people wired the IDE the quick and easy
> way and their native format is indeed ass backwards - some M68K disks and
> the Tivo are examples of that. Interworking requires byteswapping and the
> ability to handle byteswapped partition tables.

I said it already multiple times Alan - please note that the byte-swapping code
for *physically* crosswired systems is *still there*. OK?

Re: [PATCH] 2.5.8 IDE 36

[Benjamin Herrenschmidt]

>On Tue, 16 Apr 2002, Benjamin Herrenschmidt wrote:
>
>> >   I could be wrong, it's a 2.1.x kernel that they started with. I thought
>> >   that was around the time the fix went in.
>> >   
>> >Again, I did the fix 6 years ago, thats pre-2.0.x days
>> >
>> >EXT2 has been little-endian only with proper byte-swapping support
>> >across all architectures, since that time.
>> 
>> My understanding it that Tivo behaves like some Amiga's here
>> and has broken swapping of the IDE bus itself, not the ext2
>> filesystem.
>
>You mean Ataris and Q40/Q60s?
>
>I'm not aware if any Amiga IDE interface with byteswapped IDE interface.
Or it
>must be a very rare interface not supported by Linux anyway ;-)

My confusion then. We used to have these in the PPC tree, and I
suspected those were APUS related ;)

>> On PPC, we still have some historical horrible macros redefinitions
>> in asm/ide.h to let APUS (PPC Amiga) deal with these.
>
>In asm-ppc/ide.h? Didn't see them there.

Right, looks like they are gone lately. Well, the TiVO may have been one
reason for these though I don't think the support for this box has ever
made it into our main tree. It's possible that we had some broken PReP
or whatever early boxes.

>The main problem is that IDE use[sd] `inb' et al. for accesses, which is not
>valid for I/O on something other than ISA/PCI I/O space. So for m68k and APUS
>we have to do weird things. The new IN_BYTE() etc. should help a bit there,
>though.

We tweak on pmac by feeding the IDE layer with our controller virtual address
minus _IO_BASE (for non-PPC people, _IO_BASE is the virtual address of the
main PCI IO space, all inx/outx are relative to this). The pointer arithmetic
does the magic. It sucks, but works without redefining everything around.
I haven't looked at the new IN_BYTE stuff, though if it is IDE specific,
I'd rather see it called IDE_IN_BYTE. The current scheme sucks also because
inx/outx, at least on PPC, are a lot slower than normal MMIO access (one
reason beeing their ability to recovert from machine checks). It would be
nice for IDE to use it's own accessors on MMIO platforms. This has to be
a per-controller things though. A global macro is no good. You can (and on
some configs, you do have on the motherboard) both MMIO mapped controllers
and old-style IO mapped ones. One example is the B&W mac G3 which has both
the Apple MMIO mapped mac-io IDE controller and the CMD646 on the PCI bus.

Also, when applying the taskfile, I suspect we don't need strong barriers as
we do currently have, only on IO write barrier before actually writing the
command byte. But I would gladly leave the whole issue of redefining barriers
especially regarding IOs to Anton Blanchard ;)

Maybe the entire function for writing a taskfile register state to the
controller should be made a hwif indirect call. (On Darwin, they more or
less do that, along with a bitmask indicating which register has to be
applied, though I suspect the tests against this bitmask would eats pretty
much all of the benefit of removing the useless barriers).

>> Now, the problem of dealing with DMA along with the swapping is
>> something scary. I beleive the sanest solution that won't please
>> affected people is to _not_ support DMA on these broken HW ;)
>
>Agreed. And you have to disable DMA when accessing a disk that
originates from
>such a system on a sane box.

Agreed.

Ben.

Re: [PATCH] 2.5.8 IDE 36

[Brian Gerst]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

Martin Dalecki wrote:
> Tue Apr 16 01:02:47 CEST 2002 ide-clean-36
> 
> - Consolidate ide_choose_drive() and choose_drive() in to one function.
> 
> - Remove sector data byteswpapping support. Byte-swapping the data is 
> supported
>   on the file-system level where applicable.  Byte-swapped interfaces are
>   supported on a lower level anyway. And finally it was used 
> inconsistently.
> 
> - Eliminate taskfile_input_data() and taskfile_output_data(). This 
> allowed us
>   to split up ideproc and eliminate the ugly action switch as well as the
>   corresponding defines.

There is a typo in the cris ide driver ata_write value.  Also,
e100_ideproc is now dead and can be removed.  Patch attached (untested, 
but obvious).

-- 

						Brian Gerst

[-- Attachment #2: ide36-1 --]
[-- Type: text/plain, Size: 1689 bytes --]

diff -urN linux-ide/arch/cris/drivers/ide.c linux/arch/cris/drivers/ide.c
--- linux-ide/arch/cris/drivers/ide.c	Tue Apr 16 18:37:43 2002
+++ linux/arch/cris/drivers/ide.c	Tue Apr 16 18:40:18 2002
@@ -192,8 +192,6 @@
 #define ATA_PIO0_HOLD    4
 
 static int e100_dmaproc (ide_dma_action_t func, ide_drive_t *drive);
-static void e100_ideproc (ide_ide_action_t func, ide_drive_t *drive,
-			  void *buffer, unsigned int length);
 
 /*
  * good_dma_drives() lists the model names (from "hdparm -i")
@@ -280,7 +278,7 @@
 		hwif->tuneproc = &tune_e100_ide;
 		hwif->dmaproc = &e100_dmaproc;
 		hwif->ata_read = e100_ide_input_data;
-		hwif->ata_write = e100_ide_input_data;
+		hwif->ata_write = e100_ide_output_data;
 		hwif->atapi_read = e100_atapi_read;
 		hwif->atapi_write = e100_atapi_write;
 	}
@@ -560,32 +558,6 @@
 	e100_atapi_write(drive, buffer, wcount << 2);
 }
 
-/*
- * The multiplexor for ide_xxxput_data and atapi calls
- */
-static void 
-e100_ideproc (ide_ide_action_t func, ide_drive_t *drive,
-	      void *buffer, unsigned int length)
-{
-	switch (func) {
-		case ideproc_ide_input_data:
-			e100_ide_input_data(drive, buffer, length);
-			break;
-		case ideproc_ide_output_data:
-			e100_ide_input_data(drive, buffer, length);
-			break;
-		case ideproc_atapi_read:
-			e100_atapi_read(drive, buffer, length);
-			break;
-		case ideproc_atapi_write:
-			e100_atapi_write(drive, buffer, length);
-			break;
-		default:
-			printk("e100_ideproc: unsupported func %d!\n", func);
-			break;
-	}
-}
-
 /* we only have one DMA channel on the chip for ATA, so we can keep these statically */
 static etrax_dma_descr ata_descrs[MAX_DMA_DESCRS];
 static unsigned int ata_tot_size;

Re: [PATCH] 2.5.8 IDE 36

[Andreas Dilger]

On Apr 16, 2002  09:01 -0700, Linus Torvalds wrote:
> And I know from personal experience that allowing partitioning of a
> loopback thing would certainly have made some things a _lot_ easier (ie
> not having to figure out the damn offsets in order to mount a filesystem
> on a loopback volume), so having support for partitioning would be good.

This can be done trivially in user-space without breaking any existing
code and without having to enable partitioning of a single loop device.
All that we need to do is take a tool like partx (from util-linux)
to decode the partition table and find the partition offset+size, and then
feed that to losetup (or call the loop setup ioctl directly) for each
partition.  You get a bunch of loop devices set up, one for each partition.

The only thing one might want to have is some option to losetup to list
all of the loop devices currently set up ("losetup -a" or something) so
you can see which loop device corresponds to what disk partition.  Of
course the partx tool would also print that out at setup time, but
people have short memories for this sort of stuff.

> Although I do have this suspicion that that partitioning support should be
> in user space (along with all the rest of the partitioning support, but
> that's another matter and has some rather more serious backwards
> compatibility issues, of course.

The partx tool (and GNU parted as well) _already_ can grok partitions
from user-space and tell the kernel about them (I had set up some
GPT/ia64 partitions on my ia32 box and mounted them just fine).  All
that needs to be done is run this in early setup before we need to
mount the root filesystem.

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

Re: [PATCH] 2.5.8 IDE 36

[Benjamin Herrenschmidt]

>   I could be wrong, it's a 2.1.x kernel that they started with. I thought
>   that was around the time the fix went in.
>   
>Again, I did the fix 6 years ago, thats pre-2.0.x days
>
>EXT2 has been little-endian only with proper byte-swapping support
>across all architectures, since that time.

My understanding it that Tivo behaves like some Amiga's here
and has broken swapping of the IDE bus itself, not the ext2
filesystem.

On PPC, we still have some historical horrible macros redefinitions
in asm/ide.h to let APUS (PPC Amiga) deal with these.

Now, the problem of dealing with DMA along with the swapping is
something scary. I beleive the sanest solution that won't please
affected people is to _not_ support DMA on these broken HW ;)

Another way would be, for such broken controllers, to break the
request into 1 page max requests and let them all go through a
bounce buffer.

Ben.

Re: [PATCH] 2.5.8 IDE 36

[David Lang]

Ok, in that case it must be a wierd wiring of the IDE or something.

I thought it was something more logical (silly me :-)

David Lang

On Tue, 16 Apr 2002, David S. Miller wrote:

> Date: Tue, 16 Apr 2002 10:06:10 -0700 (PDT)
> From: David S. Miller <davem@redhat.com>
> To: david.lang@digitalinsight.com
> Cc: vojtech@suse.cz, dalecki@evision-ventures.com, rgooch@ras.ucalgary.ca,
>      torvalds@transmeta.com, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] 2.5.8 IDE 36
>
>    From: David Lang <david.lang@digitalinsight.com>
>    Date: Tue, 16 Apr 2002 10:09:38 -0700 (PDT)
>
>    I could be wrong, it's a 2.1.x kernel that they started with. I thought
>    that was around the time the fix went in.
>
> Again, I did the fix 6 years ago, thats pre-2.0.x days
>
> EXT2 has been little-endian only with proper byte-swapping support
> across all architectures, since that time.
>

Re: [PATCH] 2.5.8 IDE 36

[David Lang]

I could be wrong, it's a 2.1.x kernel that they started with. I thought
that was around the time the fix went in.

David Lang

On Tue, 16 Apr 2002, David S. Miller wrote:

> Date: Tue, 16 Apr 2002 10:00:55 -0700 (PDT)
> From: David S. Miller <davem@redhat.com>
> To: david.lang@digitalinsight.com
> Cc: vojtech@suse.cz, dalecki@evision-ventures.com, rgooch@ras.ucalgary.ca,
>      torvalds@transmeta.com, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] 2.5.8 IDE 36
>
>    From: David Lang <david.lang@digitalinsight.com>
>    Date: Tue, 16 Apr 2002 10:04:02 -0700 (PDT)
>
>    On Tue, 16 Apr 2002, Vojtech Pavlik wrote:
>
>    > Because for Linux filesystems it was decided some time ago (after people
>    > HAD huge byteswap problems) that ext2 is always LE, etc, etc. So
>    > filesystems are supposed to be the same on every system.
>
>    In the case of Tivo they are useing a kernel from the time before the fix
>    went in so even their ext2 partitions are incorrect (not to mention their
>    other partitions that aren't ext2)
>
> That's absurd.  I made the fix 6 years ago, I doubt they are using a
> kernel older than that.
>

Re: [PATCH] 2.5.8 IDE 36

[Vojtech Pavlik]

On Tue, Apr 16, 2002 at 05:23:11PM +0100, Alan Cox wrote:
> > No, you just need to do the loopback over nbd - you need something to do
> > the byte swapping anyway (ie you can't really use the normal "loop"
> > device: I really just meant the more generic "loop the data back"
> > approach).
> 
> nbd goes via the networking layer and deadlocks if looped. The loop driver
> is also much faster. Partitioned loop doesnt seem hard.

And it'd be very cool for stuff like mounting Bochs disk images and
similar.

> 
> > nbd devices already do partitioning, I'm fairly certain.
> 
> Not when I checked.
> 
> > > the Tivo are examples of that. Interworking requires byteswapping and the
> > > ability to handle byteswapped partition tables.
> > 
> > Note that THAT case is an architecture issue, and should probably be
> > handled by just making the IDE "insw" macro do the byteswapping natively.
> > That way you don't get the current "it can actually corrupt your
> > filesystem on SMP" behaviour.
> 
> Thats still not enough. If you have the ide insw macro then control 
> transfers come out wrong. And to maximise the pain - some Amiga controllers
> are backwards some are not. 
> 	"The use of excessive force has been authorised in the ..."
> 
> And then people stick TiVo disks in their PC's in order to prep them for
> various TiVo hackery.

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] 2.5.8 IDE 36

[David S. Miller]

   From: David Lang <david.lang@digitalinsight.com>
   Date: Tue, 16 Apr 2002 10:09:38 -0700 (PDT)

   I could be wrong, it's a 2.1.x kernel that they started with. I thought
   that was around the time the fix went in.
   
Again, I did the fix 6 years ago, thats pre-2.0.x days

EXT2 has been little-endian only with proper byte-swapping support
across all architectures, since that time.

Re: [PATCH] 2.5.8 IDE 36

[David Lang]

On Tue, 16 Apr 2002, Vojtech Pavlik wrote:

> Because for Linux filesystems it was decided some time ago (after people
> HAD huge byteswap problems) that ext2 is always LE, etc, etc. So
> filesystems are supposed to be the same on every system.

In the case of Tivo they are useing a kernel from the time before the fix
went in so even their ext2 partitions are incorrect (not to mention their
other partitions that aren't ext2)

David Lang

Re: [PATCH] 2.5.8 IDE 36

[David S. Miller]

   From: David Lang <david.lang@digitalinsight.com>
   Date: Tue, 16 Apr 2002 10:04:02 -0700 (PDT)

   On Tue, 16 Apr 2002, Vojtech Pavlik wrote:
   
   > Because for Linux filesystems it was decided some time ago (after people
   > HAD huge byteswap problems) that ext2 is always LE, etc, etc. So
   > filesystems are supposed to be the same on every system.
   
   In the case of Tivo they are useing a kernel from the time before the fix
   went in so even their ext2 partitions are incorrect (not to mention their
   other partitions that aren't ext2)
   
That's absurd.  I made the fix 6 years ago, I doubt they are using a
kernel older than that.

Re: [PATCH] 2.5.8 IDE 36

[Vojtech Pavlik]

On Tue, Apr 16, 2002 at 08:46:31AM -0700, Linus Torvalds wrote:
> 
> On Tue, 16 Apr 2002, Vojtech Pavlik wrote:
> > 
> > Note that the above commands are no help in case of plugging TIVO
> > drive into a PC. While they assure that all ext2 filesystems are LE on
> > the media and all sun disklabels are BE on the media, still if you plug
> > in a BE ext2 into the system (or a BE PC partition table), the kernel
> > won't understand them.
> 
> Please use a the network block device, and teach the ndb deamon to just 
> byteswap each word.
> 
> Problem solved, WITHOUT keeping bugs in the IDE driver.

Yeah, that's a pretty cool idea.

> Oh, and performance improved at the same time.
> 
> What are you guys thinging about? There are two rules here:
>  - optimize for the common case
>  - keep the code clean.

There is also this one:

   - don't remove existing features if you don't have an usable
     replacement or users will hate you.

> Both of them say that Martin is 100% right.

Now that you've come up with the NBD idea, I have to agree.

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] 2.5.8 IDE 36

[Padraig Brady]

Linus Torvalds wrote:
> 
> On Tue, 16 Apr 2002, Alan Cox wrote:
> 
>>>Please use a the network block device, and teach the ndb deamon to just
>>>byteswap each word.
>>
>>You need to use loop not nbd - loopback nbd can deadlock. Byteswap as a
>>new revolutionary crypto system for the loopback driver isnt hard
> 
> 
> Even better - I did indeed miss the "security" aspect of the byteswapping
> ;)
> 
> And I know from personal experience that allowing partitioning of a
> loopback thing would certainly have made some things a _lot_ easier (ie
> not having to figure out the damn offsets in order to mount a filesystem
> on a loopback volume), so having support for partitioning would be good.

gpart is good for this:
For e.g:

$gpart -vgd partitions.img

dev(partitions.img) mss(512)

Primary partition(1)
    type: 131(0x83)(Linux ext2 filesystem)
    size: 2mb #s(4576) s(32-4607)
    chs:  (0/1/1)-(8/15/32)d (0/0/0)-(0/0/0)r
    hex:  00 01 01 00 83 0F 20 08 20 00 00 00 E0 11 00 00

Primary partition(2)
    type: 131(0x83)(Linux ext2 filesystem)
    size: 59mb #s(121856) s(4608-126463)
    chs:  (9/0/1)-(246/15/32)d (0/0/0)-(0/0/0)r
    hex:  00 00 01 09 83 0F 20 F6 00 12 00 00 00 DC 01 00

The pertinent info here is s(32-4607) & s(4608-126463).
Blocks are 512 bytes so in this e.g. the offsets for
the first and second partitions respectively are:
16384 & 2359296

> Although I do have this suspicion that that partitioning support should be
> in user space (along with all the rest of the partitioning support, but
> that's another matter and has some rather more serious backwards
> compatibility issues, of course. Is anybody still working on the new early
> initrd?).
> 
> 		Linus

Padraig.

Re: [PATCH] 2.5.8 IDE 36

[Alan Cox]

> compatibility issues, of course. Is anybody still working on the new early
> initrd?).

Bits of it exist - the dhcp/bootp client in userspace initrd is out there and
in real world use courtesy of the LTSP - who wrote it before anyone had even
thought about the initrd stuff 8)

Re: [PATCH] 2.5.8 IDE 36

[Alan Cox]

> No, you just need to do the loopback over nbd - you need something to do
> the byte swapping anyway (ie you can't really use the normal "loop"
> device: I really just meant the more generic "loop the data back"
> approach).

nbd goes via the networking layer and deadlocks if looped. The loop driver
is also much faster. Partitioned loop doesnt seem hard.

> nbd devices already do partitioning, I'm fairly certain.

Not when I checked.

> > the Tivo are examples of that. Interworking requires byteswapping and the
> > ability to handle byteswapped partition tables.
> 
> Note that THAT case is an architecture issue, and should probably be
> handled by just making the IDE "insw" macro do the byteswapping natively.
> That way you don't get the current "it can actually corrupt your
> filesystem on SMP" behaviour.

Thats still not enough. If you have the ide insw macro then control 
transfers come out wrong. And to maximise the pain - some Amiga controllers
are backwards some are not. 
	"The use of excessive force has been authorised in the ..."

And then people stick TiVo disks in their PC's in order to prep them for
various TiVo hackery.

Re: [PATCH] 2.5.8 IDE 36

[Alan Cox]

> Please use a the network block device, and teach the ndb deamon to just 
> byteswap each word.

You need to use loop not nbd - loopback nbd can deadlock. Byteswap as a 
new revolutionary crypto system for the loopback driver isnt hard

Alan

Re: [PATCH] 2.5.8 IDE 36

[Linus Torvalds]

On Tue, 16 Apr 2002, Richard Gooch wrote:
>
> What I object to is the removal of a feature that people depend on,
> *without a replacement being made available prior to removal*. If you
> want to remove a feature, build the replacement *first*. Don't remove
> the feature and say "the rest of you can pick up the pieces".

Hey, that happens all the time - look how many times VFS changes have made
various filesystems unusable (including yours ;)

The fact is, many things are easier to fix afterwards. Particularly
because that's the only time you'll find people motivated enough to bother
about it. If you were to need to fix everything before-the-fact, nothing
fundamental would ever get fixed, simply because the people who can fix
one thing are not usually the same people who can fix another.

(Just to take an example that _isn't_ the IDE driver, this is exactly what
the more generic bio changes have been an example of - it's just
inconceivable to fix every use of the old request interface, so somebody
has to take the first step and taker the heat about it. Otherwise it never
gets anywhere)

		Linus

Re: [PATCH] 2.5.8 IDE 36

[Alan Cox]

> Doing it with a loopback like interface at a higher level is the much 
> saner operation - I understand why Martin removed the byteswap support, 
> and agree with it 100%. It just didn't make any sense from a driver 
> standpoint.

We need to support partitioning on loopback devices in that case.

> The only reason byteswapping exists is a rather historical one: Linux did 
> the wrong thing for "insw/outsw" on big-endian architectures at one point 
> (it byteswapped the data).

A small number of other setups people wired the IDE the quick and easy
way and their native format is indeed ass backwards - some M68K disks and
the Tivo are examples of that. Interworking requires byteswapping and the
ability to handle byteswapped partition tables.

Given the ability to see partitions on loop devices all works out I think

Alan

Re: [PATCH] 2.5.8 IDE 36

[Linus Torvalds]

On Tue, 16 Apr 2002, Alan Cox wrote:
> > Please use a the network block device, and teach the ndb deamon to just
> > byteswap each word.
>
> You need to use loop not nbd - loopback nbd can deadlock. Byteswap as a
> new revolutionary crypto system for the loopback driver isnt hard

Even better - I did indeed miss the "security" aspect of the byteswapping
;)

And I know from personal experience that allowing partitioning of a
loopback thing would certainly have made some things a _lot_ easier (ie
not having to figure out the damn offsets in order to mount a filesystem
on a loopback volume), so having support for partitioning would be good.

Although I do have this suspicion that that partitioning support should be
in user space (along with all the rest of the partitioning support, but
that's another matter and has some rather more serious backwards
compatibility issues, of course. Is anybody still working on the new early
initrd?).

		Linus

Re: [PATCH] 2.5.8 IDE 36

[Richard Gooch]

Linus Torvalds writes:
> 
> On Tue, 16 Apr 2002, Richard Gooch wrote:
> > 
> > This gratuitous removal of features in the guise of "cleanups" is why
> > you got flamed earlier this year. I thought you'd learned :-/
> 
> Richard, have you looked at the IDE mess?

Yeah, years ago when I was adding devfs calls. I've tried to forget
about it since then...

> Also note that performance is likely to _increase_ by removing that
> stupid feature - using DMA to do the actual IO and them byteswapping
> in some higher level than the driver is likely to be a _lot_ faster
> than doing PIO (and byteswap in-place, resulting in random mmap
> corruption).

I'm actually not that concerned about performance for this case,
because it's not a common operation. If we had some kind of loop
driver that supported partitioning then I'd be satisfied. In fact, I
agree that would probably be better.

What I object to is the removal of a feature that people depend on,
*without a replacement being made available prior to removal*. If you
want to remove a feature, build the replacement *first*. Don't remove
the feature and say "the rest of you can pick up the pieces".

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] 2.5.8 IDE 36

[Linus Torvalds]

On Tue, 16 Apr 2002, Alan Cox wrote:
>
> We need to support partitioning on loopback devices in that case.

No, you just need to do the loopback over nbd - you need something to do
the byte swapping anyway (ie you can't really use the normal "loop"
device: I really just meant the more generic "loop the data back"
approach).

nbd devices already do partitioning, I'm fairly certain.

(But no, I've never tested it, obviously).

Btw, while I'm at it - who out there actually uses the new "enbd"
(Enhanced NBD)? I have this feeling that that would be the better choice,
since unlike plain nbd it should be deadlock-free on localhost (ie you
don't need a remote machine).

> > The only reason byteswapping exists is a rather historical one: Linux did
> > the wrong thing for "insw/outsw" on big-endian architectures at one point
> > (it byteswapped the data).
>
> A small number of other setups people wired the IDE the quick and easy
> way and their native format is indeed ass backwards - some M68K disks and
> the Tivo are examples of that. Interworking requires byteswapping and the
> ability to handle byteswapped partition tables.

Note that THAT case is an architecture issue, and should probably be
handled by just making the IDE "insw" macro do the byteswapping natively.
That way you don't get the current "it can actually corrupt your
filesystem on SMP" behaviour.

(Although I suspect that none of the architectures in question have ever
even jokingly considered SMP support ;)

			Linus

Re: [PATCH] 2.5.8 IDE 36

[Linus Torvalds]

On Tue, 16 Apr 2002, Vojtech Pavlik wrote:
> 
> Note that the above commands are no help in case of plugging TIVO
> drive into a PC. While they assure that all ext2 filesystems are LE on
> the media and all sun disklabels are BE on the media, still if you plug
> in a BE ext2 into the system (or a BE PC partition table), the kernel
> won't understand them.

Please use a the network block device, and teach the ndb deamon to just 
byteswap each word.

Problem solved, WITHOUT keeping bugs in the IDE driver.

Oh, and performance improved at the same time.

What are you guys thinging about? There are two rules here:
 - optimize for the common case
 - keep the code clean.

Both of them say that Martin is 100% right.

		Linus

Re: [PATCH] 2.5.8 IDE 36

[Linus Torvalds]

On Tue, 16 Apr 2002, Richard Gooch wrote:
> 
> This gratuitous removal of features in the guise of "cleanups" is why
> you got flamed earlier this year. I thought you'd learned :-/

Richard, have you looked at the IDE mess? That "feature" is a bug, the way 
it was implemented - and considering that it's implementable at a 
different level much more cleanly for the (few) people who actually need 
it...

Also note that performance is likely to _increase_ by removing that stupid
feature - using DMA to do the actual IO and them byteswapping in some
higher level than the driver is likely to be a _lot_ faster than doing PIO
(and byteswap in-place, resulting in random mmap corruption).

Do you realize that because the current bswap writeback reverses the bytes 
in place, you can actually seriously corrupt your filesystem by just being 
unlucky in timing (ie a bswap on a dirty metadata block at the same time 
another process accesses it)?

It's a BUG guys, not a feature.

		Linus

Re: [PATCH] 2.5.8 IDE 36

[Linus Torvalds]

On Tue, 16 Apr 2002, David Lang wrote:
> 
> It sounds as if you are removing this capability, am I misunderstaning you
> or is there some other way to do this? (and duplicating the drive to use
> dd to byteswap is not practical for 100G+)

Doing it with a loopback like interface at a higher level is the much 
saner operation - I understand why Martin removed the byteswap support, 
and agree with it 100%. It just didn't make any sense from a driver 
standpoint.

In fact, the byteswapping was actively incorrect, in that it swapped data 
in-place - which means that it corrupts the data area while IO is in 
progress. It also only works for PIO.

The only reason byteswapping exists is a rather historical one: Linux did 
the wrong thing for "insw/outsw" on big-endian architectures at one point 
(it byteswapped the data).

(Oh, and coupled with the fact that the IDE ID string is in a "big-endian"  
word order, which may have been one more reason to add a "do byteswapped 
IO" thing).

		Linus

Re: [PATCH] 2.5.8 IDE 36

[Vojtech Pavlik]

On Tue, Apr 16, 2002 at 03:49:57PM +0200, Martin Dalecki wrote:
> Richard Gooch wrote:
> > Martin Dalecki writes:
> > 
> >>David Lang wrote:
> >>
> >>>The common thing I use byteswap for is to mount my tivo (kernel 2.1.x)
> >>>drives on my PC (2.4/5.x).  those drives are byteswapped throughout the
> >>>entire drive, including the partition table.
> >>>
> >>>It sounds as if you are removing this capability, am I misunderstaning you
> >>>or is there some other way to do this? (and duplicating the drive to use
> >>>dd to byteswap is not practical for 100G+)
> >>
> >>Same problem as with SCSI disks, which are even more commonly moved
> >>between different system types - please look there for a solution.
> >>BTW. I hardly beleve that your tivo is containing a DOS partition
> >>table - otherwise the partition table will handle it all
> >>autmagically.
> > 
> > 
> > Well, there *is* a partition table on the drive, but the byte-swapping
> > isn't handled automatically. Otherwise people wouldn't need to bswap
> > their TiVo drives when plugging into an x86 box.
> > 
> > Having the bswap option is definately useful. With it, you can "bless"
> > the drive and then mount the partitions and poke around. Please don't
> > remove the bswap option. You'll make life harder for a bunch of
> > people.
> 
> Please note one sample from ext2 code:
> 
> 	if (le32_to_cpu(es->s_rev_level) > EXT2_GOOD_OLD_REV)
> 
> And from partition handling code linux/fs/partitions/sun.c:
> 
> 	/* All Sun disks have 8 partition entries */
> 	spc = be16_to_cpu(label->ntrks) * be16_to_cpu(label->nsect);
> 
> You see those many le32_to_cpu() and reverse commands?
> If there is something that has to be fixed for linux/fs/partition/whatever.c -
> please fix it there if you are actually *needing* it.

Note that the above commands are no help in case of plugging TIVO
drive into a PC. While they assure that all ext2 filesystems are LE on
the media and all sun disklabels are BE on the media, still if you plug
in a BE ext2 into the system (or a BE PC partition table), the kernel
won't understand them.

> You also notice that the option was used only by the
> taskfile_(in|out)put_bytes() function, which in turn was only used for
> taks_in_intr, which in turn only matters for PIO mode but did NOTHING to DMA
> transfers for example? DMA transfers are those days the >> 90% most common
> on disks in PC systems. And you notice that the IDE driver sometimes starts in
> DMA and falls backs to DMA if it has for example to fill in a not full memmory
> page?

I think you wanted to say 'falls back to PIO'. Oh, OK. But still I can
disable DMA and access the damn TIVO drive data.

> And you notice that the SCSI people are more likely to exchange disks
> regularly between some RISC big endian host and linux on intel?
> Please ask yourself why they don't have someting like the byteswap option.

Because for Linux filesystems it was decided some time ago (after people
HAD huge byteswap problems) that ext2 is always LE, etc, etc. So
filesystems are supposed to be the same on every system.

But IDE is used in some pretty weird hardware configurations, where you
actually get one more byteswap on IDE, even when Linux takes care of the
system itself being LE/BE. And furthermore, IDE is now used in stuff
like TIVO or X-Box, where you can't really control what's on the drive,
and thus you can't rely on that it'll be in a format you decided is the
'right' one.

> Do you remember that byteswap was something introduced by the Atari people
> a long long time ago even before the fs code started to understand
> about native endianess issues and is today both: broken (it *may* work
> for someone but it *is* broken) unneccessary and not the proper
> way to deal with the problems in question?

It may be it's broken now. But IMO it should be fixed instead of
removed. Limited to PIO perhaps (if you set bswap on a drive, then it
the driver won't allow DMA on it), but still it's a feature that's
useful now and then.

> You notice as well that the arch specific bytswapping for physically
> cross wired ata host chips on some historical archs is still there
> where it should be?
> 
> You don't? So please flame me now as much as you desire...

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] 2.5.8 IDE 36

[Richard Gooch]

Martin Dalecki writes:
> David Lang wrote:
> > The common thing I use byteswap for is to mount my tivo (kernel 2.1.x)
> > drives on my PC (2.4/5.x).  those drives are byteswapped throughout the
> > entire drive, including the partition table.
> > 
> > It sounds as if you are removing this capability, am I misunderstaning you
> > or is there some other way to do this? (and duplicating the drive to use
> > dd to byteswap is not practical for 100G+)
> 
> Same problem as with SCSI disks, which are even more commonly moved
> between different system types - please look there for a solution.
> BTW. I hardly beleve that your tivo is containing a DOS partition
> table - otherwise the partition table will handle it all
> autmagically.

Well, there *is* a partition table on the drive, but the byte-swapping
isn't handled automatically. Otherwise people wouldn't need to bswap
their TiVo drives when plugging into an x86 box.

Having the bswap option is definately useful. With it, you can "bless"
the drive and then mount the partitions and poke around. Please don't
remove the bswap option. You'll make life harder for a bunch of
people.

This gratuitous removal of features in the guise of "cleanups" is why
you got flamed earlier this year. I thought you'd learned :-/

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

Richard Gooch wrote:
> Martin Dalecki writes:
> 
>>David Lang wrote:
>>
>>>The common thing I use byteswap for is to mount my tivo (kernel 2.1.x)
>>>drives on my PC (2.4/5.x).  those drives are byteswapped throughout the
>>>entire drive, including the partition table.
>>>
>>>It sounds as if you are removing this capability, am I misunderstaning you
>>>or is there some other way to do this? (and duplicating the drive to use
>>>dd to byteswap is not practical for 100G+)
>>
>>Same problem as with SCSI disks, which are even more commonly moved
>>between different system types - please look there for a solution.
>>BTW. I hardly beleve that your tivo is containing a DOS partition
>>table - otherwise the partition table will handle it all
>>autmagically.
> 
> 
> Well, there *is* a partition table on the drive, but the byte-swapping
> isn't handled automatically. Otherwise people wouldn't need to bswap
> their TiVo drives when plugging into an x86 box.
> 
> Having the bswap option is definately useful. With it, you can "bless"
> the drive and then mount the partitions and poke around. Please don't
> remove the bswap option. You'll make life harder for a bunch of
> people.

Please note one sample from ext2 code:

	if (le32_to_cpu(es->s_rev_level) > EXT2_GOOD_OLD_REV)

And from partition handling code linux/fs/partitions/sun.c:

	/* All Sun disks have 8 partition entries */
	spc = be16_to_cpu(label->ntrks) * be16_to_cpu(label->nsect);

You see those many le32_to_cpu() and reverse commands?
If there is something that has to be fixed for linux/fs/partition/whatever.c -
please fix it there if you are actually *needing* it.

You also notice that the option was used only by the
taskfile_(in|out)put_bytes() function, which in turn was only used for
taks_in_intr, which in turn only matters for PIO mode but did NOTHING to DMA
transfers for example? DMA transfers are those days the >> 90% most common
on disks in PC systems. And you notice that the IDE driver sometimes starts in
DMA and falls backs to DMA if it has for example to fill in a not full memmory
page?

And you notice that the SCSI people are more likely to exchange disks
regularly between some RISC big endian host and linux on intel?
Please ask yourself why they don't have someting like the byteswap option.

Do you remember that byteswap was something introduced by the Atari people
a long long time ago even before the fs code started to understand
about native endianess issues and is today both: broken (it *may* work
for someone but it *is* broken) unneccessary and not the proper
way to deal with the problems in question?

You notice as well that the arch specific bytswapping for physically
cross wired ata host chips on some historical archs is still there
where it should be?

You don't? So please flame me now as much as you desire...

Re: [PATCH] 2.5.8 IDE 36

[David Lang]

The common thing I use byteswap for is to mount my tivo (kernel 2.1.x)
drives on my PC (2.4/5.x).  those drives are byteswapped throughout the
entire drive, including the partition table.

It sounds as if you are removing this capability, am I misunderstaning you
or is there some other way to do this? (and duplicating the drive to use
dd to byteswap is not practical for 100G+)

David Lang

 On Tue, 16 Apr 2002, Martin Dalecki wrote:

> Vojtech Pavlik wrote:
> > On Tue, Apr 16, 2002 at 09:05:21AM +0200, Martin Dalecki wrote:
> >
> >>Tue Apr 16 01:02:47 CEST 2002 ide-clean-36
> >>
> >>- Consolidate ide_choose_drive() and choose_drive() in to one function.
> >>
> >>- Remove sector data byteswpapping support. Byte-swapping the data is supported
> >>   on the file-system level where applicable.  Byte-swapped interfaces are
> >>   supported on a lower level anyway. And finally it was used inconsistently.
> >
> >
> > Are you sure about this? I think file systems support LE/BE, but not
> > byteswapping because of IDE being LE on a BE system.
>
> I'm sure about this. For the following reasons:
>
> 1. The removed functionality affected only sector data transfers.
>
> 2. The following code for interfaces with byte swapped BUS setups
>     still remains intact:
>
> #if defined(CONFIG_ATARI) || defined(CONFIG_Q40)
> 	if (MACH_IS_ATARI || MACH_IS_Q40) {
> 		/* Atari has a byte-swapped IDE interface */
> 		insw_swapw(IDE_DATA_REG, buffer, bytecount / 2);
> 		return;
> 	}
> #endif
>
> And indeed as you show - there was confusion about this issue
> throughout the whole driver, since the taskfile_in(out)
> functions where basically just the byteswapped variants and
> where not uses consistently.
>
> -
> 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/
>

Re: [PATCH] 2.5.8 IDE 36

[Vojtech Pavlik]

On Tue, Apr 16, 2002 at 09:33:00AM +0200, Martin Dalecki wrote:
> Vojtech Pavlik wrote:
> > On Tue, Apr 16, 2002 at 09:05:21AM +0200, Martin Dalecki wrote:
> > 
> >>Tue Apr 16 01:02:47 CEST 2002 ide-clean-36
> >>
> >>- Consolidate ide_choose_drive() and choose_drive() in to one function.
> >>
> >>- Remove sector data byteswpapping support. Byte-swapping the data is supported
> >>   on the file-system level where applicable.  Byte-swapped interfaces are
> >>   supported on a lower level anyway. And finally it was used inconsistently.
> > 
> > 
> > Are you sure about this? I think file systems support LE/BE, but not
> > byteswapping because of IDE being LE on a BE system.
> 
> I'm sure about this. For the following reasons:
> 
> 1. The removed functionality affected only sector data transfers.
> 
> 2. The following code for interfaces with byte swapped BUS setups
>     still remains intact:
> 
> #if defined(CONFIG_ATARI) || defined(CONFIG_Q40)
> 	if (MACH_IS_ATARI || MACH_IS_Q40) {
> 		/* Atari has a byte-swapped IDE interface */
> 		insw_swapw(IDE_DATA_REG, buffer, bytecount / 2);
> 		return;
> 	}
> #endif

If this is kept, then OK.

> 
> And indeed as you show - there was confusion about this issue
> throughout the whole driver, since the taskfile_in(out)
> functions where basically just the byteswapped variants and
> where not uses consistently.

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

David Lang wrote:
> The common thing I use byteswap for is to mount my tivo (kernel 2.1.x)
> drives on my PC (2.4/5.x).  those drives are byteswapped throughout the
> entire drive, including the partition table.
> 
> It sounds as if you are removing this capability, am I misunderstaning you
> or is there some other way to do this? (and duplicating the drive to use
> dd to byteswap is not practical for 100G+)


Same problem as with SCSI disks, which are even more commonly moved
between different system types - please look there for a solution.
BTW.> I hardly beleve that your tivo is containing a DOS partition table -
otherwise the partition table will handle it all autmagically.

Re: [PATCH] 2.5.8 IDE 36

[Vojtech Pavlik]

On Tue, Apr 16, 2002 at 09:05:21AM +0200, Martin Dalecki wrote:
> Tue Apr 16 01:02:47 CEST 2002 ide-clean-36
> 
> - Consolidate ide_choose_drive() and choose_drive() in to one function.
> 
> - Remove sector data byteswpapping support. Byte-swapping the data is supported
>    on the file-system level where applicable.  Byte-swapped interfaces are
>    supported on a lower level anyway. And finally it was used inconsistently.

Are you sure about this? I think file systems support LE/BE, but not
byteswapping because of IDE being LE on a BE system.

> - Eliminate taskfile_input_data() and taskfile_output_data(). This allowed us
>    to split up ideproc and eliminate the ugly action switch as well as the
>    corresponding defines.
> 
> - Remove tons of unnecessary typedefs from ide.h
> 
> - Prepate the PIO read write code for soon overhaul.
> 
> - Misc small bits here and there :-).
> 

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] 2.5.8 IDE 36

[Martin Dalecki]

Vojtech Pavlik wrote:
> On Tue, Apr 16, 2002 at 09:05:21AM +0200, Martin Dalecki wrote:
> 
>>Tue Apr 16 01:02:47 CEST 2002 ide-clean-36
>>
>>- Consolidate ide_choose_drive() and choose_drive() in to one function.
>>
>>- Remove sector data byteswpapping support. Byte-swapping the data is supported
>>   on the file-system level where applicable.  Byte-swapped interfaces are
>>   supported on a lower level anyway. And finally it was used inconsistently.
> 
> 
> Are you sure about this? I think file systems support LE/BE, but not
> byteswapping because of IDE being LE on a BE system.

I'm sure about this. For the following reasons:

1. The removed functionality affected only sector data transfers.

2. The following code for interfaces with byte swapped BUS setups
    still remains intact:

#if defined(CONFIG_ATARI) || defined(CONFIG_Q40)
	if (MACH_IS_ATARI || MACH_IS_Q40) {
		/* Atari has a byte-swapped IDE interface */
		insw_swapw(IDE_DATA_REG, buffer, bytecount / 2);
		return;
	}
#endif

And indeed as you show - there was confusion about this issue
throughout the whole driver, since the taskfile_in(out)
functions where basically just the byteswapped variants and
where not uses consistently.

[PATCH] 2.5.8 IDE 36

[Martin Dalecki]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

Tue Apr 16 01:02:47 CEST 2002 ide-clean-36

- Consolidate ide_choose_drive() and choose_drive() in to one function.

- Remove sector data byteswpapping support. Byte-swapping the data is supported
   on the file-system level where applicable.  Byte-swapped interfaces are
   supported on a lower level anyway. And finally it was used inconsistently.

- Eliminate taskfile_input_data() and taskfile_output_data(). This allowed us
   to split up ideproc and eliminate the ugly action switch as well as the
   corresponding defines.

- Remove tons of unnecessary typedefs from ide.h

- Prepate the PIO read write code for soon overhaul.

- Misc small bits here and there :-).


[-- Attachment #2: ide-clean-36.diff --]
[-- Type: text/plain, Size: 46556 bytes --]

diff -urN linux-2.5.8/arch/cris/drivers/ide.c linux/arch/cris/drivers/ide.c
--- linux-2.5.8/arch/cris/drivers/ide.c	Mon Apr 15 08:53:39 2002
+++ linux/arch/cris/drivers/ide.c	Tue Apr 16 03:44:10 2002
@@ -272,13 +272,17 @@
 	printk("ide: ETRAX 100LX built-in ATA DMA controller\n");
 
 	/* first initialize the channel interface data */
-	
+
 	for(h = 0; h < MAX_HWIFS; h++) {
 		struct ata_channel *hwif = &ide_hwifs[h];
+
 		hwif->chipset = ide_etrax100;
 		hwif->tuneproc = &tune_e100_ide;
 		hwif->dmaproc = &e100_dmaproc;
-		hwif->ideproc = &e100_ideproc;
+		hwif->ata_read = e100_ide_input_data;
+		hwif->ata_write = e100_ide_input_data;
+		hwif->atapi_read = e100_atapi_read;
+		hwif->atapi_write = e100_atapi_write;
 	}
 	/* actually reset and configure the etrax100 ide/ata interface */
 
@@ -375,12 +379,12 @@
  * so if an odd bytecount is specified, be sure that there's at least one
  * extra byte allocated for the buffer.
  */
-static void 
-e100_atapi_input_bytes (ide_drive_t *drive, void *buffer, unsigned int bytecount)
+static void
+e100_atapi_read(ide_drive_t *drive, void *buffer, unsigned int bytecount)
 {
 	ide_ioreg_t data_reg = IDE_DATA_REG;
 
-	D(printk("atapi_input_bytes, dreg 0x%x, buffer 0x%x, count %d\n",
+	D(printk("atapi_read, dreg 0x%x, buffer 0x%x, count %d\n",
 		 data_reg, buffer, bytecount));
 	
 	if(bytecount & 1) {
@@ -454,12 +458,12 @@
 #endif
 }
 
-static void 
-e100_atapi_output_bytes (ide_drive_t *drive, void *buffer, unsigned int bytecount)
+static void
+e100_atapi_write(ide_drive_t *drive, void *buffer, unsigned int bytecount)
 {
 	ide_ioreg_t data_reg = IDE_DATA_REG;
 	
-	D(printk("atapi_output_bytes, dreg 0x%x, buffer 0x%x, count %d\n",
+	D(printk("atapi_write, dreg 0x%x, buffer 0x%x, count %d\n",
 		 data_reg, buffer, bytecount));
 
 	if(bytecount & 1) {
@@ -544,7 +548,7 @@
 static void 
 e100_ide_input_data (ide_drive_t *drive, void *buffer, unsigned int wcount)
 {
-	e100_atapi_input_bytes(drive, buffer, wcount << 2);
+	e100_atapi_read(drive, buffer, wcount << 2);
 }
 
 /*
@@ -553,7 +557,7 @@
 static void
 e100_ide_output_data (ide_drive_t *drive, void *buffer, unsigned int wcount)
 {
-	e100_atapi_output_bytes(drive, buffer, wcount << 2);
+	e100_atapi_write(drive, buffer, wcount << 2);
 }
 
 /*
@@ -570,11 +574,11 @@
 		case ideproc_ide_output_data:
 			e100_ide_input_data(drive, buffer, length);
 			break;
-		case ideproc_atapi_input_bytes:
-			e100_atapi_input_bytes(drive, buffer, length);
+		case ideproc_atapi_read:
+			e100_atapi_read(drive, buffer, length);
 			break;
-		case ideproc_atapi_output_bytes:
-			e100_atapi_output_bytes(drive, buffer, length);
+		case ideproc_atapi_write:
+			e100_atapi_write(drive, buffer, length);
 			break;
 		default:
 			printk("e100_ideproc: unsupported func %d!\n", func);
diff -urN linux-2.5.8/drivers/ide/ide-cd.c linux/drivers/ide/ide-cd.c
--- linux-2.5.8/drivers/ide/ide-cd.c	Tue Apr 16 06:01:04 2002
+++ linux/drivers/ide/ide-cd.c	Tue Apr 16 03:23:56 2002
@@ -791,7 +791,7 @@
 	ide_set_handler(drive, handler, timeout, cdrom_timer_expiry);
 
 	/* Send the command to the device. */
-	atapi_output_bytes(drive, cmd, CDROM_PACKET_SIZE);
+	atapi_write(drive, cmd, CDROM_PACKET_SIZE);
 
 	return ide_started;
 }
@@ -830,7 +830,7 @@
 	/* Read the data into the buffer. */
 	dest = info->buffer + info->nsectors_buffered * SECTOR_SIZE;
 	while (sectors_to_buffer > 0) {
-		atapi_input_bytes (drive, dest, SECTOR_SIZE);
+		atapi_read(drive, dest, SECTOR_SIZE);
 		--sectors_to_buffer;
 		--sectors_to_transfer;
 		++info->nsectors_buffered;
@@ -840,7 +840,7 @@
 	/* Throw away any remaining data. */
 	while (sectors_to_transfer > 0) {
 		char dum[SECTOR_SIZE];
-		atapi_input_bytes (drive, dum, sizeof (dum));
+		atapi_read(drive, dum, sizeof (dum));
 		--sectors_to_transfer;
 	}
 }
@@ -866,7 +866,7 @@
 		   and quit this request. */
 		while (len > 0) {
 			int dum = 0;
-			atapi_output_bytes (drive, &dum, sizeof (dum));
+			atapi_write(drive, &dum, sizeof (dum));
 			len -= sizeof (dum);
 		}
 	} else  if (ireason == 1) {
@@ -963,7 +963,7 @@
 	while (nskip > 0) {
 		/* We need to throw away a sector. */
 		char dum[SECTOR_SIZE];
-		atapi_input_bytes (drive, dum, sizeof (dum));
+		atapi_read(drive, dum, SECTOR_SIZE);
 
 		--rq->current_nr_sectors;
 		--nskip;
@@ -994,7 +994,7 @@
 			/* Read this_transfer sectors
 			   into the current buffer. */
 			while (this_transfer > 0) {
-				atapi_input_bytes(drive, rq->buffer, SECTOR_SIZE);
+				atapi_read(drive, rq->buffer, SECTOR_SIZE);
 				rq->buffer += SECTOR_SIZE;
 				--rq->nr_sectors;
 				--rq->current_nr_sectors;
@@ -1290,13 +1290,14 @@
 	/* The drive wants to be written to. */
 	if ((ireason & 3) == 0) {
 		/* Transfer the data. */
-		atapi_output_bytes (drive, pc->buffer, thislen);
+		atapi_write(drive, pc->buffer, thislen);
 
 		/* If we haven't moved enough data to satisfy the drive,
 		   add some padding. */
 		while (len > thislen) {
 			int dum = 0;
-			atapi_output_bytes (drive, &dum, sizeof (dum));
+
+			atapi_write(drive, &dum, sizeof (dum));
 			len -= sizeof (dum);
 		}
 
@@ -1307,15 +1308,14 @@
 
 	/* Same drill for reading. */
 	else if ((ireason & 3) == 2) {
-
 		/* Transfer the data. */
-		atapi_input_bytes (drive, pc->buffer, thislen);
+		atapi_read(drive, pc->buffer, thislen);
 
 		/* If we haven't moved enough data to satisfy the drive,
 		   add some padding. */
 		while (len > thislen) {
 			int dum = 0;
-			atapi_input_bytes (drive, &dum, sizeof (dum));
+			atapi_read(drive, &dum, sizeof (dum));
 			len -= sizeof (dum);
 		}
 
@@ -1458,7 +1458,7 @@
 		   and quit this request. */
 		while (len > 0) {
 			int dum = 0;
-			atapi_output_bytes(drive, &dum, sizeof(dum));
+			atapi_write(drive, &dum, sizeof(dum));
 			len -= sizeof(dum);
 		}
 	} else {
@@ -1549,7 +1549,7 @@
 		this_transfer = MIN(sectors_to_transfer,rq->current_nr_sectors);
 
 		while (this_transfer > 0) {
-			atapi_output_bytes(drive, rq->buffer, SECTOR_SIZE);
+			atapi_write(drive, rq->buffer, SECTOR_SIZE);
 			rq->buffer += SECTOR_SIZE;
 			--rq->nr_sectors;
 			--rq->current_nr_sectors;
diff -urN linux-2.5.8/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
--- linux-2.5.8/drivers/ide/ide-disk.c	Tue Apr 16 06:01:07 2002
+++ linux/drivers/ide/ide-disk.c	Tue Apr 16 05:38:16 2002
@@ -1051,7 +1051,6 @@
 	ide_add_setting(drive,	"bios_head",		SETTING_RW,					-1,			-1,			TYPE_BYTE,	0,	255,				1,	1,	&drive->bios_head,		NULL);
 	ide_add_setting(drive,	"bios_sect",		SETTING_RW,					-1,			-1,			TYPE_BYTE,	0,	63,				1,	1,	&drive->bios_sect,		NULL);
 	ide_add_setting(drive,	"address",		SETTING_RW,					HDIO_GET_ADDRESS,	HDIO_SET_ADDRESS,	TYPE_INTA,	0,	2,				1,	1,	&drive->addressing,	set_lba_addressing);
-	ide_add_setting(drive,	"bswap",		SETTING_READ,					-1,			-1,			TYPE_BYTE,	0,	1,				1,	1,	&drive->bswap,			NULL);
 	ide_add_setting(drive,	"multcount",		id ? SETTING_RW : SETTING_READ,			HDIO_GET_MULTCOUNT,	HDIO_SET_MULTCOUNT,	TYPE_BYTE,	0,	id ? id->max_multsect : 0,	1,	1,	&drive->mult_count,		set_multcount);
 	ide_add_setting(drive,	"nowerr",		SETTING_RW,					HDIO_GET_NOWERR,	HDIO_SET_NOWERR,	TYPE_BYTE,	0,	1,				1,	1,	&drive->nowerr,			set_nowerr);
 	ide_add_setting(drive,	"lun",			SETTING_RW,					-1,			-1,			TYPE_INT,	0,	7,				1,	1,	&drive->lun,			NULL);
@@ -1198,11 +1197,11 @@
 	if ((capacity >= (drive->bios_cyl * drive->bios_sect * drive->bios_head)) &&
 	    (!drive->forced_geom) && drive->bios_sect && drive->bios_head)
 		drive->bios_cyl = (capacity / drive->bios_sect) / drive->bios_head;
-	printk (KERN_INFO "%s: %ld sectors", drive->name, capacity);
+	printk(KERN_INFO "%s: %ld sectors", drive->name, capacity);
 
 	/* Give size in megabytes (MB), not mebibytes (MiB). */
 	/* We compute the exact rounded value, avoiding overflow. */
-	printk (" (%ld MB)", (capacity - capacity/625 + 974)/1950);
+	printk(" (%ld MB)", (capacity - capacity/625 + 974)/1950);
 
 	/* Only print cache size when it was specified */
 	if (id->buf_size)
@@ -1213,7 +1212,7 @@
 #ifdef CONFIG_BLK_DEV_IDEDMA
 	if (drive->using_dma)
 		(void) drive->channel->dmaproc(ide_dma_verbose, drive);
-#endif /* CONFIG_BLK_DEV_IDEDMA */
+#endif
 	printk("\n");
 
 	drive->mult_count = 0;
@@ -1223,15 +1222,17 @@
 		id->multsect_valid = id->multsect ? 1 : 0;
 		drive->mult_req = id->multsect_valid ? id->max_multsect : INITIAL_MULT_COUNT;
 		drive->special.b.set_multmode = drive->mult_req ? 1 : 0;
-#else	/* original, pre IDE-NFG, per request of AC */
+#else
+		/* original, pre IDE-NFG, per request of AC */
 		drive->mult_req = INITIAL_MULT_COUNT;
 		if (drive->mult_req > id->max_multsect)
 			drive->mult_req = id->max_multsect;
 		if (drive->mult_req || ((id->multsect_valid & 1) && id->multsect))
 			drive->special.b.set_multmode = 1;
-#endif	/* CONFIG_IDEDISK_MULTI_MODE */
+#endif
 	}
 	drive->no_io_32bit = id->dword_io ? 1 : 0;
+
 	if (drive->id->cfs_enable_2 & 0x3000)
 		write_cache(drive, (id->cfs_enable_2 & 0x3000));
 	probe_lba_addressing(drive, 1);
diff -urN linux-2.5.8/drivers/ide/ide-features.c linux/drivers/ide/ide-features.c
--- linux-2.5.8/drivers/ide/ide-features.c	Mon Apr 15 08:53:44 2002
+++ linux/drivers/ide/ide-features.c	Tue Apr 16 02:36:34 2002
@@ -130,8 +130,8 @@
 		__restore_flags(flags);	/* local CPU only */
 		return 0;
 	}
-	ata_input_data(drive, id, SECTOR_WORDS);
-	(void) GET_STAT();	/* clear drive IRQ */
+	ata_read(drive, id, SECTOR_WORDS);
+	GET_STAT();		/* clear drive IRQ */
 	ide__sti();		/* local CPU only */
 	__restore_flags(flags);	/* local CPU only */
 	ide_fix_driveid(id);
@@ -285,7 +285,7 @@
 	enable_irq(hwif->irq);
 
 	if (error) {
-		(void) ide_dump_status(drive, "set_drive_speed_status", stat);
+		ide_dump_status(drive, "set_drive_speed_status", stat);
 		return error;
 	}
 
diff -urN linux-2.5.8/drivers/ide/ide-floppy.c linux/drivers/ide/ide-floppy.c
--- linux-2.5.8/drivers/ide/ide-floppy.c	Mon Apr 15 08:53:44 2002
+++ linux/drivers/ide/ide-floppy.c	Tue Apr 16 03:24:42 2002
@@ -717,7 +717,7 @@
 			return;
 		}
 		count = IDEFLOPPY_MIN (bio->bi_size - pc->b_count, bcount);
-		atapi_input_bytes (drive, bio_data(bio) + pc->b_count, count);
+		atapi_read(drive, bio_data(bio) + pc->b_count, count);
 		bcount -= count; pc->b_count += count;
 	}
 }
@@ -744,7 +744,7 @@
 			return;
 		}
 		count = IDEFLOPPY_MIN (pc->b_count, bcount);
-		atapi_output_bytes (drive, pc->b_data, count);
+		atapi_write(drive, pc->b_data, count);
 		bcount -= count; pc->b_data += count; pc->b_count -= count;
 	}
 }
@@ -979,12 +979,12 @@
 	}
 	if (test_bit (PC_WRITING, &pc->flags)) {
 		if (pc->buffer != NULL)
-			atapi_output_bytes (drive,pc->current_position,bcount.all);	/* Write the current buffer */
+			atapi_write(drive,pc->current_position,bcount.all);	/* Write the current buffer */
 		else
 			idefloppy_output_buffers (drive, pc, bcount.all);
 	} else {
 		if (pc->buffer != NULL)
-			atapi_input_bytes (drive,pc->current_position,bcount.all);	/* Read the current buffer */
+			atapi_read(drive,pc->current_position,bcount.all);	/* Read the current buffer */
 		else
 			idefloppy_input_buffers (drive, pc, bcount.all);
 	}
@@ -1020,7 +1020,7 @@
 
 	BUG_ON(HWGROUP(drive)->handler);
 	ide_set_handler (drive, &idefloppy_pc_intr, IDEFLOPPY_WAIT_CMD, NULL);	/* Set the interrupt routine */
-	atapi_output_bytes (drive, floppy->pc->c, 12); /* Send the actual packet */
+	atapi_write(drive, floppy->pc->c, 12); /* Send the actual packet */
 
 	return ide_started;
 }
@@ -1042,7 +1042,7 @@
 {
 	idefloppy_floppy_t *floppy = drive->driver_data;
 
-	atapi_output_bytes (drive, floppy->pc->c, 12); /* Send the actual packet */
+	atapi_write(drive, floppy->pc->c, 12); /* Send the actual packet */
 	return IDEFLOPPY_WAIT_CMD;		/* Timeout for the packet command */
 }
 
diff -urN linux-2.5.8/drivers/ide/ide-probe.c linux/drivers/ide/ide-probe.c
--- linux-2.5.8/drivers/ide/ide-probe.c	Tue Apr 16 06:01:07 2002
+++ linux/drivers/ide/ide-probe.c	Tue Apr 16 03:48:14 2002
@@ -57,9 +57,18 @@
 		printk(KERN_WARNING "(ide-probe::do_identify) Out of memory.\n");
 		goto err_kmalloc;
 	}
-	/* read 512 bytes of id info */
+
+	/* Read 512 bytes of id info.
+	 *
+	 * Please note that it is well known that some *very* old drives are
+	 * able to provide only 256 of them, since this was the amount read by
+	 * DOS.
+	 *
+	 * However let's try to get away with this...
+	 */
+
 #if 1
-	ata_input_data(drive, id, SECTOR_WORDS);		/* read 512 bytes of id info */
+	ata_read(drive, id, SECTOR_WORDS);
 #else
         {
                 unsigned long   *ptr = (unsigned long *)id ;
@@ -580,10 +589,10 @@
 	__restore_flags(flags);	/* local CPU only */
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
 		ide_drive_t *drive = &hwif->drives[unit];
-		if (drive->present) {
-			ide_tuneproc_t *tuneproc = drive->channel->tuneproc;
-			if (tuneproc != NULL && drive->autotune == 1)
-				tuneproc(drive, 255);	/* auto-tune PIO mode */
+
+		if (drive->present && (drive->autotune == 1)) {
+			if (drive->channel->tuneproc != NULL)
+				drive->channel->tuneproc(drive, 255);	/* auto-tune PIO mode */
 		}
 	}
 }
diff -urN linux-2.5.8/drivers/ide/ide-proc.c linux/drivers/ide/ide-proc.c
--- linux-2.5.8/drivers/ide/ide-proc.c	Mon Apr 15 08:53:44 2002
+++ linux/drivers/ide/ide-proc.c	Tue Apr 16 04:05:19 2002
@@ -422,7 +422,6 @@
 static void create_proc_ide_drives(struct ata_channel *hwif)
 {
 	int	d;
-	struct proc_dir_entry *ent;
 	struct proc_dir_entry *parent = hwif->proc;
 	char name[64];
 
diff -urN linux-2.5.8/drivers/ide/ide-tape.c linux/drivers/ide/ide-tape.c
--- linux-2.5.8/drivers/ide/ide-tape.c	Mon Apr 15 08:53:44 2002
+++ linux/drivers/ide/ide-tape.c	Tue Apr 16 03:25:29 2002
@@ -1503,9 +1503,9 @@
 			idetape_discard_data (drive, bcount);
 			return;
 		}
-#endif /* IDETAPE_DEBUG_BUGS */
+#endif
 		count = min(bio->bi_size - pc->b_count, bcount);
-		atapi_input_bytes (drive, bio_data(bio) + pc->b_count, count);
+		atapi_read(drive, bio_data(bio) + pc->b_count, count);
 		bcount -= count;
 		pc->b_count += bio->bi_size;
 		if (pc->b_count == bio->bi_size) {
@@ -1530,7 +1530,7 @@
 		}
 #endif /* IDETAPE_DEBUG_BUGS */
 		count = min(pc->b_count, bcount);
-		atapi_output_bytes (drive, bio_data(bio), count);
+		atapi_write(drive, bio_data(bio), count);
 		bcount -= count;
 		pc->b_data += count;
 		pc->b_count -= count;
@@ -2169,12 +2169,12 @@
 		if (pc->bio != NULL)
 			idetape_output_buffers (drive, pc, bcount.all);
 		else
-			atapi_output_bytes (drive,pc->current_position,bcount.all);	/* Write the current buffer */
+			atapi_write(drive,pc->current_position,bcount.all);	/* Write the current buffer */
 	} else {
 		if (pc->bio != NULL)
 			idetape_input_buffers (drive, pc, bcount.all);
 		else
-			atapi_input_bytes (drive,pc->current_position,bcount.all);	/* Read the current buffer */
+			atapi_read(drive,pc->current_position,bcount.all);	/* Read the current buffer */
 	}
 	pc->actually_transferred += bcount.all;					/* Update the current position */
 	pc->current_position+=bcount.all;
@@ -2259,7 +2259,7 @@
 	tape->cmd_start_time = jiffies;
 	BUG_ON(HWGROUP(drive)->handler);
 	ide_set_handler(drive, &idetape_pc_intr, IDETAPE_WAIT_CMD, NULL);	/* Set the interrupt routine */
-	atapi_output_bytes (drive,pc->c,12);			/* Send the actual packet */
+	atapi_write(drive,pc->c,12);	/* Send the actual packet */
 	return ide_started;
 }
 
diff -urN linux-2.5.8/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c
--- linux-2.5.8/drivers/ide/ide-taskfile.c	Tue Apr 16 06:01:04 2002
+++ linux/drivers/ide/ide-taskfile.c	Tue Apr 16 05:38:53 2002
@@ -1,4 +1,5 @@
 /*
+ *  Copyright (C) 2002		Marcin Dalecki <martin@dalecki.de>
  *  Copyright (C) 2000		Michael Cornwell <cornwell@acm.org>
  *  Copyright (C) 2000		Andre Hedrick <andre@linux-ide.org>
  *
@@ -58,15 +59,9 @@
 		bio_kunmap_irq(to, flags);
 }
 
-static void bswap_data (void *buffer, int wcount)
-{
-	u16 *p = buffer;
-
-	while (wcount--) {
-		*p = *p << 8 | *p >> 8; p++;
-		*p = *p << 8 | *p >> 8; p++;
-	}
-}
+/*
+ * Data transfer functions for polled IO.
+ */
 
 #if SUPPORT_VLB_SYNC
 /*
@@ -76,30 +71,88 @@
  * of the sector count register location, with interrupts disabled
  * to ensure that the reads all happen together.
  */
-static inline void task_vlb_sync(ide_drive_t *drive)
+static void ata_read_vlb(struct ata_device *drive, void *buffer, unsigned int wcount)
 {
-	ide_ioreg_t port = IDE_NSECTOR_REG;
+	unsigned long flags;
 
-	IN_BYTE(port);
-	IN_BYTE(port);
-	IN_BYTE(port);
+	__save_flags(flags);	/* local CPU only */
+	__cli();		/* local CPU only */
+	IN_BYTE(IDE_NSECTOR_REG);
+	IN_BYTE(IDE_NSECTOR_REG);
+	IN_BYTE(IDE_NSECTOR_REG);
+	insl(IDE_DATA_REG, buffer, wcount);
+	__restore_flags(flags);	/* local CPU only */
+}
+
+static void ata_write_vlb(struct ata_device *drive, void *buffer, unsigned int wcount)
+{
+	unsigned long flags;
+
+	__save_flags(flags);	/* local CPU only */
+	__cli();		/* local CPU only */
+	IN_BYTE(IDE_NSECTOR_REG);
+	IN_BYTE(IDE_NSECTOR_REG);
+	IN_BYTE(IDE_NSECTOR_REG);
+	outsl(IDE_DATA_REG, buffer, wcount);
+	__restore_flags(flags);	/* local CPU only */
 }
 #endif
 
+static void ata_read_32(struct ata_device *drive, void *buffer, unsigned int wcount)
+{
+	insl(IDE_DATA_REG, buffer, wcount);
+}
+
+static void ata_write_32(struct ata_device *drive, void *buffer, unsigned int wcount)
+{
+	outsl(IDE_DATA_REG, buffer, wcount);
+}
+
+#if SUPPORT_SLOW_DATA_PORTS
+static void ata_read_slow(struct ata_device *drive, void *buffer, unsigned int wcount)
+{
+	unsigned short *ptr = (unsigned short *) buffer;
+
+	while (wcount--) {
+		*ptr++ = inw_p(IDE_DATA_REG);
+		*ptr++ = inw_p(IDE_DATA_REG);
+	}
+}
+
+static void ata_write_slow(struct ata_device *drive, void *buffer, unsigned int wcount)
+{
+	unsigned short *ptr = (unsigned short *) buffer;
+
+	while (wcount--) {
+		outw_p(*ptr++, IDE_DATA_REG);
+		outw_p(*ptr++, IDE_DATA_REG);
+	}
+}
+#endif
+
+static void ata_read_16(ide_drive_t *drive, void *buffer, unsigned int wcount)
+{
+	insw(IDE_DATA_REG, buffer, wcount<<1);
+}
+
+static void ata_write_16(ide_drive_t *drive, void *buffer, unsigned int wcount)
+{
+	outsw(IDE_DATA_REG, buffer, wcount<<1);
+}
+
 /*
- * This is used for most PIO data transfers *from* the IDE interface
+ * This is used for most PIO data transfers *from* the device.
  */
-void ata_input_data(ide_drive_t *drive, void *buffer, unsigned int wcount)
+void ata_read(ide_drive_t *drive, void *buffer, unsigned int wcount)
 {
-	byte io_32bit;
+	int io_32bit;
 
 	/*
-	 * first check if this controller has defined a special function
-	 * for handling polled ide transfers
+	 * First check if this controller has defined a special function
+	 * for handling polled ide transfers.
 	 */
-
-	if (drive->channel->ideproc) {
-		drive->channel->ideproc(ideproc_ide_input_data, drive, buffer, wcount);
+	if (drive->channel->ata_read) {
+		drive->channel->ata_read(drive, buffer, wcount);
 		return;
 	}
 
@@ -107,39 +160,30 @@
 
 	if (io_32bit) {
 #if SUPPORT_VLB_SYNC
-		if (io_32bit & 2) {
-			unsigned long flags;
-			__save_flags(flags);	/* local CPU only */
-			__cli();		/* local CPU only */
-			task_vlb_sync(drive);
-			insl(IDE_DATA_REG, buffer, wcount);
-			__restore_flags(flags);	/* local CPU only */
-		} else
+		if (io_32bit & 2)
+			ata_read_vlb(drive, buffer, wcount);
+		else
 #endif
-			insl(IDE_DATA_REG, buffer, wcount);
+			ata_read_32(drive, buffer, wcount);
 	} else {
 #if SUPPORT_SLOW_DATA_PORTS
-		if (drive->slow) {
-			unsigned short *ptr = (unsigned short *) buffer;
-			while (wcount--) {
-				*ptr++ = inw_p(IDE_DATA_REG);
-				*ptr++ = inw_p(IDE_DATA_REG);
-			}
-		} else
+		if (drive->slow)
+			ata_read_slow(drive, buffer, wcount);
+		else
 #endif
-			insw(IDE_DATA_REG, buffer, wcount<<1);
+			ata_read_16(drive, buffer, wcount);
 	}
 }
 
 /*
- * This is used for most PIO data transfers *to* the IDE interface
+ * This is used for most PIO data transfers *to* the device interface.
  */
-void ata_output_data(ide_drive_t *drive, void *buffer, unsigned int wcount)
+void ata_write(ide_drive_t *drive, void *buffer, unsigned int wcount)
 {
-	byte io_32bit;
+	int io_32bit;
 
-	if (drive->channel->ideproc) {
-		drive->channel->ideproc(ideproc_ide_output_data, drive, buffer, wcount);
+	if (drive->channel->ata_write) {
+		drive->channel->ata_write(drive, buffer, wcount);
 		return;
 	}
 
@@ -147,27 +191,18 @@
 
 	if (io_32bit) {
 #if SUPPORT_VLB_SYNC
-		if (io_32bit & 2) {
-			unsigned long flags;
-			__save_flags(flags);	/* local CPU only */
-			__cli();		/* local CPU only */
-			task_vlb_sync(drive);
-			outsl(IDE_DATA_REG, buffer, wcount);
-			__restore_flags(flags);	/* local CPU only */
-		} else
+		if (io_32bit & 2)
+			ata_write_vlb(drive, buffer, wcount);
+		else
 #endif
-			outsl(IDE_DATA_REG, buffer, wcount);
+			ata_write_32(drive, buffer, wcount);
 	} else {
 #if SUPPORT_SLOW_DATA_PORTS
-		if (drive->slow) {
-			unsigned short *ptr = (unsigned short *) buffer;
-			while (wcount--) {
-				outw_p(*ptr++, IDE_DATA_REG);
-				outw_p(*ptr++, IDE_DATA_REG);
-			}
-		} else
+		if (drive->slow)
+			ata_write_slow(drive, buffer, wcount);
+		else
 #endif
-			outsw(IDE_DATA_REG, buffer, wcount<<1);
+			ata_write_16(drive, buffer, wcount<<1);
 	}
 }
 
@@ -178,10 +213,10 @@
  * so if an odd bytecount is specified, be sure that there's at least one
  * extra byte allocated for the buffer.
  */
-void atapi_input_bytes (ide_drive_t *drive, void *buffer, unsigned int bytecount)
+void atapi_read(ide_drive_t *drive, void *buffer, unsigned int bytecount)
 {
-	if (drive->channel->ideproc) {
-		drive->channel->ideproc(ideproc_atapi_input_bytes, drive, buffer, bytecount);
+	if (drive->channel->atapi_read) {
+		drive->channel->atapi_read(drive, buffer, bytecount);
 		return;
 	}
 
@@ -193,15 +228,15 @@
 		return;
 	}
 #endif
-	ata_input_data (drive, buffer, bytecount / 4);
+	ata_read(drive, buffer, bytecount / 4);
 	if ((bytecount & 0x03) >= 2)
-		insw (IDE_DATA_REG, ((byte *)buffer) + (bytecount & ~0x03), 1);
+		insw(IDE_DATA_REG, ((byte *)buffer) + (bytecount & ~0x03), 1);
 }
 
-void atapi_output_bytes (ide_drive_t *drive, void *buffer, unsigned int bytecount)
+void atapi_write(ide_drive_t *drive, void *buffer, unsigned int bytecount)
 {
-	if (drive->channel->ideproc) {
-		drive->channel->ideproc(ideproc_atapi_output_bytes, drive, buffer, bytecount);
+	if (drive->channel->atapi_write) {
+		drive->channel->atapi_write(drive, buffer, bytecount);
 		return;
 	}
 
@@ -213,29 +248,11 @@
 		return;
 	}
 #endif
-	ata_output_data (drive, buffer, bytecount / 4);
+	ata_write(drive, buffer, bytecount / 4);
 	if ((bytecount & 0x03) >= 2)
 		outsw(IDE_DATA_REG, ((byte *)buffer) + (bytecount & ~0x03), 1);
 }
 
-void taskfile_input_data(ide_drive_t *drive, void *buffer, unsigned int wcount)
-{
-	ata_input_data(drive, buffer, wcount);
-	if (drive->bswap)
-		bswap_data(buffer, wcount);
-}
-
-void taskfile_output_data(ide_drive_t *drive, void *buffer, unsigned int wcount)
-{
-	if (drive->bswap) {
-		bswap_data(buffer, wcount);
-		ata_output_data(drive, buffer, wcount);
-		bswap_data(buffer, wcount);
-	} else {
-		ata_output_data(drive, buffer, wcount);
-	}
-}
-
 /*
  * Needed for PCI irq sharing
  */
@@ -311,8 +328,6 @@
 static ide_startstop_t task_mulout_intr (ide_drive_t *drive)
 {
 	byte stat		= GET_STAT();
-	/* FIXME: this should go possible as well */
-	byte io_32bit		= drive->io_32bit;
 	struct request *rq	= &HWGROUP(drive)->wrq;
 	ide_hwgroup_t *hwgroup	= HWGROUP(drive);
 	int mcount		= drive->mult_count;
@@ -378,14 +393,13 @@
 		}
 
 		/*
-		 * Ok, we're all setup for the interrupt
-		 * re-entering us on the last transfer.
+		 * Ok, we're all setup for the interrupt re-entering us on the
+		 * last transfer.
 		 */
-		taskfile_output_data(drive, buffer, nsect * SECTOR_WORDS);
+		ata_write(drive, buffer, nsect * SECTOR_WORDS);
 		bio_kunmap_irq(buffer, &flags);
 	} while (mcount);
 
-	drive->io_32bit = io_32bit;
 	rq->errors = 0;
 	if (hwgroup->handler == NULL)
 		ide_set_handler(drive, task_mulout_intr, WAIT_CMD, NULL);
@@ -542,7 +556,6 @@
 static ide_startstop_t task_in_intr (ide_drive_t *drive)
 {
 	byte stat		= GET_STAT();
-	byte io_32bit		= drive->io_32bit;
 	struct request *rq	= HWGROUP(drive)->rq;
 	char *pBuf		= NULL;
 	unsigned long flags;
@@ -561,10 +574,8 @@
 	pBuf = ide_map_rq(rq, &flags);
 	DTF("Read: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);
 
-	drive->io_32bit = 0;
-	taskfile_input_data(drive, pBuf, SECTOR_WORDS);
+	ata_read(drive, pBuf, SECTOR_WORDS);
 	ide_unmap_rq(rq, pBuf, &flags);
-	drive->io_32bit = io_32bit;
 
 	if (--rq->current_nr_sectors <= 0) {
 		/* (hs): swapped next 2 lines */
@@ -597,7 +608,7 @@
 		unsigned long flags;
 		char *buf = ide_map_rq(rq, &flags);
 		/* For Write_sectors we need to stuff the first sector */
-		taskfile_output_data(drive, buf, SECTOR_WORDS);
+		ata_write(drive, buf, SECTOR_WORDS);
 		rq->current_nr_sectors--;
 		ide_unmap_rq(rq, buf, &flags);
 	} else {
@@ -613,8 +624,6 @@
 static ide_startstop_t task_out_intr(ide_drive_t *drive)
 {
 	byte stat		= GET_STAT();
-	/* FIXME: this should go possible as well */
-	byte io_32bit		= drive->io_32bit;
 	struct request *rq	= HWGROUP(drive)->rq;
 	char *pBuf		= NULL;
 	unsigned long flags;
@@ -631,9 +640,8 @@
 		pBuf = ide_map_rq(rq, &flags);
 		DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);
 
-		taskfile_output_data(drive, pBuf, SECTOR_WORDS);
+		ata_write(drive, pBuf, SECTOR_WORDS);
 		ide_unmap_rq(rq, pBuf, &flags);
-		drive->io_32bit = io_32bit;
 		rq->errors = 0;
 		rq->current_nr_sectors--;
 	}
@@ -649,8 +657,6 @@
 {
 	unsigned int		msect, nsect;
 	byte stat		= GET_STAT();
-	/* FIXME: this should go possible as well */
-	byte io_32bit		= drive->io_32bit;
 	struct request *rq	= HWGROUP(drive)->rq;
 	char *pBuf		= NULL;
 	unsigned long flags;
@@ -676,10 +682,8 @@
 
 		DTF("Multiread: %p, nsect: %d , rq->current_nr_sectors: %d\n",
 			pBuf, nsect, rq->current_nr_sectors);
-		drive->io_32bit = 0;
-		taskfile_input_data(drive, pBuf, nsect * SECTOR_WORDS);
+		ata_read(drive, pBuf, nsect * SECTOR_WORDS);
 		ide_unmap_rq(rq, pBuf, &flags);
-		drive->io_32bit = io_32bit;
 		rq->errors = 0;
 		rq->current_nr_sectors -= nsect;
 		msect -= nsect;
@@ -1025,12 +1029,11 @@
 }
 
 EXPORT_SYMBOL(drive_is_ready);
-EXPORT_SYMBOL(ata_input_data);
-EXPORT_SYMBOL(ata_output_data);
-EXPORT_SYMBOL(atapi_input_bytes);
-EXPORT_SYMBOL(atapi_output_bytes);
-EXPORT_SYMBOL(taskfile_input_data);
-EXPORT_SYMBOL(taskfile_output_data);
+
+EXPORT_SYMBOL(ata_read);
+EXPORT_SYMBOL(ata_write);
+EXPORT_SYMBOL(atapi_read);
+EXPORT_SYMBOL(atapi_write);
 
 EXPORT_SYMBOL(ata_taskfile);
 EXPORT_SYMBOL(recal_intr);
diff -urN linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
--- linux-2.5.8/drivers/ide/ide.c	Tue Apr 16 06:01:07 2002
+++ linux/drivers/ide/ide.c	Tue Apr 16 05:38:37 2002
@@ -383,7 +383,7 @@
 		spin_lock_irqsave(&ide_lock, flags);
 
 		if ((jiffies - ar->ar_time > ATA_AR_MAX_TURNAROUND) && drive->queue_depth > 1) {
-			printk("%s: exceeded max command turn-around time (%d seconds)\n", drive->name, ATA_AR_MAX_TURNAROUND / HZ);
+			printk(KERN_INFO "%s: exceeded max command turn-around time (%d seconds)\n", drive->name, ATA_AR_MAX_TURNAROUND / HZ);
 			drive->queue_depth >>= 1;
 		}
 
@@ -474,7 +474,7 @@
 	spin_unlock_irqrestore(&ide_lock, flags);
 }
 
-static void ata_pre_reset (ide_drive_t *drive)
+static void ata_pre_reset(ide_drive_t *drive)
 {
 	if (ata_ops(drive) && ata_ops(drive)->pre_reset)
 		ata_ops(drive)->pre_reset(drive);
@@ -528,10 +528,9 @@
 	printk("%s: ata_special: 0x%02x\n", drive->name, s->all);
 #endif
 	if (s->b.set_tune) {
-		ide_tuneproc_t *tuneproc = drive->channel->tuneproc;
 		s->b.set_tune = 0;
-		if (tuneproc != NULL)
-			tuneproc(drive, drive->tune_req);
+		if (drive->channel->tuneproc != NULL)
+			drive->channel->tuneproc(drive, drive->tune_req);
 	} else if (drive->driver != NULL) {
 		if (ata_ops(drive)->special)
 			return ata_ops(drive)->special(drive);
@@ -899,23 +898,24 @@
 }
 
 /*
- * try_to_flush_leftover_data() is invoked in response to a drive
- * unexpectedly having its DRQ_STAT bit set.  As an alternative to
- * resetting the drive, this routine tries to clear the condition
- * by read a sector's worth of data from the drive.  Of course,
+ * This gets invoked in response to a drive unexpectedly having its DRQ_STAT
+ * bit set.  As an alternative to resetting the drive, it tries to clear the
+ * condition by reading a sector's worth of data from the drive.  Of course,
  * this may not help if the drive is *waiting* for data from *us*.
  */
 static void try_to_flush_leftover_data (ide_drive_t *drive)
 {
-	int i = (drive->mult_count ? drive->mult_count : 1) * SECTOR_WORDS;
+	int i = (drive->mult_count ? drive->mult_count : 1);
 
 	if (drive->type != ATA_DISK)
 		return;
+
 	while (i > 0) {
-		u32 buffer[16];
-		unsigned int wcount = (i > 16) ? 16 : i;
-		i -= wcount;
-		ata_input_data (drive, buffer, wcount);
+		u32 buffer[SECTOR_WORDS];
+		unsigned int count = (i > 1) ? 1 : i;
+
+		ata_read(drive, buffer, count * SECTOR_WORDS);
+		i -= count;
 	}
 }
 
@@ -1002,16 +1002,18 @@
 static ide_startstop_t drive_cmd_intr (ide_drive_t *drive)
 {
 	struct request *rq = HWGROUP(drive)->rq;
-	byte *args = (byte *) rq->buffer;
-	byte stat = GET_STAT();
+	u8 *args = rq->buffer;
+	u8 stat = GET_STAT();
 	int retries = 10;
 
 	ide__sti();	/* local CPU only */
 	if ((stat & DRQ_STAT) && args && args[3]) {
-		byte io_32bit = drive->io_32bit;
+		int io_32bit = drive->io_32bit;
+
 		drive->io_32bit = 0;
-		ata_input_data(drive, &args[4], args[3] * SECTOR_WORDS);
+		ata_read(drive, &args[4], args[3] * SECTOR_WORDS);
 		drive->io_32bit = io_32bit;
+
 		while (((stat = GET_STAT()) & BUSY_STAT) && retries--)
 			udelay(100);
 	}
@@ -1019,6 +1021,7 @@
 	if (!OK_STAT(stat, READY_STAT, BAD_STAT))
 		return ide_error(drive, "drive_cmd", stat); /* calls ide_end_drive_cmd */
 	ide_end_drive_cmd (drive, stat, GET_ERR());
+
 	return ide_stopped;
 }
 
@@ -1266,31 +1269,26 @@
 /*
  * Select the next drive which will be serviced.
  */
-static inline ide_drive_t *choose_drive(ide_hwgroup_t *hwgroup)
+static ide_drive_t *choose_drive(ide_hwgroup_t *hwgroup)
 {
-	ide_drive_t *drive, *best;
+	ide_drive_t *tmp;
+	ide_drive_t *drive = NULL;
+	unsigned long sleep = 0;
 
-	best = NULL;
-	drive = hwgroup->drive;
+	tmp = hwgroup->drive;
 	do {
-		if (!list_empty(&drive->queue.queue_head)
-		&& (!drive->PADAM_sleep	|| time_after_eq(drive->PADAM_sleep, jiffies))) {
-			if (!best
-			 || (drive->PADAM_sleep && (!best->PADAM_sleep || time_after(best->PADAM_sleep, drive->PADAM_sleep)))
-			 || (!best->PADAM_sleep && time_after(best->PADAM_service_start + 2 * best->PADAM_service_time, drive->PADAM_service_start + 2 * drive->PADAM_service_time)))
+		if (!list_empty(&tmp->queue.queue_head)
+		&& (!tmp->PADAM_sleep || time_after_eq(tmp->PADAM_sleep, jiffies))) {
+			if (!drive
+			 || (tmp->PADAM_sleep && (!drive->PADAM_sleep || time_after(drive->PADAM_sleep, tmp->PADAM_sleep)))
+			 || (!drive->PADAM_sleep && time_after(drive->PADAM_service_start + 2 * drive->PADAM_service_time, tmp->PADAM_service_start + 2 * tmp->PADAM_service_time)))
 			{
-				if (!blk_queue_plugged(&drive->queue))
-					best = drive;
+				if (!blk_queue_plugged(&tmp->queue))
+					drive = tmp;
 			}
 		}
-	} while ((drive = drive->next) != hwgroup->drive);
-	return best;
-}
-
-static ide_drive_t *ide_choose_drive(ide_hwgroup_t *hwgroup)
-{
-	ide_drive_t *drive = choose_drive(hwgroup);
-	unsigned long sleep = 0;
+		tmp = tmp->next;
+	} while (tmp != hwgroup->drive);
 
 	if (drive)
 		return drive;
@@ -1495,7 +1493,7 @@
 		/*
 		 * will clear IDE_BUSY, if appropriate
 		 */
-		if ((drive = ide_choose_drive(hwgroup)) == NULL)
+		if ((drive = choose_drive(hwgroup)) == NULL)
 			break;
 
 		hwif = drive->channel;
@@ -2298,7 +2296,10 @@
 	channel->maskproc = old_hwif.maskproc;
 	channel->quirkproc = old_hwif.quirkproc;
 	channel->rwproc	= old_hwif.rwproc;
-	channel->ideproc = old_hwif.ideproc;
+	channel->ata_read = old_hwif.ata_read;
+	channel->ata_write = old_hwif.ata_write;
+	channel->atapi_read = old_hwif.atapi_read;
+	channel->atapi_write = old_hwif.atapi_write;
 	channel->dmaproc = old_hwif.dmaproc;
 	channel->busproc = old_hwif.busproc;
 	channel->bus_state = old_hwif.bus_state;
@@ -2565,13 +2566,17 @@
 	return 0;
 }
 
-static int set_io_32bit(ide_drive_t *drive, int arg)
+static int set_io_32bit(struct ata_device *drive, int arg)
 {
+	if (drive->no_io_32bit)
+		return -EIO;
+
 	drive->io_32bit = arg;
 #ifdef CONFIG_BLK_DEV_DTC2278
 	if (drive->channel->chipset == ide_dtc2278)
 		drive->channel->drives[!drive->select.b.unit].io_32bit = arg;
-#endif /* CONFIG_BLK_DEV_DTC2278 */
+#endif
+
 	return 0;
 }
 
@@ -3018,8 +3023,6 @@
  * "hdx=slow"		: insert a huge pause after each access to the data
  *				port. Should be used only as a last resort.
  *
- * "hdx=swapdata"	: when the drive is a disk, byte swap all data
- * "hdx=bswap"		: same as above..........
  * "hdxlun=xx"          : set the drive last logical unit.
  * "hdx=flash"		: allows for more than one ata_flash disk to be
  *				registered. In most cases, only one device
@@ -3127,8 +3130,7 @@
 	if (!strncmp(s, "hd", 2) && s[2] >= 'a' && s[2] <= max_drive) {
 		const char *hd_words[] = {"none", "noprobe", "nowerr", "cdrom",
 				"serialize", "autotune", "noautotune",
-				"slow", "swapdata", "bswap", "flash",
-				"remap", "noremap", "scsi", NULL};
+				"slow", "flash", "remap", "noremap", "scsi", NULL};
 		unit = s[2] - 'a';
 		hw   = unit / MAX_DRIVES;
 		unit = unit % MAX_DRIVES;
@@ -3178,20 +3180,16 @@
 			case -8: /* "slow" */
 				drive->slow = 1;
 				goto done;
-			case -9: /* "swapdata" or "bswap" */
-			case -10:
-				drive->bswap = 1;
-				goto done;
-			case -11: /* "flash" */
+			case -9: /* "flash" */
 				drive->ata_flash = 1;
 				goto done;
-			case -12: /* "remap" */
+			case -10: /* "remap" */
 				drive->remap_0_to_1 = 1;
 				goto done;
-			case -13: /* "noremap" */
+			case -11: /* "noremap" */
 				drive->remap_0_to_1 = 2;
 				goto done;
-			case -14: /* "scsi" */
+			case -12: /* "scsi" */
 #if defined(CONFIG_BLK_DEV_IDESCSI) && defined(CONFIG_SCSI)
 				drive->scsi = 1;
 				goto done;
diff -urN linux-2.5.8/drivers/ide/pdc202xx.c linux/drivers/ide/pdc202xx.c
--- linux-2.5.8/drivers/ide/pdc202xx.c	Mon Apr 15 08:53:44 2002
+++ linux/drivers/ide/pdc202xx.c	Tue Apr 16 05:39:06 2002
@@ -1270,9 +1270,10 @@
 #ifdef CONFIG_PDC202XX_32_UNMASK
 	hwif->drives[0].io_32bit = 1;
 	hwif->drives[1].io_32bit = 1;
+
 	hwif->drives[0].unmask = 1;
 	hwif->drives[1].unmask = 1;
-#endif /* CONFIG_PDC202XX_32_UNMASK */
+#endif
 
 #ifdef CONFIG_BLK_DEV_IDEDMA
 	if (hwif->dma_base) {
@@ -1285,9 +1286,9 @@
 		hwif->drives[1].autotune = 1;
 		hwif->autodma = 0;
 	}
-#else /* !CONFIG_BLK_DEV_IDEDMA */
+#else
 	hwif->drives[0].autotune = 1;
 	hwif->drives[1].autotune = 1;
 	hwif->autodma = 0;
-#endif /* CONFIG_BLK_DEV_IDEDMA */
+#endif
 }
diff -urN linux-2.5.8/drivers/ide/pdc4030.c linux/drivers/ide/pdc4030.c
--- linux-2.5.8/drivers/ide/pdc4030.c	Tue Apr 16 06:01:07 2002
+++ linux/drivers/ide/pdc4030.c	Tue Apr 16 02:59:10 2002
@@ -183,7 +183,7 @@
 			"%s: Failed Promise read config!\n",hwif->name);
 		return 0;
 	}
-	ata_input_data(drive, &ident, SECTOR_WORDS);
+	ata_read(drive, &ident, SECTOR_WORDS);
 	if (ident.id[1] != 'P' || ident.id[0] != 'T') {
 		return 0;
 	}
@@ -332,7 +332,7 @@
 		nsect = sectors_avail;
 	sectors_avail -= nsect;
 	to = bio_kmap_irq(rq->bio, &flags) + ide_rq_offset(rq);
-	ata_input_data(drive, to, nsect * SECTOR_WORDS);
+	ata_read(drive, to, nsect * SECTOR_WORDS);
 #ifdef DEBUG_READ
 	printk(KERN_DEBUG "%s:  promise_read: sectors(%ld-%ld), "
 	       "buf=0x%08lx, rem=%ld\n", drive->name, rq->sector,
@@ -460,7 +460,7 @@
 		 * Ok, we're all setup for the interrupt
 		 * re-entering us on the last transfer.
 		 */
-		taskfile_output_data(drive, buffer, nsect<<7);
+		ata_write(drive, buffer, nsect << 7);
 		bio_kunmap_irq(buffer, &flags);
 	} while (mcount);
 
diff -urN linux-2.5.8/drivers/scsi/ide-scsi.c linux/drivers/scsi/ide-scsi.c
--- linux-2.5.8/drivers/scsi/ide-scsi.c	Mon Apr 15 08:53:51 2002
+++ linux/drivers/scsi/ide-scsi.c	Tue Apr 16 03:39:25 2002
@@ -140,7 +140,7 @@
 		}
 		count = min(pc->sg->length - pc->b_count, bcount);
 		buf = page_address(pc->sg->page) + pc->sg->offset;
-		atapi_input_bytes (drive, buf + pc->b_count, count);
+		atapi_read(drive, buf + pc->b_count, count);
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -162,7 +162,7 @@
 		}
 		count = min(pc->sg->length - pc->b_count, bcount);
 		buf = page_address(pc->sg->page) + pc->sg->offset;
-		atapi_output_bytes (drive, buf + pc->b_count, count);
+		atapi_write(drive, buf + pc->b_count, count);
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -363,7 +363,7 @@
 					if (pc->sg)
 						idescsi_input_buffers(drive, pc, temp);
 					else
-						atapi_input_bytes(drive, pc->current_position, temp);
+						atapi_read(drive, pc->current_position, temp);
 					printk(KERN_ERR "ide-scsi: transferred %d of %d bytes\n", temp, bcount);
 				}
 				pc->actually_transferred += temp;
@@ -382,13 +382,13 @@
 		if (pc->sg)
 			idescsi_input_buffers (drive, pc, bcount);
 		else
-			atapi_input_bytes (drive,pc->current_position,bcount);
+			atapi_read(drive,pc->current_position,bcount);
 	} else {
 		set_bit(PC_WRITING, &pc->flags);
 		if (pc->sg)
 			idescsi_output_buffers (drive, pc, bcount);
 		else
-			atapi_output_bytes (drive,pc->current_position,bcount);
+			atapi_write(drive,pc->current_position,bcount);
 	}
 	pc->actually_transferred+=bcount;				/* Update the current position */
 	pc->current_position+=bcount;
@@ -414,7 +414,7 @@
 		return ide_stopped;
 	}
 	ide_set_handler(drive, &idescsi_pc_intr, get_timeout(pc), NULL);	/* Set the interrupt routine */
-	atapi_output_bytes (drive, scsi->pc->c, 12);			/* Send the actual packet */
+	atapi_write(drive, scsi->pc->c, 12);			/* Send the actual packet */
 	return ide_started;
 }
 
diff -urN linux-2.5.8/include/linux/ide.h linux/include/linux/ide.h
--- linux-2.5.8/include/linux/ide.h	Tue Apr 16 06:01:07 2002
+++ linux/include/linux/ide.h	Tue Apr 16 05:44:28 2002
@@ -313,9 +313,11 @@
 #define IDE_CUR_AR(drive)	(HWGROUP((drive))->rq->special)
 
 struct ide_settings_s;
-
-typedef struct ide_drive_s {
-	struct ata_channel *channel;	/* parent pointer to the channel we are attached to  */
+/* structure describing an ATA/ATAPI device */
+typedef
+struct ata_device {
+	struct ata_channel *	channel;
+	char			name[6];	/* device name */
 
 	unsigned int usage;		/* current "open()" count for drive */
 	char type; /* distingiush different devices: disk, cdrom, tape, floppy, ... */
@@ -324,11 +326,11 @@
 	 * could move this to the channel and many sync problems would
 	 * magically just go away.
 	 */
-	request_queue_t	queue;	/* per device request queue */
+	request_queue_t	queue;		/* per device request queue */
 
-	struct list_head free_req; /* free ata requests */
+	struct list_head free_req;	/* free ata requests */
 
-	struct ide_drive_s	*next;	/* circular list of hwgroup drives */
+	struct ata_device *next;	/* circular list of hwgroup drives */
 
 	/* Those are directly injected jiffie values. They should go away and
 	 * we should use generic timers instead!!!
@@ -346,8 +348,6 @@
 	byte	 retry_pio;		/* retrying dma capable host in pio */
 	byte	 state;			/* retry state */
 	byte     unmask;		/* flag: okay to unmask other irqs */
-	byte     slow;			/* flag: slow data port */
-	byte     bswap;			/* flag: byte swap data */
 	byte     dsc_overlap;		/* flag: DSC overlap */
 
 	unsigned waiting_for_dma: 1;	/* dma currently in progress */
@@ -359,7 +359,6 @@
 	unsigned removable	: 1;	/* 1 if need to do check_media_change */
 	unsigned forced_geom	: 1;	/* 1 if hdx=c,h,s was given at boot */
 	unsigned no_unmask	: 1;	/* disallow setting unmask bit */
-	unsigned no_io_32bit	: 1;	/* disallow enabling 32bit I/O */
 	unsigned nobios		: 1;	/* flag: do not probe bios for drive */
 	unsigned revalidate	: 1;	/* request revalidation */
 	unsigned atapi_overlap	: 1;	/* flag: ATAPI overlap (not supported) */
@@ -376,7 +375,6 @@
 	byte		mult_count;	/* current multiple sector setting */
 	byte		mult_req;	/* requested multiple sector setting */
 	byte		tune_req;	/* requested drive tuning setting */
-	byte		io_32bit;	/* 0=16-bit, 1=32-bit, 2/3=32bit+sync */
 	byte		bad_wstat;	/* used for ignoring WRERR_STAT */
 	byte		nowerr;		/* used for ignoring WRERR_STAT */
 	byte		sect0;		/* offset of first sector for DM6:DDO */
@@ -390,12 +388,18 @@
 	unsigned long long capacity48;	/* total number of sectors */
 	unsigned int	drive_data;	/* for use by tuneproc/selectproc as needed */
 
+	/* FIXME: Those are properties of a channel and not a drive!  Move them
+	 * later there.
+	 */
+	byte		slow;		/* flag: slow data port */
+	unsigned no_io_32bit	: 1;	/* disallow enabling 32bit I/O */
+	byte		io_32bit;	/* 0=16-bit, 1=32-bit, 2/3=32bit+sync */
+
 	wait_queue_head_t wqueue;	/* used to wait for drive in open() */
 
 	struct hd_driveid *id;		/* drive model identification info */
 	struct hd_struct  *part;	/* drive partition table */
 
-	char		name[6];	/* drive name, such as "hda" */
 	struct ata_operations *driver;
 
 	void		*driver_data;	/* extra driver data */
@@ -447,47 +451,6 @@
 
 typedef int (ide_dmaproc_t)(ide_dma_action_t, ide_drive_t *);
 
-/*
- * An ide_ideproc_t() performs CPU-polled transfers to/from a drive.
- * Arguments are: the drive, the buffer pointer, and the length (in bytes or
- * words depending on if it's an IDE or ATAPI call).
- *
- * If it is not defined for a controller, standard-code is used from ide.c.
- *
- * Controllers which are not memory-mapped in the standard way need to
- * override that mechanism using this function to work.
- *
- */
-typedef enum { ideproc_ide_input_data,    ideproc_ide_output_data,
-	       ideproc_atapi_input_bytes, ideproc_atapi_output_bytes
-} ide_ide_action_t;
-
-typedef void (ide_ideproc_t)(ide_ide_action_t, ide_drive_t *, void *, unsigned int);
-
-/*
- * An ide_tuneproc_t() is used to set the speed of an IDE interface
- * to a particular PIO mode.  The "byte" parameter is used
- * to select the PIO mode by number (0,1,2,3,4,5), and a value of 255
- * indicates that the interface driver should "auto-tune" the PIO mode
- * according to the drive capabilities in drive->id;
- *
- * Not all interface types support tuning, and not all of those
- * support all possible PIO settings.  They may silently ignore
- * or round values as they see fit.
- */
-typedef void (ide_tuneproc_t) (ide_drive_t *, byte);
-typedef int (ide_speedproc_t) (ide_drive_t *, byte);
-
-/*
- * This is used to provide support for strange interfaces
- */
-typedef void (ide_selectproc_t) (ide_drive_t *);
-typedef void (ide_resetproc_t) (ide_drive_t *);
-typedef int (ide_quirkproc_t) (ide_drive_t *);
-typedef void (ide_intrproc_t) (ide_drive_t *);
-typedef void (ide_maskproc_t) (ide_drive_t *, int);
-typedef void (ide_rw_proc_t) (ide_drive_t *, ide_dma_action_t);
-
 enum {
 	ATA_PRIMARY	= 0,
 	ATA_SECONDARY	= 1
@@ -507,15 +470,40 @@
 #endif
 	ide_drive_t	drives[MAX_DRIVES];	/* drive info */
 	struct gendisk	*gd;		/* gendisk structure */
-	ide_tuneproc_t	*tuneproc;	/* routine to tune PIO mode for drives */
-	ide_speedproc_t	*speedproc;	/* routine to retune DMA modes for drives */
-	ide_selectproc_t *selectproc;	/* tweaks hardware to select drive */
-	ide_resetproc_t	*resetproc;	/* routine to reset controller after a disk reset */
-	ide_intrproc_t	*intrproc;	/* special interrupt handling for shared pci interrupts */
-	ide_maskproc_t	*maskproc;	/* special host masking for drive selection */
-	ide_quirkproc_t	*quirkproc;	/* check host's drive quirk list */
-	ide_rw_proc_t	*rwproc;	/* adjust timing based upon rq->cmd direction */
-	ide_ideproc_t   *ideproc;       /* CPU-polled transfer routine */
+
+	/*
+	 * Routines to tune PIO and DMA mode for drives.
+	 *
+	 * A value of 255 indicates that the function should choose the optimal
+	 * mode itself.
+	 */
+	void (*tuneproc) (ide_drive_t *, byte pio);
+	int (*speedproc) (ide_drive_t *, byte pio);
+
+	/* tweaks hardware to select drive */
+	void (*selectproc) (ide_drive_t *);
+
+	/* routine to reset controller after a disk reset */
+	void (*resetproc) (ide_drive_t *);
+
+	/* special interrupt handling for shared pci interrupts */
+	void (*intrproc) (ide_drive_t *);
+
+	/* special host masking for drive selection */
+	void (*maskproc) (ide_drive_t *, int);
+
+	/* adjust timing based upon rq->cmd direction */
+	void (*rwproc) (ide_drive_t *, ide_dma_action_t);
+
+	/* check host's drive quirk list */
+	int (*quirkproc) (ide_drive_t *);
+
+	/* CPU-polled transfer routines */
+	void (*ata_read)(ide_drive_t *, void *, unsigned int);
+	void (*ata_write)(ide_drive_t *, void *, unsigned int);
+	void (*atapi_read)(ide_drive_t *, void *, unsigned int);
+	void (*atapi_write)(ide_drive_t *, void *, unsigned int);
+
 	ide_dmaproc_t	*dmaproc;	/* dma read/write/abort routine */
 	unsigned long	dma_base;	/* base addr for dma ports */
 	unsigned	dma_extra;	/* extra addr for dma ports */
@@ -829,7 +817,7 @@
  */
 struct ata_request {
 	struct request		*ar_rq;		/* real request */
-	struct ide_drive_s	*ar_drive;	/* associated drive */
+	struct ata_device	*ar_drive;	/* associated drive */
 	unsigned long		ar_flags;	/* ATA_AR_* flags */
 	int			ar_tag;		/* tag number, if any */
 	struct list_head	ar_queue;	/* pending list */
@@ -848,12 +836,11 @@
 
 #define AR_TASK_CMD(ar)	((ar)->ar_task.taskfile.command)
 
-void ata_input_data (ide_drive_t *drive, void *buffer, unsigned int wcount);
-void ata_output_data (ide_drive_t *drive, void *buffer, unsigned int wcount);
-void atapi_input_bytes (ide_drive_t *drive, void *buffer, unsigned int bytecount);
-void atapi_output_bytes (ide_drive_t *drive, void *buffer, unsigned int bytecount);
-void taskfile_input_data (ide_drive_t *drive, void *buffer, unsigned int wcount);
-void taskfile_output_data (ide_drive_t *drive, void *buffer, unsigned int wcount);
+extern void ata_read(ide_drive_t *drive, void *buffer, unsigned int wcount);
+extern void ata_write(ide_drive_t *drive, void *buffer, unsigned int wcount);
+
+extern void atapi_read(ide_drive_t *drive, void *buffer, unsigned int bytecount);
+extern void atapi_write(ide_drive_t *drive, void *buffer, unsigned int bytecount);
 
 extern ide_startstop_t ata_taskfile(ide_drive_t *drive,
 		struct ata_taskfile *args, struct request *rq);