Messages in this thread | | | Date | Tue, 23 Apr 2002 11:54:59 +0200 | From | Jens Axboe <> | Subject | Re: 2.5.9 -- OOPS in IDE code (symbolic dump and boot log included) |
| |
On Tue, Apr 23 2002, Martin Dalecki wrote: > >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 ;-).
Didn't read that :)
> >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...
Why would you care what type of transport they use??
> >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.
Right, it should rather be a bug trigger in ide-cd... Doesn't matter, it will be killed soon.
> >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.
How so? Even with pusing ar at the level discussed here, ide-xx.c does really not need to care. Change the do_request() strategy to just take the drive and ar, driver is free to just:
/* something to this effect */ struct request *rq = ar->ar_rq; sector_t block = ar->ar_block;
and the rest could remain unchanged. It's up to the driver how far it wants to take this. The only "problem" is that rq->special will always contain the pointer to the ar used, so none of them can touch it.
> > /* > > * 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()..
I disagree, there's no need to have a ->tcq if you don't support queueing. The "trivial queue depth" is already done, it's called drive->queue_depth and is 1 for non-tcq (or tcq with depth 1 :-).
-- Jens Axboe
- 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/
| |