Any comments would be, as always, appreciated.
-Nish
Description: Reorder add_wait_queue() and set_current_state() as a
signal could be lost in between the two functions.
Signed-off-by: Nishanth Aravamudan <[email protected]>
---
linux-2.6.10-rc2-bk4-max/drivers/char/snsc.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff -puN drivers/char/snsc.c~reorder-state-drivers_char_snsc drivers/char/snsc.c
--- linux-2.6.10-rc2-bk4/drivers/char/snsc.c~reorder-state-drivers_char_snsc 2004-11-20 00:29:55.000000000 +0100
+++ linux-2.6.10-rc2-bk4-max/drivers/char/snsc.c 2004-11-20 00:29:55.000000000 +0100
@@ -192,8 +192,8 @@ scdrv_read(struct file *file, char __use
}
len = CHUNKSIZE;
- add_wait_queue(&sd->sd_rq, &wait);
set_current_state(TASK_INTERRUPTIBLE);
+ add_wait_queue(&sd->sd_rq, &wait);
spin_unlock_irqrestore(&sd->sd_rlock, flags);
schedule_timeout(SCDRV_TIMEOUT);
@@ -288,8 +288,8 @@ scdrv_write(struct file *file, const cha
return -EAGAIN;
}
- add_wait_queue(&sd->sd_wq, &wait);
set_current_state(TASK_INTERRUPTIBLE);
+ add_wait_queue(&sd->sd_wq, &wait);
spin_unlock_irqrestore(&sd->sd_wlock, flags);
schedule_timeout(SCDRV_TIMEOUT);
_
hoi :)
On Sat, Nov 20, 2004 at 03:47:03AM +0100, [email protected] wrote:
> Description: Reorder add_wait_queue() and set_current_state() as a
> signal could be lost in between the two functions.
couldn't you loose a wake event that way?
Perhaps you want to use prepare_to_wait()?
--
Martin Waitz
On Mon, Nov 22, 2004 at 09:56:20PM +0100, Martin Waitz wrote:
> hoi :)
>
> On Sat, Nov 20, 2004 at 03:47:03AM +0100, [email protected] wrote:
> > Description: Reorder add_wait_queue() and set_current_state() as a
> > signal could be lost in between the two functions.
>
> couldn't you loose a wake event that way?
The patch in question was made specifically because the given code may
miss wake events. Hence the reorder and my description. Since I'm not an
expert on locking, I asked Oliver Neukum about this exact issue a while
ago. Here is his response:
--------
>Am Mittwoch, 15. September 2004 19:34 schrieb Nishanth Aravamudan:
> Oliver,
>
> It was recommended to me to ask you a question about the proper ordering
> of add_wait_queue() and set_current_state().
>
> In some drivers the order is
>
> set_current_state(TASK_INTERRUPTIBLE);
> add_wait_queue(...);
That is correct.
> and in others it is
>
> add_wait_queue(...);
Here in between is a window in which a wake up would be missed.
> set_current_state(TASK_INTERRUPTIBLE);
>
> It seems like one of these should be oorrect and not the other, but I'm
> not sure which is which. Any insight you could provide would be greatly
> appreciated.
Iff you test whether you should indeed sleep before you call schedule,
it doesn't matter. In the other case, you need to use the first form.
-------
Hope that clears things up a bit.
-Nish