RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout() withmsleep_interruptible()

[James Bottomley <James.Bottomley@SteelEye.com>]

On Thu, 2004-10-21 at 09:17, Salyzyn, Mark wrote:
> The driver does set the state to TASK_INTERRUPTIBLE a handful of lines
> above, is there some other state that the driver should have set?

No, it should be set to TASK_[UN]INTERRUPTIBLE.  However, you can't just
do it once.  When the schedule_timeout() returns, the state is once more
TASK_RUNNING.  If you want to do another schedule_timeout() like you're
doing in this case, you have to set it to TASK_[UN]INTERRUPTIBLE again.

> We should remove the set_current_state(TASK_INTERRUPTIBLE) line when
> replacing with the msleep_interruptible(timeout * 1000) line.

Actually, the whole thing:

	if((status = adpt_i2o_post_this(pHba, msg, len)) == 0){
		set_current_state(TASK_INTERRUPTIBLE);
		if(pHba->host)
			spin_unlock_irq(pHba->host->host_lock);
		if (!timeout)
			schedule();
		else{
			timeout = schedule_timeout(timeout);
			if (timeout == 0) {
				// I/O issued, but cannot get result in
				// specified time. Freeing resorces is
				// dangerous.
				status = -ETIME;
			}
			schedule_timeout(timeout*HZ);
		}
		if(pHba->host)
			spin_lock_irq(pHba->host->host_lock);
	}

Looks a bit bogus.  Are the conditional spinlocks really necesary? Why
is timeout multiplied by HZ in the second case, but not in the first
case?  The only reason why timeout would be non-zero in the first
schedule timeout would be if the sleep were interrupted.

James