char/snsc: reorder set_current_state() and add_wait_queue()

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.

---

 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
@@ -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 :)

couldn't you loose a wake event that way?

Perhaps you want to use prepare_to_wait()?

Martin Waitz

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:

--------

That is correct.

Here in between is a window in which a wake up would be missed.

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