2.5.30 IDE 113

diff -durNp -X /tmp/diff.hDAz3d linux-2.5.30/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c

drivers/ide/ide-taskfile.c 2002-08-04 02:24:49.000000000 +0200 renamed from drivers/ide/ide-taskfile.c 2002-08-06 01:18:47.000000000 +0200 +1 -4

@@ -188,41 +188,26 @@ static ide_startstop_t special_intr(stru
 
 int ide_raw_taskfile(struct ata_device *drive, struct ata_taskfile *ar, char *buf)
 {
-   struct request *rq;
-   unsigned long flags;
+   struct request *rq = &drive->srequest;
    struct ata_channel *ch = drive->channel;
    request_queue_t *q = &drive->queue;
-   struct list_head *queue_head = &q->queue_head;
    DECLARE_COMPLETION(wait);
-   struct request req;
 
 #ifdef CONFIG_BLK_DEV_PDC4030

Hi Marcin,
what synchronizes these accesses to make sure that you do not have
two ide_raw_taskfile requests on the flight, both using same drive->srequest? It looks to me like that nothing, so you can overwrite request's contents while somebody else already uses this buffer.

device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 5 lines

device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

That's nonsense! What exactly would you return from a request_fn after having queued, eg, 20 commands? Error count is per request, anything else would be stupid.

Uz.ytkownik Jens Axboe napisa?:

Quoted 11 lines · Marcin Dalecki

Quoted 5 lines

device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

That's nonsense! What exactly would you return from a request_fn after having queued, eg, 20 commands? Error count is per request, anything else would be stupid.

Returning the error count in the case q->request_fn is called for a self submitted request like for example REQ_SPECIAL would be handy and well defined. For the cumulative case it would of course make sense to return the cumulative error count. Tough not very meaningfull, it would indicate the occurrence of the error very fine.

Quoted 16 lines

Uz.ytkownik Jens Axboe napisa?:

Quoted 9 lines · Marcin Dalecki

Quoted 6 lines

device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 3 lines

That's nonsense! What exactly would you return from a request_fn after having queued, eg, 20 commands? Error count is per request, anything else would be stupid.

Quoted 5 lines

Returning the error count in the case q->request_fn is called for a self submitted request like for example REQ_SPECIAL would be handy and well defined. For the cumulative case it would of course make sense to return the cumulative error count. Tough not very meaningfull, it would indicate the occurrence of the error very fine.

It's much nicer to maintain a sane API that doesn't depend on stuff like the above. Cumulative error count, come on, you can't possibly be serious?!

Uz.ytkownik Jens Axboe napisa?:

Quoted 24 lines · Marcin Dalecki

Quoted 21 lines

Uz.ytkownik Jens Axboe napisa?:

Quoted 9 lines · Marcin Dalecki

Quoted 6 lines

device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 3 lines

That's nonsense! What exactly would you return from a request_fn after having queued, eg, 20 commands? Error count is per request, anything else would be stupid.

Returning the error count in the case q->request_fn is called for a self submitted request like for example REQ_SPECIAL would be handy and well defined. For the cumulative case it would of course make sense to return the cumulative error count. Tough not very meaningfull, it would indicate the occurrence of the error very fine.

Quoted 3 lines

It's much nicer to maintain a sane API that doesn't depend on stuff like the above. Cumulative error count, come on, you can't possibly be serious?!

Hey don't get me wrong - I *do not* suggest adding it becouse I don't think we are going to change the "eat as many as possible requests" instead of "eat one request" semantics of the q->reuqest_fn(). OK?

Quoted 30 lines

Uz.ytkownik Jens Axboe napisa?:

Quoted 27 lines · Marcin Dalecki

Quoted 24 lines

Uz.ytkownik Jens Axboe napisa?:

Quoted 10 lines · Marcin Dalecki

Quoted 7 lines

device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 3 lines

That's nonsense! What exactly would you return from a request_fn after having queued, eg, 20 commands? Error count is per request, anything else would be stupid.

Returning the error count in the case q->request_fn is called for a self submitted request like for example REQ_SPECIAL would be handy and well defined. For the cumulative case it would of course make sense to return the cumulative error count. Tough not very meaningfull, it would indicate the occurrence of the error very fine.

Quoted 8 lines

Quoted 3 lines

It's much nicer to maintain a sane API that doesn't depend on stuff like the above. Cumulative error count, come on, you can't possibly be serious?!

Hey don't get me wrong - I *do not* suggest adding it becouse I don't think we are going to change the "eat as many as possible requests" instead of "eat one request" semantics of the q->reuqest_fn(). OK?

You look from the IDE perspective, I look from the interface perspective. There's is no "eat one request" semantic of request_fn(), in fact there's just the opposite. If you quit after having just consumed one request, you must make sure to invoke request_fn _yourself_ later on -- or use the recent blk_start/stop_queue helpers.

Uz.ytkownik Jens Axboe napisa?:

Quoted 42 lines · Marcin Dalecki

Quoted 31 lines

Uz.ytkownik Jens Axboe napisa?:

Quoted 28 lines · Marcin Dalecki

Quoted 25 lines

Uz.ytkownik Jens Axboe napisa?:

Quoted 10 lines · Marcin Dalecki

Quoted 7 lines

device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 3 lines

That's nonsense! What exactly would you return from a request_fn after having queued, eg, 20 commands? Error count is per request, anything else would be stupid.

Returning the error count in the case q->request_fn is called for a self submitted request like for example REQ_SPECIAL would be handy and well defined. For the cumulative case it would of course make sense to return the cumulative error count. Tough not very meaningfull, it would
indicate the occurrence of the error very fine.

Quoted 8 lines

Quoted 3 lines

It's much nicer to maintain a sane API that doesn't depend on stuff like the above. Cumulative error count, come on, you can't possibly be serious?!

Hey don't get me wrong - I *do not* suggest adding it becouse I don't think we are going to change the "eat as many as possible requests" instead of "eat one request" semantics of the q->reuqest_fn(). OK?

Quoted 5 lines

You look from the IDE perspective, I look from the interface perspective. There's is no "eat one request" semantic of request_fn(), in fact there's just the opposite. If you quit after having just consumed one request, you must make sure to invoke request_fn _yourself_ later on -- or use the recent blk_start/stop_queue helpers.

Yes of course I know that there is not "eat one request" semantic of request_fn(). However looking at the interface perspective (out of my small corner) I think the above is precisely what leads to ugly things (and I think you will agree that this is ugly) like calling do_ide_request() back out from ata_irq_handler() - shrug.

Ho hum, well I think it's only ugly in the way it had to be done previously. Right now I think the usage is pretty nice, actually

request_fn(q)

{
	rq = elv_next_request();
	start_request(rq);
	blk_stop_queue(q);
}

isr()

{
	handle_completion();
	blk_start_queue(queue);
}

The API works nicely regardless of queue depth and how many requests request_fn consumes.