* [PATCH 2/8] n_gsm: uplink SKBs accumulate on list
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
@ 2012-08-13 12:43 ` Alan Cox
2012-08-13 12:43 ` [PATCH 3/8] n_gsm : Flow control handling in Mux driver Alan Cox
` (7 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2012-08-13 12:43 UTC (permalink / raw)
To: greg, linux-serial
From: Russ Gorby <russ.gorby@intel.com>
gsm_dlci_data_kick will not call any output function if tx_bytes > THRESH_LO
furthermore it will call the output function only once if tx_bytes == 0
If the size of the IP writes are on the order of THRESH_LO
we can get into a situation where skbs accumulate on the outbound list
being starved for events to call the output function.
gsm_dlci_data_kick now calls the sweep function when tx_bytes==0
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Kappel, LaurentX <laurentx.kappel@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Hay and Water <stable@kernel.org>
---
drivers/tty/n_gsm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5c6c2e2..9b0a44d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -971,16 +971,19 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
{
unsigned long flags;
+ int sweep;
spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
/* If we have nothing running then we need to fire up */
+ sweep = (dlci->gsm->tx_bytes < TX_THRESH_LO);
if (dlci->gsm->tx_bytes == 0) {
if (dlci->net)
gsm_dlci_data_output_framed(dlci->gsm, dlci);
else
gsm_dlci_data_output(dlci->gsm, dlci);
- } else if (dlci->gsm->tx_bytes < TX_THRESH_LO)
- gsm_dlci_data_sweep(dlci->gsm);
+ }
+ if (sweep)
+ gsm_dlci_data_sweep(dlci->gsm);
spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
}
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 3/8] n_gsm : Flow control handling in Mux driver
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
2012-08-13 12:43 ` [PATCH 2/8] n_gsm: uplink SKBs accumulate on list Alan Cox
@ 2012-08-13 12:43 ` Alan Cox
2012-08-13 12:44 ` [PATCH 4/8] char: n_gsm: remove message filtering for contipated DLCI Alan Cox
` (6 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2012-08-13 12:43 UTC (permalink / raw)
To: greg, linux-serial
From: Frederic Berat <fredericx.berat@intel.com>
- Correcting handling of FCon/FCoff in order to respect 27.010 spec
- Consider FCon/off will overide all dlci flow control except for
dlci0 as we must be able to send control frames.
- Dlci constipated handling according to FC, RTC and RTR values.
- Modifying gsm_dlci_data_kick and gsm_dlci_data_sweep according
to dlci constipated value
Signed-off-by: Frederic Berat <fredericx.berat@intel.com>
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/n_gsm.c | 79 +++++++++++++++++++++++++++++++++++++--------------
1 file changed, 57 insertions(+), 22 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9b0a44d..6651285 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -673,6 +673,8 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
*
* The tty device has called us to indicate that room has appeared in
* the transmit queue. Ram more data into the pipe if we have any
+ * If we have been flow-stopped by a CMD_FCOFF, then we can only
+ * send messages on DLCI0 until CMD_FCON
*
* FIXME: lock against link layer control transmissions
*/
@@ -680,15 +682,19 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
static void gsm_data_kick(struct gsm_mux *gsm)
{
struct gsm_msg *msg = gsm->tx_head;
+ struct gsm_msg *free_msg;
int len;
int skip_sof = 0;
- /* FIXME: We need to apply this solely to data messages */
- if (gsm->constipated)
- return;
-
- while (gsm->tx_head != NULL) {
- msg = gsm->tx_head;
+ while (msg) {
+ if (gsm->constipated && msg->addr) {
+ msg = msg->next;
+ continue;
+ }
+ if (gsm->dlci[msg->addr]->constipated) {
+ msg = msg->next;
+ continue;
+ }
if (gsm->encoding != 0) {
gsm->txframe[0] = GSM1_SOF;
len = gsm_stuff_frame(msg->data,
@@ -711,15 +717,19 @@ static void gsm_data_kick(struct gsm_mux *gsm)
len - skip_sof) < 0)
break;
/* FIXME: Can eliminate one SOF in many more cases */
- gsm->tx_head = msg->next;
- if (gsm->tx_head == NULL)
- gsm->tx_tail = NULL;
gsm->tx_bytes -= msg->len;
- kfree(msg);
/* For a burst of frames skip the extra SOF within the
burst */
skip_sof = 1;
+
+ if (gsm->tx_head == msg)
+ gsm->tx_head = msg->next;
+ free_msg = msg;
+ msg = msg->next;
+ kfree(free_msg);
}
+ if (!gsm->tx_head)
+ gsm->tx_tail = NULL;
}
/**
@@ -738,6 +748,8 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
u8 *dp = msg->data;
u8 *fcs = dp + msg->len;
+ WARN_ONCE(dlci->constipated, "%s: queueing from a constipated DLCI",
+ __func__);
/* Fill in the header */
if (gsm->encoding == 0) {
if (msg->len < 128)
@@ -944,6 +956,9 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
break;
dlci = gsm->dlci[i];
if (dlci == NULL || dlci->constipated) {
+ if (dlci && (debug & 0x20))
+ pr_info("%s: DLCI %d is constipated",
+ __func__, i);
i++;
continue;
}
@@ -973,6 +988,13 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
unsigned long flags;
int sweep;
+ if (dlci->constipated) {
+ if (debug & 0x20)
+ pr_info("%s: DLCI %d is constipated",
+ __func__, dlci->addr);
+ return;
+ }
+
spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
/* If we have nothing running then we need to fire up */
sweep = (dlci->gsm->tx_bytes < TX_THRESH_LO);
@@ -1030,6 +1052,7 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
{
int mlines = 0;
u8 brk = 0;
+ int fc;
/* The modem status command can either contain one octet (v.24 signals)
or two octets (v.24 signals + break signals). The length field will
@@ -1041,19 +1064,27 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
else {
brk = modem & 0x7f;
modem = (modem >> 7) & 0x7f;
- };
+ }
/* Flow control/ready to communicate */
- if (modem & MDM_FC) {
+ fc = (modem & MDM_FC) || !(modem & MDM_RTR);
+ if (fc && !dlci->constipated) {
+ if (debug & 0x20)
+ pr_info("%s: DLCI %d START constipated (tx_bytes=%d)",
+ __func__, dlci->addr, dlci->gsm->tx_bytes);
/* Need to throttle our output on this device */
dlci->constipated = 1;
- }
- if (modem & MDM_RTC) {
- mlines |= TIOCM_DSR | TIOCM_DTR;
+ } else if (!fc && dlci->constipated) {
+ if (debug & 0x20)
+ pr_info("%s: DLCI %d END constipated (tx_bytes=%d)",
+ __func__, dlci->addr, dlci->gsm->tx_bytes);
dlci->constipated = 0;
gsm_dlci_data_kick(dlci);
}
+
/* Map modem bits */
+ if (modem & MDM_RTC)
+ mlines |= TIOCM_DSR | TIOCM_DTR;
if (modem & MDM_RTR)
mlines |= TIOCM_RTS | TIOCM_CTS;
if (modem & MDM_IC)
@@ -1209,17 +1240,21 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
gsm_control_reply(gsm, CMD_TEST, data, clen);
break;
case CMD_FCON:
- /* Modem wants us to STFU */
- gsm->constipated = 1;
- gsm_control_reply(gsm, CMD_FCON, NULL, 0);
- break;
- case CMD_FCOFF:
/* Modem can accept data again */
+ if (debug & 0x20)
+ pr_info("%s: GSM END constipation", __func__);
gsm->constipated = 0;
- gsm_control_reply(gsm, CMD_FCOFF, NULL, 0);
+ gsm_control_reply(gsm, CMD_FCON, NULL, 0);
/* Kick the link in case it is idling */
gsm_data_kick(gsm);
break;
+ case CMD_FCOFF:
+ /* Modem wants us to STFU */
+ if (debug & 0x20)
+ pr_info("%s: GSM START constipation", __func__);
+ gsm->constipated = 1;
+ gsm_control_reply(gsm, CMD_FCOFF, NULL, 0);
+ break;
case CMD_MSC:
/* Out of band modem line change indicator for a DLCI */
gsm_control_modem(gsm, data, clen);
@@ -2276,7 +2311,7 @@ static void gsmld_receive_buf(struct tty_struct *tty, const unsigned char *cp,
gsm->error(gsm, *dp, flags);
break;
default:
- WARN_ONCE("%s: unknown flag %d\n",
+ WARN_ONCE(1, "%s: unknown flag %d\n",
tty_name(tty, buf), flags);
break;
}
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 4/8] char: n_gsm: remove message filtering for contipated DLCI
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
2012-08-13 12:43 ` [PATCH 2/8] n_gsm: uplink SKBs accumulate on list Alan Cox
2012-08-13 12:43 ` [PATCH 3/8] n_gsm : Flow control handling in Mux driver Alan Cox
@ 2012-08-13 12:44 ` Alan Cox
2012-08-13 12:44 ` [PATCH 5/8] n_gsm: added interlocking for gsm_data_lock for certain code paths Alan Cox
` (5 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2012-08-13 12:44 UTC (permalink / raw)
To: greg, linux-serial
From: samix.lebsir <samix.lebsir@intel.com>
The design of uplink flow control in the mux driver is
that for constipated channels data will backup into the
per-channel fifos, and any messages that make it to the
outbound message queue will still go out.
Code was added to also stop messages that were in the outbound
queue but this requires filtering through all the messages on the
queue for stopped dlcis and changes some of the mux logic unneccessarily.
The message fiiltering was removed to be in line w/ the original design
as the message filtering does not provide any solution.
Extra debug messages used during investigation were also removed.
Signed-off-by: samix.lebsir <samix.lebsir@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Dressage <stable@kernel.org>
---
drivers/tty/n_gsm.c | 25 +------------------------
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 6651285..9854edf 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -691,10 +691,6 @@ static void gsm_data_kick(struct gsm_mux *gsm)
msg = msg->next;
continue;
}
- if (gsm->dlci[msg->addr]->constipated) {
- msg = msg->next;
- continue;
- }
if (gsm->encoding != 0) {
gsm->txframe[0] = GSM1_SOF;
len = gsm_stuff_frame(msg->data,
@@ -748,8 +744,6 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
u8 *dp = msg->data;
u8 *fcs = dp + msg->len;
- WARN_ONCE(dlci->constipated, "%s: queueing from a constipated DLCI",
- __func__);
/* Fill in the header */
if (gsm->encoding == 0) {
if (msg->len < 128)
@@ -956,9 +950,6 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
break;
dlci = gsm->dlci[i];
if (dlci == NULL || dlci->constipated) {
- if (dlci && (debug & 0x20))
- pr_info("%s: DLCI %d is constipated",
- __func__, i);
i++;
continue;
}
@@ -988,12 +979,8 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
unsigned long flags;
int sweep;
- if (dlci->constipated) {
- if (debug & 0x20)
- pr_info("%s: DLCI %d is constipated",
- __func__, dlci->addr);
+ if (dlci->constipated)
return;
- }
spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
/* If we have nothing running then we need to fire up */
@@ -1069,15 +1056,9 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
/* Flow control/ready to communicate */
fc = (modem & MDM_FC) || !(modem & MDM_RTR);
if (fc && !dlci->constipated) {
- if (debug & 0x20)
- pr_info("%s: DLCI %d START constipated (tx_bytes=%d)",
- __func__, dlci->addr, dlci->gsm->tx_bytes);
/* Need to throttle our output on this device */
dlci->constipated = 1;
} else if (!fc && dlci->constipated) {
- if (debug & 0x20)
- pr_info("%s: DLCI %d END constipated (tx_bytes=%d)",
- __func__, dlci->addr, dlci->gsm->tx_bytes);
dlci->constipated = 0;
gsm_dlci_data_kick(dlci);
}
@@ -1241,8 +1222,6 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
break;
case CMD_FCON:
/* Modem can accept data again */
- if (debug & 0x20)
- pr_info("%s: GSM END constipation", __func__);
gsm->constipated = 0;
gsm_control_reply(gsm, CMD_FCON, NULL, 0);
/* Kick the link in case it is idling */
@@ -1250,8 +1229,6 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
break;
case CMD_FCOFF:
/* Modem wants us to STFU */
- if (debug & 0x20)
- pr_info("%s: GSM START constipation", __func__);
gsm->constipated = 1;
gsm_control_reply(gsm, CMD_FCOFF, NULL, 0);
break;
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 5/8] n_gsm: added interlocking for gsm_data_lock for certain code paths
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
` (2 preceding siblings ...)
2012-08-13 12:44 ` [PATCH 4/8] char: n_gsm: remove message filtering for contipated DLCI Alan Cox
@ 2012-08-13 12:44 ` Alan Cox
2012-08-13 12:44 ` [PATCH 6/8] n_gsm: avoid accessing freed memory during CMD_FCOFF condition Alan Cox
` (4 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2012-08-13 12:44 UTC (permalink / raw)
To: greg, linux-serial
From: Russ Gorby <russ.gorby@intel.com>
There were some locking holes in the management of the MUX's
message queue for 2 code paths:
1) gsmld_write_wakeup
2) receipt of CMD_FCON flow-control message
In both cases gsm_data_kick is called w/o locking so it can collide
with other other instances of gsm_data_kick (pulling messages tx_tail)
or potentially other instances of __gsm_data_queu (adding messages to tx_head)
Changed to take the tx_lock in these 2 cases
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Riding School <stable@kernel.org>
---
drivers/tty/n_gsm.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9854edf..51ba2f2 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1205,6 +1205,8 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
u8 *data, int clen)
{
u8 buf[1];
+ unsigned long flags;
+
switch (command) {
case CMD_CLD: {
struct gsm_dlci *dlci = gsm->dlci[0];
@@ -1225,7 +1227,9 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
gsm->constipated = 0;
gsm_control_reply(gsm, CMD_FCON, NULL, 0);
/* Kick the link in case it is idling */
+ spin_lock_irqsave(&gsm->tx_lock, flags);
gsm_data_kick(gsm);
+ spin_unlock_irqrestore(&gsm->tx_lock, flags);
break;
case CMD_FCOFF:
/* Modem wants us to STFU */
@@ -2392,12 +2396,12 @@ static void gsmld_write_wakeup(struct tty_struct *tty)
/* Queue poll */
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+ spin_lock_irqsave(&gsm->tx_lock, flags);
gsm_data_kick(gsm);
if (gsm->tx_bytes < TX_THRESH_LO) {
- spin_lock_irqsave(&gsm->tx_lock, flags);
gsm_dlci_data_sweep(gsm);
- spin_unlock_irqrestore(&gsm->tx_lock, flags);
}
+ spin_unlock_irqrestore(&gsm->tx_lock, flags);
}
/**
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 6/8] n_gsm: avoid accessing freed memory during CMD_FCOFF condition
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
` (3 preceding siblings ...)
2012-08-13 12:44 ` [PATCH 5/8] n_gsm: added interlocking for gsm_data_lock for certain code paths Alan Cox
@ 2012-08-13 12:44 ` Alan Cox
2012-08-13 12:45 ` [PATCH 7/8] n_gsm: replace kfree_skb w/ appropriate dev_* versions Alan Cox
` (3 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2012-08-13 12:44 UTC (permalink / raw)
To: greg, linux-serial
From: Russ Gorby <russ.gorby@intel.com>
gsm_data_kick was recently modified to allow messages on the
tx queue bound for DLCI0 to flow even during FCOFF conditions.
Unfortunately we introduced a bug discovered by code inspection
where subsequent list traversers can access freed memory if
the DLCI0 messages were not all at the head of the list.
Replaced singly linked tx list w/ a list_head and used
provided interfaces for traversing and deleting members.
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Riding School <stable@kernel.org>
---
drivers/tty/n_gsm.c | 40 +++++++++++++---------------------------
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 51ba2f2..e2bdb8b 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -108,7 +108,7 @@ struct gsm_mux_net {
*/
struct gsm_msg {
- struct gsm_msg *next;
+ struct list_head list;
u8 addr; /* DLCI address + flags */
u8 ctrl; /* Control byte + flags */
unsigned int len; /* Length of data block (can be zero) */
@@ -245,8 +245,7 @@ struct gsm_mux {
unsigned int tx_bytes; /* TX data outstanding */
#define TX_THRESH_HI 8192
#define TX_THRESH_LO 2048
- struct gsm_msg *tx_head; /* Pending data packets */
- struct gsm_msg *tx_tail;
+ struct list_head tx_list; /* Pending data packets */
/* Control messages */
struct timer_list t2_timer; /* Retransmit timer for commands */
@@ -663,7 +662,7 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
m->len = len;
m->addr = addr;
m->ctrl = ctrl;
- m->next = NULL;
+ INIT_LIST_HEAD(&m->list);
return m;
}
@@ -681,16 +680,13 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
static void gsm_data_kick(struct gsm_mux *gsm)
{
- struct gsm_msg *msg = gsm->tx_head;
- struct gsm_msg *free_msg;
+ struct gsm_msg *msg, *nmsg;
int len;
int skip_sof = 0;
- while (msg) {
- if (gsm->constipated && msg->addr) {
- msg = msg->next;
+ list_for_each_entry_safe(msg, nmsg, &gsm->tx_list, list) {
+ if (gsm->constipated && msg->addr)
continue;
- }
if (gsm->encoding != 0) {
gsm->txframe[0] = GSM1_SOF;
len = gsm_stuff_frame(msg->data,
@@ -718,14 +714,9 @@ static void gsm_data_kick(struct gsm_mux *gsm)
burst */
skip_sof = 1;
- if (gsm->tx_head == msg)
- gsm->tx_head = msg->next;
- free_msg = msg;
- msg = msg->next;
- kfree(free_msg);
+ list_del(&msg->list);
+ kfree(msg);
}
- if (!gsm->tx_head)
- gsm->tx_tail = NULL;
}
/**
@@ -774,11 +765,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
msg->data = dp;
/* Add to the actual output queue */
- if (gsm->tx_tail)
- gsm->tx_tail->next = msg;
- else
- gsm->tx_head = msg;
- gsm->tx_tail = msg;
+ list_add_tail(&msg->list, &gsm->tx_list);
gsm->tx_bytes += msg->len;
gsm_data_kick(gsm);
}
@@ -2026,7 +2013,7 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
{
int i;
struct gsm_dlci *dlci = gsm->dlci[0];
- struct gsm_msg *txq;
+ struct gsm_msg *txq, *utxq;
struct gsm_control *gc;
gsm->dead = 1;
@@ -2061,11 +2048,9 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
if (gsm->dlci[i])
gsm_dlci_release(gsm->dlci[i]);
/* Now wipe the queues */
- for (txq = gsm->tx_head; txq != NULL; txq = gsm->tx_head) {
- gsm->tx_head = txq->next;
+ list_for_each_entry_safe(txq, ntxq, &gsm->tx_list, list)
kfree(txq);
- }
- gsm->tx_tail = NULL;
+ INIT_LIST_HEAD(&gsm->tx_list);
}
EXPORT_SYMBOL_GPL(gsm_cleanup_mux);
@@ -2176,6 +2161,7 @@ struct gsm_mux *gsm_alloc_mux(void)
}
spin_lock_init(&gsm->lock);
kref_init(&gsm->ref);
+ INIT_LIST_HEAD(&gsm->tx_list);
gsm->t1 = T1;
gsm->t2 = T2;
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 7/8] n_gsm: replace kfree_skb w/ appropriate dev_* versions
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
` (4 preceding siblings ...)
2012-08-13 12:44 ` [PATCH 6/8] n_gsm: avoid accessing freed memory during CMD_FCOFF condition Alan Cox
@ 2012-08-13 12:45 ` Alan Cox
2012-08-13 12:45 ` [PATCH 8/8] n_gsm: memory leak in uplink error path Alan Cox
` (2 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2012-08-13 12:45 UTC (permalink / raw)
To: greg, linux-serial
From: Russ Gorby <russ.gorby@intel.com>
Drivers are supposed to use the dev_* versions of the kfree_skb
interfaces. In a couple of cases we were called with IRQs
disabled as well which kfree_skb() does not expect.
Replaced kfree_skb calls w/ dev_kfree_skb and dev_kfree_skb_any
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Grooming <stable@kernel.org>
---
drivers/tty/n_gsm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e2bdb8b..17c9e94 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -879,7 +879,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
if (len > gsm->mtu) {
if (dlci->adaption == 3) {
/* Over long frame, bin it */
- kfree_skb(dlci->skb);
+ dev_kfree_skb_any(dlci->skb);
dlci->skb = NULL;
return 0;
}
@@ -905,7 +905,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
skb_pull(dlci->skb, len);
__gsm_data_queue(dlci, msg);
if (last) {
- kfree_skb(dlci->skb);
+ dev_kfree_skb_any(dlci->skb);
dlci->skb = NULL;
}
return size;
@@ -1674,7 +1674,7 @@ static void gsm_dlci_free(struct kref *ref)
dlci->gsm->dlci[dlci->addr] = NULL;
kfifo_free(dlci->fifo);
while ((dlci->skb = skb_dequeue(&dlci->skb_list)))
- kfree_skb(dlci->skb);
+ dev_kfree_skb(dlci->skb);
kfree(dlci);
}
@@ -2013,7 +2013,7 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
{
int i;
struct gsm_dlci *dlci = gsm->dlci[0];
- struct gsm_msg *txq, *utxq;
+ struct gsm_msg *txq, *ntxq;
struct gsm_control *gc;
gsm->dead = 1;
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 8/8] n_gsm: memory leak in uplink error path
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
` (5 preceding siblings ...)
2012-08-13 12:45 ` [PATCH 7/8] n_gsm: replace kfree_skb w/ appropriate dev_* versions Alan Cox
@ 2012-08-13 12:45 ` Alan Cox
2012-08-16 18:57 ` [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Greg KH
2012-08-16 19:01 ` Greg KH
8 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2012-08-13 12:45 UTC (permalink / raw)
To: greg, linux-serial
From: Russ Gorby <russ.gorby@intel.com>
Uplink (TX) network data will go through gsm_dlci_data_output_framed
there is a bug where if memory allocation fails, the skb which
has already been pulled off the list will be lost.
In addition TX skbs were being processed in LIFO order
Fixed the memory leak, and changed to FIFO order processing
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Kappel, LaurentX <laurentx.kappel@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Showjumping <stable@kernel.org>
---
drivers/tty/n_gsm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 17c9e94..793bc38 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -868,7 +868,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
/* dlci->skb is locked by tx_lock */
if (dlci->skb == NULL) {
- dlci->skb = skb_dequeue(&dlci->skb_list);
+ dlci->skb = skb_dequeue_tail(&dlci->skb_list);
if (dlci->skb == NULL)
return 0;
first = 1;
@@ -892,8 +892,11 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
/* FIXME: need a timer or something to kick this so it can't
get stuck with no work outstanding and no buffer free */
- if (msg == NULL)
+ if (msg == NULL) {
+ skb_queue_tail(&dlci->skb_list, dlci->skb);
+ dlci->skb = NULL;
return -ENOMEM;
+ }
dp = msg->data;
if (dlci->adaption == 4) { /* Interruptible framed (Packetised Data) */
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
` (6 preceding siblings ...)
2012-08-13 12:45 ` [PATCH 8/8] n_gsm: memory leak in uplink error path Alan Cox
@ 2012-08-16 18:57 ` Greg KH
2012-08-16 19:01 ` Greg KH
8 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2012-08-16 18:57 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-serial
On Mon, Aug 13, 2012 at 01:43:15PM +0100, Alan Cox wrote:
> From: xiaojin <jin.xiao@intel.com>
>
> In 3GPP27.010 5.8.1, it defined:
> The TE multiplexer initiates the establishment of the multiplexer control channel by sending a SABM frame on DLCI 0 using the procedures of clause 5.4.1.
> Once the multiplexer channel is established other DLCs may be established using the procedures of clause 5.4.1.
> This patch implement 5.8.1 in MUX level, it make sure DLC0 is the first channel to be setup.
>
> [or for those not familiar with the specification: it was possible to try
> and open a data connection while the control channel was not yet fully
> open, which is a spec violation and confuses some modems]
>
> Signed-off-by: xiaojin <jin.xiao@intel.com>
> Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
> [tweaked the order we check things and error code]
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Cc: The Horsebox <stable@kernel.org>
In the future, it's <stable@vger.kernel.org>, I can catch these
addresses, but it's not as automated.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX
2012-08-13 12:43 [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Alan Cox
` (7 preceding siblings ...)
2012-08-16 18:57 ` [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX Greg KH
@ 2012-08-16 19:01 ` Greg KH
2012-08-16 19:12 ` Alan Cox
8 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2012-08-16 19:01 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-serial
On Mon, Aug 13, 2012 at 01:43:15PM +0100, Alan Cox wrote:
> From: xiaojin <jin.xiao@intel.com>
>
> In 3GPP27.010 5.8.1, it defined:
> The TE multiplexer initiates the establishment of the multiplexer control channel by sending a SABM frame on DLCI 0 using the procedures of clause 5.4.1.
> Once the multiplexer channel is established other DLCs may be established using the procedures of clause 5.4.1.
> This patch implement 5.8.1 in MUX level, it make sure DLC0 is the first channel to be setup.
>
> [or for those not familiar with the specification: it was possible to try
> and open a data connection while the control channel was not yet fully
> open, which is a spec violation and confuses some modems]
>
> Signed-off-by: xiaojin <jin.xiao@intel.com>
> Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
> [tweaked the order we check things and error code]
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Cc: The Horsebox <stable@vger.kernel.org>
> ---
>
> drivers/tty/n_gsm.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 7a4bf30..5c6c2e2 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2889,6 +2889,10 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
> gsm = gsm_mux[mux];
> if (gsm->dead)
> return -EL2HLT;
> + /* If DLCI 0 is not yet fully open return an error. 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)
> + return -EL2NSYNC;
Odd, what tree did you make this against?
This applies in the gsmtty_init() function, not gsmtty_open(), is that
correct? It also has a bunch of fuzz.
I'll apply it, but can you verify I didn't do anything wrong?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX
2012-08-16 19:01 ` Greg KH
@ 2012-08-16 19:12 ` Alan Cox
2012-08-16 19:17 ` Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: Alan Cox @ 2012-08-16 19:12 UTC (permalink / raw)
To: Greg KH; +Cc: linux-serial
On Thu, 16 Aug 2012 12:01:04 -0700
Greg KH <greg@kroah.com> wrote:
> On Mon, Aug 13, 2012 at 01:43:15PM +0100, Alan Cox wrote:
> > From: xiaojin <jin.xiao@intel.com>
> >
> > In 3GPP27.010 5.8.1, it defined:
> > The TE multiplexer initiates the establishment of the multiplexer control channel by sending a SABM frame on DLCI 0 using the procedures of clause 5.4.1.
> > Once the multiplexer channel is established other DLCs may be established using the procedures of clause 5.4.1.
> > This patch implement 5.8.1 in MUX level, it make sure DLC0 is the first channel to be setup.
> >
> > [or for those not familiar with the specification: it was possible to try
> > and open a data connection while the control channel was not yet fully
> > open, which is a spec violation and confuses some modems]
> >
> > Signed-off-by: xiaojin <jin.xiao@intel.com>
> > Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
> > [tweaked the order we check things and error code]
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > Cc: The Horsebox <stable@vger.kernel.org>
> > ---
> >
> > drivers/tty/n_gsm.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index 7a4bf30..5c6c2e2 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -2889,6 +2889,10 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
> > gsm = gsm_mux[mux];
> > if (gsm->dead)
> > return -EL2HLT;
> > + /* If DLCI 0 is not yet fully open return an error. 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)
> > + return -EL2NSYNC;
>
> Odd, what tree did you make this against?
>
> This applies in the gsmtty_init() function, not gsmtty_open(), is that
> correct? It also has a bunch of fuzz.
No it's not correct..
Ah its colliding with the tty_port changes - my bad. I'll rebase my tree.
Alan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/8] n_gsm.c: Implement 3GPP27.010 DLC start-up procedure in MUX
2012-08-16 19:12 ` Alan Cox
@ 2012-08-16 19:17 ` Greg KH
0 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2012-08-16 19:17 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-serial
On Thu, Aug 16, 2012 at 08:12:55PM +0100, Alan Cox wrote:
> On Thu, 16 Aug 2012 12:01:04 -0700
> Greg KH <greg@kroah.com> wrote:
>
> > On Mon, Aug 13, 2012 at 01:43:15PM +0100, Alan Cox wrote:
> > > From: xiaojin <jin.xiao@intel.com>
> > >
> > > In 3GPP27.010 5.8.1, it defined:
> > > The TE multiplexer initiates the establishment of the multiplexer control channel by sending a SABM frame on DLCI 0 using the procedures of clause 5.4.1.
> > > Once the multiplexer channel is established other DLCs may be established using the procedures of clause 5.4.1.
> > > This patch implement 5.8.1 in MUX level, it make sure DLC0 is the first channel to be setup.
> > >
> > > [or for those not familiar with the specification: it was possible to try
> > > and open a data connection while the control channel was not yet fully
> > > open, which is a spec violation and confuses some modems]
> > >
> > > Signed-off-by: xiaojin <jin.xiao@intel.com>
> > > Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
> > > [tweaked the order we check things and error code]
> > > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > > Cc: The Horsebox <stable@vger.kernel.org>
> > > ---
> > >
> > > drivers/tty/n_gsm.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > > index 7a4bf30..5c6c2e2 100644
> > > --- a/drivers/tty/n_gsm.c
> > > +++ b/drivers/tty/n_gsm.c
> > > @@ -2889,6 +2889,10 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
> > > gsm = gsm_mux[mux];
> > > if (gsm->dead)
> > > return -EL2HLT;
> > > + /* If DLCI 0 is not yet fully open return an error. 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)
> > > + return -EL2NSYNC;
> >
> > Odd, what tree did you make this against?
> >
> > This applies in the gsmtty_init() function, not gsmtty_open(), is that
> > correct? It also has a bunch of fuzz.
>
> No it's not correct..
Odd, git's 3-way merge thinks it figured it out as being ok.
> Ah its colliding with the tty_port changes - my bad. I'll rebase my tree.
Ok, I've applied it now, if there's a fixup needed, please send it
instead.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread