Re: 2.5.9 -- OOPS in IDE code (symbolic dump and boot log included)

[Martin Dalecki <dalecki@evision-ventures.com>]

> Martin,
> 
> There are several 'issues' with the ide-cd changes, in fact I think they
> are horrible. I'll take part of the blame for that, I'll explain.

Well... I refer you to my change long, where I indeed admitted directly
that it's an ugly band aid ;-).

> The ata_ar_get() doesn't belong inside the do_request() strategies, the
> reason I did that for ide-disk was to get going on the tcq stuff and not
> spend too much time rewriting the ide request handling at that point. It

Right. it belongs one level up. The request handling should
possible learn whatever it it's handling ATA or ATAPI devices.
In esp. the ide_start_dma() changes where no pretty...

> was _never_ meant to propagate into the other ide drivers, and in fact
> the code in ide-disk has several tcq specific parts that really cannot
> work in ide-cd. Such as (ide-cd.c:ide_cdrom_do_request()):
> 
> 	spin_lock...
> 
> 	ar = ata_ar_get()
> 	if (!ar) {
> 		spin_unlock;
> 		return ide_started;
> 	}
> 	...
> 
> ide-disk guarentees that if ata_ar_get() fails, it's because we have
> some pending commands on the drive. The ide_started is bogus too, in
> this context it really should be ide_didnt_start_jack, but it works for
> ide-disk because of the above assumptions.

Fortunately it can't happen becouse the other devices don't
support TCQ.

> Don't tell me you can read ide_cdrom_do_request() right now without a
> barf bag :-)

Ohhh. there are a lot of other things I'm unhappy about there as well.

> I'd suggest moving the ata_ar_get() at the ide_queue_commands() level,
> and just pass { drive, ar } to the do_request() strategies. That's also
> why ide-disk.c:idedisk_do_request() has this comment:

Yes this was my intention for the future. The only driver which will have
problems with this is ide-scsi.c - it's not obvious (at least right now)
to me how to change the do_request signature there.

> 	/*
> 	 * get a new command (push ar further down to avoid grabbing
> 	 * lock here
> 	 */
> 	spin_lock_irqsave(DRIVE_LOCK(drive), flags);
> 
> 	ar = ata_ar_get(drive);
> 	...
> 
> I've been meaning to do this once tcq settled down, just didn't get
> around to it yet. But please don't start moving stuff like this into
> ide-cd too.

You notice that I didn't even care to change the write request code-path?
BTW.> It became obvious to me as well that even all the drivers out
there not supporting TCQ will have to get the TCQ parts of struct ata_device
initialized - with a trivial queue depth. drive->tcq should therefore be really
just a memmber of struct ata_device()..