IDE Power Management try 1

The point is to pipe the power management requests through the request queue for proper locking. Since those requests involve several operations that have to be tied together with the queue beeing locked for further 'user' requests, they are implemented as a state machine with specific callbacks in the subdrivers

One thing that should probably be cleaned up is the difference between the suspend and the resume request. I didn't want to implement 2 different request bits to avoid using too much of that bit-space, and because most of the core handling is the same. So right now, I carry in the special structure attached to the request, 2 fields. An int indicating if we are doing a suspend or a resume op, and an int that is the actual state machine step.

===== include/linux/blkdev.h 1.100 vs edited =====

Hello,

Quoted 6 lines · Benjamin Herrenschmidt

The point is to pipe the power management requests through the request queue for proper locking. Since those requests involve several operations that have to be tied together with the queue beeing locked for further 'user' requests, they are implemented as a state machine with specific callbacks in the subdrivers

[cut]

Quoted 13 lines

One thing that should probably be cleaned up is the difference between the suspend and the resume request. I didn't want to implement 2 different request bits to avoid using too much of that bit-space, and because most of the core handling is the same. So right now, I carry in the special structure attached to the request, 2 fields. An int indicating if we are doing a suspend or a resume op, and an int that is the actual state machine step.

===== include/linux/blkdev.h 1.100 vs edited =====

include/linux/blkdev.h line 121

Quoted 6 lines

include/linux/blkdev.h +1 -0

@@ -116,6 +116,7 @@
 	__REQ_DRIVE_CMD,
 	__REQ_DRIVE_TASK,
 	__REQ_DRIVE_TASKFILE,
+	__REQ_POWER_MANAGEMENT,
 	__REQ_NR_BITS,	/* stops here */
 };

		What about this - add __REQ_DRIVE_INTERNAL, and carry args 
		in rq->cmd[16] [0] = PM, [1] = SUSPEND/RESUME, [2]= STATE ? IDE can use it 

for power managment, error handling (do not do it from interrupt context, but queue it), may be more. This way it would really makes things a bit better with the complicated IDE locking. SCSI and probably other block devices can benefit from this internal requests too, so the bit is not wasted.

There are already lots of "INTERNAL" - basically take your pick from all the ones you quote above (DRIVE_TASK, DRIVE_CMD, DRIVE_TASKFILE - it's a MESS). A power management special request makes sense to me.

Quoted 35 lines · Alexander Atanasov

Hello,

Quoted 6 lines · Benjamin Herrenschmidt

The point is to pipe the power management requests through the request queue for proper locking. Since those requests involve several operations that have to be tied together with the queue beeing locked for further 'user' requests, they are implemented as a state machine with specific callbacks in the subdrivers

[cut]

Quoted 13 lines

One thing that should probably be cleaned up is the difference between the suspend and the resume request. I didn't want to implement 2 different request bits to avoid using too much of that bit-space, and because most of the core handling is the same. So right now, I carry in the special structure attached to the request, 2 fields. An int indicating if we are doing a suspend or a resume op, and an int that is the actual state machine step.

===== include/linux/blkdev.h 1.100 vs edited =====

include/linux/blkdev.h line 121

Quoted 6 lines

include/linux/blkdev.h +1 -0

@@ -116,6 +116,7 @@
 	__REQ_DRIVE_CMD,
 	__REQ_DRIVE_TASK,
 	__REQ_DRIVE_TASKFILE,
+	__REQ_POWER_MANAGEMENT,
 	__REQ_NR_BITS,	/* stops here */
 };

		What about this - add __REQ_DRIVE_INTERNAL, and carry args
		in rq->cmd[16] [0] = PM, [1] = SUSPEND/RESUME, [2]= STATE ? IDE can use it

Quoted 5 lines

for power managment, error handling (do not do it from interrupt context, but queue it), may be more. This way it would really makes things a bit better with the complicated IDE locking. SCSI and probably other block devices can benefit from this internal requests too, so the bit is not wasted.

There are already lots of "INTERNAL" - basically take your pick from all the ones you quote above (DRIVE_TASK, DRIVE_CMD, DRIVE_TASKFILE - it's a MESS). A power management special request makes sense to me.

What about this - add __REQ_DRIVE_INTERNAL, and carry args in
rq->cmd[16] [0] = PM, [1] = SUSPEND/RESUME, [2]= STATE ? IDE can use it for power managment, error handling (do not do it from interrupt context, but queue it), may be more. This way it would really makes things a bit better with the complicated IDE locking. SCSI and probably other block devices can benefit from this internal requests too, so the bit is not wasted.

Quoted 8 lines

What about this - add __REQ_DRIVE_INTERNAL, and carry args in
rq->cmd[16] [0] = PM, [1] = SUSPEND/RESUME, [2]= STATE ? IDE can use it for power managment, error handling (do not do it from interrupt context, but queue it), may be more. This way it would really makes things a bit better with the complicated IDE locking. SCSI and probably other block devices can benefit from this internal requests too, so the bit is not wasted.

I agree. IDE locking isn't _that_ complicated ;) Though currently, we do

From the request side of things, IDE is very simple.

handle requests right on interrupt completion so error handling wouldn't be deferred by this trick.

Jens, what do you think ? You are the blkdev.h guy :)

If you add REQ_DRIVE_INTERNAL, and kill the other ones I mentioned, fine with me then.

	rq->flags & REQ_DRIVE_INTERNAL
		rq->cmd[0] == PM
			pm stuf
		rq->cmd[0] = taskfile
			taskfile

etc. Make sense?

Quoted 9 lines

If you add REQ_DRIVE_INTERNAL, and kill the other ones I mentioned, fine with me then.

	rq->flags & REQ_DRIVE_INTERNAL
		rq->cmd[0] == PM
			pm stuf
		rq->cmd[0] = taskfile
			taskfile

Quoted 1 line

etc. Make sense?

As I just wrote, I'd rather go the whole way then and break up flags (which is a very bad name btw) into req_type & req_subtype, though that would mean a bit of driver fixing....

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 9 lines

If you add REQ_DRIVE_INTERNAL, and kill the other ones I mentioned, fine with me then.

	rq->flags & REQ_DRIVE_INTERNAL
		rq->cmd[0] == PM
			pm stuf
		rq->cmd[0] = taskfile
			taskfile

Quoted 5 lines

Quoted 1 line

etc. Make sense?

As I just wrote, I'd rather go the whole way then and break up flags (which is a very bad name btw) into req_type & req_subtype, though that would mean a bit of driver fixing....

Also, I noticed that my patch has a nice bug in the resume path, I use ide_preempt, which doesn't wait for the request to complete, but the request & struct state are allocated on the stack... ouch...

It would be interesting to not wait for completion of the resume still here, there's no reason why resume of the disk can't be done asynchronously since we only release the request queue once completed, so I probably need to allocate the suspend request and release it from interrupt.

Also, having a separate structure pointed to by ->special only makes this more complicated, there are plenty of fields in struct request that I could indeed use for my state information (like the cmd[] stuff)

Ben.

Why can't we simply change ide_do_drive_command() to take extra flag specifing wait/do not wait and use it for ide_preempt?

Quoted 9 lines

If you add REQ_DRIVE_INTERNAL, and kill the other ones I mentioned, fine with me then.

	rq->flags & REQ_DRIVE_INTERNAL
		rq->cmd[0] == PM
			pm stuf
		rq->cmd[0] = taskfile
			taskfile

Quoted 1 line

etc. Make sense?

As I just wrote, I'd rather go the whole way then and break up flags (which is a very bad name btw) into req_type & req_subtype, though that would mean a bit of driver fixing....

Ben.

req_type & req_subtype makes sense,
but it is future since driver work is needed

Quoted 11 lines · Benjamin Herrenschmidt

Quoted 10 lines

If you add REQ_DRIVE_INTERNAL, and kill the other ones I mentioned, fine with me then.

	rq->flags & REQ_DRIVE_INTERNAL
		rq->cmd[0] == PM
			pm stuf
		rq->cmd[0] = taskfile
			taskfile

etc. Make sense?

Quoted 5 lines

As I just wrote, I'd rather go the whole way then and break up flags (which is a very bad name btw) into req_type & req_subtype, though that would mean a bit of driver fixing....

Ben.

req_type & req_subtype makes sense,
but it is future since driver work is needed