On Tue, May 11, 2004 at 11:44:47PM +0200, Oliver Neukum wrote:
> set_current_state(TASK_INTERRUPTIBLE);
> if (!mdc800->irq_woken)
> {
> - schedule_timeout (msec*HZ/1000);
> + long timeout = msec*HZ/1000;
> + while(timeout)
> + timeout = schedule_timeout (timeout);
> }
> remove_wait_queue(&mdc800->irq_wait, &wait);
> set_current_state(TASK_RUNNING);
This is wrong:
1) You need to call set_current_state(...) before every call to
schedule[_timeout] - when schedule() returns, the state has been set
to TASK_RUNNING again, so the task will not go to sleep without first
resetting its state to something other. With plain schedule() this
will result in busy-wait, with schedule_timeout() called as above this
can become an infinite loop (or at least a very long wait, depending
on what else the system has to do).
2) What's the point of TASK_INTERRUPTIBLE if interruptions are not
handled? If you really want an interruptible wait, call
schedule_timeout() only once and bail out with -ERESTARTSYS if you got
signal_pending(). Or just use TASK_UNINTERRUPTIBLE.
> @@ -718,7 +720,9 @@
> set_current_state(TASK_INTERRUPTIBLE);
> if (!mdc800->downloaded)
> {
> - schedule_timeout
> (TO_DOWNLOAD_GET_READY*HZ/1000);
> + long timeout = TO_DOWNLOAD_GET_READY*HZ/1000;
> + while(timeout)
> + timeout = schedule_timeout (timeout);
> }
> set_current_state(TASK_RUNNING);
> remove_wait_queue(&mdc800->download_wait, &wait);
> @@ -842,7 +846,9 @@
> set_current_state(TASK_INTERRUPTIBLE);
> if (!mdc800->written)
> {
> - schedule_timeout (TO_WRITE_GET_READY*HZ/1000);
> + long timeout = TO_WRITE_GET_READY*HZ/1000;
> + while(timeout)
> + timeout = schedule_timeout (timeout);
> }
> set_current_state(TASK_RUNNING);
> remove_wait_queue(&mdc800->write_wait, &wait);
>
These parts have the same problems.
pgp00000.pgp
Description: PGP signature
