Skip to contentSkip to conversation

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

PATCH In review [email protected] 4 participants 6 messages Mar 6, 2005 – Mar 14, 2005 Char and misc drivers

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/14 char/snsc: reorder set_current_state() and add_wait_queue()

Any comments would be, as always, appreciated.

-Nish

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]>
Signed-off-by Domen Puncer <[email protected]>

---

 kj-domen/drivers/char/snsc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/char/snsc.c renamed from drivers/char/snsc.c~reorder-state-drivers_char_snsc +2 -2
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);

_

#1 lore raw
Quoted 1 line · On Sunday, March 6, 2005 2:36 pm, [email protected] wrote:

Any comments would be, as always, appreciated.

I don't have a problem with this change, but the maintainer probably should have been Cc'd. Greg, does this change look ok to you? Note that it's already been committed to the upstream tree, so if it's a bad change we'll need to have the cset excluded or something.

Quoted 45 lines

-Nish

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]>
Signed-off-by Domen Puncer <[email protected]>

---

 kj-domen/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 --- kj/drivers/char/snsc.c~reorder-state-drivers_char_snsc 2005-03-05 16:09:54.000000000 +0100 +++ kj-domen/drivers/char/snsc.c 2005-03-05 16:09:54.000000000 +0100 @@ -192,8 +192,8 @@ scdrv_read(struct file *file, char __use
}

   len = CHUNKSIZE;
drivers/char/snsc.c renamed from drivers/char/snsc.c~reorder-state-drivers_char_snsc +2 -2
-  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);

_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#2 lore raw
Quoted 7 lines · Jesse Barnes
Quoted 1 line · On Sunday, March 6, 2005 2:36 pm, [email protected] wrote:

Any comments would be, as always, appreciated.

I don't have a problem with this change, but the maintainer probably should have been Cc'd. Greg, does this change look ok to you? Note that it's already been committed to the upstream tree, so if it's a bad change we'll need to have the cset excluded or something.

Thanks for the feedback. I don't see a maintainer entry for Greg anywhere in the snsc.{c,h} files or MAINTAINERS. Could someone throw a patch upstream so that whomever should be contacted for this driver will be in the future?

Thanks,
Nish

#3 lore raw
Quoted 9 lines · Nishanth Aravamudan
Quoted 7 lines · Jesse Barnes
Quoted 1 line · On Sunday, March 6, 2005 2:36 pm, [email protected] wrote:

Any comments would be, as always, appreciated.

I don't have a problem with this change, but the maintainer probably should have been Cc'd. Greg, does this change look ok to you? Note that it's already been committed to the upstream tree, so if it's a bad change we'll need to have the cset excluded or something.

Quoted 4 lines

Thanks for the feedback. I don't see a maintainer entry for Greg anywhere in the snsc.{c,h} files or MAINTAINERS. Could someone throw a patch upstream so that whomever should be contacted for this driver will be in the future?

Good point :) I assumed there was a MODULE_AUTHOR entry. Here's a patch to get Greg more spam^W^W^W^Wadd the relevant info. Look ok to you Greg?

Signed-off-by Jesse Barnes <[email protected]>

Jesse

===== drivers/char/snsc.c 1.4 vs edited =====

drivers/char/snsc.c +4 -0
@@ -28,6 +28,10 @@
 #include <asm/sn/nodepda.h>
 #include "snsc.h"
 
+MODULE_AUTHOR("Greg Howard <[email protected]>");
+MODULE_DESCRIPTION("SGI System Controller driver");
+MODULE_LICENSE("GPL");
+
 #define SYSCTL_BASENAME	"snsc"
 
 #define SCDRV_BUFSZ	2048
#4 lore raw
Quoted 10 lines · Jesse Barnes
Quoted 9 lines · Nishanth Aravamudan
Quoted 7 lines · Jesse Barnes
Quoted 1 line · On Sunday, March 6, 2005 2:36 pm, [email protected] wrote:

Any comments would be, as always, appreciated.

I don't have a problem with this change, but the maintainer probably should have been Cc'd. Greg, does this change look ok to you? Note that it's already been committed to the upstream tree, so if it's a bad change we'll need to have the cset excluded or something.

Quoted 7 lines
Quoted 4 lines

Thanks for the feedback. I don't see a maintainer entry for Greg anywhere in the snsc.{c,h} files or MAINTAINERS. Could someone throw a patch upstream so that whomever should be contacted for this driver will be in the future?

Good point :) I assumed there was a MODULE_AUTHOR entry. Here's a patch to get Greg more spam^W^W^W^Wadd the relevant info. Look ok to you Greg?

Looks good. Apologies to Nish for dropping my
apparently-orphaned code on his doorstep. :-)

Thanks - Greg

#5 lore raw
In reply to Jesse Barnes
Quoted 7 lines · Jesse Barnes
Quoted 1 line · On Sunday, March 6, 2005 2:36 pm, [email protected] wrote:

Any comments would be, as always, appreciated.

I don't have a problem with this change, but the maintainer probably should have been Cc'd. Greg, does this change look ok to you? Note that it's already been committed to the upstream tree, so if it's a bad change we'll need to have the cset excluded or something.

I think it's safe enough. Since interrupts are off at this point, I don't think the order of the two functions actually matters (i.e. we couldn't have received a signal until the call to spin_unlock_irqrestore() anyway).

Thanks - Greg