[patch 4/4] char/snsc: reorder set_current_state() and add_wait_queue()

[patch 4/4] char/snsc: reorder set_current_state() and add_wait_queue()

[janitor]

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 <nacc@us.ibm.com>

---

 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);
_

Re: [patch 4/4] char/snsc: reorder set_current_state() and add_wait_queue()

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

hoi :)

On Sat, Nov 20, 2004 at 03:47:03AM +0100, janitor@sternwelten.at 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 4/4] char/snsc: reorder set_current_state() and add_wait_queue()

[Nishanth Aravamudan]

On Mon, Nov 22, 2004 at 09:56:20PM +0100, Martin Waitz wrote:
> hoi :)
> 
> On Sat, Nov 20, 2004 at 03:47:03AM +0100, janitor@sternwelten.at 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