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

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

[domen]

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 <nacc@us.ibm.com>
Signed-off-by: Domen Puncer <domen@coderock.org>
---


 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;
-		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 01/14] char/snsc: reorder set_current_state() and add_wait_queue()

[Jesse Barnes]

On Sunday, March 6, 2005 2:36 pm, domen@coderock.org 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.

>
> -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 <nacc@us.ibm.com>
> Signed-off-by: Domen Puncer <domen@coderock.org>
> ---
>
>
>  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;
> -  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Nishanth Aravamudan]

On Mon, Mar 14, 2005 at 09:45:44AM -0800, Jesse Barnes wrote:
> On Sunday, March 6, 2005 2:36 pm, domen@coderock.org 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

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

[Jesse Barnes]

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

On Monday, March 14, 2005 9:54 am, Nishanth Aravamudan wrote:
> On Mon, Mar 14, 2005 at 09:45:44AM -0800, Jesse Barnes wrote:
> > On Sunday, March 6, 2005 2:36 pm, domen@coderock.org 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?

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 <jbarnes@sgi.com>

Jesse


[-- Attachment #2: snsc-author.patch --]
[-- Type: text/plain, Size: 416 bytes --]

===== drivers/char/snsc.c 1.4 vs edited =====
--- 1.4/drivers/char/snsc.c	2005-03-13 15:30:07 -08:00
+++ edited/drivers/char/snsc.c	2005-03-14 10:01:38 -08:00
@@ -28,6 +28,10 @@
 #include <asm/sn/nodepda.h>
 #include "snsc.h"
 
+MODULE_AUTHOR("Greg Howard <ghoward@sgi.com>");
+MODULE_DESCRIPTION("SGI System Controller driver");
+MODULE_LICENSE("GPL");
+
 #define SYSCTL_BASENAME	"snsc"
 
 #define SCDRV_BUFSZ	2048

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

[Greg Howard]

On Mon, 14 Mar 2005, Jesse Barnes wrote:

> On Sunday, March 6, 2005 2:36 pm, domen@coderock.org 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

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

[Greg Howard]

On Mon, 14 Mar 2005, Jesse Barnes wrote:

> On Monday, March 14, 2005 9:54 am, Nishanth Aravamudan wrote:
> > On Mon, Mar 14, 2005 at 09:45:44AM -0800, Jesse Barnes wrote:
> > > On Sunday, March 6, 2005 2:36 pm, domen@coderock.org 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?
>
> 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