Re: [PATCH V2] n_gsm: race between ld close and gsmtty open

2013-11-25 Thread channing 
On Mon, 2013-11-25 at 19:16 -0800, Greg KH wrote:
> > > 
> > > What is different from the previous version?  That information needs to
> > > be somewhere, otherwise I'm just going to guess and say this is the same
> > > as your last one, which was incorrect.
> > The difference with previous one is to use a mutex instead of spin lock
> > to avoid race, purpose is to avoid sleep in atomic context. I've also
> > updated commit a little as above.
> 
> Then be explicit as to what has changed somewhere.  We deal with
> thousands of patches a week, we can not know that you changed one
> sentance in a patch description of a few hundred lines long to know you
> made a change to the patch itself as well...
OK, in next patch set, I'll highlight difference with previous patch
set.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 V2] n_gsm: race between ld close and gsmtty open

2013-11-25 Thread channing 
On Mon, 2013-11-25 at 18:54 -0800, Greg KH wrote:
> On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote:

> > This patch is try to avoid it by:
> > 
> > 1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release() 
> > run in
> > parallel with gsmtty_install();
The commit is updated here than formal patch set: we use mutex lock in
patch V2, while use spin lock in patch V1.

> > 
> > 2) Increase dlci's ref count in gsmtty_install() instead of in 
> > gsmtty_open(), the
> > purpose is to prevent gsm_dlci_release() releasing dlci after 
> > gsmtty_install()
> > allocats dlci but before gsmtty_open increases dlci's ref count;
> > 
> > 3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this 
> > is the
> > opposite process of step 2).
> > 
> > Signed-off-by: Chao Bi 
> > Signed-off-by: Greg Kroah-Hartman 
> 
> I have not signed off on this additional patch.
> 
> What is different from the previous version?  That information needs to
> be somewhere, otherwise I'm just going to guess and say this is the same
> as your last one, which was incorrect.
The difference with previous one is to use a mutex instead of spin lock
to avoid race, purpose is to avoid sleep in atomic context. I've also
updated commit a little as above.

> 
> Also, please fix your "From:" line in your email client to match your
> Signed-off-by: line, or else add the proper "From:" line to your patch,
> as the Documentation/SubmittingPatches file says.
> 
> Care to try again?
Yes, I'll correct it. thanks.
> 
> greg k-h


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

[PATCH V2] n_gsm: race between ld close and gsmtty open

2013-11-25 Thread channing 

ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
is opening in parallel.

Here are race cases we found recently in test:

CASE #1

releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(gsmttyB), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[B]) -
 |   |
 gsm_dlci_free(dlci[B])-
 |   |
   -   gsmtty_open(gsmttyB)

 gsmtty_open()
 {
 struct gsm_dlci *dlci = tty->driver_data; => here it uses dlci[B]
 ...
 }

 In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
=

CASE #2
=
releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
 |   |
   - gsmtty_open(gsmttyB) fail
 |   |
   -   tty_release(gsmttyB)
 |   |
   -   gsmtty_close(gsmttyB)
 |   |
   -gsmtty_detach_dlci(dlci[B])
 |   |
   - dlci_put(dlci[B])
 |   |
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[0]) -
 |   |
 gsm_dlci_free(dlci[0])-
 |   |
   - dlci_put(dlci[0])

 In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
 then hit panic.
=

IMHO, n_gsm tty operations would refer released ldisc,  as long as
gsm_dlci_release() has chance to release ldisc data when some gsmtty operations
are ongoing..

This patch is try to avoid it by:

1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release() run 
in
parallel with gsmtty_install();

2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), 
the
purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
allocats dlci but before gsmtty_open increases dlci's ref count;

3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this is 
the
opposite process of step 2).

Signed-off-by: Chao Bi 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/tty/n_gsm.c |   39 +--
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c0f76da..d514396 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -194,6 +194,7 @@ struct gsm_control {
 struct gsm_mux {
struct tty_struct *tty; /* The tty our ldisc is bound to */
spinlock_t lock;
+   struct mutex mutex;
unsigned int num;
struct kref ref;
 
@@ -2054,9 +2055,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
dlci->state == DLCI_CLOSED);
}
/* Free up any link layer users */
+   mutex_lock(>mutex);
for (i = 0; i < NUM_DLCI; i++)
if (gsm->dlci[i])
gsm_dlci_release(gsm->dlci[i]);
+   mutex_unlock(>mutex);
/* Now wipe the queues */
list_for_each_entry_safe(txq, ntxq, >tx_list, list)
kfree(txq);
@@ -2170,6 +2173,7 @@ struct gsm_mux *gsm_alloc_mux(void)
return NULL;
}
spin_lock_init(>lock);
+   mutex_init(>mutex);
kref_init(>ref);
INIT_LIST_HEAD(>tx_list);
 
@@ -2909,23 +2913,33 @@ static int gsmtty_install(struct tty_driver *driver, 
struct tty_struct *tty)
This is ok from a locking
perspective as we don't have to worry about this
if DLCI0 is lost */
-   if (gsm->dlci[0] && gsm->dlci[0]->state != DLCI_OPEN)
+   mutex_lock(>mutex);
+   if (gsm->dlci[0] && gsm->dlci[0]->state !=

[PATCH V2] n_gsm: race between ld close and gsmtty open

2013-11-25 Thread channing 

ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
is opening in parallel.

Here are race cases we found recently in test:

CASE #1

releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(gsmttyB), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[B]) -
 |   |
 gsm_dlci_free(dlci[B])-
 |   |
   -   gsmtty_open(gsmttyB)

 gsmtty_open()
 {
 struct gsm_dlci *dlci = tty-driver_data; = here it uses dlci[B]
 ...
 }

 In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
=

CASE #2
=
releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
 |   |
   - gsmtty_open(gsmttyB) fail
 |   |
   -   tty_release(gsmttyB)
 |   |
   -   gsmtty_close(gsmttyB)
 |   |
   -gsmtty_detach_dlci(dlci[B])
 |   |
   - dlci_put(dlci[B])
 |   |
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[0]) -
 |   |
 gsm_dlci_free(dlci[0])-
 |   |
   - dlci_put(dlci[0])

 In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
 then hit panic.
=

IMHO, n_gsm tty operations would refer released ldisc,  as long as
gsm_dlci_release() has chance to release ldisc data when some gsmtty operations
are ongoing..

This patch is try to avoid it by:

1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release() run 
in
parallel with gsmtty_install();

2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), 
the
purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
allocats dlci but before gsmtty_open increases dlci's ref count;

3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this is 
the
opposite process of step 2).

Signed-off-by: Chao Bi chao...@intel.com
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
---
 drivers/tty/n_gsm.c |   39 +--
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c0f76da..d514396 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -194,6 +194,7 @@ struct gsm_control {
 struct gsm_mux {
struct tty_struct *tty; /* The tty our ldisc is bound to */
spinlock_t lock;
+   struct mutex mutex;
unsigned int num;
struct kref ref;
 
@@ -2054,9 +2055,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
dlci-state == DLCI_CLOSED);
}
/* Free up any link layer users */
+   mutex_lock(gsm-mutex);
for (i = 0; i  NUM_DLCI; i++)
if (gsm-dlci[i])
gsm_dlci_release(gsm-dlci[i]);
+   mutex_unlock(gsm-mutex);
/* Now wipe the queues */
list_for_each_entry_safe(txq, ntxq, gsm-tx_list, list)
kfree(txq);
@@ -2170,6 +2173,7 @@ struct gsm_mux *gsm_alloc_mux(void)
return NULL;
}
spin_lock_init(gsm-lock);
+   mutex_init(gsm-mutex);
kref_init(gsm-ref);
INIT_LIST_HEAD(gsm-tx_list);
 
@@ -2909,23 +2913,33 @@ static int gsmtty_install(struct tty_driver *driver, 
struct tty_struct *tty)
This is ok from a locking
perspective as we don't have to worry about this
if DLCI0 is lost */
-   if (gsm-dlci[0]  gsm-dlci[0]-state != DLCI_OPEN)
+   mutex_lock(gsm-mutex);
+

Re: [PATCH V2] n_gsm: race between ld close and gsmtty open

2013-11-25 Thread channing 
On Mon, 2013-11-25 at 18:54 -0800, Greg KH wrote:
 On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote:

  This patch is try to avoid it by:
  
  1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release() 
  run in
  parallel with gsmtty_install();
The commit is updated here than formal patch set: we use mutex lock in
patch V2, while use spin lock in patch V1.

  
  2) Increase dlci's ref count in gsmtty_install() instead of in 
  gsmtty_open(), the
  purpose is to prevent gsm_dlci_release() releasing dlci after 
  gsmtty_install()
  allocats dlci but before gsmtty_open increases dlci's ref count;
  
  3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this 
  is the
  opposite process of step 2).
  
  Signed-off-by: Chao Bi chao...@intel.com
  Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
 
 I have not signed off on this additional patch.
 
 What is different from the previous version?  That information needs to
 be somewhere, otherwise I'm just going to guess and say this is the same
 as your last one, which was incorrect.
The difference with previous one is to use a mutex instead of spin lock
to avoid race, purpose is to avoid sleep in atomic context. I've also
updated commit a little as above.

 
 Also, please fix your From: line in your email client to match your
 Signed-off-by: line, or else add the proper From: line to your patch,
 as the Documentation/SubmittingPatches file says.
 
 Care to try again?
Yes, I'll correct it. thanks.
 
 greg k-h


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 V2] n_gsm: race between ld close and gsmtty open

2013-11-25 Thread channing 
On Mon, 2013-11-25 at 19:16 -0800, Greg KH wrote:
   
   What is different from the previous version?  That information needs to
   be somewhere, otherwise I'm just going to guess and say this is the same
   as your last one, which was incorrect.
  The difference with previous one is to use a mutex instead of spin lock
  to avoid race, purpose is to avoid sleep in atomic context. I've also
  updated commit a little as above.
 
 Then be explicit as to what has changed somewhere.  We deal with
 thousands of patches a week, we can not know that you changed one
 sentance in a patch description of a few hundred lines long to know you
 made a change to the patch itself as well...
OK, in next patch set, I'll highlight difference with previous patch
set.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] n_gsm: race between ld close and gsmtty open

2013-10-17 Thread channing 
On Thu, 2013-10-17 at 10:14 -0700, Greg KH wrote:
> On Thu, Oct 17, 2013 at 11:29:10AM +0800, channing wrote:
> > 
> > ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
> > to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
> > is opening in parallel.
> > 
> > Here are some of race cases we found recently in test:
> 
> Are these still an issue with 3.12-rc4?  Lots of pty rework has gone
> into there that should help to resolve some of these problems.
> 

Hi Greg,
I viewed 3.12-rc4 and IMHO it still has this issue, Please correct me if I'm 
wrong.
This patch was based on 3.12-rc4, following is version info of my code base:

linux/Makefile:
VERSION = 3
PATCHLEVEL = 12
SUBLEVEL = 0
EXTRAVERSION = -rc4

And this version of patch has one line not correct, I updated it in another 
mail 
with title "[PATCH v2]n_gsm:...", would you please check?

thanks a lot,
chao

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

[PATCH v2] n_gsm: race between ld close and gsmtty open

2013-10-17 Thread channing 

ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
is opening in parallel.

Here are race cases we found recently in test:

CASE #1

releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(gsmttyB), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[B]) -
 |   |
 gsm_dlci_free(dlci[B])-
 |   |
   -   gsmtty_open(gsmttyB)

 gsmtty_open()
 {
 struct gsm_dlci *dlci = tty->driver_data; => here it uses dlci[B]
 ...
 }

 In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
=

CASE #2
=
releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
 |   |
   - gsmtty_open(gsmttyB) fail
 |   |
   -   tty_release(gsmttyB)
 |   |
   -   gsmtty_close(gsmttyB)
 |   |
   -gsmtty_detach_dlci(dlci[B])
 |   |
   - dlci_put(dlci[B])
 |   |
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[0]) -
 |   |
 gsm_dlci_free(dlci[0])-
 |   |
   - dlci_put(dlci[0])

 In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
 then hit panic.
=

IMHO, n_gsm tty operations would refer released ldisc,  as long as
gsm_dlci_release() has chance to release ldisc data when some gsmtty operations
are not completed..

This patch is try to avoid it by:

1) in n_gsm driver, use a global gsm spin lock to avoid gsm_dlci_release() run 
in
parallel with gsmtty_install();

2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), 
the
purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
allocats dlci but before gsmtty_open increases dlci's ref count;

3) Decrease dlci's ref count in gsmtty_remove(), which is a tty framework api, 
and
this is the opposite process of step 2).

Signed-off-by: Chao Bi 
---
 drivers/tty/n_gsm.c |   37 +++--
 1 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c0f76da..069bfd6 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2054,9 +2054,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
dlci->state == DLCI_CLOSED);
}
/* Free up any link layer users */
+   spin_lock(>lock);
for (i = 0; i < NUM_DLCI; i++)
if (gsm->dlci[i])
gsm_dlci_release(gsm->dlci[i]);
+   spin_unlock(>lock);
/* Now wipe the queues */
list_for_each_entry_safe(txq, ntxq, >tx_list, list)
kfree(txq);
@@ -2909,23 +2911,33 @@ static int gsmtty_install(struct tty_driver *driver, 
struct tty_struct *tty)
This is ok from a locking
perspective as we don't have to worry about this
if DLCI0 is lost */
-   if (gsm->dlci[0] && gsm->dlci[0]->state != DLCI_OPEN)
+   spin_lock(>lock);
+   if (gsm->dlci[0] && gsm->dlci[0]->state != DLCI_OPEN) {
+   spin_unlock(>lock);
return -EL2NSYNC;
+   }
dlci = gsm->dlci[line];
if (dlci == NULL) {
alloc = true;
dlci = gsm_dlci_alloc(gsm, line);
}
-   if (dlci == NULL)
+   if (dlci == NULL) {
+   spin_unlock(>lock);
return -ENOMEM;
+   }
ret = tty_port_install(>port, driver, tty);
if (ret) {
if (alloc)

[PATCH v2] n_gsm: race between ld close and gsmtty open

2013-10-17 Thread channing 

ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
is opening in parallel.

Here are race cases we found recently in test:

CASE #1

releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(gsmttyB), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[B]) -
 |   |
 gsm_dlci_free(dlci[B])-
 |   |
   -   gsmtty_open(gsmttyB)

 gsmtty_open()
 {
 struct gsm_dlci *dlci = tty-driver_data; = here it uses dlci[B]
 ...
 }

 In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
=

CASE #2
=
releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
 |   |
   - gsmtty_open(gsmttyB) fail
 |   |
   -   tty_release(gsmttyB)
 |   |
   -   gsmtty_close(gsmttyB)
 |   |
   -gsmtty_detach_dlci(dlci[B])
 |   |
   - dlci_put(dlci[B])
 |   |
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[0]) -
 |   |
 gsm_dlci_free(dlci[0])-
 |   |
   - dlci_put(dlci[0])

 In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
 then hit panic.
=

IMHO, n_gsm tty operations would refer released ldisc,  as long as
gsm_dlci_release() has chance to release ldisc data when some gsmtty operations
are not completed..

This patch is try to avoid it by:

1) in n_gsm driver, use a global gsm spin lock to avoid gsm_dlci_release() run 
in
parallel with gsmtty_install();

2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), 
the
purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
allocats dlci but before gsmtty_open increases dlci's ref count;

3) Decrease dlci's ref count in gsmtty_remove(), which is a tty framework api, 
and
this is the opposite process of step 2).

Signed-off-by: Chao Bi chao...@intel.com
---
 drivers/tty/n_gsm.c |   37 +++--
 1 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c0f76da..069bfd6 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2054,9 +2054,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
dlci-state == DLCI_CLOSED);
}
/* Free up any link layer users */
+   spin_lock(gsm-lock);
for (i = 0; i  NUM_DLCI; i++)
if (gsm-dlci[i])
gsm_dlci_release(gsm-dlci[i]);
+   spin_unlock(gsm-lock);
/* Now wipe the queues */
list_for_each_entry_safe(txq, ntxq, gsm-tx_list, list)
kfree(txq);
@@ -2909,23 +2911,33 @@ static int gsmtty_install(struct tty_driver *driver, 
struct tty_struct *tty)
This is ok from a locking
perspective as we don't have to worry about this
if DLCI0 is lost */
-   if (gsm-dlci[0]  gsm-dlci[0]-state != DLCI_OPEN)
+   spin_lock(gsm-lock);
+   if (gsm-dlci[0]  gsm-dlci[0]-state != DLCI_OPEN) {
+   spin_unlock(gsm-lock);
return -EL2NSYNC;
+   }
dlci = gsm-dlci[line];
if (dlci == NULL) {
alloc = true;
dlci = gsm_dlci_alloc(gsm, line);
}
-   if (dlci == NULL)
+   if (dlci == NULL) {
+   spin_unlock(gsm-lock);
return -ENOMEM;
+   }
ret = tty_port_install(dlci-port, driver, tty);
if (ret) {
if

Re: [PATCH] n_gsm: race between ld close and gsmtty open

2013-10-17 Thread channing 
On Thu, 2013-10-17 at 10:14 -0700, Greg KH wrote:
 On Thu, Oct 17, 2013 at 11:29:10AM +0800, channing wrote:
  
  ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
  to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
  is opening in parallel.
  
  Here are some of race cases we found recently in test:
 
 Are these still an issue with 3.12-rc4?  Lots of pty rework has gone
 into there that should help to resolve some of these problems.
 

Hi Greg,
I viewed 3.12-rc4 and IMHO it still has this issue, Please correct me if I'm 
wrong.
This patch was based on 3.12-rc4, following is version info of my code base:

linux/Makefile:
VERSION = 3
PATCHLEVEL = 12
SUBLEVEL = 0
EXTRAVERSION = -rc4

And this version of patch has one line not correct, I updated it in another 
mail 
with title [PATCH v2]n_gsm:..., would you please check?

thanks a lot,
chao

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

[PATCH] n_gsm: race between ld close and gsmtty open

2013-10-16 Thread channing 

ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
is opening in parallel.

Here are some of race cases we found recently in test:

CASE #1

releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(gsmttyB), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[B]) -
 |   |
 gsm_dlci_free(dlci[B])-
 |   |
   -   gsmtty_open(gsmttyB)

 gsmtty_open()
 {
 struct gsm_dlci *dlci = tty->driver_data; => here it uses dlci[B]
 ...
 }

 In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
=

CASE #2
=
releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
 |   |
   - gsmtty_open(gsmttyB) fail
 |   |
   -   tty_release(gsmttyB)
 |   |
   -   gsmtty_close(gsmttyB)
 |   |
   -gsmtty_detach_dlci(dlci[B])
 |   |
   - dlci_put(dlci[B])
 |   |
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[0]) -
 |   |
 gsm_dlci_free(dlci[0])-
 |   |
   - dlci_put(dlci[0])

 In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
 then hit panic.
=

IMHO, n_gsm tty operations would refer released ldisc,  as long as
gsm_dlci_release() has chance to release ldisc data when some gsmtty operations
are not completed..

This patch is try to avoid it by:

1) in n_gsm driver, use a global gsm spin lock to avoid gsm_dlci_release() run 
in
parallel with gsmtty_install();

2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), 
the
purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
allocats dlci but before gsmtty_open increases dlci's ref count;

3) Decrease dlci's ref count in gsmtty_remove(), which is a tty framework api, 
and
this is the opposite process of step 2).

Signed-off-by: Chao Bi 
---
 drivers/tty/n_gsm.c |   37 +++--
 1 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c0f76da..41ef7d7 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2054,9 +2054,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
dlci->state == DLCI_CLOSED);
}
/* Free up any link layer users */
+   spin_lock(>lock);
for (i = 0; i < NUM_DLCI; i++)
if (gsm->dlci[i])
gsm_dlci_release(gsm->dlci[i]);
+   spin_unlock(>lock);
/* Now wipe the queues */
list_for_each_entry_safe(txq, ntxq, >tx_list, list)
kfree(txq);
@@ -2909,23 +2911,33 @@ static int gsmtty_install(struct tty_driver *driver, 
struct tty_struct *tty)
This is ok from a locking
perspective as we don't have to worry about this
if DLCI0 is lost */
-   if (gsm->dlci[0] && gsm->dlci[0]->state != DLCI_OPEN)
+   spin_lock(>lock);
+   if (gsm->dlci[0] && gsm->dlci[0]->state != DLCI_OPEN) {
+   spin_unlock(>lock);
return -EL2NSYNC;
+   }
dlci = gsm->dlci[line];
if (dlci == NULL) {
alloc = true;
dlci = gsm_dlci_alloc(gsm, line);
}
-   if (dlci == NULL)
+   if (dlci == NULL) {
+   spin_unlock(>lock);
return -ENOMEM;
+   }
ret = tty_port_install(>port, driver, tty);
if (ret) {
if (alloc)

[PATCH] n_gsm: race between ld close and gsmtty open

2013-10-16 Thread channing 

ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
is opening in parallel.

Here are some of race cases we found recently in test:

CASE #1

releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(gsmttyB), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[B]) -
 |   |
 gsm_dlci_free(dlci[B])-
 |   |
   -   gsmtty_open(gsmttyB)

 gsmtty_open()
 {
 struct gsm_dlci *dlci = tty-driver_data; = here it uses dlci[B]
 ...
 }

 In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
=

CASE #2
=
releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(), as below:

 tty_release(ttyA)  tty_open(gsmttyB)
 |   |
   -   gsmtty_install(gsmttyB)
 |   |
   -gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
 |   |
   - gsmtty_open(gsmttyB) fail
 |   |
   -   tty_release(gsmttyB)
 |   |
   -   gsmtty_close(gsmttyB)
 |   |
   -gsmtty_detach_dlci(dlci[B])
 |   |
   - dlci_put(dlci[B])
 |   |
 tty_ldisc_release(ttyA)   -
 |   |
 gsm_dlci_release(dlci[0]) -
 |   |
 gsm_dlci_free(dlci[0])-
 |   |
   - dlci_put(dlci[0])

 In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
 then hit panic.
=

IMHO, n_gsm tty operations would refer released ldisc,  as long as
gsm_dlci_release() has chance to release ldisc data when some gsmtty operations
are not completed..

This patch is try to avoid it by:

1) in n_gsm driver, use a global gsm spin lock to avoid gsm_dlci_release() run 
in
parallel with gsmtty_install();

2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), 
the
purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
allocats dlci but before gsmtty_open increases dlci's ref count;

3) Decrease dlci's ref count in gsmtty_remove(), which is a tty framework api, 
and
this is the opposite process of step 2).

Signed-off-by: Chao Bi chao...@intel.com
---
 drivers/tty/n_gsm.c |   37 +++--
 1 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c0f76da..41ef7d7 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2054,9 +2054,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
dlci-state == DLCI_CLOSED);
}
/* Free up any link layer users */
+   spin_lock(gsm-lock);
for (i = 0; i  NUM_DLCI; i++)
if (gsm-dlci[i])
gsm_dlci_release(gsm-dlci[i]);
+   spin_unlock(gsm-lock);
/* Now wipe the queues */
list_for_each_entry_safe(txq, ntxq, gsm-tx_list, list)
kfree(txq);
@@ -2909,23 +2911,33 @@ static int gsmtty_install(struct tty_driver *driver, 
struct tty_struct *tty)
This is ok from a locking
perspective as we don't have to worry about this
if DLCI0 is lost */
-   if (gsm-dlci[0]  gsm-dlci[0]-state != DLCI_OPEN)
+   spin_lock(gsm-lock);
+   if (gsm-dlci[0]  gsm-dlci[0]-state != DLCI_OPEN) {
+   spin_unlock(gsm-lock);
return -EL2NSYNC;
+   }
dlci = gsm-dlci[line];
if (dlci == NULL) {
alloc = true;
dlci = gsm_dlci_alloc(gsm, line);
}
-   if (dlci == NULL)
+   if (dlci == NULL) {
+   spin_unlock(gsm-lock);
return -ENOMEM;
+   }
ret = tty_port_install(dlci-port, driver, tty);
if (ret) {

Re: [PATCH] TTY: memory leakage in tty_buffer_find()

2013-06-26 Thread channing 
On Wed, 2013-06-26 at 08:43 -0400, Peter Hurley wrote:
> On 06/26/2013 04:51 AM, channing wrote:
> >
> > In tty_buffer_find(), it scans all tty buffers in
> > free buffer queue, if it finds matched one,
> > tty->buf.free will point to matched one's next buffer,
> > so tty buffers that ahead of matched one are removed
> > from free queue, they will never be used but they
> > are not released, then memory leak happen.
> 
> Actually, the whole scan loop is wrong: only tty buffers of
> size 256 are added to the free list.
> 
Agree that currently all tty buffers of free list are with size
of 256, but are we sure that the scan loop in tty_buffer_find()
is wrong and should abandon? From the purpose of tty_buffer_find(), 
I understand it shall scan the free list, but now it doesn't make 
sense because tty_buffer_free() makes all the free list buffers
with size of 256:

tty_buffer_free()
{
if (b->size >= 512)
kfree(b);
}

I don't know why it's 512? looks like a hard configuration?
Can we make it configurable instead of a fixed value?

I understand, although no memory leak, there is logic mess between 
tty_buffer_find() and tty_buffer_free(), either one shall make 
change to keep accordance? 
which one to make change might depends on original purpose of 
creating the free list. I tried to find the history of tty_buffer_free(),
but "512" is here since 2.6.32.61, I didn't find older version.

> So this can't leak because a buffer will never be found
> mid-list.
> 
> Greg has a patch series from me that reduces this but it's not
> yet in next.
> 
> Regards,
> Peter Hurley
> 


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

[PATCH] TTY: memory leakage in tty_buffer_find()

2013-06-26 Thread channing 

In tty_buffer_find(), it scans all tty buffers in
free buffer queue, if it finds matched one,
tty->buf.free will point to matched one's next buffer,
so tty buffers that ahead of matched one are removed
from free queue, they will never be used but they
are not released, then memory leak happen.

This patch is to make tty_buffer_find() only extract
the matched tty buffer, and keep others left inside
free queue, so that they could be found and used next
time.

Signed-off-by: Chao Bi 
---
 drivers/tty/tty_buffer.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..7b10f7a 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -178,10 +178,14 @@ void tty_buffer_flush(struct tty_struct *tty)
 static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size)
 {
struct tty_buffer **tbh = >buf.free;
+   struct tty_buffer *prev = NULL;
while ((*tbh) != NULL) {
struct tty_buffer *t = *tbh;
if (t->size >= size) {
-   *tbh = t->next;
+   if (prev == NULL)
+   *tbh = t->next;
+   else
+   prev->next = t->next;
t->next = NULL;
t->used = 0;
t->commit = 0;
@@ -189,6 +193,7 @@ static struct tty_buffer *tty_buffer_find(struct tty_port 
*port, size_t size)
port->buf.memory_used += t->size;
return t;
}
+   prev = t;
tbh = &((*tbh)->next);
}
/* Round the buffer size out */
-- 
1.7.1



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

[PATCH] TTY: fix memory leakage in tty_buffer_find()

2013-06-26 Thread channing 

In tty_buffer_find(), it scans all tty buffers in
free buffer queue, if it finds matched one,
tty->buf.free will point to matched one's next buffer,
so tty buffers that ahead of matched one are removed
from free queue, they will never be used but they
are not released, then memory leak happen.

This patch is to make tty_buffer_find() only extract
the matched tty buffer, and keep others left inside
free queue, so that they could be found and used next
time.

Signed-off-by: Chao Bi 
---
 drivers/tty/tty_buffer.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..d587742 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -178,10 +178,14 @@ void tty_buffer_flush(struct tty_struct *tty)
 static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size)
 {
struct tty_buffer **tbh = >buf.free;
+   struct tty_buffer *prev = port->buf.free;
while ((*tbh) != NULL) {
struct tty_buffer *t = *tbh;
if (t->size >= size) {
-   *tbh = t->next;
+   if (prev == NULL)
+   *tbh = t->next;
+   else
+   prev->next = t->next;
t->next = NULL;
t->used = 0;
t->commit = 0;
@@ -189,6 +193,7 @@ static struct tty_buffer *tty_buffer_find(struct tty_port 
*port, size_t size)
port->buf.memory_used += t->size;
return t;
}
+   prev = t;
tbh = &((*tbh)->next);
}
/* Round the buffer size out */
-- 
1.7.1



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

[PATCH] TTY: fix memory leakage in tty_buffer_find()

2013-06-26 Thread channing 

In tty_buffer_find(), it scans all tty buffers in
free buffer queue, if it finds matched one,
tty-buf.free will point to matched one's next buffer,
so tty buffers that ahead of matched one are removed
from free queue, they will never be used but they
are not released, then memory leak happen.

This patch is to make tty_buffer_find() only extract
the matched tty buffer, and keep others left inside
free queue, so that they could be found and used next
time.

Signed-off-by: Chao Bi chao...@intel.com
---
 drivers/tty/tty_buffer.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..d587742 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -178,10 +178,14 @@ void tty_buffer_flush(struct tty_struct *tty)
 static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size)
 {
struct tty_buffer **tbh = port-buf.free;
+   struct tty_buffer *prev = port-buf.free;
while ((*tbh) != NULL) {
struct tty_buffer *t = *tbh;
if (t-size = size) {
-   *tbh = t-next;
+   if (prev == NULL)
+   *tbh = t-next;
+   else
+   prev-next = t-next;
t-next = NULL;
t-used = 0;
t-commit = 0;
@@ -189,6 +193,7 @@ static struct tty_buffer *tty_buffer_find(struct tty_port 
*port, size_t size)
port-buf.memory_used += t-size;
return t;
}
+   prev = t;
tbh = ((*tbh)-next);
}
/* Round the buffer size out */
-- 
1.7.1



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

[PATCH] TTY: memory leakage in tty_buffer_find()

2013-06-26 Thread channing 

In tty_buffer_find(), it scans all tty buffers in
free buffer queue, if it finds matched one,
tty-buf.free will point to matched one's next buffer,
so tty buffers that ahead of matched one are removed
from free queue, they will never be used but they
are not released, then memory leak happen.

This patch is to make tty_buffer_find() only extract
the matched tty buffer, and keep others left inside
free queue, so that they could be found and used next
time.

Signed-off-by: Chao Bi chao...@intel.com
---
 drivers/tty/tty_buffer.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..7b10f7a 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -178,10 +178,14 @@ void tty_buffer_flush(struct tty_struct *tty)
 static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size)
 {
struct tty_buffer **tbh = port-buf.free;
+   struct tty_buffer *prev = NULL;
while ((*tbh) != NULL) {
struct tty_buffer *t = *tbh;
if (t-size = size) {
-   *tbh = t-next;
+   if (prev == NULL)
+   *tbh = t-next;
+   else
+   prev-next = t-next;
t-next = NULL;
t-used = 0;
t-commit = 0;
@@ -189,6 +193,7 @@ static struct tty_buffer *tty_buffer_find(struct tty_port 
*port, size_t size)
port-buf.memory_used += t-size;
return t;
}
+   prev = t;
tbh = ((*tbh)-next);
}
/* Round the buffer size out */
-- 
1.7.1



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] TTY: memory leakage in tty_buffer_find()

2013-06-26 Thread channing 
On Wed, 2013-06-26 at 08:43 -0400, Peter Hurley wrote:
 On 06/26/2013 04:51 AM, channing wrote:
 
  In tty_buffer_find(), it scans all tty buffers in
  free buffer queue, if it finds matched one,
  tty-buf.free will point to matched one's next buffer,
  so tty buffers that ahead of matched one are removed
  from free queue, they will never be used but they
  are not released, then memory leak happen.
 
 Actually, the whole scan loop is wrong: only tty buffers of
 size 256 are added to the free list.
 
Agree that currently all tty buffers of free list are with size
of 256, but are we sure that the scan loop in tty_buffer_find()
is wrong and should abandon? From the purpose of tty_buffer_find(), 
I understand it shall scan the free list, but now it doesn't make 
sense because tty_buffer_free() makes all the free list buffers
with size of 256:

tty_buffer_free()
{
if (b-size = 512)
kfree(b);
}

I don't know why it's 512? looks like a hard configuration?
Can we make it configurable instead of a fixed value?

I understand, although no memory leak, there is logic mess between 
tty_buffer_find() and tty_buffer_free(), either one shall make 
change to keep accordance? 
which one to make change might depends on original purpose of 
creating the free list. I tried to find the history of tty_buffer_free(),
but 512 is here since 2.6.32.61, I didn't find older version.

 So this can't leak because a buffer will never be found
 mid-list.
 
 Greg has a patch series from me that reduces this but it's not
 yet in next.
 
 Regards,
 Peter Hurley
 


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] tty_buffer: avoid race due to tty_buffer_free_all() being misused

2013-05-17 Thread channing 
On Thu, 2013-05-16 at 08:54 -0400, Peter Hurley wrote:
> On 05/16/2013 04:59 AM, channing wrote:
> >
> > In tty_buffer.c, function tty_buffer_free_all() is used to remove
> > all buffers for a tty, although it's declared that it mustn't be called
> > when the tty is in use, it cannot guarantee that. we can observe some
> > device driver make use it by mistake, for example, while tty device is
> > releasing, the tty data forwarding is not stopped, then it might hit
> > the case that tty buffer is being used while tty_buffer_free_all()
> > free this tty buffer, and finally lead to random error at any places,
> > and it's not clear to debug.
> 
> What kernel version?
3.4
> 
> > Although device driver could do better, it's simpler and safer to
> > strengthen protection in the view of tty buffer, by adding a tty->buf.lock
> > in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
> > operations.
> 
> Sorry, but this isn't correct.
> 
> The driver cannot continue to perform i/o concurrently with
> tty_port_destroy().
> 
Thanks for remind, 3.4 haven't tty_port_destroy(), the mainline has
changed the way to call tty_buffer_free_all().

> If the concurrent use you're observing is with flush_to_ldisc(),
> that should be fixed in current mainline.
> 
Yes, when calling flush_to_ldisc() in Kernel 3.4, we could observe the
tty buffer is corrupted, and dummped stack shows that
tty_buffer_free_all() was called before. Is it a known issue fixed in
old version? would you please tell me related patch to solve this
flush_to_ldisc() issue? Thanks very much.

Chao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tty_buffer: avoid race due to tty_buffer_free_all() being misused

2013-05-17 Thread channing 
On Thu, 2013-05-16 at 08:54 -0400, Peter Hurley wrote:
 On 05/16/2013 04:59 AM, channing wrote:
 
  In tty_buffer.c, function tty_buffer_free_all() is used to remove
  all buffers for a tty, although it's declared that it mustn't be called
  when the tty is in use, it cannot guarantee that. we can observe some
  device driver make use it by mistake, for example, while tty device is
  releasing, the tty data forwarding is not stopped, then it might hit
  the case that tty buffer is being used while tty_buffer_free_all()
  free this tty buffer, and finally lead to random error at any places,
  and it's not clear to debug.
 
 What kernel version?
3.4
 
  Although device driver could do better, it's simpler and safer to
  strengthen protection in the view of tty buffer, by adding a tty-buf.lock
  in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
  operations.
 
 Sorry, but this isn't correct.
 
 The driver cannot continue to perform i/o concurrently with
 tty_port_destroy().
 
Thanks for remind, 3.4 haven't tty_port_destroy(), the mainline has
changed the way to call tty_buffer_free_all().

 If the concurrent use you're observing is with flush_to_ldisc(),
 that should be fixed in current mainline.
 
Yes, when calling flush_to_ldisc() in Kernel 3.4, we could observe the
tty buffer is corrupted, and dummped stack shows that
tty_buffer_free_all() was called before. Is it a known issue fixed in
old version? would you please tell me related patch to solve this
flush_to_ldisc() issue? Thanks very much.

Chao

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

[PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

2013-05-16 Thread channing 

In tty_buffer.c, function tty_buffer_free_all() is used to remove
all buffers for a tty, although it's declared that it mustn't be called
when the tty is in use, it cannot guarantee that. we can observe some
device driver make use it by mistake, for example, while tty device is
releasing, the tty data forwarding is not stopped, then it might hit
the case that tty buffer is being used while tty_buffer_free_all()
free this tty buffer, and finally lead to random error at any places,
and it's not clear to debug.

Although device driver could do better, it's simpler and safer to
strengthen protection in the view of tty buffer, by adding a tty->buf.lock
in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
operations.

Signed-off-by: channing 
---
 drivers/tty/tty_buffer.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..c7c100d 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -32,7 +32,9 @@ void tty_buffer_free_all(struct tty_port *port)
 {
struct tty_bufhead *buf = >buf;
struct tty_buffer *thead;
+   unsigned long flags;
 
+   spin_lock_irqsave(>lock, flags);
while ((thead = buf->head) != NULL) {
buf->head = thead->next;
kfree(thead);
@@ -43,6 +45,7 @@ void tty_buffer_free_all(struct tty_port *port)
}
buf->tail = NULL;
buf->memory_used = 0;
+   spin_unlock_irqrestore(>lock, flags);
 }
 
 /**
-- 
1.7.1



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

[PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

2013-05-16 Thread channing 

In tty_buffer.c, function tty_buffer_free_all() is used to remove
all buffers for a tty, although it's declared that it mustn't be called
when the tty is in use, it cannot guarantee that. we can observe some
device driver make use it by mistake, for example, while tty device is
releasing, the tty data forwarding is not stopped, then it might hit
the case that tty buffer is being used while tty_buffer_free_all()
free this tty buffer, and finally lead to random error at any places,
and it's not clear to debug.

Although device driver could do better, it's simpler and safer to
strengthen protection in the view of tty buffer, by adding a tty-buf.lock
in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
operations.

Signed-off-by: channing chao...@intel.com
---
 drivers/tty/tty_buffer.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..c7c100d 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -32,7 +32,9 @@ void tty_buffer_free_all(struct tty_port *port)
 {
struct tty_bufhead *buf = port-buf;
struct tty_buffer *thead;
+   unsigned long flags;
 
+   spin_lock_irqsave(buf-lock, flags);
while ((thead = buf-head) != NULL) {
buf-head = thead-next;
kfree(thead);
@@ -43,6 +45,7 @@ void tty_buffer_free_all(struct tty_port *port)
}
buf-tail = NULL;
buf-memory_used = 0;
+   spin_unlock_irqrestore(buf-lock, flags);
 }
 
 /**
-- 
1.7.1



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

[PATCH] Notify and correct preempt count if irq/x86 handler change it

2013-03-01 Thread channing 

On x86 platform, irq handler might runs in hardirq stack per cpu,
or kernel stack, it's possible that any irq handler modify
preempt count by mistake, and it would bring badly impact in below
3 scenarios:

1) irq interrupt a process in user mode, then irq handler will be
carry out in kernel stack, if handler changed preempt count, it will
impact the blocked process;

2) irq A's handler is executing on irq stack, it changes preempt
count and set IF EFLAG to enable local interrupt, then irq B is
coming and nest in hard irq stack, whose preempt count was fault
modified by irq A handler, then irq B's handler may hit preempt
count related issues.

3) irq handler changes preempt count's NMI bit by mistake, NMI may
come at this point and executed in hard irq stack or kernel stack,
and it would find that it's inside NMI context, and report a BUG().

This patch is to print out a notification when hardirq handler
change preempt count, it would helpful to faster debuger to find
misbehavior irq handler; On the other hand, when above case happen,
set back current preempt count to its previous value, to avoid
impacting on blocked process or other irq handler.

Signed-off-by: channing 
Signed-off-by: liu chuansheng 
---
 arch/x86/kernel/irq_32.c |   15 +++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
index 344faf8..c92ab44 100644
--- a/arch/x86/kernel/irq_32.c
+++ b/arch/x86/kernel/irq_32.c
@@ -82,6 +82,7 @@ execute_on_irq_stack(int overflow, struct irq_desc *desc, int 
irq)
 {
union irq_ctx *curctx, *irqctx;
u32 *isp, arg1, arg2;
+   int prev_count;
 
curctx = (union irq_ctx *) current_thread_info();
irqctx = __this_cpu_read(hardirq_ctx);
@@ -102,6 +103,7 @@ execute_on_irq_stack(int overflow, struct irq_desc *desc, 
int irq)
 
/* Copy the preempt_count so that the [soft]irq checks work. */
irqctx->tinfo.preempt_count = curctx->tinfo.preempt_count;
+   prev_count = irqctx->tinfo.preempt_count;
 
if (unlikely(overflow))
call_on_stack(print_stack_overflow, isp);
@@ -113,6 +115,12 @@ execute_on_irq_stack(int overflow, struct irq_desc *desc, 
int irq)
 :  "0" (irq),   "1" (desc),  "2" (isp),
"D" (desc->handle_irq)
 : "memory", "cc", "ecx");
+
+   if (unlikely(prev_count != irqctx->tinfo.preempt_count)) {
+   pr_err("huh, enterd irq %u handler with preempt_count 
%08x,exited with %08x?\n"
+   , irq, prev_count, irqctx->tinfo.preempt_count);
+   irqctx->tinfo.preempt_count = prev_count;
+   }
return 1;
 }
 
@@ -184,6 +192,7 @@ bool handle_irq(unsigned irq, struct pt_regs *regs)
 {
struct irq_desc *desc;
int overflow;
+   int prev_count;
 
overflow = check_stack_overflow();
 
@@ -194,7 +203,13 @@ bool handle_irq(unsigned irq, struct pt_regs *regs)
if (user_mode_vm(regs) || !execute_on_irq_stack(overflow, desc, irq)) {
if (unlikely(overflow))
print_stack_overflow();
+   prev_count = preempt_count();
desc->handle_irq(irq, desc);
+   if (unlikely(prev_count != preempt_count())) {
+   pr_err("huh, enterd irq %u handler with preempt_count 
%08x,exited with %08x?\n"
+   , irq, prev_count, preempt_count());
+   preempt_count() = prev_count;
+   }
}
 
return true;
-- 
1.7.1



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] n_gsm: Add Mutex to avoid race when net destroy

2013-03-01 Thread channing 
On Thu, 2013-02-28 at 10:53 +0100, Jiri Slaby wrote:
> On 02/28/2013 06:31 AM, channing wrote:
> > 
> > when gsm Net is enabled, data on dlci is transferrd by
> > gsm_mux_net_start_xmit(), while userspace may trigger
> > ioctrl to call gsm_destroy_network() during data was
> > transferring, because there is no mutex protection between
> > the two functions, following scenario may happen:
> > 
> > 1) gsm_mux_net_start_xmit() calls muxnet_get(mux_net);
> > 2) gsm_destroy_network() is called from ioctrl, and it
> > will not call net_free() to release net device because
> > net device is still referred in step 1)
> > 3) continue execute step 1), gsm_mux_net_start_xmit()
> > calls muxnet_put(mux_net), and then calls net_free() to
> > release net device.
> > 4) if userspace triggers gsm_create_network() at same time
> > with net_free() in step 3). it will hit race on dlci->net.
> > 
> > This patch is to add a mutex in tx function to avoid race
> > between it and destroy function.
> > 
> > Signed-off-by: Chao Bi 
> > Signed-off-by: Pillet Vincent 
> > ---
> >  drivers/tty/n_gsm.c |2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index 4a43ef5..0ca810a 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -2660,6 +2660,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
> >  {
> > struct gsm_mux_net *mux_net = (struct gsm_mux_net *)netdev_priv(net);
> > struct gsm_dlci *dlci = mux_net->dlci;
> > +   mutex_lock(>mutex);
> 
> Nack, start_xmit may be called in an atomic context -- you cannot call
> mutex.
> 
> > muxnet_get(mux_net);
> >  
> > skb_queue_head(>skb_list, skb);
> > @@ -2669,6 +2670,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
> > /* And tell the kernel when the last transmit started. */
> > net->trans_start = jiffies;
> > muxnet_put(mux_net);
> 
> Instead the concept is broken. If this was the last reference (as
> described in your steps above), it would blow up for the same reason I
> refer to above, i.e. net_free here would call unregister_netdev which is
> not atomic. Plus it will definitely deadlock because unregister_netdev
> waits for start_xmit to finish.
> 
> It should stop the queue and schedule a workqueue to lock the mutex,
> unregister the hetdev and reset dlci->net. (Or maybe just call
> muxnet_put with the lock held.)

Thanks, Jiri, you're right, I didn't notice that in validation because
DEBUG_ATOMIC_SLEEP is not enabled in my platform :( Now I'm trying to
work out the workqueue solution, when it finished I'll re-submit for
review. What do you mean by "call muxnet_put with lock held"? do you
mean to use spin lock instead of mutex?
 
> 
> That will fix 4), but there is still a bug: what protects
> gsm_create_network to be called twice or more in a sequence thus
> re-setting dlci->net to a new and new pointer?

Yes, that's a problem, Vincent has already noticed that and has a check
in gsmtty_ioctl to avoid call net creation multi time, I thought it
might be patch for other issue so didn't put them together.

> 
> > +   mutex_unlock(>mutex);
> > return NETDEV_TX_OK;
> >  }
> 
> thanks,


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] n_gsm: Add Mutex to avoid race when net destroy

2013-03-01 Thread channing 
On Thu, 2013-02-28 at 10:53 +0100, Jiri Slaby wrote:
 On 02/28/2013 06:31 AM, channing wrote:
  
  when gsm Net is enabled, data on dlci is transferrd by
  gsm_mux_net_start_xmit(), while userspace may trigger
  ioctrl to call gsm_destroy_network() during data was
  transferring, because there is no mutex protection between
  the two functions, following scenario may happen:
  
  1) gsm_mux_net_start_xmit() calls muxnet_get(mux_net);
  2) gsm_destroy_network() is called from ioctrl, and it
  will not call net_free() to release net device because
  net device is still referred in step 1)
  3) continue execute step 1), gsm_mux_net_start_xmit()
  calls muxnet_put(mux_net), and then calls net_free() to
  release net device.
  4) if userspace triggers gsm_create_network() at same time
  with net_free() in step 3). it will hit race on dlci-net.
  
  This patch is to add a mutex in tx function to avoid race
  between it and destroy function.
  
  Signed-off-by: Chao Bi chao...@intel.com
  Signed-off-by: Pillet Vincent vincentx.pil...@intel.com
  ---
   drivers/tty/n_gsm.c |2 ++
   1 files changed, 2 insertions(+), 0 deletions(-)
  
  diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
  index 4a43ef5..0ca810a 100644
  --- a/drivers/tty/n_gsm.c
  +++ b/drivers/tty/n_gsm.c
  @@ -2660,6 +2660,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
   {
  struct gsm_mux_net *mux_net = (struct gsm_mux_net *)netdev_priv(net);
  struct gsm_dlci *dlci = mux_net-dlci;
  +   mutex_lock(dlci-mutex);
 
 Nack, start_xmit may be called in an atomic context -- you cannot call
 mutex.
 
  muxnet_get(mux_net);
   
  skb_queue_head(dlci-skb_list, skb);
  @@ -2669,6 +2670,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
  /* And tell the kernel when the last transmit started. */
  net-trans_start = jiffies;
  muxnet_put(mux_net);
 
 Instead the concept is broken. If this was the last reference (as
 described in your steps above), it would blow up for the same reason I
 refer to above, i.e. net_free here would call unregister_netdev which is
 not atomic. Plus it will definitely deadlock because unregister_netdev
 waits for start_xmit to finish.
 
 It should stop the queue and schedule a workqueue to lock the mutex,
 unregister the hetdev and reset dlci-net. (Or maybe just call
 muxnet_put with the lock held.)

Thanks, Jiri, you're right, I didn't notice that in validation because
DEBUG_ATOMIC_SLEEP is not enabled in my platform :( Now I'm trying to
work out the workqueue solution, when it finished I'll re-submit for
review. What do you mean by call muxnet_put with lock held? do you
mean to use spin lock instead of mutex?
 
 
 That will fix 4), but there is still a bug: what protects
 gsm_create_network to be called twice or more in a sequence thus
 re-setting dlci-net to a new and new pointer?

Yes, that's a problem, Vincent has already noticed that and has a check
in gsmtty_ioctl to avoid call net creation multi time, I thought it
might be patch for other issue so didn't put them together.

 
  +   mutex_unlock(dlci-mutex);
  return NETDEV_TX_OK;
   }
 
 thanks,


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

[PATCH] Notify and correct preempt count if irq/x86 handler change it

2013-03-01 Thread channing 

On x86 platform, irq handler might runs in hardirq stack per cpu,
or kernel stack, it's possible that any irq handler modify
preempt count by mistake, and it would bring badly impact in below
3 scenarios:

1) irq interrupt a process in user mode, then irq handler will be
carry out in kernel stack, if handler changed preempt count, it will
impact the blocked process;

2) irq A's handler is executing on irq stack, it changes preempt
count and set IF EFLAG to enable local interrupt, then irq B is
coming and nest in hard irq stack, whose preempt count was fault
modified by irq A handler, then irq B's handler may hit preempt
count related issues.

3) irq handler changes preempt count's NMI bit by mistake, NMI may
come at this point and executed in hard irq stack or kernel stack,
and it would find that it's inside NMI context, and report a BUG().

This patch is to print out a notification when hardirq handler
change preempt count, it would helpful to faster debuger to find
misbehavior irq handler; On the other hand, when above case happen,
set back current preempt count to its previous value, to avoid
impacting on blocked process or other irq handler.

Signed-off-by: channing chao...@intel.com
Signed-off-by: liu chuansheng chuansheng@intel.com
---
 arch/x86/kernel/irq_32.c |   15 +++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
index 344faf8..c92ab44 100644
--- a/arch/x86/kernel/irq_32.c
+++ b/arch/x86/kernel/irq_32.c
@@ -82,6 +82,7 @@ execute_on_irq_stack(int overflow, struct irq_desc *desc, int 
irq)
 {
union irq_ctx *curctx, *irqctx;
u32 *isp, arg1, arg2;
+   int prev_count;
 
curctx = (union irq_ctx *) current_thread_info();
irqctx = __this_cpu_read(hardirq_ctx);
@@ -102,6 +103,7 @@ execute_on_irq_stack(int overflow, struct irq_desc *desc, 
int irq)
 
/* Copy the preempt_count so that the [soft]irq checks work. */
irqctx-tinfo.preempt_count = curctx-tinfo.preempt_count;
+   prev_count = irqctx-tinfo.preempt_count;
 
if (unlikely(overflow))
call_on_stack(print_stack_overflow, isp);
@@ -113,6 +115,12 @@ execute_on_irq_stack(int overflow, struct irq_desc *desc, 
int irq)
 :  0 (irq),   1 (desc),  2 (isp),
D (desc-handle_irq)
 : memory, cc, ecx);
+
+   if (unlikely(prev_count != irqctx-tinfo.preempt_count)) {
+   pr_err(huh, enterd irq %u handler with preempt_count 
%08x,exited with %08x?\n
+   , irq, prev_count, irqctx-tinfo.preempt_count);
+   irqctx-tinfo.preempt_count = prev_count;
+   }
return 1;
 }
 
@@ -184,6 +192,7 @@ bool handle_irq(unsigned irq, struct pt_regs *regs)
 {
struct irq_desc *desc;
int overflow;
+   int prev_count;
 
overflow = check_stack_overflow();
 
@@ -194,7 +203,13 @@ bool handle_irq(unsigned irq, struct pt_regs *regs)
if (user_mode_vm(regs) || !execute_on_irq_stack(overflow, desc, irq)) {
if (unlikely(overflow))
print_stack_overflow();
+   prev_count = preempt_count();
desc-handle_irq(irq, desc);
+   if (unlikely(prev_count != preempt_count())) {
+   pr_err(huh, enterd irq %u handler with preempt_count 
%08x,exited with %08x?\n
+   , irq, prev_count, preempt_count());
+   preempt_count() = prev_count;
+   }
}
 
return true;
-- 
1.7.1



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

[PATCH] n_gsm: Add Mutex to avoid race when net destroy

2013-02-27 Thread channing 

when gsm Net is enabled, data on dlci is transferrd by
gsm_mux_net_start_xmit(), while userspace may trigger
ioctrl to call gsm_destroy_network() during data was
transferring, because there is no mutex protection between
the two functions, following scenario may happen:

1) gsm_mux_net_start_xmit() calls muxnet_get(mux_net);
2) gsm_destroy_network() is called from ioctrl, and it
will not call net_free() to release net device because
net device is still referred in step 1)
3) continue execute step 1), gsm_mux_net_start_xmit()
calls muxnet_put(mux_net), and then calls net_free() to
release net device.
4) if userspace triggers gsm_create_network() at same time
with net_free() in step 3). it will hit race on dlci->net.

This patch is to add a mutex in tx function to avoid race
between it and destroy function.

Signed-off-by: Chao Bi 
Signed-off-by: Pillet Vincent 
---
 drivers/tty/n_gsm.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 4a43ef5..0ca810a 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2660,6 +2660,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
 {
struct gsm_mux_net *mux_net = (struct gsm_mux_net *)netdev_priv(net);
struct gsm_dlci *dlci = mux_net->dlci;
+   mutex_lock(>mutex);
muxnet_get(mux_net);
 
skb_queue_head(>skb_list, skb);
@@ -2669,6 +2670,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
/* And tell the kernel when the last transmit started. */
net->trans_start = jiffies;
muxnet_put(mux_net);
+   mutex_unlock(>mutex);
return NETDEV_TX_OK;
 }
 
-- 
1.7.1



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

[PATCH] n_gsm: Add Mutex to avoid race when net destroy

2013-02-27 Thread channing 

when gsm Net is enabled, data on dlci is transferrd by
gsm_mux_net_start_xmit(), while userspace may trigger
ioctrl to call gsm_destroy_network() during data was
transferring, because there is no mutex protection between
the two functions, following scenario may happen:

1) gsm_mux_net_start_xmit() calls muxnet_get(mux_net);
2) gsm_destroy_network() is called from ioctrl, and it
will not call net_free() to release net device because
net device is still referred in step 1)
3) continue execute step 1), gsm_mux_net_start_xmit()
calls muxnet_put(mux_net), and then calls net_free() to
release net device.
4) if userspace triggers gsm_create_network() at same time
with net_free() in step 3). it will hit race on dlci-net.

This patch is to add a mutex in tx function to avoid race
between it and destroy function.

Signed-off-by: Chao Bi chao...@intel.com
Signed-off-by: Pillet Vincent vincentx.pil...@intel.com
---
 drivers/tty/n_gsm.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 4a43ef5..0ca810a 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2660,6 +2660,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
 {
struct gsm_mux_net *mux_net = (struct gsm_mux_net *)netdev_priv(net);
struct gsm_dlci *dlci = mux_net-dlci;
+   mutex_lock(dlci-mutex);
muxnet_get(mux_net);
 
skb_queue_head(dlci-skb_list, skb);
@@ -2669,6 +2670,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb,
/* And tell the kernel when the last transmit started. */
net-trans_start = jiffies;
muxnet_put(mux_net);
+   mutex_unlock(dlci-mutex);
return NETDEV_TX_OK;
 }
 
-- 
1.7.1



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

[PATCH] serial:ifx6x60: Remove memset for SPI frame

2013-01-21 Thread channing 

There is no need to memset 0 to SPI frame memory before preparing
transfer frame bits, because SPI frame header are encoded with valid
data size, so don't need to worry about adopting dirty bits, more,
memset zero for each SPI frame may impact the spi throughput efficiency.

Signed-off-by: Chen Jun 
Signed-off-by: channing 
---
 drivers/tty/serial/ifx6x60.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 8cb6d8d..fa4ec7e 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -481,7 +481,6 @@ static int ifx_spi_prepare_tx_buffer(struct ifx_spi_device 
*ifx_dev)
unsigned char *tx_buffer;
 
tx_buffer = ifx_dev->tx_buffer;
-   memset(tx_buffer, 0, IFX_SPI_TRANSFER_SIZE);
 
/* make room for required SPI header */
tx_buffer += IFX_SPI_HEADER_OVERHEAD;
-- 
1.7.1



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

[PATCH] serial:ifx6x60: Remove memset for SPI frame

2013-01-21 Thread channing 

There is no need to memset 0 to SPI frame memory before preparing
transfer frame bits, because SPI frame header are encoded with valid
data size, so don't need to worry about adopting dirty bits, more,
memset zero for each SPI frame may impact the spi throughput efficiency.

Signed-off-by: Chen Jun jun.d.c...@intel.com
Signed-off-by: channing chao...@intel.com
---
 drivers/tty/serial/ifx6x60.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 8cb6d8d..fa4ec7e 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -481,7 +481,6 @@ static int ifx_spi_prepare_tx_buffer(struct ifx_spi_device 
*ifx_dev)
unsigned char *tx_buffer;
 
tx_buffer = ifx_dev-tx_buffer;
-   memset(tx_buffer, 0, IFX_SPI_TRANSFER_SIZE);
 
/* make room for required SPI header */
tx_buffer += IFX_SPI_HEADER_OVERHEAD;
-- 
1.7.1



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

[PATCH] serial:ifx6x60:Keep word size accordance with SPI controller

2013-01-15 Thread channing 

As protocol driver, IFX SPI driver initiate to setup SPI master with default
SPI word size as 16 bit/word, however, SPI master may not adopt this default
value due to SPI controller's capability, it might choose an available value by
itself and set it to spi_device.bits_per_word. In order to keep align with
Controller, IFX driver should make use of this value during SPI transfer,
but the default one.

Signed-off-by: Chen Jun 
Signed-off-by: channing 
---
 drivers/tty/serial/ifx6x60.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 675d94a..3e337b8 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -810,7 +810,8 @@ static void ifx_spi_io(unsigned long data)
ifx_dev->spi_xfer.cs_change = 0;
ifx_dev->spi_xfer.speed_hz = ifx_dev->spi_dev->max_speed_hz;
/* ifx_dev->spi_xfer.speed_hz = 390625; */
-   ifx_dev->spi_xfer.bits_per_word = spi_bpw;
+   ifx_dev->spi_xfer.bits_per_word =
+   ifx_dev->spi_dev->bits_per_word;
 
ifx_dev->spi_xfer.tx_buf = ifx_dev->tx_buffer;
ifx_dev->spi_xfer.rx_buf = ifx_dev->rx_buffer;
-- 
1.7.1



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

[PATCH] serial:ifx6x60:Keep word size accordance with SPI controller

2013-01-15 Thread channing 

As protocol driver, IFX SPI driver initiate to setup SPI master with default
SPI word size as 16 bit/word, however, SPI master may not adopt this default
value due to SPI controller's capability, it might choose an available value by
itself and set it to spi_device.bits_per_word. In order to keep align with
Controller, IFX driver should make use of this value during SPI transfer,
but the default one.

Signed-off-by: Chen Jun jun.d.c...@intel.com
Signed-off-by: channing chao...@intel.com
---
 drivers/tty/serial/ifx6x60.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 675d94a..3e337b8 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -810,7 +810,8 @@ static void ifx_spi_io(unsigned long data)
ifx_dev-spi_xfer.cs_change = 0;
ifx_dev-spi_xfer.speed_hz = ifx_dev-spi_dev-max_speed_hz;
/* ifx_dev-spi_xfer.speed_hz = 390625; */
-   ifx_dev-spi_xfer.bits_per_word = spi_bpw;
+   ifx_dev-spi_xfer.bits_per_word =
+   ifx_dev-spi_dev-bits_per_word;
 
ifx_dev-spi_xfer.tx_buf = ifx_dev-tx_buffer;
ifx_dev-spi_xfer.rx_buf = ifx_dev-rx_buffer;
-- 
1.7.1



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

[PATCH] ST_CORE: Error triggered by convert "char" to "int"

2013-01-10 Thread channing 

When st driver decodes protocol index received from raw data,
it does a value convert from "char" to "int". Because it's sign
extension from bit8 to bit32, the "int" value maybe minus, in
another word, the protocol index might be minus, but driver doesn't
filter such case and may continue access memory pointed by this
minus index.

This patch is to change the variable type of index from "int"
to "unsigned char", so that it avoids do such kind of type
conversion.

cc: liu chuansheng 
Signed-off-by: channing 
---
 drivers/misc/ti-st/st_core.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
index b90a224..0a14280 100644
--- a/drivers/misc/ti-st/st_core.c
+++ b/drivers/misc/ti-st/st_core.c
@@ -240,7 +240,8 @@ void st_int_recv(void *disc_data,
char *ptr;
struct st_proto_s *proto;
unsigned short payload_len = 0;
-   int len = 0, type = 0;
+   int len = 0;
+   unsigned char type = 0;
unsigned char *plen;
struct st_data_s *st_gdata = (struct st_data_s *)disc_data;
unsigned long flags;
-- 
1.7.1

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

[PATCH] ST_CORE: Error triggered by convert char to int

2013-01-10 Thread channing 

When st driver decodes protocol index received from raw data,
it does a value convert from char to int. Because it's sign
extension from bit8 to bit32, the int value maybe minus, in
another word, the protocol index might be minus, but driver doesn't
filter such case and may continue access memory pointed by this
minus index.

This patch is to change the variable type of index from int
to unsigned char, so that it avoids do such kind of type
conversion.

cc: liu chuansheng chuansheng@intel.com
Signed-off-by: channing chao...@intel.com
---
 drivers/misc/ti-st/st_core.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
index b90a224..0a14280 100644
--- a/drivers/misc/ti-st/st_core.c
+++ b/drivers/misc/ti-st/st_core.c
@@ -240,7 +240,8 @@ void st_int_recv(void *disc_data,
char *ptr;
struct st_proto_s *proto;
unsigned short payload_len = 0;
-   int len = 0, type = 0;
+   int len = 0;
+   unsigned char type = 0;
unsigned char *plen;
struct st_data_s *st_gdata = (struct st_data_s *)disc_data;
unsigned long flags;
-- 
1.7.1

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

[PATCH] serial:ifx6x60:Delete SPI timer when shut down port

2013-01-09 Thread channing 

When shut down SPI port, it's possible that MRDY has been asserted and a SPI
timer was activated waiting for SRDY assert, in the case, it needs to delete
this timer.

Signed-off-by: Chen Jun 
Signed-off-by: channing 
---
 drivers/tty/serial/ifx6x60.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 675d94a..7eed323 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -637,6 +637,7 @@ static void ifx_port_shutdown(struct tty_port *port)
 
clear_bit(IFX_SPI_STATE_IO_AVAILABLE, _dev->flags);
mrdy_set_low(ifx_dev);
+   del_timer(_dev->spi_timer);
clear_bit(IFX_SPI_STATE_TIMER_PENDING, _dev->flags);
tasklet_kill(_dev->io_work_tasklet);
 }
-- 
1.7.1



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

[PATCH] serial:ifx6x60:Delete SPI timer when shut down port

2013-01-09 Thread channing 

When shut down SPI port, it's possible that MRDY has been asserted and a SPI
timer was activated waiting for SRDY assert, in the case, it needs to delete
this timer.

Signed-off-by: Chen Jun jun.d.c...@intel.com
Signed-off-by: channing chao...@intel.com
---
 drivers/tty/serial/ifx6x60.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 675d94a..7eed323 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -637,6 +637,7 @@ static void ifx_port_shutdown(struct tty_port *port)
 
clear_bit(IFX_SPI_STATE_IO_AVAILABLE, ifx_dev-flags);
mrdy_set_low(ifx_dev);
+   del_timer(ifx_dev-spi_timer);
clear_bit(IFX_SPI_STATE_TIMER_PENDING, ifx_dev-flags);
tasklet_kill(ifx_dev-io_work_tasklet);
 }
-- 
1.7.1



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] SPI: SSP SPI Controller driver v3

2013-01-06 Thread channing 
On Mon, 2013-01-07 at 00:36 +0100, Linus Walleij wrote:
> On Wed, Dec 19, 2012 at 10:56 AM, Mika Westerberg
>  wrote:
> > On Tue, Dec 18, 2012 at 04:11:36PM +0800, chao bi wrote:
> >>
> >> This patch is to implement SSP SPI controller driver, which has been 
> >> applied and
> >> validated on intel Moorestown & Medfield platform. The patch are 
> >> originated by
> >> Ken Mills  and Sylvain Centelles 
> >> ,
> >> migrating to lateset Linux mainline SPI framework by Channing 
> >> 
> >> and Chen Jun  according to their integration & 
> >> validation
> >> on Medfield platform.
> >
> > This is the same IP block as used in PXA, right? With few modifications
> > here and there. Is there a reason not to use spi-pxa2xx.c?
> 
> This needs to be investigated. Two drivers for the same or closely related
> hardware block is never a good sign...
> 
> Yours,
> Linus Walleij

Dear Linus ,Mika and Grant,

Thanks for your remind.

Frankly I'm currently not sure whether they share same IP.. per your reminds, I 
tried to find but get 
limited info about PXA SSP's IP, from the code, looks like they have part of 
registers the same. 

As far as I know, spi-pxa2xx.c is specific for SSP controller of PXA2XX/PXA3XX 
core, right? While Medfield 
platform is embedded with ATOM core, the SSP driver we upload is validated on 
SSP controller of ATOM. In my 
view, they're specific for different AP & Platforms, if compare the 2 files, 
there are still many difference 
in how they works, if to choose a driver for Intel Medfild/Moorestown platform, 
I believe spi-intel-mid-ssp.c 
driver could be a more mature solution. 

What do you think? please correct me if I'm mistaken.

-chao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] SPI: SSP SPI Controller driver v3

2013-01-06 Thread channing 
On Mon, 2013-01-07 at 00:36 +0100, Linus Walleij wrote:
 On Wed, Dec 19, 2012 at 10:56 AM, Mika Westerberg
 mika.westerb...@linux.intel.com wrote:
  On Tue, Dec 18, 2012 at 04:11:36PM +0800, chao bi wrote:
 
  This patch is to implement SSP SPI controller driver, which has been 
  applied and
  validated on intel Moorestown  Medfield platform. The patch are 
  originated by
  Ken Mills ken.k.mi...@intel.com and Sylvain Centelles 
  sylvain.centel...@intel.com,
  migrating to lateset Linux mainline SPI framework by Channing 
  chao...@intel.com
  and Chen Jun jun.d.c...@intel.com according to their integration  
  validation
  on Medfield platform.
 
  This is the same IP block as used in PXA, right? With few modifications
  here and there. Is there a reason not to use spi-pxa2xx.c?
 
 This needs to be investigated. Two drivers for the same or closely related
 hardware block is never a good sign...
 
 Yours,
 Linus Walleij

Dear Linus ,Mika and Grant,

Thanks for your remind.

Frankly I'm currently not sure whether they share same IP.. per your reminds, I 
tried to find but get 
limited info about PXA SSP's IP, from the code, looks like they have part of 
registers the same. 

As far as I know, spi-pxa2xx.c is specific for SSP controller of PXA2XX/PXA3XX 
core, right? While Medfield 
platform is embedded with ATOM core, the SSP driver we upload is validated on 
SSP controller of ATOM. In my 
view, they're specific for different AP  Platforms, if compare the 2 files, 
there are still many difference 
in how they works, if to choose a driver for Intel Medfild/Moorestown platform, 
I believe spi-intel-mid-ssp.c 
driver could be a more mature solution. 

What do you think? please correct me if I'm mistaken.

-chao

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