Re: [PATCH] 2.5.15 IDE 61

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Hi...  Please correct me if I'm wrong but:

I think this patch is broken wrt serialization of channels on buggy
chipsets.

The hwgroup was serialized so that in certain cases, it can contain BOTH
channels, and thus only one channel is active at a time (e.g. cmd640). 
With this patch, you are now serializing only channels, not hwgroups
(which makes hwgroup totally redundant, yes?), and I can't see which bit
of your patch protects the chipsets that need both channels to be
serialized.

I think I see where you're going with the cleanup (and this isn't
unrelated to the conversation about IDE-62) but as it stands, this patch
will IMHO totally fsck any machines with dodgy chipsets.

Neil

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Neil Conway napisa?:
> Hi...  Please correct me if I'm wrong but:
> 
> I think this patch is broken wrt serialization of channels on buggy
> chipsets.
> 
> The hwgroup was serialized so that in certain cases, it can contain BOTH
> channels, and thus only one channel is active at a time (e.g. cmd640). 
> With this patch, you are now serializing only channels, not hwgroups
> (which makes hwgroup totally redundant, yes?), and I can't see which bit
> of your patch protects the chipsets that need both channels to be
> serialized.
> 
> I think I see where you're going with the cleanup (and this isn't
> unrelated to the conversation about IDE-62) but as it stands, this patch
> will IMHO totally fsck any machines with dodgy chipsets.
> 
> Neil

No it will not, since we act serialized on ide_lock anyway.
However I have right now per channel (or serialization group)
lock running right now / modulo locking order problems.

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Martin Dalecki wrote:
> 
> Uz.ytkownik Neil Conway napisa?:
> > The hwgroup was serialized so that in certain cases, it can contain BOTH
> > channels, and thus only one channel is active at a time (e.g. cmd640).
> > With this patch, you are now serializing only channels, not hwgroups
> > (which makes hwgroup totally redundant, yes?), and I can't see which bit
> > of your patch protects the chipsets that need both channels to be
> > serialized.
> >
> > I think I see where you're going with the cleanup (and this isn't
> > unrelated to the conversation about IDE-62) but as it stands, this patch
> > will IMHO totally fsck any machines with dodgy chipsets.
> 
> No it will not, since we act serialized on ide_lock anyway.
> However I have right now per channel (or serialization group)
> lock running right now / modulo locking order problems.

One of us is missing the point (and I'm the newbie so blame me ;-)), so
here goes:

Only the calls from the block layer to the request_fn are serialized by
ide_lock. Not the actual data transfers.  Here's the scenario: 

Firstly, an I/O request is queued by ide_do_request(), and then it
returns.  Let's assume that DMA is now in progress.  Once
ide_do_request() returns, the lock is released by the block layer.  Now
the corruption scenario: another request can come in for the other
channel while our first I/O is in flight, and since the ide_lock isn't
held, and the second channel isn't BUSY, ide_do_request() will be happy
to try and start an I/O on that channel too.  BOOM.

Or is there a dumb mistake in my logic?

Neil
PS: I appreciate that your code is in a transition phase but I think
it's desirable to avoid badly broken 2.5's all the same.

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Neil Conway napisa?:
> Martin Dalecki wrote:
> 
>>Uz.ytkownik Neil Conway napisa?:
>>
>>>The hwgroup was serialized so that in certain cases, it can contain BOTH
>>>channels, and thus only one channel is active at a time (e.g. cmd640).
>>>With this patch, you are now serializing only channels, not hwgroups
>>>(which makes hwgroup totally redundant, yes?), and I can't see which bit
>>>of your patch protects the chipsets that need both channels to be
>>>serialized.
>>>
>>>I think I see where you're going with the cleanup (and this isn't
>>>unrelated to the conversation about IDE-62) but as it stands, this patch
>>>will IMHO totally fsck any machines with dodgy chipsets.
>>
>>No it will not, since we act serialized on ide_lock anyway.
>>However I have right now per channel (or serialization group)
>>lock running right now / modulo locking order problems.
> 
> 
> One of us is missing the point (and I'm the newbie so blame me ;-)), so
> here goes:
> 
> Only the calls from the block layer to the request_fn are serialized by
> ide_lock. Not the actual data transfers.  Here's the scenario: 
> 
> Firstly, an I/O request is queued by ide_do_request(), and then it
> returns.  Let's assume that DMA is now in progress.  Once
> ide_do_request() returns, the lock is released by the block layer.  Now
> the corruption scenario: another request can come in for the other
> channel while our first I/O is in flight, and since the ide_lock isn't
> held, and the second channel isn't BUSY, ide_do_request() will be happy
> to try and start an I/O on that channel too.  BOOM.
> 
> Or is there a dumb mistake in my logic?

There is no problem to go in paralell on different channels for
requests. The serialization has only to be done
for the drive setup.

> Neil
> PS: I appreciate that your code is in a transition phase but I think
> it's desirable to avoid badly broken 2.5's all the same.
> 
> 

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Martin Dalecki wrote:
> There is no problem to go in paralell on different channels for
> requests. The serialization has only to be done
> for the drive setup.

I agree for general chipsets, but my whole point was with regard to
buggy chipsets which need to be serialized on both channels.

If you're saying that even these broken chipsets are OK with having
transfers on one channel while setting up transfers on another channel,
then perhaps you are right but that's not what I believed to be the case
(can't find info to tell me either way right now).

But if that really were the case, then how could the (e.g.) cmd640
problem ever have been manifested?  A spinlock is ALWAYS held while
ide_do_request is executing.  Even if it weren't, only an SMP machine
could be trying to program both channels simultaneously because
interrupts are disabled too.

Neil

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Neil Conway napisa?:
> Martin Dalecki wrote:
> 
>>There is no problem to go in paralell on different channels for
>>requests. The serialization has only to be done
>>for the drive setup.
> 
> 
> I agree for general chipsets, but my whole point was with regard to
> buggy chipsets which need to be serialized on both channels.
> 
> If you're saying that even these broken chipsets are OK with having
> transfers on one channel while setting up transfers on another channel,
> then perhaps you are right but that's not what I believed to be the case
> (can't find info to tell me either way right now).
> 
> But if that really were the case, then how could the (e.g.) cmd640
> problem ever have been manifested?  A spinlock is ALWAYS held while
> ide_do_request is executing.  Even if it weren't, only an SMP machine
> could be trying to program both channels simultaneously because
> interrupts are disabled too.


Well in the next patch round the hwgroup will be replaced with
a spin lock, which is supposed to be shared between channels which need
forced access serialization between them. Please look
at patches 62a and 63 :-).

Re: [PATCH] 2.5.15 IDE 61

[Russell King]

On Tue, May 14, 2002 at 12:21:44PM +0200, Martin Dalecki wrote:
> Well in the next patch round the hwgroup will be replaced with
> a spin lock, which is supposed to be shared between channels which need
> forced access serialization between them. Please look
> at patches 62a and 63 :-).

Something here smells fishy here - you shouldn't hold a spinlock for a long
time (a long time === spinlocking, setting up the drive, possibly scheduling,
transferring data, getting status, then unlocking).  Also, remember,
spinlocks are no-ops on uniprocessor systems.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Russell King napisa?:
> On Tue, May 14, 2002 at 12:21:44PM +0200, Martin Dalecki wrote:
> 
>>Well in the next patch round the hwgroup will be replaced with
>>a spin lock, which is supposed to be shared between channels which need
>>forced access serialization between them. Please look
>>at patches 62a and 63 :-).
> 
> 
> Something here smells fishy here - you shouldn't hold a spinlock for a long
> time (a long time === spinlocking, setting up the drive, possibly scheduling,
> transferring data, getting status, then unlocking).  Also, remember,
> spinlocks are no-ops on uniprocessor systems.

Well, let's just have a look at how to share request queues between
channels which need this treatment in addition then?

Re: [PATCH] 2.5.15 IDE 61

[Alan Cox]

> Something here smells fishy here - you shouldn't hold a spinlock for a long
> time (a long time === spinlocking, setting up the drive, possibly scheduling,

You can't hold it while scheduling or you may deadlock

> transferring data, getting status, then unlocking).  Also, remember,
> spinlocks are no-ops on uniprocessor systems.

Its possible it can be done with a semaphore but the whole business is
pretty tricky. IDE command processing occurs a fair bit at interrupt level
and you definitely don't want to block interrupts for long periods.

If the queue abstraction is right then the block layer should do all the
synchronization work that is required. It may cost a few cycles on the odd
case you can do overlapped command setup but that versus a nasty locking
mess its got to be better to lose those few cycles.

I don't even Martin here, the ide locking is currently utterly vile

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Alan Cox napisa?:
>>Something here smells fishy here - you shouldn't hold a spinlock for a long
>>time (a long time === spinlocking, setting up the drive, possibly scheduling,
> 
> 
> You can't hold it while scheduling or you may deadlock
> 
> 
>>transferring data, getting status, then unlocking).  Also, remember,
>>spinlocks are no-ops on uniprocessor systems.
> 
> 
> Its possible it can be done with a semaphore but the whole business is
> pretty tricky. IDE command processing occurs a fair bit at interrupt level
> and you definitely don't want to block interrupts for long periods.

... Becouse the chances are fscking high - that you will miss command
completion interrupts for the "other drive" on the same channel.
The dready heritage of "dangling ISA bus" with *idiotic* edge triggered
interrupts bite us here. Someone just please shoot this enginer
who saved the few pullup resistors in the head or send him alternatively
for "hunting white bears" in Siberia... about 15 years would be fine in my
opinnion.

> If the queue abstraction is right then the block layer should do all the
> synchronization work that is required. It may cost a few cycles on the odd
> case you can do overlapped command setup but that versus a nasty locking
> mess its got to be better to lose those few cycles.
> 
> I don't even Martin here, the ide locking is currently utterly vile

Don't worry - I got your point.

Re: [PATCH] 2.5.15 IDE 61

[Alan Cox]

> > Its possible it can be done with a semaphore but the whole business is
> > pretty tricky. IDE command processing occurs a fair bit at interrupt level
> > and you definitely don't want to block interrupts for long periods.
> 
> ... Becouse the chances are fscking high - that you will miss command
> completion interrupts for the "other drive" on the same channel.

The shared IRQ capable IDE ards I am aware of all do have proper tristates
 but you still have to handle the edge trigger very carefully.

If you can miss a command completion interrupt you have a bug. Since you
know each drive on the bus you can poll each afflicted device for interrupts
until you reach a point where you completed an entire poll loop and nobody
had an IRQ pending.

At that point you know an edge transition has occurred and that a real
IRQ will be posted when the next event occurs because that too will cause
an edge.

A good place for examples of this in the DOS world is things like serial
drivers, many of which could handle broken shared IRQ ISA setups correctly
using this technique.

In the case without tristates the stronger driver tends to win the argument
about the line in either direction and nothing works at all.

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Alan Cox napisa?:
>>>Its possible it can be done with a semaphore but the whole business is
>>>pretty tricky. IDE command processing occurs a fair bit at interrupt level
>>>and you definitely don't want to block interrupts for long periods.
>>
>>... Becouse the chances are fscking high - that you will miss command
>>completion interrupts for the "other drive" on the same channel.
> 
> 
> The shared IRQ capable IDE ards I am aware of all do have proper tristates
>  but you still have to handle the edge trigger very carefully.
> 
> If you can miss a command completion interrupt you have a bug. Since you
> know each drive on the bus you can poll each afflicted device for interrupts
> until you reach a point where you completed an entire poll loop and nobody
> had an IRQ pending.
> 
> At that point you know an edge transition has occurred and that a real
> IRQ will be posted when the next event occurs because that too will cause
> an edge.
> 
> A good place for examples of this in the DOS world is things like serial
> drivers, many of which could handle broken shared IRQ ISA setups correctly
> using this technique.
> 
> In the case without tristates the stronger driver tends to win the argument
> about the line in either direction and nothing works at all.


Well anyway. What we have right now, looking for the channel perspective,
is indeed some nIEN tricks done here and there. However the problem is
that we postpone the disabling of interface interrupts now until the
time a next request gets queued. In addition the driver is doing quite
a lot of polling for the next expected interrutp as well.

Right now the consequences are indeed very simple for me. The time I
started to sanitize the data structures I first had to paint down
diagram of pointers between them. Now it's simple time to write down
a state diagram for the IRQ / request handling code paths :-).

Re: [PATCH] 2.5.15 IDE 61

[Pavel Machek]

Hi!

> interrupts bite us here. Someone just please shoot this enginer
> who saved the few pullup resistors in the head or send him alternatively
> for "hunting white bears" in Siberia... about 15 years would be fine in my
> opinnion.

Actually, at least unknown card wanting attetion does not kill whole system
like in level-triggered case. You can use timer to recover lost interrupts,
or you just should not loose them in the first place.
								Pavel
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: [PATCH] 2.5.15 IDE 61

[Russell King]

On Tue, May 14, 2002 at 01:10:58PM +0100, Alan Cox wrote:
> I don't even Martin here, the ide locking is currently utterly vile

Agreed.

I'm adding BUG_ON()s to the IDE drivers I maintain where we must ensure
only one channel is DMAing to catch possible data corruption before it
happens.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Russell King napisa?:
> On Tue, May 14, 2002 at 01:10:58PM +0100, Alan Cox wrote:
> 
>>I don't even Martin here, the ide locking is currently utterly vile
> 
> 
> Agreed.
> 
> I'm adding BUG_ON()s to the IDE drivers I maintain where we must ensure
> only one channel is DMAing to catch possible data corruption before it
> happens.

That is indeed a good idea!

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Alan Cox wrote:
> If the queue abstraction is right then the block layer should do all the
> synchronization work that is required. 

I think you're wrong Alan.  Take a good IDE chipset as an example: both
channels can be active at the same time, but you still can't talk to one
drive while the other drive on the same channel is DMAing.

I'm not a block layer expert, but it appears to me that the block layer
only synchronises requests by use of the spinlock.  If I'm right, then
the block layer has no way of knowing that hda is DMAing when a request
is initiated for hdb.  This was the whole reason (as I see it) that
hwgroup->busy existed: to prevent attempts to use the same IDE cable for
two things at the same time.

It doesn't matter how you perform the queue abstraction in this case:
the fact that the device+channel+cable is busy in an asynchronous manner
makes it impossible for the block layer to deal with this.  [[Or am I
way off base?!]]

The right way is the way it is being done at present surely: if the busy
flag on the hwgroup is set, then ide_do_request() just returns.  (NB:
When I say "right way", I don't mean to imply that the code is elegant,
desirable, or even bug-free, just that it correctly handles this busy
state.)

>It may cost a few cycles on the odd
> case you can do overlapped command setup but that versus a nasty locking
> mess its got to be better to lose those few cycles.

Well, Jens and others are busy implementing TCQ where things are just so
much easier to fsck up :-))

> I don't even Martin here, the ide locking is currently utterly vile

Agreed, but surely with some concerted effort we can truly fix the IDE
code.  Can't be beyond us all can it?

Neil

Re: [PATCH] 2.5.15 IDE 61

[Andre Hedrick]

Arbitrate your incoming interrupts.  In the classic dual interrupt you are
scott free for the most part, erm interrupt sharing will eat you alive.
HBA's w/ N channels will eat the BLOCK Layer of lunch and purge it for
dinner.  If you notice SCSI originally had queues based on the HBA for a
reason,  one can read all day long form any devices on the chain, but
issue a write and everything grinds to a halt.

The original taskfile driver was to permit set and go calls of a
multi-level queues.  It was also to permit fake local tags with additional
load balancing on the channel w/ mixed devices and/or w/ broken hardware
force a simplex behavior.

hwif[n].drives[m].queue     \
			     --- hwif[n].queue
hwif[n].drives[m+1].queue   /
							hwgroup.queue
hwif[n+1].drives[m].queue   \
			     --- hwif[n+1].queue
hwif[n+1].drives[m+1].queue /

 
In the future:

hwif[0].drives[0].queue   <> hwif[0].queue
hwif[1].drives[0].queue   <> hwif[1].queue
hwif[2].drives[0].queue   <> hwif[2].queue
...
hwif[n-1].drives[0].queue <> hwif[n-1].queue
hwif[n].drives[0].queue   <> hwif[n].queue

Where "n" ranges from 2->20  all on the same hwgroup.queue

Now how is your spinlock going to process all of those in parallel?
I can tell you first hand, it can't.

Cheers,

On Tue, 14 May 2002, Neil Conway wrote:

> Alan Cox wrote:
> > If the queue abstraction is right then the block layer should do all the
> > synchronization work that is required. 
> 
> I think you're wrong Alan.  Take a good IDE chipset as an example: both
> channels can be active at the same time, but you still can't talk to one
> drive while the other drive on the same channel is DMAing.
> 
> I'm not a block layer expert, but it appears to me that the block layer
> only synchronises requests by use of the spinlock.  If I'm right, then
> the block layer has no way of knowing that hda is DMAing when a request
> is initiated for hdb.  This was the whole reason (as I see it) that
> hwgroup->busy existed: to prevent attempts to use the same IDE cable for
> two things at the same time.
> 
> It doesn't matter how you perform the queue abstraction in this case:
> the fact that the device+channel+cable is busy in an asynchronous manner
> makes it impossible for the block layer to deal with this.  [[Or am I
> way off base?!]]
> 
> The right way is the way it is being done at present surely: if the busy
> flag on the hwgroup is set, then ide_do_request() just returns.  (NB:
> When I say "right way", I don't mean to imply that the code is elegant,
> desirable, or even bug-free, just that it correctly handles this busy
> state.)
> 
> >It may cost a few cycles on the odd
> > case you can do overlapped command setup but that versus a nasty locking
> > mess its got to be better to lose those few cycles.
> 
> Well, Jens and others are busy implementing TCQ where things are just so
> much easier to fsck up :-))
> 
> > I don't even Martin here, the ide locking is currently utterly vile
> 
> Agreed, but surely with some concerted effort we can truly fix the IDE
> code.  Can't be beyond us all can it?
> 
> Neil
> -
> 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/
> 

Andre Hedrick
LAD Storage Consulting Group

Re: [PATCH] 2.5.15 IDE 61

[Alan Cox]

> I think you're wrong Alan.  Take a good IDE chipset as an example: both
> channels can be active at the same time, but you still can't talk to one
> drive while the other drive on the same channel is DMAing.

Sure.

> I'm not a block layer expert, but it appears to me that the block layer
> only synchronises requests by use of the spinlock.  If I'm right, then
> the block layer has no way of knowing that hda is DMAing when a request
> is initiated for hdb.  This was the whole reason (as I see it) that
> hwgroup->busy existed: to prevent attempts to use the same IDE cable for
> two things at the same time.

The newer block code has queues. Its up to the block layer to deal with
the queue locking.

> It doesn't matter how you perform the queue abstraction in this case:
> the fact that the device+channel+cable is busy in an asynchronous manner
> makes it impossible for the block layer to deal with this.  [[Or am I
> way off base?!]]

I think you are way off base. If you have a single queue for both hda and
hdb then requests will get dumped into that in a way that processing that
queue implicitly does the ordering you require.

>From an abstract hardware point of view each ide controller is a queue not
each device. Not following that is I think the cause of much of the existing
pain and suffering.

Alan

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Alan Cox napisa?:
>>I think you're wrong Alan.  Take a good IDE chipset as an example: both
>>channels can be active at the same time, but you still can't talk to one
>>drive while the other drive on the same channel is DMAing.
> 
> 
> Sure.
> 
> 
>>I'm not a block layer expert, but it appears to me that the block layer
>>only synchronises requests by use of the spinlock.  If I'm right, then
>>the block layer has no way of knowing that hda is DMAing when a request
>>is initiated for hdb.  This was the whole reason (as I see it) that
>>hwgroup->busy existed: to prevent attempts to use the same IDE cable for
>>two things at the same time.
> 
> 
> The newer block code has queues. Its up to the block layer to deal with
> the queue locking.
> 
> 
>>It doesn't matter how you perform the queue abstraction in this case:
>>the fact that the device+channel+cable is busy in an asynchronous manner
>>makes it impossible for the block layer to deal with this.  [[Or am I
>>way off base?!]]
> 
> 
> I think you are way off base. If you have a single queue for both hda and
> hdb then requests will get dumped into that in a way that processing that
> queue implicitly does the ordering you require.
> 
>>From an abstract hardware point of view each ide controller is a queue not
> each device. Not following that is I think the cause of much of the existing
> pain and suffering.


Yes thinking about it longer and longer I tend to the same conclusion,
that we just shouldn't have per device queue but per channel queues instead.
The only problem here is the fact that some device properties
are attached to the queue right now. Like for example sector size and friends.

I didn't have a too deep look in to the generic blk layer. But I would
rather expect that since the lower layers are allowed to pass
an spin lock up to the queue intialization, sharing a spin lock
between two request queues should just serialize them with respect to
each other. And this is precisely what 63 does.

BTW> FreeBSD does simple use per channel queues.

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Martin Dalecki wrote:
> 
> Uz.ytkownik Alan Cox napisa?:
> >>From an abstract hardware point of view each ide controller is a queue not
> > each device. Not following that is I think the cause of much of the existing
> > pain and suffering.
> 
> Yes thinking about it longer and longer I tend to the same conclusion,
> that we just shouldn't have per device queue but per channel queues instead.
> The only problem here is the fact that some device properties
> are attached to the queue right now. Like for example sector size and friends.

OK..

> I didn't have a too deep look in to the generic blk layer. But I would
> rather expect that since the lower layers are allowed to pass
> an spin lock up to the queue intialization, sharing a spin lock
> between two request queues should just serialize them with respect to
> each other. And this is precisely what 63 does.

(You're planning to have two queues per channel but sharing the same
lock?)

On the serialisation issue: what does serialisation of the queues with
respect to each other mean to you?  I understand it to mean that we
won't ever call the request_fn of both queues at the same time - because
that's all the actual spinlock buys you.  It does not IIUC mean that you
can't get a call to request_fn of one queue while the other queue has
lots of requests in it (which are potentially being serviced by DMA). 
Or does it?  Does the block layer track which requests are "active"
somehow?

My main question could be posed thus: am I right in thinking that we
MUST track the busy-ness of the channel at all times?  (and for broken
chipsets, track the logical OR of both channels' busy-ness)

Neil
PS: I'm hoping someone will either say "here's why you're wrong" or
"you're right" RSN...

Re: [PATCH] 2.5.15 IDE 61

[Jens Axboe]

On Tue, May 14 2002, Neil Conway wrote:
> On the serialisation issue: what does serialisation of the queues with
> respect to each other mean to you?  I understand it to mean that we
> won't ever call the request_fn of both queues at the same time - because
> that's all the actual spinlock buys you.  It does not IIUC mean that you
> can't get a call to request_fn of one queue while the other queue has
> lots of requests in it (which are potentially being serviced by DMA). 

Bingo, this is exactly right and makes the point a hell of a lot better
than I did in my previous mail. Shared locks will only buy you that
noone fiddles with one list while the other is busy (ie nothing for us).
To really serialize operations the queue _must_ be shared with whoever
requires serialiation.

If not, the problem will have to be solved at the IDE level, not the
block level. And that has not looked pretty in the past.

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Jens Axboe wrote:
> On Tue, May 14 2002, Neil Conway wrote:
> > that's all the actual spinlock buys you.  It does not IIUC mean that you
> > can't get a call to request_fn of one queue while the other queue has
> > lots of requests in it (which are potentially being serviced by DMA).
> 
> Bingo, this is exactly right and makes the point a hell of a lot better
> than I did in my previous mail. Shared locks will only buy you that
> noone fiddles with one list while the other is busy (ie nothing for us).

Cool, thanks ;-)  Now watch me blow all my cred with this post:

> To really serialize operations the queue _must_ be shared with whoever
> requires serialiation.

Why will this help?  The hardware can still be doing DMA on hda while
the queue's request_fn is called quite legitimately for a hdb request -
and the IDE code MUST impose the serialization here to avoid hitting the
cable with commands destined for hdb. (For example, by waiting for
!channel->busy.)

> If not, the problem will have to be solved at the IDE level, not the
> block level. And that has not looked pretty in the past.

I just can't see a way for the block level to remove the need for the
busy flag.  I _think_ Alan just agreed with me.  I'm not sure but I get
the impression that you are saying the IDE code doesn't need to do this
serialization...

I'm certainly learning, thanks guys.
Neil

Re: [PATCH] 2.5.15 IDE 61

[Jens Axboe]

On Tue, May 14 2002, Neil Conway wrote:
> > To really serialize operations the queue _must_ be shared with whoever
> > requires serialiation.
> 
> Why will this help?  The hardware can still be doing DMA on hda while
> the queue's request_fn is called quite legitimately for a hdb request -
> and the IDE code MUST impose the serialization here to avoid hitting the
> cable with commands destined for hdb. (For example, by waiting for
> !channel->busy.)

Current IDE code leaves a request on the list until it has completed
(this is ignoring TCQ of course), so there's no way that you could start
serving a second request before the first one completes.

> > If not, the problem will have to be solved at the IDE level, not the
> > block level. And that has not looked pretty in the past.
> 
> I just can't see a way for the block level to remove the need for the
> busy flag.  I _think_ Alan just agreed with me.  I'm not sure but I get
> the impression that you are saying the IDE code doesn't need to do this
> serialization...

To be honest, I haven't given it too much thought right now. The nice
thing about the queue level serialization is that it all happens
automagically for IDE, without having it maintain any busy state on that
itself.

However, I may just talking out of my ass and implementation details
will mean it's still better to at least manage some of the serialization
at the ide level.

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Jens Axboe wrote:
> On Tue, May 14 2002, Neil Conway wrote:
> > > To really serialize operations the queue _must_ be shared with whoever
> > > requires serialiation.
> > Why will this help?  The hardware can still be doing DMA on hda while
> > the queue's request_fn is called quite legitimately for a hdb request -
> > and the IDE code MUST impose the serialization here to avoid hitting the
> > cable with commands destined for hdb. (For example, by waiting for
> > !channel->busy.)
>
> Current IDE code leaves a request on the list until it has completed
> (this is ignoring TCQ of course), so there's no way that you could start
> serving a second request before the first one completes.

I didn't understand why this was the case, so I've been reading source
code (and sleeping for a few minutes too ;-)).  Wow, is it hard for a
newbie to follow - if I didn't know better I'd swear the kernel was
obfuscated.

I _think_ I now understand what's going on here: you guys already know
this stuff but perhaps others can learn from my slog through the code
(?) (and/or spot the dumb mistakes).  (I did notice what could be a
potential problem too, skip to the end if you want to find it.)

On a "totally" idle system, if a process decides to read from a file,
the sequence appears to be (with minor simplifications for clarity):

sys_read(), file->fops->read(), usually then into generic_file_read(),
page_cache_readahead(), and then the important one:
do_page_cache_readahead().  This does a couple of seriously important
things.  (The name implies it's only used for readahead but the comments
show this isn't the case (if one reads them!)).

Firstly, do_page_cache_readahead() invokes the call chain:
a_ops->readpage(), block_read_full_page(), submit_bh(), submit_bio(),
generic_make_request() (this one finds the right queue for the I/O) and
down into __make_request_fcn(q,bio).  This routine is also key: it adds
requests to the device queue, and a little more: if the queue is empty
it "plugs" the device (before adding the request),  ("plugging" the
device refers to preventing I/O while a request queue is populated), and
also queues a task to the tq_disk task-queue which will unplug the
device at some later time.  The "unplug" routine is central: see below.

Control returns to do_page_cache_readahead() after the call to
readpage().  It then calls run_task_queue(&tq_disk), thus starting the
call chain: generic_unplug_device(),..., q->request_fn().  This is the
end of the line for the block-layer: request_fn() is part of the device
code - for IDE it's do_ide_request().

So this call to request_fn()/do_ide_request() is the first time the IDE
code is really involved in the loop.

The key bit to notice here is that request_fn() is only called if the
device was plugged.  This in turn only happens (well almost, see below)
if the queue was empty.  Thus one concludes that if do_ide_request() is
busy servicing requests and thus the queue is non-empty, the block layer
will never again call do_ide_request(); it will be allowed to get on
with things in its own time.

This I believe, is what you meant Jens when you said "so there's no way
that you could start serving a second request before the first one
completes".  Am I right so far?

Now a possible fly in the ointment: __make_request_fcn() will actually
plug a non-empty queue if BIO_RW_BARRIER is set.  This appears right now
to be impossible because I can't find any code in the entire kernel that
uses bio_barrier() or any similar construct.  But if it's a work in
progress and this bit is set one day, then it seems to me that the block
layer could plug a non-empty queue, and then subsequently any call to
run_task_queue(&tq_disk) would call the block-device's request_fn(). 
This would violate the assumption that request_fn() is never called
twice without the queue emptying in between.  The current IDE code would
survive this because it checks for hwgroup->busy.

Anyway, IFF the BARRIER stuff is not an issue, then I guess the
block-layer really can do all the serialization we need if we set things
up right.  I now probably have to retract my assertions that we vitally
need hwgroup->busy (or equiv) because there really does appear to be no
route into request_fn() from the block layer other than if the queue was
empty...  In the real world though, as you suggested, it's probably
worth having.

all the best,
Neil

PS: Errors and Omissions Expected.

Re: [PATCH] 2.5.15 IDE 61

[Mike Fedyk]

On Tue, May 14, 2002 at 05:47:21PM +0100, Neil Conway wrote:
> Jens Axboe wrote:
> > To really serialize operations the queue _must_ be shared with whoever
> > requires serialiation.
> 
> Why will this help?  The hardware can still be doing DMA on hda while
> the queue's request_fn is called quite legitimately for a hdb request -
> and the IDE code MUST impose the serialization here to avoid hitting the
> cable with commands destined for hdb. (For example, by waiting for
> !channel->busy.)

First of all, why do you think that when a new request is queued it hits the
hardware immediately?  I mean, it is a queue after all...

I'd immagine that you would have one function that is triggered by DMA
completion interrupts that reads the queue and starts the hardware for a
request.

While that function is operating the hardware, it should't care about the
queue, so you can add requests to the queue for both (think hda and hdb)
devices on the channel.

channel = ide cable

There is a hardware limitation for IDE in that it doesn't allow more than
one device to be active on one channel at a time.  So, there is no point in
having seperate queues for each device on the channel since the two queues
would just be waiting on each other, and would probably end up a pretty big
mess.

If a host controller (think ide chipset with multiple channels 2,4,8 etc)
needs serialization between multiple channels, then you can simply use one
queue for each group of channels that need searialization between each other.

Mike

(OT, now that IDE is getting TCQ, will the main difference between SCSI and
IDE be just the number of devices per cable?  I'm not talking electrically
or about the protocol, but mainly performance from say the block layer's
perspective)

Re: [PATCH] 2.5.15 IDE 61

[Jens Axboe]

On Tue, May 14 2002, Martin Dalecki wrote:
> Yes thinking about it longer and longer I tend to the same conclusion,
> that we just shouldn't have per device queue but per channel queues instead.

Right, I see that as the only right way to get the right synchronization
too.

> The only problem here is the fact that some device properties
> are attached to the queue right now. Like for example sector size and 
> friends.

Hmm yes, hardsect_size comes to mind. It will just have to be 'lowest
common denominator' for a while I suppose.

> I didn't have a too deep look in to the generic blk layer. But I would
> rather expect that since the lower layers are allowed to pass
> an spin lock up to the queue intialization, sharing a spin lock
> between two request queues should just serialize them with respect to
> each other. And this is precisely what 63 does.

I think you are mixing up two very different serialization issues. A
shared queue lock will indeed protect however much you want, but only at
the queue level. It will _not_ provide synchronization for hardware
access in any sane way, like a shared queue between two devices will.

You could alternatively move requests to an internal queue of your own
width, that would synchronize drive operations at any level you want
(you set the rules). The block layer can still sanely handle the locking
for you, the scope of that lock will just be a bit wider.

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 61

[Anton Altaparmakov]

At 15:30 14/05/02, Martin Dalecki wrote:
>Uz.ytkownik Alan Cox napisa?:
>>I think you are way off base. If you have a single queue for both hda and
>>hdb then requests will get dumped into that in a way that processing that
>>queue implicitly does the ordering you require.
>> From an abstract hardware point of view each ide controller is a queue not
>>each device. Not following that is I think the cause of much of the existing
>>pain and suffering.
>
>Yes thinking about it longer and longer I tend to the same conclusion,
>that we just shouldn't have per device queue but per channel queues instead.
>The only problem here is the fact that some device properties
>are attached to the queue right now. Like for example sector size and friends.
>
>I didn't have a too deep look in to the generic blk layer. But I would
>rather expect that since the lower layers are allowed to pass
>an spin lock up to the queue intialization, sharing a spin lock
>between two request queues should just serialize them with respect to
>each other. And this is precisely what 63 does.

Hi Martin,

instead of having per channel queue, you could have per device queue, but 
use the same lock for both, i.e. don't make the lock part of "struct queue" 
(or whatever it is called) but instead make the address of the lock be 
attached to "struct queue".

That allows you on "good" controllers to use individual locks for 
individual drives and also allows you to share the same lock (multiple 
"struct queue" point to same lock) among _any_ number of devices on same 
channel.

Further if a controller is truly broken and you need to synchronize 
multiple channels you could share the lock among those.

I know that means allocating a lock, etc, but heck you can make a slab 
cache for spinlocks or semaphores or whatever locking primite you use if 
you consider that important.

I don't know much about the IDE or block layers but it strikes me as the 
simplest approach to control the level of "lock sharing".

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.15 IDE 61

[Jens Axboe]

On Tue, May 14 2002, Anton Altaparmakov wrote:
> At 15:30 14/05/02, Martin Dalecki wrote:
> >Uz.ytkownik Alan Cox napisa?:
> >>I think you are way off base. If you have a single queue for both hda and
> >>hdb then requests will get dumped into that in a way that processing that
> >>queue implicitly does the ordering you require.
> >>From an abstract hardware point of view each ide controller is a queue not
> >>each device. Not following that is I think the cause of much of the 
> >>existing
> >>pain and suffering.
> >
> >Yes thinking about it longer and longer I tend to the same conclusion,
> >that we just shouldn't have per device queue but per channel queues 
> >instead.
> >The only problem here is the fact that some device properties
> >are attached to the queue right now. Like for example sector size and 
> >friends.
> >
> >I didn't have a too deep look in to the generic blk layer. But I would
> >rather expect that since the lower layers are allowed to pass
> >an spin lock up to the queue intialization, sharing a spin lock
> >between two request queues should just serialize them with respect to
> >each other. And this is precisely what 63 does.
> 
> Hi Martin,
> 
> instead of having per channel queue, you could have per device queue, but 
> use the same lock for both, i.e. don't make the lock part of "struct queue" 
> (or whatever it is called) but instead make the address of the lock be 
> attached to "struct queue".

See request_queue_t, the lock can already be shared. And in fact the ide
layer used a global ide_lock shared between all queues until just
recently.

> Further if a controller is truly broken and you need to synchronize 
> multiple channels you could share the lock among those.

Again, this is not enough! The lock will only, at best, serialize direct
queue actions. So I can share a lock between queue A and B and only one
of them will start a request at any given time, but as soon as request X
is started for queue A, then we can happily start request Y for queue B.

This is what the hwgroup busy flag protects right now, only one queue is
allowed to mark the hwgroup busy naturally. So only when request X for
queue A completes will the hwgroup busy flag be cleared, and queue B can
start request Y.

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 61

[Anton Altaparmakov]

At 07:16 15/05/02, Jens Axboe wrote:
>On Tue, May 14 2002, Anton Altaparmakov wrote:
> > instead of having per channel queue, you could have per device queue, but
> > use the same lock for both, i.e. don't make the lock part of "struct 
> queue"
> > (or whatever it is called) but instead make the address of the lock be
> > attached to "struct queue".
>
>See request_queue_t, the lock can already be shared.

/me looks.

So it can. And I thought I had come up with a clever idea... (-;

>And in fact the ide layer used a global ide_lock shared between all queues 
>until just
>recently.
>
> > Further if a controller is truly broken and you need to synchronize
> > multiple channels you could share the lock among those.
>
>Again, this is not enough! The lock will only, at best, serialize direct
>queue actions. So I can share a lock between queue A and B and only one
>of them will start a request at any given time, but as soon as request X
>is started for queue A, then we can happily start request Y for queue B.
>
>This is what the hwgroup busy flag protects right now, only one queue is
>allowed to mark the hwgroup busy naturally. So only when request X for
>queue A completes will the hwgroup busy flag be cleared, and queue B can
>start request Y.

Yes, I understand that, could you not extend the shared lock idea to a 
shared flags idea? The two could even be in the same memory allocated block 
as both the lock and the flags would always be shared by the same users. 
That would just now contain only QUEUE_SHARED_FLAG_BUSY but could be 
extended later if the need arises.

 From what I gather from the posts on this topic, this would be entirely 
sufficient to fully lock both request queue handling and request submission 
to the hardware while maintaining per-device queues.

I may be way off base but I would think that per-device queues are 
desirable to improve the request merging abilities of the elevator. (Again, 
code I haven't looked at so it may well be intelligent enough to 
resort/merge requests with different destinations on the same queue but I 
am sure you will tell me if this is the case. (-;)

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.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Anton Altaparmakov napisa?:
> At 07:16 15/05/02, Jens Axboe wrote:
> 
>> On Tue, May 14 2002, Anton Altaparmakov wrote:
>> > instead of having per channel queue, you could have per device 
>> queue, but
>> > use the same lock for both, i.e. don't make the lock part of "struct 
>> queue"
>> > (or whatever it is called) but instead make the address of the lock be
>> > attached to "struct queue".
>>
>> See request_queue_t, the lock can already be shared.
> 
> 
> /me looks.
> 
> So it can. And I thought I had come up with a clever idea... (-;
> 
>> And in fact the ide layer used a global ide_lock shared between all 
>> queues until just
>> recently.
>>
>> > Further if a controller is truly broken and you need to synchronize
>> > multiple channels you could share the lock among those.
>>
>> Again, this is not enough! The lock will only, at best, serialize direct
>> queue actions. So I can share a lock between queue A and B and only one
>> of them will start a request at any given time, but as soon as request X
>> is started for queue A, then we can happily start request Y for queue B.
>>
>> This is what the hwgroup busy flag protects right now, only one queue is
>> allowed to mark the hwgroup busy naturally. So only when request X for
>> queue A completes will the hwgroup busy flag be cleared, and queue B can
>> start request Y.
> 
> 
> Yes, I understand that, could you not extend the shared lock idea to a 
> shared flags idea? The two could even be in the same memory allocated 
> block as both the lock and the flags would always be shared by the same 
> users. That would just now contain only QUEUE_SHARED_FLAG_BUSY but could 
> be extended later if the need arises.
> 
>  From what I gather from the posts on this topic, this would be entirely 
> sufficient to fully lock both request queue handling and request 
> submission to the hardware while maintaining per-device queues.

The clean solution whould be to make it possible to be able to use
multiple hardsect and friend on a single queue. Becouse then
one could just use the same queue for all operations.

But... wait a minute. The only really problematic case where the queue
properties differ is the case where we have a disk and ATAPI device
mixed on the same channel. Hmm what about using two distinguished queues
on a channel - one for ATAPI and the second for ATA requests then?
In esp. since it's quite easy to identify ATAPI request as
beeing in flight. Hmm... the longer I think about it the more
apeal this solution has to me.

> I may be way off base but I would think that per-device queues are 
> desirable to improve the request merging abilities of the elevator. 
> (Again, code I haven't looked at so it may well be intelligent enough to 
> resort/merge requests with different destinations on the same queue but 
> I am sure you will tell me if this is the case. (-;)
> 
> Best regards,
> 
>         Anton
> 
> 



-- 
- phone: +49 214 8656 283
- job:   eVision-Ventures AG, LEV .de (MY OPINIONS ARE MY OWN!)
- langs: de_DE.ISO8859-1, en_US, pl_PL.ISO8859-2, last ressort: ru_RU.KOI8-R

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Anton Altaparmakov napisa?:
> At 15:30 14/05/02, Martin Dalecki wrote:
> 
>> Uz.ytkownik Alan Cox napisa?:
>>
>>> I think you are way off base. If you have a single queue for both hda 
>>> and
>>> hdb then requests will get dumped into that in a way that processing 
>>> that
>>> queue implicitly does the ordering you require.
>>> From an abstract hardware point of view each ide controller is a 
>>> queue not
>>> each device. Not following that is I think the cause of much of the 
>>> existing
>>> pain and suffering.
>>
>>
>> Yes thinking about it longer and longer I tend to the same conclusion,
>> that we just shouldn't have per device queue but per channel queues 
>> instead.
>> The only problem here is the fact that some device properties
>> are attached to the queue right now. Like for example sector size and 
>> friends.
>>
>> I didn't have a too deep look in to the generic blk layer. But I would
>> rather expect that since the lower layers are allowed to pass
>> an spin lock up to the queue intialization, sharing a spin lock
>> between two request queues should just serialize them with respect to
>> each other. And this is precisely what 63 does.
> 
> 
> Hi Martin,
> 
> instead of having per channel queue, you could have per device queue, 
> but use the same lock for both, i.e. don't make the lock part of "struct 
> queue" (or whatever it is called) but instead make the address of the 
> lock be attached to "struct queue".

In 63 we have:

	blk_init_queue(q, do_ide_request, drive->channel->lock);

struct ata_channel {
	struct device	dev;		/* device handle */
	int		unit;		/* channel number */

	/* This lock is used to serialize requests on the same device queue or
	 * between differen queues sharing the same irq line.
	 */
	spinlock_t *lock;

+ The whole glory of lock sharing for IRQ sharing and serialization.

Pretty much what you describe.

> That allows you on "good" controllers to use individual locks for 
> individual drives and also allows you to share the same lock (multiple 
> "struct queue" point to same lock) among _any_ number of devices on same 
> channel.
> 
> Further if a controller is truly broken and you need to synchronize 
> multiple channels you could share the lock among those.
> 
> I know that means allocating a lock, etc, but heck you can make a slab 
> cache for spinlocks or semaphores or whatever locking primite you use if 
> you consider that important.

Not worth the trouble for the 2 or 4 allocations at initialization.

> I don't know much about the IDE or block layers but it strikes me as the 
> simplest approach to control the level of "lock sharing".

The only problem is that having a shared lock between two queues apparently
doesn't imply that the queues are behaving atomic on the request level
among each others. The lock is just only making sure that access to the
lists and stragety routines is atomic between them.

However 63 doesn't really change the previous behaviour of the driver,
since we are simply using the lock pointer instead of the
custom hwgroup structure as our "serialization token" between channels
and the busy flag is preserved, since it's only used to protect reentrancy
bfore request completion (the long while(test_bit loop). It doesn't have
to be shared between channels, since in every case we hve to
look up the channel anyway.

Apparenty the "sublimation" of the hwgroup and overall cleanup of
data structures, just made many people awake and be aware of problems which
where there already for a very very long time...

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Martin Dalecki wrote:
> The only problem is that having a shared lock between two queues apparently
> doesn't imply that the queues are behaving atomic on the request level
> among each others.

Correct - both queues can be active with I/O in flight at the same
time.  But think about it: if this weren't the case, then the older
kernels (using global io_request_lock) would have had to serialize ALL
I/O, one request-queue active at a time, for every single
block-device...

> Apparenty the "sublimation" of the hwgroup and overall cleanup of
> data structures, just made many people awake and be aware of problems which
> where there already for a very very long time...

I'm not quite sure which problems you mean.  The busy flag prevents any
clash. (But sure, if you change to per-device queues AND you ditch the
busy flag you're screwed.) Where is the problem?

cheers
Neil

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Neil Conway napisa?:
> Martin Dalecki wrote:
> 
>>The only problem is that having a shared lock between two queues apparently
>>doesn't imply that the queues are behaving atomic on the request level
>>among each others.
> 
> 
> Correct - both queues can be active with I/O in flight at the same
> time.  But think about it: if this weren't the case, then the older
> kernels (using global io_request_lock) would have had to serialize ALL
> I/O, one request-queue active at a time, for every single
> block-device...
> 
> 
>>Apparenty the "sublimation" of the hwgroup and overall cleanup of
>>data structures, just made many people awake and be aware of problems which
>>where there already for a very very long time...
> 
> 
> I'm not quite sure which problems you mean.  The busy flag prevents any
> clash. (But sure, if you change to per-device queues AND you ditch the
> busy flag you're screwed.) Where is the problem?

The problem is that with the busy flag on we are wasting quite
a significant amount of CPU time spinning around it for no good...

Right now I'm quite confident about the idea of just having
two queues ata_queue and atapi_queue on the channel possible
shared among two channels. This will make the CPU cycle waste
occur only in the case of channels where ATA and ATAPI devices
are mixed as master and slave. This will work becouse the
only really crucial queue property which has to be device type
specific is the hardsect size.
Quite the same design as in, well, FreeBSD.
As a second step we could do just the following - block one
of the queues if the second one is active right now... This should
be simpler than doing it on the per device level by the busy flag and
wasting CPU time around it.

Re: [PATCH] 2.5.15 IDE 61

[Alan Cox]

> The problem is that with the busy flag on we are wasting quite
> a significant amount of CPU time spinning around it for no good...

Why spin on the busy flag. Instead you just let the person who clears
the flag check the pending work and do it. 

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Alan Cox wrote:
> 
> > The problem is that with the busy flag on we are wasting quite
> > a significant amount of CPU time spinning around it for no good...
> 
> Why spin on the busy flag. Instead you just let the person who clears
> the flag check the pending work and do it.

Isn't that what happens?  Since when are we spinning on busy?  Certainly
not in vanilla 2.5.14, unless I'm much mistaken.

Martin - I haven't read your last couple of patches yet but did you
really change it this drastically?

Neil

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Neil Conway napisa?:
> Alan Cox wrote:
> 
>>>The problem is that with the busy flag on we are wasting quite
>>>a significant amount of CPU time spinning around it for no good...
>>
>>Why spin on the busy flag. Instead you just let the person who clears
>>the flag check the pending work and do it.
> 
> 
> Isn't that what happens?  Since when are we spinning on busy?  Certainly
> not in vanilla 2.5.14, unless I'm much mistaken.
> 
> Martin - I haven't read your last couple of patches yet but did you
> really change it this drastically?

Yeep. Not quite that drastically but basically it's the
upper layer going down the driver and up again.

Anyway. This all will be gone soon, since I have just already
made the distinction between ata_queue and atapi_queue... and
well things turn out nicely at least in the lieu of code.
The synchronization between them can be done by deploying
precisely the same blk_queue_plug and blk_queue_unplug games
already played in tcq.c. And it's very likely that
we will end up unwinding the REQ_DRIVE_CMD and REQ_DRIVE_ACB
cases by just providing two different request handlers...
And thus finally unwinding ATA ver. ATAPI handling entierly,
which should indeed simplyfy things quite a lot...

Re: [PATCH] 2.5.15 IDE 61

[benh]

>> The problem is that with the busy flag on we are wasting quite
>> a significant amount of CPU time spinning around it for no good...
>
>Why spin on the busy flag. Instead you just let the person who clears
>the flag check the pending work and do it. 

Which is what happened in most cases, pending work could be resumed by
calling ide_do_request() (in the previous codebase).

I used/needed this feature when implementing machine sleep support on
PowerMac laptops. I basically got the lock, set busy flag on all interfaces
then release the lock (well, I did the proper wait for interface not to
be busy etc... but you get the point).

That way, I am sure that newly incoming requests would be queued and not
sent to HW while the controller is going to sleep (and then the entire
machine).

On wakeup, I do the opposite. After reviving the controller and the disk,
I clear the busy flag and call ide_do_request() to get things back.

I need to be able to do something similar with the new codebase, though
I beleive that should be part of the generic code for power management
when the controller gets a suspend request. In theory, it should issue
a device-specific suspend command (that is SLEEP for IDE disks, etc...)
and make sure the busy flag doesn't get cleared upon termination on this
command (thus blocking queues) until the machine gets woken up.

Ben.

Re: [PATCH] 2.5.15 IDE 61

[Denis Vlasenko]

On 15 May 2002 07:32, Martin Dalecki wrote:
> >> Yes thinking about it longer and longer I tend to the same conclusion,
> >> that we just shouldn't have per device queue but per channel queues
> >> instead.

IMHO logically request queue is a separate entity for each disk.
IDE can have 2 devices on one cable (or more: think about
buggy cmd640 like chipset as a weird IDE with four disks
on a channel since we can talk to one disk only at a time).

This fact is a hardware limitation which can be hidden,
IDE code should arbitrare access to IDE channel.
But why mix up queues?

> >> The only problem here is the fact that some device properties
> >> are attached to the queue right now. Like for example sector size and
> >> friends.

This is naturally happening with one queue per disk.

> > Hi Martin,
> > instead of having per channel queue, you could have per device queue,
> > but use the same lock for both, i.e. don't make the lock part of "struct
> > queue" (or whatever it is called) but instead make the address of the
> > lock be attached to "struct queue".

> In 63 we have:
> 	blk_init_queue(q, do_ide_request, drive->channel->lock);
> struct ata_channel {
> 	struct device	dev;		/* device handle */
> 	int		unit;		/* channel number */
>
> 	/* This lock is used to serialize requests on the same device queue or
> 	 * between differen queues sharing the same irq line.
> 	 */
> 	spinlock_t *lock;
>
> + The whole glory of lock sharing for IRQ sharing and serialization.

It is a _spin_ lock.
Does this mean you will spin on it while IDE request for other disk
is processed?

Somebody enlighten me: can IDE mix reqests and completion like this:

host ----read reqest---> master
host ----read reqest---> slave  (is this possible?)
host <---interrupt------ master
host ----okay,send it--> master
host <---data----------- master
host <---data----------- master (do slave has to wait until end here?)
host <---data----------- master
host <---that's all----- master
host <---interrupt------ slave
	(can host issue another read for master here?)
host ----okay,send it--> slave
host <---data----------- slave

Please comment what is allowed and what is not.
I suspect we are in IDE chipset bugs land here.

Does current IDE code (as Martin sees it) utilize this fully but
does not push IDE over limit?

Hm, seems like IDE code needs to remember set of ATA capabilities
(i.e. what should be serialized and what can be safely intermixed
in time) for each controller in order to run full speed on good chips
yet don't croak on buggy ones.
--
vda

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Denis Vlasenko wrote:
> 
> On 15 May 2002 07:32, Martin Dalecki wrote:
> > >> Yes thinking about it longer and longer I tend to the same conclusion,
> > >> that we just shouldn't have per device queue but per channel queues
> > >> instead.
> 
> IMHO logically request queue is a separate entity for each disk.
> IDE can have 2 devices on one cable (or more: think about
> buggy cmd640 like chipset as a weird IDE with four disks
> on a channel since we can talk to one disk only at a time).

Actually, since Martin pointed out the hardsect stuff, I'm inclined to
think per-device queues but with a per-channel busy flag are the best
idea.  Or perhaps I should call it a per-channel "Don't touch the cable"
flag.  See below.

> It is a _spin_ lock.
> Does this mean you will spin on it while IDE request for other disk
> is processed?

No.  An SMP machine will spin on it only while the queue-handling code
is active.  During the pause while the disk fetches the sectors from the
media, no problem.

> Somebody enlighten me: can IDE mix reqests and completion like this:

I'll try (!)  Treat my words with caution ;-)

> host ----read reqest---> master
> host ----read reqest---> slave  (is this possible?)

No, in general.  Not unless you're using tagged command queueing.  This
is because the master won't have performed a bus-release on any normal
R/W command.  Once you are using TCQ it's OK though (tricky but OK).

You can (and must) safely "touch the cable" in between TCQ commands in
the right circumstances.  You are therefore touching the cable while the
hwgroup is busy, hence my suggestion that the flag we use to prevent
touching the cable during DMA should be named something other than busy.

Neil

Re: [PATCH] 2.5.15 IDE 61

[Mike Fedyk]

On Wed, May 15, 2002 at 12:55:37PM +0100, Neil Conway wrote:
> You can (and must) safely "touch the cable" in between TCQ commands in
> the right circumstances.  You are therefore touching the cable while the
> hwgroup is busy, hence my suggestion that the flag we use to prevent
> touching the cable during DMA should be named something other than busy.
>

Ahh, but with TCQ the concept of busy changes.  The wire (simplified) is
only busy when the tags are being transfered, otherwise the cable is unused
unless the cable has been "locked" by one of the devices.

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Mike Fedyk wrote:
> On Wed, May 15, 2002 at 12:55:37PM +0100, Neil Conway wrote:
> > You can (and must) safely "touch the cable" in between TCQ commands in
> > the right circumstances.  You are therefore touching the cable while the
> > hwgroup is busy, hence my suggestion that the flag we use to prevent
> > touching the cable during DMA should be named something other than busy.
> Ahh, but with TCQ the concept of busy changes.  The wire (simplified) is
> only busy when the tags are being transfered, otherwise the cable is unused
> unless the cable has been "locked" by one of the devices.

Hmm: "locked by one of the devices": do you mean a DMA transfer for
example?  These are initiated by the host, but proceed asynchronously,
so I'm not sure I'd describe it as being locked "by the device" as
such.  At any rate, the IDE code has to remember that the cable is
asynchronously active until DMA ends...  (Or I suppose it could just
check the hwif BMDMA bits for the active state.)

I don't think you're actually disagreeing with me here btw; if you are
then I've obviously missed your point ;-))

Neil

Re: [PATCH] 2.5.15 IDE 61

[Martin Dalecki]

Uz.ytkownik Neil Conway napisa?:
> Mike Fedyk wrote:
> 
>>On Wed, May 15, 2002 at 12:55:37PM +0100, Neil Conway wrote:
>>
>>>You can (and must) safely "touch the cable" in between TCQ commands in
>>>the right circumstances.  You are therefore touching the cable while the
>>>hwgroup is busy, hence my suggestion that the flag we use to prevent
>>>touching the cable during DMA should be named something other than busy.
>>
>>Ahh, but with TCQ the concept of busy changes.  The wire (simplified) is
>>only busy when the tags are being transfered, otherwise the cable is unused
>>unless the cable has been "locked" by one of the devices.
> 
> 
> Hmm: "locked by one of the devices": do you mean a DMA transfer for
> example?  These are initiated by the host, but proceed asynchronously,
> so I'm not sure I'd describe it as being locked "by the device" as
> such.  At any rate, the IDE code has to remember that the cable is
> asynchronously active until DMA ends...  (Or I suppose it could just
> check the hwif BMDMA bits for the active state.)

Grep for IDE_DMA busy bits to see it - it does precisely this.

Re: [PATCH] 2.5.15 IDE 61

[Neil Conway]

Alan Cox wrote:
> 
> > I think you're wrong Alan.  Take a good IDE chipset as an example: both
> > channels can be active at the same time, but you still can't talk to one
> > drive while the other drive on the same channel is DMAing.
> 
> Sure.

Phew, at least we agree on something.  In fact I _think_ we were maybe
talking at cross-purposes and not disagreeing about anything that
actually mattered :-)

> > It doesn't matter how you perform the queue abstraction in this case:
> > the fact that the device+channel+cable is busy in an asynchronous manner
> > makes it impossible for the block layer to deal with this.  [[Or am I
> > way off base?!]]
> 
> I think you are way off base. If you have a single queue for both hda and
> hdb then requests will get dumped into that in a way that processing that
> queue implicitly does the ordering you require.

Hmm, OK. So the queue will potentially fill up with requests for both
hda and hdb.  The request_fn for this queue will get called, sometimes
by the block layer, sometimes not, and can handle all/some/none of the
requests at each call.  If it decides to handle (say) one of the
requests by initiating DMA and then returns while bytes are
transferring, then let's think what happens next time it gets called... 
If it's called as a result of the DMA-complete interrupt, then no
worries because the DMAing is finished.  If it's called instead by the
block-layer because another request has been added, it needs to know
that it shouldn't do anything until the DMAing finishes.  It could find
that out by looking at a channel->busy flag.  If it doesn't use a busy
flag, then what provides the locking?

I'm slowly growing more sure that I've missed the point.  But please
point to the mistake in the above logic to help me learn...

> From an abstract hardware point of view each ide controller is a queue not
> each device. Not following that is I think the cause of much of the existing
> pain and suffering.

Agreed.  And in the case of a cmd640 (etc.), the queue should handle
both channels.

cheers
Neil

Re: [PATCH] 2.5.15 IDE 61

[Alan Cox]

> block-layer because another request has been added, it needs to know
> that it shouldn't do anything until the DMAing finishes.  It could find
> that out by looking at a channel->busy flag.  If it doesn't use a busy
> flag, then what provides the locking?

You can either use device status information or the queue lock. It might
well be using channel->busy or queue->channel->busy type flags. However
you've now got a single queue and a single channel lock flowing through
a single function - which seems cleaner to me than splitting stuff into
multiple queues, locking them against one another and the like

> > From an abstract hardware point of view each ide controller is a queue not
> > each device. Not following that is I think the cause of much of the existing
> > pain and suffering.
> 
> Agreed.  And in the case of a cmd640 (etc.), the queue should handle
> both channels.

Yep

Re: [PATCH] 2.5.15 IDE 61

[Daniela Engert]

On Tue, 14 May 2002 11:30:58 +0200, Martin Dalecki wrote:

>There is no problem to go in paralell on different channels for
>requests. The serialization has only to be done
>for the drive setup.

This assumption is *not* valid in all cases!

For example, get dtips10.pdf from the CMD website and learn how the
CMD646 may fail on concurrent requests on both channels.

There are other chips which need runtime serialization.

Ciao,
  Dani

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Daniela Engert, systems engineer at MEDAV GmbH
Gräfenberger Str. 34, 91080 Uttenreuth, Germany
Phone ++49-9131-583-348, Fax ++49-9131-583-11

Linux-2.5.14..

[Linus Torvalds]

There's a lot of stuff that has happened in the 2.5.x series lately, and
you can see the gory details in the ChangeLog files that accompany
releases these days, but I thought I'd point out 2.5.14, since it has some
interesting fundamental changes to how dirty state is maintained in the
VM.

(The big changes were actually in 2.5.12, but 2.5.13 contained various
minor fixes and tweaks, and 2.5.14 contains a number of fixes especially
wrt truncate, so hopefully it's fairly _stable_ as of 2.5.14.)

Credit goes to Andrew Morton, and not only does it clean up the code a
lot, it also seems to perform a lot better in many circumstances.

There's a lot of other stuff in the 2.5.x tree too, but few things are so
fundamental. Please test (but also, please be careful - backups are always
a good idea).

		Linus

[PATCH] 2.5.15 IDE 61

[Martin Dalecki]

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

Sat May 11 18:45:08 CEST 2002 ide-clean-61

- Fix typo in pdc202xx driver.

- Fix locking order in ioctl.

- Fix wrong time_after usage introduced in 60. Maybe the fact I always get is
   wrong is related to the fact that I'm using the mouse with the left hand!?

- Apply arch-clean-2 by Bartlomiej Zolnierkiewicz.

- Don't disable interrupts during ide_wait_stat(). I see no reason too.

- Push flags down from hwgroup to the ata_chaannel structure.

- Apply small fixes from Franz Sirl to make AEC6280 working properly again.


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

diff -urN linux-2.5.15/drivers/ide/ide.c linux/drivers/ide/ide.c
--- linux-2.5.15/drivers/ide/ide.c	2002-05-13 12:44:17.000000000 +0200
+++ linux/drivers/ide/ide.c	2002-05-13 02:13:30.000000000 +0200
@@ -317,7 +317,7 @@
 			blkdev_dequeue_request(rq);
 		else
 			blk_queue_end_tag(&drive->queue, rq);
-		HWGROUP(drive)->rq = NULL;
+		drive->rq = NULL;
 		end_that_request_last(rq);
 		ret = 0;
 	}
@@ -635,7 +635,7 @@
 	}
 
 	blkdev_dequeue_request(rq);
-	HWGROUP(drive)->rq = NULL;
+	drive->rq = NULL;
 	end_that_request_last(rq);
 }
 
@@ -886,7 +886,6 @@
 {
 	u8 stat;
 	int i;
-	unsigned long flags;
 
 	/* bail early if we've exceeded max_failures */
 	if (drive->max_failures && (drive->failures > drive->max_failures)) {
@@ -896,24 +895,20 @@
 
 	udelay(1);	/* spec allows drive 400ns to assert "BUSY" */
 	if ((stat = GET_STAT()) & BUSY_STAT) {
-		__save_flags(flags);	/* local CPU only */
-		ide__sti();		/* local CPU only */
 		timeout += jiffies;
 		while ((stat = GET_STAT()) & BUSY_STAT) {
-			if (time_after(timeout, jiffies)) {
-				__restore_flags(flags);	/* local CPU only */
+			if (time_after(jiffies, timeout)) {
 				*startstop = ide_error(drive, rq, "status timeout", stat);
 				return 1;
 			}
 		}
-		__restore_flags(flags);	/* local CPU only */
 	}
+
 	/*
-	 * Allow status to settle, then read it again.
-	 * A few rare drives vastly violate the 400ns spec here,
-	 * so we'll wait up to 10usec for a "good" status
-	 * rather than expensively fail things immediately.
-	 * This fix courtesy of Matthew Faupel & Niccolo Rigacci.
+	 * Allow status to settle, then read it again.  A few rare drives
+	 * vastly violate the 400ns spec here, so we'll wait up to 10usec for a
+	 * "good" status rather than expensively fail things immediately.  This
+	 * fix courtesy of Matthew Faupel & Niccolo Rigacci.
 	 */
 	for (i = 0; i < 10; i++) {
 		udelay(1);
@@ -1074,15 +1069,13 @@
 	struct ata_channel *ch = drive->channel;
 	ide_hwgroup_t *hwgroup = ch->hwgroup;
 	unsigned long flags;
-	struct request *rq;
 
 	spin_lock_irqsave(&ide_lock, flags);
 	hwgroup->handler = NULL;
 	del_timer(&ch->timer);
-	rq = hwgroup->rq;
 	spin_unlock_irqrestore(&ide_lock, flags);
 
-	return start_request(drive, rq);
+	return start_request(drive, drive->rq);
 }
 
 /*
@@ -1180,7 +1173,6 @@
 	if (choice)
 		return choice;
 
-	channel->hwgroup->rq = NULL;
 	sleep = longest_sleep(channel);
 
 	if (sleep) {
@@ -1197,14 +1189,14 @@
 		if (timer_pending(&channel->timer))
 			printk(KERN_ERR "ide_set_handler: timer already active\n");
 #endif
-		set_bit(IDE_SLEEP, &channel->hwgroup->flags);
+		set_bit(IDE_SLEEP, &channel->active);
 		mod_timer(&channel->timer, sleep);
 		/* we purposely leave hwgroup busy while sleeping */
 	} else {
 		/* Ugly, but how can we sleep for the lock otherwise? perhaps
 		 * from tq_disk? */
 		ide_release_lock(&irq_lock);/* for atari only */
-		clear_bit(IDE_BUSY, &channel->hwgroup->flags);
+		clear_bit(IDE_BUSY, &channel->active);
 	}
 
 	return NULL;
@@ -1217,13 +1209,13 @@
  */
 static void queue_commands(struct ata_device *drive, int masked_irq)
 {
-	ide_hwgroup_t *hwgroup = drive->channel->hwgroup;
+	struct ata_channel *ch = drive->channel;
 	ide_startstop_t startstop = -1;
 
 	for (;;) {
 		struct request *rq = NULL;
 
-		if (!test_bit(IDE_BUSY, &hwgroup->flags))
+		if (!test_bit(IDE_BUSY, &ch->active))
 			printk(KERN_ERR"%s: error: not busy while queueing!\n", drive->name);
 
 		/* Abort early if we can't queue another command. for non
@@ -1232,13 +1224,13 @@
 		 */
 		if (!ata_can_queue(drive)) {
 			if (!ata_pending_commands(drive))
-				clear_bit(IDE_BUSY, &hwgroup->flags);
+				clear_bit(IDE_BUSY, &ch->active);
 			break;
 		}
 
 		drive->sleep = 0;
 
-		if (test_bit(IDE_DMA, &hwgroup->flags)) {
+		if (test_bit(IDE_DMA, &ch->active)) {
 			printk("ide_do_request: DMA in progress...\n");
 			break;
 		}
@@ -1256,8 +1248,8 @@
 
 		if (!(rq = elv_next_request(&drive->queue))) {
 			if (!ata_pending_commands(drive))
-				clear_bit(IDE_BUSY, &hwgroup->flags);
-			hwgroup->rq = NULL;
+				clear_bit(IDE_BUSY, &ch->active);
+			drive->rq = NULL;
 			break;
 		}
 
@@ -1268,7 +1260,7 @@
 		if (!(rq->flags & REQ_CMD) && ata_pending_commands(drive))
 			break;
 
-		hwgroup->rq = rq;
+		drive->rq = rq;
 
 		/* Some systems have trouble with IDE IRQs arriving while the
 		 * driver is still setting things up.  So, here we disable the
@@ -1339,12 +1331,10 @@
  */
 static void ide_do_request(struct ata_channel *channel, int masked_irq)
 {
-	ide_hwgroup_t *hwgroup = channel->hwgroup;
-
 	ide_get_lock(&irq_lock, ata_irq_request, hwgroup);/* for atari only: POSSIBLY BROKEN HERE(?) */
 	__cli();	/* necessary paranoia: ensure IRQs are masked on local CPU */
 
-	while (!test_and_set_bit(IDE_BUSY, &hwgroup->flags)) {
+	while (!test_and_set_bit(IDE_BUSY, &channel->active)) {
 		struct ata_channel *ch;
 		struct ata_device *drive;
 
@@ -1405,7 +1395,7 @@
 	 * un-busy drive etc (hwgroup->busy is cleared on return) and
 	 * make sure request is sane
 	 */
-	HWGROUP(drive)->rq = NULL;
+	drive->rq = NULL;
 
 	rq->errors = 0;
 	if (rq->bio) {
@@ -1446,8 +1436,8 @@
 		 * complain about anything.
 		 */
 
-		if (test_and_clear_bit(IDE_SLEEP, &hwgroup->flags))
-			clear_bit(IDE_BUSY, &hwgroup->flags);
+		if (test_and_clear_bit(IDE_SLEEP, &ch->active))
+			clear_bit(IDE_BUSY, &ch->active);
 	} else {
 		struct ata_device *drive = ch->drive;
 		if (!drive) {
@@ -1457,11 +1447,11 @@
 			ide_startstop_t startstop;
 
 			/* paranoia */
-			if (!test_and_set_bit(IDE_BUSY, &hwgroup->flags))
+			if (!test_and_set_bit(IDE_BUSY, &ch->active))
 				printk("%s: ide_timer_expiry: hwgroup was not busy??\n", drive->name);
 			if ((expiry = ch->expiry) != NULL) {
 				/* continue */
-				if ((wait = expiry(drive, HWGROUP(drive)->rq)) != 0) {
+				if ((wait = expiry(drive, drive->rq)) != 0) {
 					/* reengage timer */
 					ch->timer.expires  = jiffies + wait;
 					add_timer(&ch->timer);
@@ -1484,25 +1474,25 @@
 #endif
 			__cli();	/* local CPU only, as if we were handling an interrupt */
 			if (ch->poll_timeout != 0) {
-				startstop = handler(drive, ch->hwgroup->rq);
+				startstop = handler(drive, drive->rq);
 			} else if (drive_is_ready(drive)) {
 				if (drive->waiting_for_dma)
 					udma_irq_lost(drive);
 				(void) ide_ack_intr(ch);
 				printk("%s: lost interrupt\n", drive->name);
-				startstop = handler(drive, ch->hwgroup->rq);
+				startstop = handler(drive, drive->rq);
 			} else {
 				if (drive->waiting_for_dma) {
 					startstop = ide_stopped;
-					dma_timeout_retry(drive, ch->hwgroup->rq);
+					dma_timeout_retry(drive, drive->rq);
 				} else
-					startstop = ide_error(drive, ch->hwgroup->rq, "irq timeout", GET_STAT());
+					startstop = ide_error(drive, drive->rq, "irq timeout", GET_STAT());
 			}
 			set_recovery_timer(ch);
 			enable_irq(ch->irq);
 			spin_lock_irq(&ide_lock);
 			if (startstop == ide_stopped)
-				clear_bit(IDE_BUSY, &hwgroup->flags);
+				clear_bit(IDE_BUSY, &ch->active);
 		}
 	}
 
@@ -1627,7 +1617,7 @@
 		goto out_lock;
 	}
 	/* paranoia */
-	if (!test_and_set_bit(IDE_BUSY, &hwgroup->flags))
+	if (!test_and_set_bit(IDE_BUSY, &ch->active))
 		printk(KERN_ERR "%s: %s: hwgroup was not busy!?\n", drive->name, __FUNCTION__);
 	hwgroup->handler = NULL;
 	del_timer(&ch->timer);
@@ -1637,7 +1627,7 @@
 		ide__sti();	/* local CPU only */
 
 	/* service this interrupt, may set handler for next interrupt */
-	startstop = handler(drive, hwgroup->rq);
+	startstop = handler(drive, drive->rq);
 	spin_lock_irq(&ide_lock);
 
 	/*
@@ -1650,7 +1640,7 @@
 	set_recovery_timer(drive->channel);
 	if (startstop == ide_stopped) {
 		if (hwgroup->handler == NULL) {	/* paranoia */
-			clear_bit(IDE_BUSY, &hwgroup->flags);
+			clear_bit(IDE_BUSY, &ch->active);
 			ide_do_request(ch, ch->irq);
 		} else {
 			printk("%s: %s: huh? expected NULL handler on exit\n", drive->name, __FUNCTION__);
@@ -1738,7 +1728,7 @@
 	spin_lock_irqsave(&ide_lock, flags);
 	if (blk_queue_empty(&drive->queue) || action == ide_preempt) {
 		if (action == ide_preempt)
-			HWGROUP(drive)->rq = NULL;
+			drive->rq = NULL;
 	} else {
 		if (action == ide_wait || action == ide_end)
 			queue_head = queue_head->prev;
@@ -2222,8 +2212,6 @@
 
 int ide_spin_wait_hwgroup(struct ata_device *drive)
 {
-	ide_hwgroup_t *hwgroup = HWGROUP(drive);
-
 	/* FIXME: Wait on a proper timer. Instead of playing games on the
 	 * spin_lock().
 	 */
@@ -2232,7 +2220,7 @@
 
 	spin_lock_irq(&ide_lock);
 
-	while (test_bit(IDE_BUSY, &hwgroup->flags)) {
+	while (test_bit(IDE_BUSY, &drive->channel->active)) {
 		spin_unlock_irq(&ide_lock);
 		if (time_after(jiffies, timeout)) {
 			printk("%s: channel busy\n", drive->name);
@@ -2316,7 +2304,9 @@
 	kdev_t dev;
 
 	dev = inode->i_rdev;
-	major = major(dev); minor = minor(dev);
+	major = major(dev);
+	minor = minor(dev);
+
 	if ((drive = get_info_ptr(inode->i_rdev)) == NULL)
 		return -ENODEV;
 
@@ -2376,6 +2366,7 @@
 
 			if (put_user(val, (unsigned long *) arg))
 				return -EFAULT;
+
 			return 0;
 		}
 
@@ -2384,12 +2375,12 @@
 			if (arg < 0 || arg > 1)
 				return -EINVAL;
 
-			if (ide_spin_wait_hwgroup(drive))
-				return -EBUSY;
-
 			if (drive->channel->no_unmask)
 				return -EIO;
 
+			if (ide_spin_wait_hwgroup(drive))
+				return -EBUSY;
+
 			drive->channel->unmask = arg;
 			spin_unlock_irq(&ide_lock);
 
@@ -2426,11 +2417,20 @@
 
 			if (!loc || (drive->type != ATA_DISK && drive->type != ATA_FLOPPY))
 				return -EINVAL;
-			if (put_user(drive->bios_head, (byte *) &loc->heads)) return -EFAULT;
-			if (put_user(drive->bios_sect, (byte *) &loc->sectors)) return -EFAULT;
-			if (put_user(bios_cyl, (unsigned short *) &loc->cylinders)) return -EFAULT;
+
+			if (put_user(drive->bios_head, (byte *) &loc->heads))
+				return -EFAULT;
+
+			if (put_user(drive->bios_sect, (byte *) &loc->sectors))
+				return -EFAULT;
+
+			if (put_user(bios_cyl, (unsigned short *) &loc->cylinders))
+				return -EFAULT;
+
 			if (put_user((unsigned)drive->part[minor(inode->i_rdev)&PARTN_MASK].start_sect,
-				(unsigned long *) &loc->start)) return -EFAULT;
+				(unsigned long *) &loc->start))
+				return -EFAULT;
+
 			return 0;
 		}
 
@@ -2440,48 +2440,59 @@
 			if (!loc || (drive->type != ATA_DISK && drive->type != ATA_FLOPPY))
 				return -EINVAL;
 
-			if (put_user(drive->head, (u8 *) &loc->heads)) return -EFAULT;
-			if (put_user(drive->sect, (u8 *) &loc->sectors)) return -EFAULT;
-			if (put_user(drive->cyl, (unsigned int *) &loc->cylinders)) return -EFAULT;
+			if (put_user(drive->head, (u8 *) &loc->heads))
+				return -EFAULT;
+
+			if (put_user(drive->sect, (u8 *) &loc->sectors))
+				return -EFAULT;
+
+			if (put_user(drive->cyl, (unsigned int *) &loc->cylinders))
+				return -EFAULT;
+
 			if (put_user((unsigned)drive->part[minor(inode->i_rdev)&PARTN_MASK].start_sect,
-				(unsigned long *) &loc->start)) return -EFAULT;
+				(unsigned long *) &loc->start))
+				return -EFAULT;
+
 			return 0;
 		}
 
 		case BLKRRPART: /* Re-read partition tables */
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
 			return ide_revalidate_disk(inode->i_rdev);
 
 		case HDIO_GET_IDENTITY:
 			if (minor(inode->i_rdev) & PARTN_MASK)
 				return -EINVAL;
+
 			if (drive->id == NULL)
 				return -ENOMSG;
+
 			if (copy_to_user((char *)arg, (char *)drive->id, sizeof(*drive->id)))
 				return -EFAULT;
+
 			return 0;
 
 		case HDIO_GET_NICE:
-			return put_user(drive->dsc_overlap	<<	IDE_NICE_DSC_OVERLAP	|
-					drive->atapi_overlap	<<	IDE_NICE_ATAPI_OVERLAP,
+			return put_user(drive->dsc_overlap << IDE_NICE_DSC_OVERLAP |
+					drive->atapi_overlap << IDE_NICE_ATAPI_OVERLAP,
 					(long *) arg);
 
 		case HDIO_DRIVE_CMD:
-			if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
+			if (!capable(CAP_SYS_RAWIO))
 				return -EACCES;
+
 			return ide_cmd_ioctl(drive, arg);
 
 		case HDIO_SET_NICE:
-			if (!capable(CAP_SYS_ADMIN)) return -EACCES;
 			if (arg != (arg & ((1 << IDE_NICE_DSC_OVERLAP))))
 				return -EPERM;
+
 			drive->dsc_overlap = (arg >> IDE_NICE_DSC_OVERLAP) & 1;
 			/* Only CD-ROM's and tapes support DSC overlap. */
 			if (drive->dsc_overlap && !(drive->type == ATA_ROM || drive->type == ATA_TAPE)) {
 				drive->dsc_overlap = 0;
 				return -EPERM;
 			}
+
 			return 0;
 
 		case BLKGETSIZE:
@@ -2505,25 +2516,24 @@
 			return block_ioctl(inode->i_bdev, cmd, arg);
 
 		case HDIO_GET_BUSSTATE:
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
 			if (put_user(drive->channel->bus_state, (long *)arg))
 				return -EFAULT;
+
 			return 0;
 
 		case HDIO_SET_BUSSTATE:
-			if (!capable(CAP_SYS_ADMIN))
-				return -EACCES;
 			if (drive->channel->busproc)
 				drive->channel->busproc(drive, (int)arg);
+
 			return 0;
 
-		/* Now check whatever this particular ioctl has a special
-		 * implementation.
+		/* Now check whatever this particular ioctl has a device type
+		 * specific implementation.
 		 */
 		default:
 			if (ata_ops(drive) && ata_ops(drive)->ioctl)
 				return ata_ops(drive)->ioctl(drive, inode, file, cmd, arg);
+
 			return -EINVAL;
 	}
 }
@@ -2545,6 +2555,7 @@
 			res = 1; /* assume it was changed */
 		ata_put(ata_ops(drive));
 	}
+
 	return res;
 }
 
diff -urN linux-2.5.15/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
--- linux-2.5.15/drivers/ide/ide-disk.c	2002-05-13 12:44:17.000000000 +0200
+++ linux/drivers/ide/ide-disk.c	2002-05-12 16:12:30.000000000 +0200
@@ -1058,6 +1058,7 @@
 
 			if (put_user(val, (unsigned long *) arg))
 				return -EFAULT;
+
 			return 0;
 		}
 
@@ -1081,6 +1082,7 @@
 
 			if (put_user(val, (unsigned long *) arg))
 				return -EFAULT;
+
 			return 0;
 		}
 
@@ -1100,7 +1102,7 @@
 		}
 
 		case HDIO_GET_ACOUSTIC: {
-			u8 val = drive->acoustic;
+			unsigned long val = drive->acoustic;
 
 			if (put_user(val, (u8 *) arg))
 				return -EFAULT;
@@ -1128,6 +1130,7 @@
 
 			if (put_user(val, (u8 *) arg))
 				return -EFAULT;
+
 			return 0;
 		}
 
@@ -1153,7 +1156,7 @@
 
 
 /*
- *      IDE subdriver functions, registered with ide.c
+ * Subdriver functions.
  */
 static struct ata_operations idedisk_driver = {
 	owner:			THIS_MODULE,
@@ -1178,11 +1181,9 @@
 
 	while ((drive = ide_scan_devices(ATA_DISK, "ide-disk", &idedisk_driver, failed)) != NULL) {
 		if (idedisk_cleanup (drive)) {
-			printk (KERN_ERR "%s: cleanup_module() called while still busy\n", drive->name);
-			failed++;
+			printk(KERN_ERR "%s: cleanup_module() called while still busy\n", drive->name);
+			++failed;
 		}
-		/* We must remove proc entries defined in this module.
-		   Otherwise we oops while accessing these entries */
 	}
 }
 
@@ -1203,10 +1204,11 @@
 			idedisk_cleanup(drive);
 			continue;
 		}
-		failed--;
+		--failed;
 	}
 	revalidate_drives();
 	MOD_DEC_USE_COUNT;
+
 	return 0;
 }
 
diff -urN linux-2.5.15/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
--- linux-2.5.15/drivers/ide/ide-dma.c	2002-05-13 12:44:17.000000000 +0200
+++ linux/drivers/ide/ide-dma.c	2002-05-11 23:23:14.000000000 +0200
@@ -382,10 +382,10 @@
 
 #ifdef DEBUG
 	printk("%s: dma_timer_expiry: dma status == 0x%02x\n", drive->name, dma_stat);
-#endif /* DEBUG */
+#endif
 
 #if 0
-	HWGROUP(drive)->expiry = NULL;	/* one free ride for now */
+	drive->expiry = NULL;	/* one free ride for now */
 #endif
 
 	if (dma_stat & 2) {	/* ERROR */
diff -urN linux-2.5.15/drivers/ide/ide-pci.c linux/drivers/ide/ide-pci.c
--- linux-2.5.15/drivers/ide/ide-pci.c	2002-05-10 00:25:29.000000000 +0200
+++ linux/drivers/ide/ide-pci.c	2002-05-13 12:37:07.000000000 +0200
@@ -122,7 +122,7 @@
 	 * Unless there is a bootable card that does not use the standard
 	 * ports 1f0/170 (the ide0/ide1 defaults). The (bootable) flag.
 	 */
-	if (bootable) {
+	if (bootable == ON_BOARD) {
 		for (h = 0; h < MAX_HWIFS; ++h) {
 			hwif = &ide_hwifs[h];
 			if (hwif->chipset == ide_unknown)
@@ -703,7 +703,7 @@
 			hpt374_device_order_fixup(dev, d);
 	} else if (d->vendor == PCI_VENDOR_ID_PROMISE && d->device == PCI_DEVICE_ID_PROMISE_20268R)
 		pdc20270_device_order_fixup(dev, d);
-	else if ((dev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
+	else {
 		printk(KERN_INFO "ATA: %s (%04x:%04x) on PCI slot %s\n",
 				dev->name, vendor, device, dev->slot_name);
 		setup_pci_device(dev, d);
diff -urN linux-2.5.15/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c
--- linux-2.5.15/drivers/ide/ide-taskfile.c	2002-05-13 12:44:17.000000000 +0200
+++ linux/drivers/ide/ide-taskfile.c	2002-05-12 17:47:58.000000000 +0200
@@ -279,6 +279,7 @@
 
 	if (stat & BUSY_STAT)
 		return 0;	/* drive busy:  definitely not interrupting */
+
 	return 1;		/* drive ready: *might* be interrupting */
 }
 
diff -urN linux-2.5.15/drivers/ide/pdc202xx.c linux/drivers/ide/pdc202xx.c
--- linux-2.5.15/drivers/ide/pdc202xx.c	2002-05-10 00:25:39.000000000 +0200
+++ linux/drivers/ide/pdc202xx.c	2002-05-12 16:16:37.000000000 +0200
@@ -1450,7 +1450,7 @@
 		init_chipset: pdc202xx_init_chipset,
 		ata66_check: ata66_pdc202xx,
 		init_channel: ide_init_pdc202xx,
-		exnablebits: {{0x50,0x02,0x02}, {0x50,0x04,0x04}},
+		enablebits: {{0x50,0x02,0x02}, {0x50,0x04,0x04}},
 		bootable: OFF_BOARD,
 		extra: 48,
 		flags: ATA_F_IRQ  | ATA_F_DMA
diff -urN linux-2.5.15/drivers/ide/sl82c105.c linux/drivers/ide/sl82c105.c
--- linux-2.5.15/drivers/ide/sl82c105.c	2002-05-10 00:23:12.000000000 +0200
+++ linux/drivers/ide/sl82c105.c	2002-05-12 03:35:22.000000000 +0200
@@ -76,7 +76,7 @@
 	if (ide_config_drive_speed(drive, xfer_mode) == 0)
 		drv_ctrl = get_timing_sl82c105(t);
 
-	if (drive->using_dma == 0) {
+	if (!drive->using_dma) {
 		/*
 		 * If we are actually using MW DMA, then we can not
 		 * reprogram the interface drive control register.
diff -urN linux-2.5.15/drivers/ide/tcq.c linux/drivers/ide/tcq.c
--- linux-2.5.15/drivers/ide/tcq.c	2002-05-13 12:44:17.000000000 +0200
+++ linux/drivers/ide/tcq.c	2002-05-13 02:17:33.000000000 +0200
@@ -95,15 +95,15 @@
 
 	del_timer(&ch->timer);
 
-	if (test_bit(IDE_DMA, &hwgroup->flags))
+	if (test_bit(IDE_DMA, &ch->active))
 		udma_stop(drive);
 
 	blk_queue_invalidate_tags(q);
 
 	drive->using_tcq = 0;
 	drive->queue_depth = 1;
-	clear_bit(IDE_BUSY, &hwgroup->flags);
-	clear_bit(IDE_DMA, &hwgroup->flags);
+	clear_bit(IDE_BUSY, &ch->active);
+	clear_bit(IDE_DMA, &ch->active);
 	hwgroup->handler = NULL;
 
 	/*
@@ -152,6 +152,7 @@
 static void ata_tcq_irq_timeout(unsigned long data)
 {
 	struct ata_device *drive = (struct ata_device *) data;
+	struct ata_channel *ch = drive->channel;
 	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	unsigned long flags;
 
@@ -159,7 +160,7 @@
 
 	spin_lock_irqsave(&ide_lock, flags);
 
-	if (test_and_set_bit(IDE_BUSY, &hwgroup->flags))
+	if (test_and_set_bit(IDE_BUSY, &ch->active))
 		printk(KERN_ERR "ATA: %s: hwgroup not busy\n", __FUNCTION__);
 	if (hwgroup->handler == NULL)
 		printk(KERN_ERR "ATA: %s: missing isr!\n", __FUNCTION__);
@@ -170,7 +171,7 @@
 	 * if pending commands, try service before giving up
 	 */
 	if (ata_pending_commands(drive) && (GET_STAT() & SERVICE_STAT))
-		if (service(drive, hwgroup->rq) == ide_started)
+		if (service(drive, drive->rq) == ide_started)
 			return;
 
 	if (drive)
@@ -241,7 +242,7 @@
 	 * Could be called with IDE_DMA in-progress from invalidate
 	 * handler, refuse to do anything.
 	 */
-	if (test_bit(IDE_DMA, &HWGROUP(drive)->flags))
+	if (test_bit(IDE_DMA, &drive->channel->active))
 		return ide_stopped;
 
 	/*
@@ -283,7 +284,7 @@
 	 * should not happen, a buggy device could introduce loop
 	 */
 	if ((feat = GET_FEAT()) & NSEC_REL) {
-		HWGROUP(drive)->rq = NULL;
+		drive->rq = NULL;
 		printk("%s: release in service\n", drive->name);
 		return ide_stopped;
 	}
@@ -298,7 +299,7 @@
 		return ide_stopped;
 	}
 
-	HWGROUP(drive)->rq = rq;
+	drive->rq = rq;
 
 	/*
 	 * we'll start a dma read or write, device will trigger
@@ -529,7 +530,7 @@
 	struct ata_channel *ch = drive->channel;
 
 	TCQ_PRINTK("%s: setting up queued %d\n", __FUNCTION__, rq->tag);
-	if (!test_bit(IDE_BUSY, &ch->hwgroup->flags))
+	if (!test_bit(IDE_BUSY, &ch->active))
 		printk("queued_rw: IDE_BUSY not set\n");
 
 	if (tcq_wait_dataphase(drive))
@@ -584,7 +585,7 @@
 	 */
 	if ((feat = GET_FEAT()) & NSEC_REL) {
 		drive->immed_rel++;
-		HWGROUP(drive)->rq = NULL;
+		drive->rq = NULL;
 		set_irq(drive, ide_dmaq_intr);
 
 		TCQ_PRINTK("REL in queued_start\n");
diff -urN linux-2.5.15/include/asm-alpha/ide.h linux/include/asm-alpha/ide.h
--- linux-2.5.15/include/asm-alpha/ide.h	2002-05-10 00:23:17.000000000 +0200
+++ linux/include/asm-alpha/ide.h	2002-05-12 16:28:28.000000000 +0200
@@ -82,10 +82,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASMalpha_IDE_H */
diff -urN linux-2.5.15/include/asm-arm/ide.h linux/include/asm-arm/ide.h
--- linux-2.5.15/include/asm-arm/ide.h	2002-05-10 00:24:07.000000000 +0200
+++ linux/include/asm-arm/ide.h	2002-05-12 16:28:28.000000000 +0200
@@ -21,10 +21,6 @@
 
 #include <asm/arch/ide.h>
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 /*
  * We always use the new IDE port registering,
  * so these are fixed here.
diff -urN linux-2.5.15/include/asm-cris/ide.h linux/include/asm-cris/ide.h
--- linux-2.5.15/include/asm-cris/ide.h	2002-05-10 00:22:28.000000000 +0200
+++ linux/include/asm-cris/ide.h	2002-05-12 16:28:28.000000000 +0200
@@ -96,10 +96,6 @@
 #undef SUPPORT_SLOW_DATA_PORTS
 #define SUPPORT_SLOW_DATA_PORTS	0
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 /* the drive addressing is done through a controller register on the Etrax CPU */
 void OUT_BYTE(unsigned char data, ide_ioreg_t reg);
 unsigned char IN_BYTE(ide_ioreg_t reg);
diff -urN linux-2.5.15/include/asm-i386/ide.h linux/include/asm-i386/ide.h
--- linux-2.5.15/include/asm-i386/ide.h	2002-05-10 00:21:52.000000000 +0200
+++ linux/include/asm-i386/ide.h	2002-05-13 03:09:01.000000000 +0200
@@ -86,10 +86,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASMi386_IDE_H */
diff -urN linux-2.5.15/include/asm-ia64/ide.h linux/include/asm-ia64/ide.h
--- linux-2.5.15/include/asm-ia64/ide.h	2002-05-10 00:24:57.000000000 +0200
+++ linux/include/asm-ia64/ide.h	2002-05-12 16:28:28.000000000 +0200
@@ -92,10 +92,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASM_IA64_IDE_H */
diff -urN linux-2.5.15/include/asm-m68k/ide.h linux/include/asm-m68k/ide.h
--- linux-2.5.15/include/asm-m68k/ide.h	2002-05-10 00:21:32.000000000 +0200
+++ linux/include/asm-m68k/ide.h	2002-05-12 16:28:28.000000000 +0200
@@ -145,10 +145,13 @@
 
 #endif /* CONFIG_ATARI || CONFIG_Q40 */
 
+#define ATA_ARCH_ACK_INTR
+
+#ifdef CONFIG_ATARI
+#define ATA_ARCH_LOCK
 
 static __inline__ void ide_release_lock (int *ide_lock)
 {
-#ifdef CONFIG_ATARI
 	if (MACH_IS_ATARI) {
 		if (*ide_lock == 0) {
 			printk("ide_release_lock: bug\n");
@@ -157,12 +160,10 @@
 		*ide_lock = 0;
 		stdma_release();
 	}
-#endif /* CONFIG_ATARI */
 }
 
 static __inline__ void ide_get_lock (int *ide_lock, void (*handler)(int, void *, struct pt_regs *), void *data)
 {
-#ifdef CONFIG_ATARI
 	if (MACH_IS_ATARI) {
 		if (*ide_lock == 0) {
 			if (in_interrupt() > 0)
@@ -171,10 +172,8 @@
 			*ide_lock = 1;
 		}
 	}
-#endif /* CONFIG_ATARI */
 }
-
-#define ide_ack_intr(hwif)	((hwif)->hw.ack_intr ? (hwif)->hw.ack_intr(hwif) : 1)
+#endif /* CONFIG_ATARI */
 
 /*
  * On the Atari, we sometimes can't enable interrupts:
diff -urN linux-2.5.15/include/asm-mips/ide.h linux/include/asm-mips/ide.h
--- linux-2.5.15/include/asm-mips/ide.h	2002-05-10 00:22:38.000000000 +0200
+++ linux/include/asm-mips/ide.h	2002-05-12 16:28:28.000000000 +0200
@@ -68,10 +68,6 @@
 #undef  SUPPORT_VLB_SYNC
 #define SUPPORT_VLB_SYNC 0
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASM_IDE_H */
diff -urN linux-2.5.15/include/asm-mips64/ide.h linux/include/asm-mips64/ide.h
--- linux-2.5.15/include/asm-mips64/ide.h	2002-05-10 00:21:32.000000000 +0200
+++ linux/include/asm-mips64/ide.h	2002-05-12 16:28:28.000000000 +0200
@@ -68,10 +68,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASM_IDE_H */
diff -urN linux-2.5.15/include/asm-parisc/ide.h linux/include/asm-parisc/ide.h
--- linux-2.5.15/include/asm-parisc/ide.h	2002-05-10 00:23:34.000000000 +0200
+++ linux/include/asm-parisc/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -81,10 +81,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASMi386_IDE_H */
diff -urN linux-2.5.15/include/asm-ppc/ide.h linux/include/asm-ppc/ide.h
--- linux-2.5.15/include/asm-ppc/ide.h	2002-05-10 00:23:34.000000000 +0200
+++ linux/include/asm-ppc/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -108,12 +108,8 @@
 }
 
 #if (defined CONFIG_APUS || defined CONFIG_BLK_DEV_MPC8xx_IDE )
-#define ide_ack_intr(hwif) (hwif->hw.ack_intr ? hwif->hw.ack_intr(hwif) : 1)
-#else
-#define ide_ack_intr(hwif)		(1)
+#define ATA_ARCH_ACK_INTR
 #endif
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
 
 #endif /* __KERNEL__ */
 
diff -urN linux-2.5.15/include/asm-ppc64/ide.h linux/include/asm-ppc64/ide.h
--- linux-2.5.15/include/asm-ppc64/ide.h	2002-05-10 00:24:22.000000000 +0200
+++ linux/include/asm-ppc64/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -50,10 +50,6 @@
 {
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASMPPC64_IDE_H */
diff -urN linux-2.5.15/include/asm-s390/ide.h linux/include/asm-s390/ide.h
--- linux-2.5.15/include/asm-s390/ide.h	2002-05-10 00:24:47.000000000 +0200
+++ linux/include/asm-s390/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -18,14 +18,6 @@
 #define ide__sti()	do {} while (0)
 
 /*
- * The following are not needed for the non-m68k ports
- */
-#define ide_ack_intr(hwif)		(1)
-#define ide_fix_driveid(id)		do {} while (0)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
-/*
  * We always use the new IDE port registering,
  * so these are fixed here.
  */
diff -urN linux-2.5.15/include/asm-s390x/ide.h linux/include/asm-s390x/ide.h
--- linux-2.5.15/include/asm-s390x/ide.h	2002-05-10 00:21:33.000000000 +0200
+++ linux/include/asm-s390x/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -17,10 +17,6 @@
 
 #define ide__sti()	do {} while (0)
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 /*
  * We always use the new IDE port registering,
  * so these are fixed here.
diff -urN linux-2.5.15/include/asm-sh/ide.h linux/include/asm-sh/ide.h
--- linux-2.5.15/include/asm-sh/ide.h	2002-05-10 00:22:55.000000000 +0200
+++ linux/include/asm-sh/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -107,10 +107,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASM_SH_IDE_H */
diff -urN linux-2.5.15/include/asm-sparc/ide.h linux/include/asm-sparc/ide.h
--- linux-2.5.15/include/asm-sparc/ide.h	2002-05-10 00:24:45.000000000 +0200
+++ linux/include/asm-sparc/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -165,11 +165,6 @@
 	/* __flush_dcache_range((unsigned long)src, end); */ /* P3 see hme */
 }
 
-#define ide_ack_intr(hwif)		(1)
-/* #define ide_ack_intr(hwif)	((hwif)->hw.ack_intr ? (hwif)->hw.ack_intr(hwif) : 1) */
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* _SPARC_IDE_H */
diff -urN linux-2.5.15/include/asm-sparc64/ide.h linux/include/asm-sparc64/ide.h
--- linux-2.5.15/include/asm-sparc64/ide.h	2002-05-10 00:23:37.000000000 +0200
+++ linux/include/asm-sparc64/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -181,10 +181,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* _SPARC64_IDE_H */
diff -urN linux-2.5.15/include/asm-x86_64/ide.h linux/include/asm-x86_64/ide.h
--- linux-2.5.15/include/asm-x86_64/ide.h	2002-05-10 00:24:22.000000000 +0200
+++ linux/include/asm-x86_64/ide.h	2002-05-12 16:28:29.000000000 +0200
@@ -86,10 +86,6 @@
 #endif
 }
 
-#define ide_ack_intr(hwif)		(1)
-#define ide_release_lock(lock)		do {} while (0)
-#define ide_get_lock(lock, hdlr, data)	do {} while (0)
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASMi386_IDE_H */
diff -urN linux-2.5.15/include/linux/ide.h linux/include/linux/ide.h
--- linux-2.5.15/include/linux/ide.h	2002-05-13 12:44:17.000000000 +0200
+++ linux/include/linux/ide.h	2002-05-13 03:09:01.000000000 +0200
@@ -239,6 +239,19 @@
 
 #include <asm/ide.h>
 
+/* Currently only m68k, apus and m8xx need it */
+#ifdef ATA_ARCH_ACK_INTR
+# define ide_ack_intr(hwif) (hwif->hw.ack_intr ? hwif->hw.ack_intr(hwif) : 1)
+#else
+# define ide_ack_intr(hwif) (1)
+#endif
+
+/* Currently only Atari needs it */
+#ifndef ATA_ARCH_LOCK
+# define ide_release_lock(lock)		do {} while (0)
+# define ide_get_lock(lock, hdlr, data)	do {} while (0)
+#endif
+
 /*
  * If the arch-dependant ide.h did not declare/define any OUT_BYTE or IN_BYTE
  * functions, we make some defaults here. The only architecture currently
@@ -324,14 +337,16 @@
 	 * magically just go away.
 	 */
 	request_queue_t	queue;	/* per device request queue */
+	struct request *rq;		/* current request */
 
 	unsigned long sleep;	/* sleep until this time */
 
-	byte     using_dma;		/* disk is using dma for read/write */
-	byte	 using_tcq;		/* disk is using queueing */
 	byte	 retry_pio;		/* retrying dma capable host in pio */
 	byte	 state;			/* retry state */
-	byte     dsc_overlap;		/* flag: DSC overlap */
+
+	unsigned using_dma	: 1;	/* disk is using dma for read/write */
+	unsigned using_tcq	: 1;	/* disk is using queueing */
+	unsigned dsc_overlap	: 1;	/* flag: DSC overlap */
 
 	unsigned waiting_for_dma: 1;	/* dma currently in progress */
 	unsigned busy		: 1;	/* currently doing revalidate_disk() */
@@ -403,11 +418,39 @@
 	int		max_depth;
 } ide_drive_t;
 
+/*
+ * Status returned by various functions.
+ */
+typedef enum {
+	ide_stopped,	/* no drive operation was started */
+	ide_started,	/* a drive operation was started, and a handler was set */
+	ide_released	/* started and released bus */
+} ide_startstop_t;
+
+/*
+ *  Interrupt and timeout handler type.
+ */
+typedef ide_startstop_t (ata_handler_t)(struct ata_device *, struct request *);
+typedef int (ata_expiry_t)(struct ata_device *, struct request *);
+
 enum {
 	ATA_PRIMARY	= 0,
 	ATA_SECONDARY	= 1
 };
 
+enum {
+	IDE_BUSY,	/* awaiting an interrupt */
+	IDE_SLEEP,
+	IDE_DMA		/* DMA in progress */
+};
+
+typedef struct hwgroup_s {
+	/* FIXME: We should look for busy request queues instead of looking at
+	 * the !NULL state of this field.
+	 */
+	ide_startstop_t (*handler)(struct ata_device *, struct request *);	/* irq handler, if active */
+} ide_hwgroup_t;
+
 struct ata_channel {
 	struct device	dev;		/* device handle */
 	int		unit;		/* channel number */
@@ -415,7 +458,9 @@
 	struct hwgroup_s *hwgroup;	/* actually (ide_hwgroup_t *) */
 	struct timer_list timer;	/* failsafe timer */
 	int (*expiry)(struct ata_device *, struct request *);	/* irq handler, if active */
+	unsigned long poll_timeout;	/* timeout value during polled operations */
 	struct ata_device *drive;	/* last serviced drive */
+	unsigned long active;		/* active processing request */
 
 	ide_ioreg_t	io_ports[IDE_NR_PORTS];	/* task file registers */
 	hw_regs_t	hw;		/* Hardware info */
@@ -506,9 +551,8 @@
 #endif
 	/* driver soft-power interface */
 	int (*busproc)(struct ata_device *, int);
-	byte		bus_state;	/* power state of the IDE bus */
 
-	unsigned long poll_timeout; /* timeout value during polled operations */
+	byte		bus_state;	/* power state of the IDE bus */
 };
 
 /*
@@ -517,27 +561,8 @@
 extern int ide_register_hw(hw_regs_t *hw, struct ata_channel **hwifp);
 extern void ide_unregister(struct ata_channel *hwif);
 
-/*
- * Status returned by various functions.
- */
-typedef enum {
-	ide_stopped,	/* no drive operation was started */
-	ide_started,	/* a drive operation was started, and a handler was set */
-	ide_released	/* started and released bus */
-} ide_startstop_t;
-
-/*
- *  Interrupt and timeout handler type.
- */
-typedef ide_startstop_t (ata_handler_t)(struct ata_device *, struct request *);
-typedef int (ata_expiry_t)(struct ata_device *, struct request *);
-
 struct ata_taskfile;
 
-#define IDE_BUSY	0	/* awaiting an interrupt */
-#define IDE_SLEEP	1
-#define IDE_DMA		2	/* DMA in progress */
-
 #define IDE_MAX_TAG	32
 
 #ifdef CONFIG_BLK_DEV_IDE_TCQ
@@ -561,15 +586,6 @@
 # define ata_can_queue(drive)		(1)
 #endif
 
-typedef struct hwgroup_s {
-	/* FIXME: We should look for busy request queues instead of looking at
-	 * the !NULL state of this field.
-	 */
-	ide_startstop_t (*handler)(struct ata_device *, struct request *);	/* irq handler, if active */
-	unsigned long flags;		/* BUSY, SLEEPING */
-	struct request *rq;		/* current request */
-} ide_hwgroup_t;
-
 /* FIXME: kill this as soon as possible */
 #define PROC_IDE_READ_RETURN(page,start,off,count,eof,len) return 0;