public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] tty: hold lock across tty buffer finding and buffer filling
@ 2012-03-16  3:00 Tu, Xiaobing
  2012-03-16 20:14 ` gregkh
  0 siblings, 1 reply; 6+ messages in thread
From: Tu, Xiaobing @ 2012-03-16  3:00 UTC (permalink / raw)
  To: gregkh@linuxfoundation.org
  Cc: alan@lxorguk.ukuu.org.uk, linux-kernel@vger.kernel.org,
	Zhang, Yanmin, Du, Alek, Zuo, Jiao

From: Xiaobing Tu <xiaobing.tu@intel.com>

tty_buffer_request_room is well protected, but while after it returns,
 it releases the port->lock. tty->buf.tail might be modified
by either irq handler or other threads. The patch adds more protection
by holding the lock across tty buffer finding and buffer filling.
Signed-off-by: Alek Du <alek.du@intel.com>
Signed-off-by: Xiaobing Tu <xiaobing.tu@intel.com>
---
 drivers/tty/tty_buffer.c |   85 +++++++++++++++++++++++++++++++++++-----------
 1 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6c9b7cd..91e326f 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -185,25 +185,19 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size)
 	/* Should possibly check if this fails for the largest buffer we
 	   have queued and recycle that ? */
 }
-
 /**
- *	tty_buffer_request_room		-	grow tty buffer if needed
+ *	__tty_buffer_request_room		-	grow tty buffer if needed
  *	@tty: tty structure
  *	@size: size desired
  *
  *	Make at least size bytes of linear space available for the tty
  *	buffer. If we fail return the size we managed to find.
- *
- *	Locking: Takes tty->buf.lock
+ *      Locking: Caller must hold tty->buf.lock
  */
-int tty_buffer_request_room(struct tty_struct *tty, size_t size)
+static int __tty_buffer_request_room(struct tty_struct *tty, size_t size)
 {
 	struct tty_buffer *b, *n;
 	int left;
-	unsigned long flags;
-
-	spin_lock_irqsave(&tty->buf.lock, flags);
-
 	/* OPTIMISATION: We could keep a per tty "zero" sized buffer to
 	   remove this conditional if its worth it. This would be invisible
 	   to the callers */
@@ -225,9 +219,30 @@ int tty_buffer_request_room(struct tty_struct *tty, size_t size)
 			size = left;
 	}
 
-	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return size;
 }
+
+
+/**
+ *	tty_buffer_request_room		-	grow tty buffer if needed
+ *	@tty: tty structure
+ *	@size: size desired
+ *
+ *	Make at least size bytes of linear space available for the tty
+ *	buffer. If we fail return the size we managed to find.
+ *
+ *	Locking: Takes tty->buf.lock
+ */
+int tty_buffer_request_room(struct tty_struct *tty, size_t size)
+{
+	unsigned long flags;
+	int length;
+
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	length = __tty_buffer_request_room(tty, size);
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
+	return length;
+}
 EXPORT_SYMBOL_GPL(tty_buffer_request_room);
 
 /**
@@ -249,14 +264,22 @@ int tty_insert_flip_string_fixed_flag(struct tty_struct *tty,
 	int copied = 0;
 	do {
 		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int space = tty_buffer_request_room(tty, goal);
-		struct tty_buffer *tb = tty->buf.tail;
+		int space;
+		unsigned long flags;
+		struct tty_buffer *tb;
+
+		spin_lock_irqsave(&tty->buf.lock, flags);
+		space = __tty_buffer_request_room(tty, goal);
+		tb = tty->buf.tail;
 		/* If there is no space then tb may be NULL */
-		if (unlikely(space == 0))
+		if (unlikely(space == 0)) {
+			spin_unlock_irqrestore(&tty->buf.lock, flags);
 			break;
+		}
 		memcpy(tb->char_buf_ptr + tb->used, chars, space);
 		memset(tb->flag_buf_ptr + tb->used, flag, space);
 		tb->used += space;
+		spin_unlock_irqrestore(&tty->buf.lock, flags);
 		copied += space;
 		chars += space;
 		/* There is a small chance that we need to split the data over
@@ -286,14 +309,22 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
 	int copied = 0;
 	do {
 		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int space = tty_buffer_request_room(tty, goal);
-		struct tty_buffer *tb = tty->buf.tail;
+		int space;
+		unsigned long __flags;
+		struct tty_buffer *tb;
+
+		spin_lock_irqsave(&tty->buf.lock, __flags);
+		space = __tty_buffer_request_room(tty, goal);
+		tb = tty->buf.tail;
 		/* If there is no space then tb may be NULL */
-		if (unlikely(space == 0))
+		if (unlikely(space == 0)) {
+			spin_unlock_irqrestore(&tty->buf.lock, __flags);
 			break;
+		}
 		memcpy(tb->char_buf_ptr + tb->used, chars, space);
 		memcpy(tb->flag_buf_ptr + tb->used, flags, space);
 		tb->used += space;
+		spin_unlock_irqrestore(&tty->buf.lock, __flags);
 		copied += space;
 		chars += space;
 		flags += space;
@@ -344,13 +375,20 @@ EXPORT_SYMBOL(tty_schedule_flip);
 int tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars,
 								size_t size)
 {
-	int space = tty_buffer_request_room(tty, size);
+	int space;
+	unsigned long flags;
+	struct tty_buffer *tb;
+
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	space = __tty_buffer_request_room(tty, size);
+
+	tb = tty->buf.tail;
 	if (likely(space)) {
-		struct tty_buffer *tb = tty->buf.tail;
 		*chars = tb->char_buf_ptr + tb->used;
 		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
 		tb->used += space;
 	}
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return space;
 }
 EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
@@ -374,13 +412,20 @@ EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
 int tty_prepare_flip_string_flags(struct tty_struct *tty,
 			unsigned char **chars, char **flags, size_t size)
 {
-	int space = tty_buffer_request_room(tty, size);
+	int space;
+	unsigned long __flags;
+	struct tty_buffer *tb;
+
+	spin_lock_irqsave(&tty->buf.lock, __flags);
+	space = __tty_buffer_request_room(tty, size);
+
+	tb = tty->buf.tail;
 	if (likely(space)) {
-		struct tty_buffer *tb = tty->buf.tail;
 		*chars = tb->char_buf_ptr + tb->used;
 		*flags = tb->flag_buf_ptr + tb->used;
 		tb->used += space;
 	}
+	spin_unlock_irqrestore(&tty->buf.lock, __flags);
 	return space;
 }
 EXPORT_SYMBOL_GPL(tty_prepare_flip_string_flags);
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] tty: hold lock across tty buffer finding and buffer filling
  2012-03-16  3:00 [PATCH v2] tty: hold lock across tty buffer finding and buffer filling Tu, Xiaobing
@ 2012-03-16 20:14 ` gregkh
  2012-03-16 21:01   ` Jiri Slaby
  2012-03-30 10:26   ` Tu, Xiaobing
  0 siblings, 2 replies; 6+ messages in thread
From: gregkh @ 2012-03-16 20:14 UTC (permalink / raw)
  To: Tu, Xiaobing, Jiri Slaby
  Cc: alan@lxorguk.ukuu.org.uk, linux-kernel@vger.kernel.org,
	Zhang, Yanmin, Du, Alek, Zuo, Jiao

Jiri,

Do you have any objections to me applying the patch below?

thanks,

greg k-h

On Fri, Mar 16, 2012 at 03:00:26AM +0000, Tu, Xiaobing wrote:
> From: Xiaobing Tu <xiaobing.tu@intel.com>
> 
> tty_buffer_request_room is well protected, but while after it returns,
>  it releases the port->lock. tty->buf.tail might be modified
> by either irq handler or other threads. The patch adds more protection
> by holding the lock across tty buffer finding and buffer filling.
> Signed-off-by: Alek Du <alek.du@intel.com>
> Signed-off-by: Xiaobing Tu <xiaobing.tu@intel.com>
> ---
>  drivers/tty/tty_buffer.c |   85 +++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 6c9b7cd..91e326f 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -185,25 +185,19 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size)
>  	/* Should possibly check if this fails for the largest buffer we
>  	   have queued and recycle that ? */
>  }
> -
>  /**
> - *	tty_buffer_request_room		-	grow tty buffer if needed
> + *	__tty_buffer_request_room		-	grow tty buffer if needed
>   *	@tty: tty structure
>   *	@size: size desired
>   *
>   *	Make at least size bytes of linear space available for the tty
>   *	buffer. If we fail return the size we managed to find.
> - *
> - *	Locking: Takes tty->buf.lock
> + *      Locking: Caller must hold tty->buf.lock
>   */
> -int tty_buffer_request_room(struct tty_struct *tty, size_t size)
> +static int __tty_buffer_request_room(struct tty_struct *tty, size_t size)
>  {
>  	struct tty_buffer *b, *n;
>  	int left;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&tty->buf.lock, flags);
> -
>  	/* OPTIMISATION: We could keep a per tty "zero" sized buffer to
>  	   remove this conditional if its worth it. This would be invisible
>  	   to the callers */
> @@ -225,9 +219,30 @@ int tty_buffer_request_room(struct tty_struct *tty, size_t size)
>  			size = left;
>  	}
>  
> -	spin_unlock_irqrestore(&tty->buf.lock, flags);
>  	return size;
>  }
> +
> +
> +/**
> + *	tty_buffer_request_room		-	grow tty buffer if needed
> + *	@tty: tty structure
> + *	@size: size desired
> + *
> + *	Make at least size bytes of linear space available for the tty
> + *	buffer. If we fail return the size we managed to find.
> + *
> + *	Locking: Takes tty->buf.lock
> + */
> +int tty_buffer_request_room(struct tty_struct *tty, size_t size)
> +{
> +	unsigned long flags;
> +	int length;
> +
> +	spin_lock_irqsave(&tty->buf.lock, flags);
> +	length = __tty_buffer_request_room(tty, size);
> +	spin_unlock_irqrestore(&tty->buf.lock, flags);
> +	return length;
> +}
>  EXPORT_SYMBOL_GPL(tty_buffer_request_room);
>  
>  /**
> @@ -249,14 +264,22 @@ int tty_insert_flip_string_fixed_flag(struct tty_struct *tty,
>  	int copied = 0;
>  	do {
>  		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
> -		int space = tty_buffer_request_room(tty, goal);
> -		struct tty_buffer *tb = tty->buf.tail;
> +		int space;
> +		unsigned long flags;
> +		struct tty_buffer *tb;
> +
> +		spin_lock_irqsave(&tty->buf.lock, flags);
> +		space = __tty_buffer_request_room(tty, goal);
> +		tb = tty->buf.tail;
>  		/* If there is no space then tb may be NULL */
> -		if (unlikely(space == 0))
> +		if (unlikely(space == 0)) {
> +			spin_unlock_irqrestore(&tty->buf.lock, flags);
>  			break;
> +		}
>  		memcpy(tb->char_buf_ptr + tb->used, chars, space);
>  		memset(tb->flag_buf_ptr + tb->used, flag, space);
>  		tb->used += space;
> +		spin_unlock_irqrestore(&tty->buf.lock, flags);
>  		copied += space;
>  		chars += space;
>  		/* There is a small chance that we need to split the data over
> @@ -286,14 +309,22 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
>  	int copied = 0;
>  	do {
>  		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
> -		int space = tty_buffer_request_room(tty, goal);
> -		struct tty_buffer *tb = tty->buf.tail;
> +		int space;
> +		unsigned long __flags;
> +		struct tty_buffer *tb;
> +
> +		spin_lock_irqsave(&tty->buf.lock, __flags);
> +		space = __tty_buffer_request_room(tty, goal);
> +		tb = tty->buf.tail;
>  		/* If there is no space then tb may be NULL */
> -		if (unlikely(space == 0))
> +		if (unlikely(space == 0)) {
> +			spin_unlock_irqrestore(&tty->buf.lock, __flags);
>  			break;
> +		}
>  		memcpy(tb->char_buf_ptr + tb->used, chars, space);
>  		memcpy(tb->flag_buf_ptr + tb->used, flags, space);
>  		tb->used += space;
> +		spin_unlock_irqrestore(&tty->buf.lock, __flags);
>  		copied += space;
>  		chars += space;
>  		flags += space;
> @@ -344,13 +375,20 @@ EXPORT_SYMBOL(tty_schedule_flip);
>  int tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars,
>  								size_t size)
>  {
> -	int space = tty_buffer_request_room(tty, size);
> +	int space;
> +	unsigned long flags;
> +	struct tty_buffer *tb;
> +
> +	spin_lock_irqsave(&tty->buf.lock, flags);
> +	space = __tty_buffer_request_room(tty, size);
> +
> +	tb = tty->buf.tail;
>  	if (likely(space)) {
> -		struct tty_buffer *tb = tty->buf.tail;
>  		*chars = tb->char_buf_ptr + tb->used;
>  		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
>  		tb->used += space;
>  	}
> +	spin_unlock_irqrestore(&tty->buf.lock, flags);
>  	return space;
>  }
>  EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
> @@ -374,13 +412,20 @@ EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
>  int tty_prepare_flip_string_flags(struct tty_struct *tty,
>  			unsigned char **chars, char **flags, size_t size)
>  {
> -	int space = tty_buffer_request_room(tty, size);
> +	int space;
> +	unsigned long __flags;
> +	struct tty_buffer *tb;
> +
> +	spin_lock_irqsave(&tty->buf.lock, __flags);
> +	space = __tty_buffer_request_room(tty, size);
> +
> +	tb = tty->buf.tail;
>  	if (likely(space)) {
> -		struct tty_buffer *tb = tty->buf.tail;
>  		*chars = tb->char_buf_ptr + tb->used;
>  		*flags = tb->flag_buf_ptr + tb->used;
>  		tb->used += space;
>  	}
> +	spin_unlock_irqrestore(&tty->buf.lock, __flags);
>  	return space;
>  }
>  EXPORT_SYMBOL_GPL(tty_prepare_flip_string_flags);
> -- 
> 1.7.4.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] tty: hold lock across tty buffer finding and buffer filling
  2012-03-16 20:14 ` gregkh
@ 2012-03-16 21:01   ` Jiri Slaby
  2012-03-30 10:26   ` Tu, Xiaobing
  1 sibling, 0 replies; 6+ messages in thread
From: Jiri Slaby @ 2012-03-16 21:01 UTC (permalink / raw)
  To: gregkh@linuxfoundation.org
  Cc: Tu, Xiaobing, alan@lxorguk.ukuu.org.uk,
	linux-kernel@vger.kernel.org, Zhang, Yanmin, Du, Alek, Zuo, Jiao

On 03/16/2012 09:14 PM, gregkh@linuxfoundation.org wrote:
> Do you have any objections to me applying the patch below?

The only issue with the patch is that I'm not convinced it fixes
tty_prepare_flip_string* properly. I don't think it improves anything there.

thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] tty: hold lock across tty buffer finding and buffer filling
  2012-03-16 20:14 ` gregkh
  2012-03-16 21:01   ` Jiri Slaby
@ 2012-03-30 10:26   ` Tu, Xiaobing
  2012-03-30 10:34     ` Alan Cox
  2012-03-30 14:19     ` gregkh
  1 sibling, 2 replies; 6+ messages in thread
From: Tu, Xiaobing @ 2012-03-30 10:26 UTC (permalink / raw)
  To: gregkh@linuxfoundation.org, Jiri Slaby
  Cc: alan@lxorguk.ukuu.org.uk, linux-kernel@vger.kernel.org,
	Zhang, Yanmin, Du, Alek, Zuo, Jiao

Hi Greg
  Even there still a prepare buffer not protected, the patch is still good to protect rest case, so this patch is still "nice to have"
it makes the situation better. for the not-fixed case, we can have another patch to address that. Do you agree?

Br
Xiaobing


-----Original Message-----
From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org] 
Sent: Saturday, March 17, 2012 4:15 AM
To: Tu, Xiaobing; Jiri Slaby
Cc: alan@lxorguk.ukuu.org.uk; linux-kernel@vger.kernel.org; Zhang, Yanmin; Du, Alek; Zuo, Jiao
Subject: Re: [PATCH v2] tty: hold lock across tty buffer finding and buffer filling

Jiri,

Do you have any objections to me applying the patch below?

thanks,

greg k-h

On Fri, Mar 16, 2012 at 03:00:26AM +0000, Tu, Xiaobing wrote:
> From: Xiaobing Tu <xiaobing.tu@intel.com>
> 
> tty_buffer_request_room is well protected, but while after it returns,  
> it releases the port->lock. tty->buf.tail might be modified by either 
> irq handler or other threads. The patch adds more protection by 
> holding the lock across tty buffer finding and buffer filling.
> Signed-off-by: Alek Du <alek.du@intel.com>
> Signed-off-by: Xiaobing Tu <xiaobing.tu@intel.com>
> ---
>  drivers/tty/tty_buffer.c |   85 +++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 
> 6c9b7cd..91e326f 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -185,25 +185,19 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size)
>  	/* Should possibly check if this fails for the largest buffer we
>  	   have queued and recycle that ? */  }
> -
>  /**
> - *	tty_buffer_request_room		-	grow tty buffer if needed
> + *	__tty_buffer_request_room		-	grow tty buffer if needed
>   *	@tty: tty structure
>   *	@size: size desired
>   *
>   *	Make at least size bytes of linear space available for the tty
>   *	buffer. If we fail return the size we managed to find.
> - *
> - *	Locking: Takes tty->buf.lock
> + *      Locking: Caller must hold tty->buf.lock
>   */
> -int tty_buffer_request_room(struct tty_struct *tty, size_t size)
> +static int __tty_buffer_request_room(struct tty_struct *tty, size_t 
> +size)
>  {
>  	struct tty_buffer *b, *n;
>  	int left;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&tty->buf.lock, flags);
> -
>  	/* OPTIMISATION: We could keep a per tty "zero" sized buffer to
>  	   remove this conditional if its worth it. This would be invisible
>  	   to the callers */
> @@ -225,9 +219,30 @@ int tty_buffer_request_room(struct tty_struct *tty, size_t size)
>  			size = left;
>  	}
>  
> -	spin_unlock_irqrestore(&tty->buf.lock, flags);
>  	return size;
>  }
> +
> +
> +/**
> + *	tty_buffer_request_room		-	grow tty buffer if needed
> + *	@tty: tty structure
> + *	@size: size desired
> + *
> + *	Make at least size bytes of linear space available for the tty
> + *	buffer. If we fail return the size we managed to find.
> + *
> + *	Locking: Takes tty->buf.lock
> + */
> +int tty_buffer_request_room(struct tty_struct *tty, size_t size) {
> +	unsigned long flags;
> +	int length;
> +
> +	spin_lock_irqsave(&tty->buf.lock, flags);
> +	length = __tty_buffer_request_room(tty, size);
> +	spin_unlock_irqrestore(&tty->buf.lock, flags);
> +	return length;
> +}
>  EXPORT_SYMBOL_GPL(tty_buffer_request_room);
>  
>  /**
> @@ -249,14 +264,22 @@ int tty_insert_flip_string_fixed_flag(struct tty_struct *tty,
>  	int copied = 0;
>  	do {
>  		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
> -		int space = tty_buffer_request_room(tty, goal);
> -		struct tty_buffer *tb = tty->buf.tail;
> +		int space;
> +		unsigned long flags;
> +		struct tty_buffer *tb;
> +
> +		spin_lock_irqsave(&tty->buf.lock, flags);
> +		space = __tty_buffer_request_room(tty, goal);
> +		tb = tty->buf.tail;
>  		/* If there is no space then tb may be NULL */
> -		if (unlikely(space == 0))
> +		if (unlikely(space == 0)) {
> +			spin_unlock_irqrestore(&tty->buf.lock, flags);
>  			break;
> +		}
>  		memcpy(tb->char_buf_ptr + tb->used, chars, space);
>  		memset(tb->flag_buf_ptr + tb->used, flag, space);
>  		tb->used += space;
> +		spin_unlock_irqrestore(&tty->buf.lock, flags);
>  		copied += space;
>  		chars += space;
>  		/* There is a small chance that we need to split the data over @@ 
> -286,14 +309,22 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
>  	int copied = 0;
>  	do {
>  		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
> -		int space = tty_buffer_request_room(tty, goal);
> -		struct tty_buffer *tb = tty->buf.tail;
> +		int space;
> +		unsigned long __flags;
> +		struct tty_buffer *tb;
> +
> +		spin_lock_irqsave(&tty->buf.lock, __flags);
> +		space = __tty_buffer_request_room(tty, goal);
> +		tb = tty->buf.tail;
>  		/* If there is no space then tb may be NULL */
> -		if (unlikely(space == 0))
> +		if (unlikely(space == 0)) {
> +			spin_unlock_irqrestore(&tty->buf.lock, __flags);
>  			break;
> +		}
>  		memcpy(tb->char_buf_ptr + tb->used, chars, space);
>  		memcpy(tb->flag_buf_ptr + tb->used, flags, space);
>  		tb->used += space;
> +		spin_unlock_irqrestore(&tty->buf.lock, __flags);
>  		copied += space;
>  		chars += space;
>  		flags += space;
> @@ -344,13 +375,20 @@ EXPORT_SYMBOL(tty_schedule_flip);  int 
> tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars,
>  								size_t size)
>  {
> -	int space = tty_buffer_request_room(tty, size);
> +	int space;
> +	unsigned long flags;
> +	struct tty_buffer *tb;
> +
> +	spin_lock_irqsave(&tty->buf.lock, flags);
> +	space = __tty_buffer_request_room(tty, size);
> +
> +	tb = tty->buf.tail;
>  	if (likely(space)) {
> -		struct tty_buffer *tb = tty->buf.tail;
>  		*chars = tb->char_buf_ptr + tb->used;
>  		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
>  		tb->used += space;
>  	}
> +	spin_unlock_irqrestore(&tty->buf.lock, flags);
>  	return space;
>  }
>  EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
> @@ -374,13 +412,20 @@ EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
>  int tty_prepare_flip_string_flags(struct tty_struct *tty,
>  			unsigned char **chars, char **flags, size_t size)  {
> -	int space = tty_buffer_request_room(tty, size);
> +	int space;
> +	unsigned long __flags;
> +	struct tty_buffer *tb;
> +
> +	spin_lock_irqsave(&tty->buf.lock, __flags);
> +	space = __tty_buffer_request_room(tty, size);
> +
> +	tb = tty->buf.tail;
>  	if (likely(space)) {
> -		struct tty_buffer *tb = tty->buf.tail;
>  		*chars = tb->char_buf_ptr + tb->used;
>  		*flags = tb->flag_buf_ptr + tb->used;
>  		tb->used += space;
>  	}
> +	spin_unlock_irqrestore(&tty->buf.lock, __flags);
>  	return space;
>  }
>  EXPORT_SYMBOL_GPL(tty_prepare_flip_string_flags);
> --
> 1.7.4.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] tty: hold lock across tty buffer finding and buffer filling
  2012-03-30 10:26   ` Tu, Xiaobing
@ 2012-03-30 10:34     ` Alan Cox
  2012-03-30 14:19     ` gregkh
  1 sibling, 0 replies; 6+ messages in thread
From: Alan Cox @ 2012-03-30 10:34 UTC (permalink / raw)
  To: Tu, Xiaobing
  Cc: gregkh@linuxfoundation.org, Jiri Slaby,
	linux-kernel@vger.kernel.org, Zhang, Yanmin, Du, Alek, Zuo, Jiao

On Fri, 30 Mar 2012 10:26:27 +0000
"Tu, Xiaobing" <xiaobing.tu@intel.com> wrote:

> Hi Greg
>   Even there still a prepare buffer not protected, the patch is still good to protect rest case, so this patch is still "nice to have"
> it makes the situation better. for the not-fixed case, we can have another patch to address that. Do you agree?

Can't speak for Greg (who I suspect is busy with the merge window to
Linus) but I think this is the right thing to do once the merge window is
over.  The unfixed case needs other things to happen first, such as the
removal of tty_lock/tty_unlock. At that point much of this should become
much simpler.

Alan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] tty: hold lock across tty buffer finding and buffer filling
  2012-03-30 10:26   ` Tu, Xiaobing
  2012-03-30 10:34     ` Alan Cox
@ 2012-03-30 14:19     ` gregkh
  1 sibling, 0 replies; 6+ messages in thread
From: gregkh @ 2012-03-30 14:19 UTC (permalink / raw)
  To: Tu, Xiaobing
  Cc: Jiri Slaby, alan@lxorguk.ukuu.org.uk,
	linux-kernel@vger.kernel.org, Zhang, Yanmin, Du, Alek, Zuo, Jiao

On Fri, Mar 30, 2012 at 10:26:27AM +0000, Tu, Xiaobing wrote:
> Hi Greg
>   Even there still a prepare buffer not protected, the patch is still
> good to protect rest case, so this patch is still "nice to have" it
> makes the situation better. for the not-fixed case, we can have
> another patch to address that. Do you agree?

As Alan says, it's in the middle of the merge window right now, and I
can't take any new patches until 3.4-rc1 is out.  It is in my queue to
review when the merge window is over.

patience please,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-03-30 14:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-16  3:00 [PATCH v2] tty: hold lock across tty buffer finding and buffer filling Tu, Xiaobing
2012-03-16 20:14 ` gregkh
2012-03-16 21:01   ` Jiri Slaby
2012-03-30 10:26   ` Tu, Xiaobing
2012-03-30 10:34     ` Alan Cox
2012-03-30 14:19     ` gregkh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox