From: Daniel Starke <[email protected]>
Currently, only the initiator resets the mux protocol if the user requests
new parameters that are incompatible to those of the current connection.
The responder also needs to reset the multiplexer if the new parameter set
requires this. Otherwise, we end up with an inconsistent parameter set
between initiator and responder.
Revert the old behavior to inform the peer upon an incompatible parameter
set change from the user on the responder side by re-establishing the mux
protocol in such case.
Fixes: 509067bbd264 ("tty: n_gsm: Delete gsm_disconnect when config requester")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index fa92f727fdf8..3d28ecebd473 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2373,7 +2373,7 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
* configuration
*/
- if (gsm->initiator && (need_close || need_restart)) {
+ if (need_close || need_restart) {
int ret;
ret = gsm_disconnect(gsm);
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.7.2 states that the maximum frame size
(N1) refers to the length of the information field (i.e. user payload).
However, 'txframe' stores the whole frame including frame header, checksum
and start/end flags. We also need to consider the byte stuffing overhead.
Define constant for the protocol overhead and adjust the 'txframe' size
calculation accordingly to reserve enough space for a complete mux frame
including byte stuffing for advanced option mode. Note that no byte
stuffing is applied to the start and end flag.
Also use MAX_MTU instead of MAX_MRU as this buffer is used for data
transmission.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 2e3da8a4697e..cc90b03ce005 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -73,6 +73,8 @@ module_param(debug, int, 0600);
*/
#define MAX_MRU 1500
#define MAX_MTU 1500
+/* SOF, ADDR, CTRL, LEN1, LEN2, ..., FCS, EOF */
+#define PROT_OVERHEAD 7
#define GSM_NET_TX_TIMEOUT (HZ*10)
/*
@@ -2264,7 +2266,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
kfree(gsm);
return NULL;
}
- gsm->txframe = kmalloc(2 * MAX_MRU + 2, GFP_KERNEL);
+ gsm->txframe = kmalloc(2 * (MAX_MTU + PROT_OVERHEAD - 1), GFP_KERNEL);
if (gsm->txframe == NULL) {
kfree(gsm->buf);
kfree(gsm);
--
2.25.1
From: Daniel Starke <[email protected]>
The active mux instances are managed in the gsm_mux array and via mux_get()
and mux_put() functions separately. This gives a very loose coupling
between the actual instance and the gsm_mux array which manages it. It also
results in unnecessary lockings which makes it prone to failures. And it
creates a race condition if more than the maximum number of mux instances
are requested while the user changes the parameters of an active instance.
The user may loose ownership of the current mux instance in this case.
Fix this by moving the gsm_mux array handling to the mux allocation and
deallocation functions.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 63 +++++++++++++++++++++++++++------------------
1 file changed, 38 insertions(+), 25 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index daaffcfadaae..f546dfe03d29 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2136,18 +2136,6 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
/* Finish outstanding timers, making sure they are done */
del_timer_sync(&gsm->t2_timer);
- spin_lock(&gsm_mux_lock);
- for (i = 0; i < MAX_MUX; i++) {
- if (gsm_mux[i] == gsm) {
- gsm_mux[i] = NULL;
- break;
- }
- }
- spin_unlock(&gsm_mux_lock);
- /* open failed before registering => nothing to do */
- if (i == MAX_MUX)
- return;
-
/* Free up any link layer users */
for (i = 0; i < NUM_DLCI; i++)
if (gsm->dlci[i])
@@ -2171,7 +2159,6 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
static int gsm_activate_mux(struct gsm_mux *gsm)
{
struct gsm_dlci *dlci;
- int i = 0;
timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0);
init_waitqueue_head(&gsm->event);
@@ -2183,18 +2170,6 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
else
gsm->receive = gsm1_receive;
- spin_lock(&gsm_mux_lock);
- for (i = 0; i < MAX_MUX; i++) {
- if (gsm_mux[i] == NULL) {
- gsm->num = i;
- gsm_mux[i] = gsm;
- break;
- }
- }
- spin_unlock(&gsm_mux_lock);
- if (i == MAX_MUX)
- return -EBUSY;
-
dlci = gsm_dlci_alloc(gsm, 0);
if (dlci == NULL)
return -ENOMEM;
@@ -2210,6 +2185,15 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
*/
static void gsm_free_mux(struct gsm_mux *gsm)
{
+ int i;
+
+ for (i = 0; i < MAX_MUX; i++) {
+ if (gsm == gsm_mux[i]) {
+ gsm_mux[i] = NULL;
+ break;
+ }
+ }
+ mutex_destroy(&gsm->mutex);
kfree(gsm->txframe);
kfree(gsm->buf);
kfree(gsm);
@@ -2229,12 +2213,20 @@ static void gsm_free_muxr(struct kref *ref)
static inline void mux_get(struct gsm_mux *gsm)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&gsm_mux_lock, flags);
kref_get(&gsm->ref);
+ spin_unlock_irqrestore(&gsm_mux_lock, flags);
}
static inline void mux_put(struct gsm_mux *gsm)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&gsm_mux_lock, flags);
kref_put(&gsm->ref, gsm_free_muxr);
+ spin_unlock_irqrestore(&gsm_mux_lock, flags);
}
static inline unsigned int mux_num_to_base(struct gsm_mux *gsm)
@@ -2255,6 +2247,7 @@ static inline unsigned int mux_line_to_num(unsigned int line)
static struct gsm_mux *gsm_alloc_mux(void)
{
+ int i;
struct gsm_mux *gsm = kzalloc(sizeof(struct gsm_mux), GFP_KERNEL);
if (gsm == NULL)
return NULL;
@@ -2284,6 +2277,26 @@ static struct gsm_mux *gsm_alloc_mux(void)
gsm->mtu = 64;
gsm->dead = true; /* Avoid early tty opens */
+ /* Store the instance to the mux array or abort if no space is
+ * available.
+ */
+ spin_lock(&gsm_mux_lock);
+ for (i = 0; i < MAX_MUX; i++) {
+ if (!gsm_mux[i]) {
+ gsm_mux[i] = gsm;
+ gsm->num = i;
+ break;
+ }
+ }
+ spin_unlock(&gsm_mux_lock);
+ if (i == MAX_MUX) {
+ mutex_destroy(&gsm->mutex);
+ kfree(gsm->txframe);
+ kfree(gsm->buf);
+ kfree(gsm);
+ return NULL;
+ }
+
return gsm;
}
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.4.6.1 states that each command frame shall
be made up from type, length and value. Looking for example in chapter
5.4.6.3.5 at the description for the encoding of a flow control on command
it becomes obvious, that the type and length field is always present
whereas the value may be zero bytes long. The current implementation omits
the length field if the value is not present. This is wrong.
Correct this by always sending the length in gsm_control_transmit().
So far only the modem status command (MSC) has included a value and encoded
its length directly. Therefore, also change gsmtty_modem_update().
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 628bda5f0622..903278145078 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1327,11 +1327,12 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
static void gsm_control_transmit(struct gsm_mux *gsm, struct gsm_control *ctrl)
{
- struct gsm_msg *msg = gsm_data_alloc(gsm, 0, ctrl->len + 1, gsm->ftype);
+ struct gsm_msg *msg = gsm_data_alloc(gsm, 0, ctrl->len + 2, gsm->ftype);
if (msg == NULL)
return;
- msg->data[0] = (ctrl->cmd << 1) | 2 | EA; /* command */
- memcpy(msg->data + 1, ctrl->data, ctrl->len);
+ msg->data[0] = (ctrl->cmd << 1) | CR | EA; /* command */
+ msg->data[1] = (ctrl->len << 1) | EA;
+ memcpy(msg->data + 2, ctrl->data, ctrl->len);
gsm_data_queue(gsm->dlci[0], msg);
}
@@ -2957,19 +2958,17 @@ static struct tty_ldisc_ops tty_ldisc_packet = {
static int gsmtty_modem_update(struct gsm_dlci *dlci, u8 brk)
{
- u8 modembits[5];
+ u8 modembits[3];
struct gsm_control *ctrl;
int len = 2;
- if (brk)
+ modembits[0] = (dlci->addr << 2) | 2 | EA; /* DLCI, Valid, EA */
+ modembits[1] = (gsm_encode_modem(dlci) << 1) | EA;
+ if (brk) {
+ modembits[2] = (brk << 4) | 2 | EA; /* Length, Break, EA */
len++;
-
- modembits[0] = len << 1 | EA; /* Data bytes */
- modembits[1] = dlci->addr << 2 | 3; /* DLCI, EA, 1 */
- modembits[2] = gsm_encode_modem(dlci) << 1 | EA;
- if (brk)
- modembits[3] = brk << 4 | 2 | EA; /* Valid, EA */
- ctrl = gsm_control_send(dlci->gsm, CMD_MSC, modembits, len + 1);
+ }
+ ctrl = gsm_control_send(dlci->gsm, CMD_MSC, modembits, len);
if (ctrl == NULL)
return -ENOMEM;
return gsm_control_wait(dlci->gsm, ctrl);
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.7.3 states that the valid range for the
maximum number of retransmissions (N2) is from 0 to 255 (both including).
gsm_config() fails to limit this range correctly. Furthermore,
gsm_control_retransmit() handles this number incorrectly by performing
N2 - 1 retransmission attempts. Setting N2 to zero results in more than 255
retransmission attempts.
Fix the range check in gsm_config() and the value handling in
gsm_control_send() and gsm_control_retransmit() to comply with 3GPP 27.010.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 1430d7f83bd2..628bda5f0622 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1354,7 +1354,6 @@ static void gsm_control_retransmit(struct timer_list *t)
spin_lock_irqsave(&gsm->control_lock, flags);
ctrl = gsm->pending_cmd;
if (ctrl) {
- gsm->cretries--;
if (gsm->cretries == 0) {
gsm->pending_cmd = NULL;
ctrl->error = -ETIMEDOUT;
@@ -1363,6 +1362,7 @@ static void gsm_control_retransmit(struct timer_list *t)
wake_up(&gsm->event);
return;
}
+ gsm->cretries--;
gsm_control_transmit(gsm, ctrl);
mod_timer(&gsm->t2_timer, jiffies + gsm->t2 * HZ / 100);
}
@@ -1403,7 +1403,7 @@ static struct gsm_control *gsm_control_send(struct gsm_mux *gsm,
/* If DLCI0 is in ADM mode skip retries, it won't respond */
if (gsm->dlci[0]->mode == DLCI_MODE_ADM)
- gsm->cretries = 1;
+ gsm->cretries = 0;
else
gsm->cretries = gsm->n2;
@@ -2343,7 +2343,7 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
/* Check the MRU/MTU range looks sane */
if (c->mru > MAX_MRU || c->mtu > MAX_MTU || c->mru < 8 || c->mtu < 8)
return -EINVAL;
- if (c->n2 < 3)
+ if (c->n2 > 255)
return -EINVAL;
if (c->encapsulation > 1) /* Basic, advanced, no I */
return -EINVAL;
--
2.25.1
From: Daniel Starke <[email protected]>
gsm_carrier_raised() returns always 1 if the kernel module parameter
'debug' has its second least significant bit set. This changes the behavior
of the module instead of just adding some debug output.
Remove this 'debug' gated early out to avoid debug settings from changing
the program flow.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index ecd2ecc61b14..1905a0fea89b 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2981,9 +2981,6 @@ static int gsm_carrier_raised(struct tty_port *port)
/* Not yet open so no carrier info */
if (dlci->state != DLCI_OPEN)
return 0;
- if (debug & 2)
- return 1;
-
/*
* Basic mode with control channel in ADM mode may not respond
* to CMD_MSC at all and modem_rx is empty.
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.5.2 describes that the signal octet in
convergence layer type 2 can be either one or two bytes. The length is
encoded in the EA bit. This is set 1 for the last byte in the sequence.
gsmtty_modem_update() handles this correctly but gsm_dlci_data_output()
fails to set EA to 1. There is no case in which we encode two signal octets
as there is no case in which we send out a break signal.
Therefore, always set the EA bit to 1 for the signal octet to fix this.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index de97a3810731..3ba2505908e3 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -832,7 +832,7 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
break;
case 2: /* Unstructed with modem bits.
Always one byte as we never send inline break data */
- *dp++ = gsm_encode_modem(dlci);
+ *dp++ = (gsm_encode_modem(dlci) << 1) | EA;
break;
}
WARN_ON(kfifo_out_locked(&dlci->fifo, dp , len, &dlci->lock) != len);
--
2.25.1
From: Daniel Starke <[email protected]>
The frame checksum (FCS) is currently handled in gsm_queue() after
reception of a frame. However, this breaks layering. A workaround with
'received_fcs' was implemented so far.
Furthermore, frames are handled as such even if no end flag was received.
Move FCS calculation from gsm_queue() to gsm0_receive() and gsm1_receive().
Also delay gsm_queue() call there until a full frame was received to fix
both points.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 53 +++++++++++++++++++++++++--------------------
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 3ba2505908e3..4ce18b42c37a 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -219,7 +219,6 @@ struct gsm_mux {
int encoding;
u8 control;
u8 fcs;
- u8 received_fcs;
u8 *txframe; /* TX framing buffer */
/* Method for the receiver side */
@@ -1794,18 +1793,7 @@ static void gsm_queue(struct gsm_mux *gsm)
u8 cr;
int address;
int i, j, k, address_tmp;
- /* We have to sneak a look at the packet body to do the FCS.
- A somewhat layering violation in the spec */
- if ((gsm->control & ~PF) == UI)
- gsm->fcs = gsm_fcs_add_block(gsm->fcs, gsm->buf, gsm->len);
- if (gsm->encoding == 0) {
- /* WARNING: gsm->received_fcs is used for
- gsm->encoding = 0 only.
- In this case it contain the last piece of data
- required to generate final CRC */
- gsm->fcs = gsm_fcs_add(gsm->fcs, gsm->received_fcs);
- }
if (gsm->fcs != GOOD_FCS) {
gsm->bad_fcs++;
if (debug & 4)
@@ -1993,19 +1981,25 @@ static void gsm0_receive(struct gsm_mux *gsm, unsigned char c)
break;
case GSM_DATA: /* Data */
gsm->buf[gsm->count++] = c;
- if (gsm->count == gsm->len)
+ if (gsm->count == gsm->len) {
+ /* Calculate final FCS for UI frames over all data */
+ if ((gsm->control & ~PF) != UIH) {
+ gsm->fcs = gsm_fcs_add_block(gsm->fcs, gsm->buf,
+ gsm->count);
+ }
gsm->state = GSM_FCS;
+ }
break;
case GSM_FCS: /* FCS follows the packet */
- gsm->received_fcs = c;
- gsm_queue(gsm);
+ gsm->fcs = gsm_fcs_add(gsm->fcs, c);
gsm->state = GSM_SSOF;
break;
case GSM_SSOF:
- if (c == GSM0_SOF) {
- gsm->state = GSM_SEARCH;
- break;
- }
+ gsm->state = GSM_SEARCH;
+ if (c == GSM0_SOF)
+ gsm_queue(gsm);
+ else
+ gsm->bad_size++;
break;
default:
pr_debug("%s: unhandled state: %d\n", __func__, gsm->state);
@@ -2024,11 +2018,24 @@ static void gsm0_receive(struct gsm_mux *gsm, unsigned char c)
static void gsm1_receive(struct gsm_mux *gsm, unsigned char c)
{
if (c == GSM1_SOF) {
- /* EOF is only valid in frame if we have got to the data state
- and received at least one byte (the FCS) */
- if (gsm->state == GSM_DATA && gsm->count) {
- /* Extract the FCS */
+ /* EOF is only valid in frame if we have got to the data state */
+ if (gsm->state == GSM_DATA) {
+ if (gsm->count < 1) {
+ /* Missing FSC */
+ gsm->malformed++;
+ gsm->state = GSM_START;
+ return;
+ }
+ /* Remove the FCS from data */
gsm->count--;
+ if ((gsm->control & ~PF) != UIH) {
+ /* Calculate final FCS for UI frames over all
+ * data but FCS
+ */
+ gsm->fcs = gsm_fcs_add_block(gsm->fcs, gsm->buf,
+ gsm->count);
+ }
+ /* Add the FCS itself to test against GOOD_FCS */
gsm->fcs = gsm_fcs_add(gsm->fcs, gsm->buf[gsm->count]);
gsm->len = gsm->count;
gsm_queue(gsm);
--
2.25.1
From: Daniel Starke <[email protected]>
Currently the peer is not informed about the initial state of the modem
control lines after a new DLCI has been opened.
Fix this by sending the initial modem control line states after DLCI open.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index f3fb66be8513..e9a7d9483c1f 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -369,7 +369,11 @@ static const u8 gsm_fcs8[256] = {
#define INIT_FCS 0xFF
#define GOOD_FCS 0xCF
+/*
+ * Prototypes
+ */
static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
+static int gsmtty_modem_update(struct gsm_dlci *dlci, u8 brk);
/**
* gsm_fcs_add - update FCS
@@ -1479,6 +1483,8 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
pr_debug("DLCI %d goes open.\n", dlci->addr);
/* Register gsmtty driver,report gsmtty dev add uevent for user */
tty_register_device(gsm_tty_driver, dlci->addr, NULL);
+ if (dlci->addr) /* Send current modem state */
+ gsmtty_modem_update(dlci, 0);
wake_up(&dlci->gsm->event);
}
--
2.25.1
From: Daniel Starke <[email protected]>
Internally, we manage the alive state of the mux channels and mux itself
with the field member 'dead'. This makes it possible to notify the user
if the accessed underlying link is already gone. On the other hand,
however, removing the virtual ttys before terminating the channels may
result in peer messages being received without any internal target. Move
the mux cleanup procedure from gsmld_detach_gsm() to gsmld_close() to fix
this by keeping the virtual ttys open until the mux has been cleaned up.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index f546dfe03d29..de97a3810731 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2479,7 +2479,6 @@ static void gsmld_detach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
for (i = 1; i < NUM_DLCI; i++)
tty_unregister_device(gsm_tty_driver, base + i);
}
- gsm_cleanup_mux(gsm, false);
tty_kref_put(gsm->tty);
gsm->tty = NULL;
}
@@ -2544,6 +2543,12 @@ static void gsmld_close(struct tty_struct *tty)
{
struct gsm_mux *gsm = tty->disc_data;
+ /* The ldisc locks and closes the port before calling our close. This
+ * means we have no way to do a proper disconnect. We will not bother
+ * to do one.
+ */
+ gsm_cleanup_mux(gsm, false);
+
gsmld_detach_gsm(tty, gsm);
gsmld_flush_buffer(tty);
--
2.25.1
From: Daniel Starke <[email protected]>
gsm_control_modem() informs the virtual tty that more data can be written
after receiving a control signal octet via modem status command (MSC).
However, gsm_dlci_data() fails to do the same after receiving a control
signal octet from the convergence layer type 2 header.
Add tty_wakeup() in gsm_dlci_data() for convergence layer type 2 to fix
this.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 23418ee93156..f3fb66be8513 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1615,6 +1615,7 @@ static void gsm_dlci_data(struct gsm_dlci *dlci, const u8 *data, int clen)
tty = tty_port_tty_get(port);
if (tty) {
gsm_process_modem(tty, dlci, modem, slen);
+ tty_wakeup(tty);
tty_kref_put(tty);
}
fallthrough;
--
2.25.1
On Thu, Apr 14, 2022 at 02:42:24AM -0700, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> gsm_carrier_raised() returns always 1 if the kernel module parameter
> 'debug' has its second least significant bit set. This changes the behavior
> of the module instead of just adding some debug output.
> Remove this 'debug' gated early out to avoid debug settings from changing
> the program flow.
>
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: [email protected]
Why is this relevant for stable backporting? It's a debugging feature
only, and you are changing how it previously worked :(
From: Daniel Starke <[email protected]>
gsmtty_write() and gsm_dlci_data_output() properly guard the fifo access.
However, gsm_dlci_close() and gsmtty_flush_buffer() modifies the fifo but
do not guard this.
Add a guard here to prevent race conditions on parallel writes to the fifo.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index f4ec48c0d6d7..1e135a71860f 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1446,13 +1446,17 @@ static int gsm_control_wait(struct gsm_mux *gsm, struct gsm_control *control)
static void gsm_dlci_close(struct gsm_dlci *dlci)
{
+ unsigned long flags;
+
del_timer(&dlci->t1);
if (debug & 8)
pr_debug("DLCI %d goes closed.\n", dlci->addr);
dlci->state = DLCI_CLOSED;
if (dlci->addr != 0) {
tty_port_tty_hangup(&dlci->port, false);
+ spin_lock_irqsave(&dlci->lock, flags);
kfifo_reset(&dlci->fifo);
+ spin_unlock_irqrestore(&dlci->lock, flags);
/* Ensure that gsmtty_open() can return. */
tty_port_set_initialized(&dlci->port, 0);
wake_up_interruptible(&dlci->port.open_wait);
@@ -3150,13 +3154,17 @@ static unsigned int gsmtty_chars_in_buffer(struct tty_struct *tty)
static void gsmtty_flush_buffer(struct tty_struct *tty)
{
struct gsm_dlci *dlci = tty->driver_data;
+ unsigned long flags;
+
if (dlci->state == DLCI_CLOSED)
return;
/* Caution needed: If we implement reliable transport classes
then the data being transmitted can't simply be junked once
it has first hit the stack. Until then we can just blow it
away */
+ spin_lock_irqsave(&dlci->lock, flags);
kfifo_reset(&dlci->fifo);
+ spin_unlock_irqrestore(&dlci->lock, flags);
/* Need to unhook this DLCI from the transmit queue logic */
}
--
2.25.1
From: Daniel Starke <[email protected]>
The current DLCI release order starts with the control channel followed by
the user channels. Reverse this order to keep the control channel open
until all user channels have been released.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index cc90b03ce005..6b953dfbb155 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2146,8 +2146,8 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
/* Finish outstanding timers, making sure they are done */
del_timer_sync(&gsm->t2_timer);
- /* Free up any link layer users */
- for (i = 0; i < NUM_DLCI; i++)
+ /* Free up any link layer users and finally the control channel */
+ for (i = NUM_DLCI - 1; i >= 0; i--)
if (gsm->dlci[i])
gsm_dlci_release(gsm->dlci[i]);
mutex_unlock(&gsm->mutex);
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.4.3.1 states the CR bit in UI and UIH
frames shall always be set 1 by the initiator and 0 by the responder.
Currently, gsm_queue() has a pre-processor gated (excluded) check which
treats all frames that conform to the standard as malformed frames.
Remove this optional code to avoid confusion and possible breaking changes
in case that someone includes it.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e9a7d9483c1f..f4ec48c0d6d7 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
case UI|PF:
case UIH:
case UIH|PF:
-#if 0
- if (cr)
- goto invalid;
-#endif
if (dlci == NULL || dlci->state != DLCI_OPEN) {
gsm_command(gsm, address, DM|PF);
return;
--
2.25.1
On Thu, Apr 14, 2022 at 02:42:21AM -0700, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
> the newer 27.010 here. Chapter 5.4.3.1 states the CR bit in UI and UIH
> frames shall always be set 1 by the initiator and 0 by the responder.
This has nothing to do with the change you made here.
> Currently, gsm_queue() has a pre-processor gated (excluded) check which
> treats all frames that conform to the standard as malformed frames.
> Remove this optional code to avoid confusion and possible breaking changes
> in case that someone includes it.
Again, nothing to do with the code change.
>
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
This "fixes" nothing :(
> Cc: [email protected]
How is commenting out unused code a stable backport requirement?
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index e9a7d9483c1f..f4ec48c0d6d7 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
> case UI|PF:
> case UIH:
> case UIH|PF:
> -#if 0
> - if (cr)
> - goto invalid;
> -#endif
All you are doing is cleaning up dead code. Not a big deal, and not
worth all the text above to confuse people :(
thanks,
greg k-h
From: Daniel Starke <[email protected]>
The gsm_mux field 'malformed' represents the number of malformed frames
received. However, gsm1_receive() also increases this counter for any out
of frame byte.
Fix this by ignoring out of frame data for the malformed counter.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 4ce18b42c37a..2e3da8a4697e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2044,7 +2044,8 @@ static void gsm1_receive(struct gsm_mux *gsm, unsigned char c)
}
/* Any partial frame was a runt so go back to start */
if (gsm->state != GSM_START) {
- gsm->malformed++;
+ if (gsm->state != GSM_SEARCH)
+ gsm->malformed++;
gsm->state = GSM_START;
}
/* A SOF in GSM_START means we are still reading idling or
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.4.4.2 states that any received unnumbered
acknowledgment (UA) with its poll/final (PF) bit set to 0 shall be
discarded. Currently, all UA frame are handled in the same way regardless
of the PF bit. This does not comply with the standard.
Remove the UA case in gsm_queue() to process only UA frames with PF bit set
to 1 to abide the standard.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 1905a0fea89b..cf861598a646 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1871,7 +1871,6 @@ static void gsm_queue(struct gsm_mux *gsm)
}
}
break;
- case UA:
case UA|PF:
if (cr == 0 || dlci == NULL)
break;
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.8.2 states that both sides will revert to
the non-multiplexed mode via a close-down message (CLD). The usual program
flow is as following:
- start multiplex mode by sending AT+CMUX to the mobile
- establish the control channel (DLCI 0)
- establish user channels (DLCI >0)
- terminate user channels
- send close-down message (CLD)
- revert to AT protocol (i.e. leave multiplexed mode)
The AT protocol is out of scope of the n_gsm driver. However,
gsm_disconnect() sends CLD if gsm_config() detects that the requested
parameters require the mux protocol to restart. The next immediate action
is to start the mux protocol by opening DLCI 0 again. Any responder side
which handles CLD commands correctly forces us to fail at this point
because AT+CMUX needs to be sent to the mobile to start the mux again.
Therefore, remove the CLD command in this phase and keep both sides in
multiplexed mode.
Remove the gsm_disconnect() function as it become unnecessary and merge the
remaining parts into gsm_cleanup_mux() to handle the termination order and
locking correctly.
Fixes: 71e077915396 ("tty: n_gsm: do not send/receive in ldisc close path")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 68 +++++++++++++--------------------------------
1 file changed, 20 insertions(+), 48 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 3d28ecebd473..daaffcfadaae 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2106,49 +2106,35 @@ static void gsm_error(struct gsm_mux *gsm)
gsm->io_error++;
}
-static int gsm_disconnect(struct gsm_mux *gsm)
-{
- struct gsm_dlci *dlci = gsm->dlci[0];
- struct gsm_control *gc;
-
- if (!dlci)
- return 0;
-
- /* In theory disconnecting DLCI 0 is sufficient but for some
- modems this is apparently not the case. */
- gc = gsm_control_send(gsm, CMD_CLD, NULL, 0);
- if (gc)
- gsm_control_wait(gsm, gc);
-
- del_timer_sync(&gsm->t2_timer);
- /* Now we are sure T2 has stopped */
-
- gsm_dlci_begin_close(dlci);
- wait_event_interruptible(gsm->event,
- dlci->state == DLCI_CLOSED);
-
- if (signal_pending(current))
- return -EINTR;
-
- return 0;
-}
-
/**
* gsm_cleanup_mux - generic GSM protocol cleanup
* @gsm: our mux
+ * @disc: disconnect link?
*
* Clean up the bits of the mux which are the same for all framing
* protocols. Remove the mux from the mux table, stop all the timers
* and then shut down each device hanging up the channels as we go.
*/
-static void gsm_cleanup_mux(struct gsm_mux *gsm)
+static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
{
int i;
struct gsm_dlci *dlci = gsm->dlci[0];
struct gsm_msg *txq, *ntxq;
gsm->dead = true;
+ mutex_lock(&gsm->mutex);
+
+ if (dlci) {
+ if (disc && dlci->state != DLCI_CLOSED) {
+ gsm_dlci_begin_close(dlci);
+ wait_event(gsm->event, dlci->state == DLCI_CLOSED);
+ }
+ dlci->dead = true;
+ }
+
+ /* Finish outstanding timers, making sure they are done */
+ del_timer_sync(&gsm->t2_timer);
spin_lock(&gsm_mux_lock);
for (i = 0; i < MAX_MUX; i++) {
@@ -2162,13 +2148,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
if (i == MAX_MUX)
return;
- del_timer_sync(&gsm->t2_timer);
- /* Now we are sure T2 has stopped */
- if (dlci)
- dlci->dead = true;
-
/* 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]);
@@ -2370,19 +2350,11 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
/*
* Close down what is needed, restart and initiate the new
- * configuration
+ * configuration. On the first time there is no DLCI[0]
+ * and closing or cleaning up is not necessary.
*/
-
- if (need_close || need_restart) {
- int ret;
-
- ret = gsm_disconnect(gsm);
-
- if (ret)
- return ret;
- }
- if (need_restart)
- gsm_cleanup_mux(gsm);
+ if (need_close || need_restart)
+ gsm_cleanup_mux(gsm, true);
gsm->initiator = c->initiator;
gsm->mru = c->mru;
@@ -2494,7 +2466,7 @@ static void gsmld_detach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
for (i = 1; i < NUM_DLCI; i++)
tty_unregister_device(gsm_tty_driver, base + i);
}
- gsm_cleanup_mux(gsm);
+ gsm_cleanup_mux(gsm, false);
tty_kref_put(gsm->tty);
gsm->tty = NULL;
}
@@ -2597,7 +2569,7 @@ static int gsmld_open(struct tty_struct *tty)
ret = gsmld_attach_gsm(tty, gsm);
if (ret != 0) {
- gsm_cleanup_mux(gsm);
+ gsm_cleanup_mux(gsm, false);
mux_put(gsm);
}
return ret;
--
2.25.1
From: Daniel Starke <[email protected]>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.2.1.2 describes the encoding of the
address field within the frame header. It is made up of the DLCI address,
command/response (CR) bit and EA bit.
Use the predefined CR value instead of a plain 2 in alignment to the
remaining code and to make the encoding obvious.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 1e135a71860f..ecd2ecc61b14 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -754,7 +754,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
*--dp = msg->ctrl;
if (gsm->initiator)
- *--dp = (msg->addr << 2) | 2 | EA;
+ *--dp = (msg->addr << 2) | CR | EA;
else
*--dp = (msg->addr << 2) | EA;
*fcs = gsm_fcs_add_block(INIT_FCS, dp , msg->data - dp);
--
2.25.1
On Thu, Apr 14, 2022 at 02:42:20AM -0700, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> Currently the peer is not informed about the initial state of the modem
> control lines after a new DLCI has been opened.
> Fix this by sending the initial modem control line states after DLCI open.
>
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: [email protected]
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index f3fb66be8513..e9a7d9483c1f 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -369,7 +369,11 @@ static const u8 gsm_fcs8[256] = {
> #define INIT_FCS 0xFF
> #define GOOD_FCS 0xCF
>
> +/*
> + * Prototypes
> + */
We know they are prototypes, no need to say it :)
> static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
> +static int gsmtty_modem_update(struct gsm_dlci *dlci, u8 brk);
>
> /**
> * gsm_fcs_add - update FCS
> @@ -1479,6 +1483,8 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
> pr_debug("DLCI %d goes open.\n", dlci->addr);
> /* Register gsmtty driver,report gsmtty dev add uevent for user */
> tty_register_device(gsm_tty_driver, dlci->addr, NULL);
> + if (dlci->addr) /* Send current modem state */
> + gsmtty_modem_update(dlci, 0);
Please do not put comments at the end of a line like this.
thanks,
greg k-h
From: Daniel Starke <[email protected]>
Remove commented out code as it is never used and if anyone accidentally
turned it on, it would be broken.
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 07d03447cdfd..1b4077006744 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1894,10 +1894,6 @@ static void gsm_queue(struct gsm_mux *gsm)
case UI|PF:
case UIH:
case UIH|PF:
-#if 0
- if (cr)
- goto invalid;
-#endif
if (dlci == NULL || dlci->state != DLCI_OPEN) {
gsm_command(gsm, address, DM|PF);
return;
--
2.25.1
From: Daniel Starke <[email protected]>
Currently the peer is not informed about the initial state of the modem
control lines after a new DLCI has been opened.
Fix this by sending the initial modem control line states after DLCI open.
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
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 f3fb66be8513..07d03447cdfd 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -370,6 +370,7 @@ static const u8 gsm_fcs8[256] = {
#define GOOD_FCS 0xCF
static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
+static int gsmtty_modem_update(struct gsm_dlci *dlci, u8 brk);
/**
* gsm_fcs_add - update FCS
@@ -1479,6 +1480,9 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
pr_debug("DLCI %d goes open.\n", dlci->addr);
/* Register gsmtty driver,report gsmtty dev add uevent for user */
tty_register_device(gsm_tty_driver, dlci->addr, NULL);
+ /* Send current modem state */
+ if (dlci->addr)
+ gsmtty_modem_update(dlci, 0);
wake_up(&dlci->gsm->event);
}
--
2.25.1
On Tue, Apr 19, 2022 at 01:07:24AM -0700, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> Currently the peer is not informed about the initial state of the modem
> control lines after a new DLCI has been opened.
> Fix this by sending the initial modem control line states after DLCI open.
>
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: [email protected]
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> 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 f3fb66be8513..07d03447cdfd 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -370,6 +370,7 @@ static const u8 gsm_fcs8[256] = {
> #define GOOD_FCS 0xCF
>
> static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
> +static int gsmtty_modem_update(struct gsm_dlci *dlci, u8 brk);
>
> /**
> * gsm_fcs_add - update FCS
> @@ -1479,6 +1480,9 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
> pr_debug("DLCI %d goes open.\n", dlci->addr);
> /* Register gsmtty driver,report gsmtty dev add uevent for user */
> tty_register_device(gsm_tty_driver, dlci->addr, NULL);
> + /* Send current modem state */
> + if (dlci->addr)
> + gsmtty_modem_update(dlci, 0);
> wake_up(&dlci->gsm->event);
> }
>
> --
> 2.25.1
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for what needs to be done
here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
On Tue, Apr 19, 2022 at 07:10:43AM +0000, Starke, Daniel wrote:
> > 42:21AM -0700, D. Starke wrote:
> > > From: Daniel Starke <[email protected]>
> > >
> > > n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> > > See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> > > The changes from 07.010 to 27.010 are non-functional. Therefore, I
> > > refer to the newer 27.010 here. Chapter 5.4.3.1 states the CR bit in
> > > UI and UIH frames shall always be set 1 by the initiator and 0 by the responder.
> >
> > This has nothing to do with the change you made here.
> >
> >
> > > Currently, gsm_queue() has a pre-processor gated (excluded) check
> > > which treats all frames that conform to the standard as malformed frames.
> > > Remove this optional code to avoid confusion and possible breaking
> > > changes in case that someone includes it.
> >
> > Again, nothing to do with the code change.
>
> Including this code (i.e. with #if 1) will treat every correct UI/UIH frame
> as invalid, because the cr flag is always set to 1 for those frames
> (as mentioned in chapter 5.4.3.1 of the standard). This is obviously wrong.
>
> > >
> > > Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> >
> > This "fixes" nothing :(
>
> What is the correct way to handle the removal of such dead and obviously
> wrong code?
>
> > > Cc: [email protected]
> >
> > How is commenting out unused code a stable backport requirement?
>
> True, it does not change the behavior but it fixes a commit which is also
> present in the current stable release. I was unsure how to handle this
> case. I will remove the backport remark.
>
> > > Signed-off-by: Daniel Starke <[email protected]>
> > > ---
> > > drivers/tty/n_gsm.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index
> > > e9a7d9483c1f..f4ec48c0d6d7 100644
> > > --- a/drivers/tty/n_gsm.c
> > > +++ b/drivers/tty/n_gsm.c
> > > @@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
> > > case UI|PF:
> > > case UIH:
> > > case UIH|PF:
> > > -#if 0
> > > - if (cr)
> > > - goto invalid;
> > > -#endif
> >
> > All you are doing is cleaning up dead code. Not a big deal, and not
> > worth all the text above to confuse people :(
>
> As mentioned above, this is not only dead but also wrong code. I tried to
> elaborate the reason for it being wrong code in the text above.
That's fine, then just submit a patch that says:
Remove commented out code as it is never used and if anyone
accidentally turned it on, it would be broken.
We remove dead code like this all the time, it's not a "fix" as nothing
is broken as-is.
thanks,
greg k-h
On Tue, Apr 19, 2022 at 01:17:13AM -0700, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> Remove commented out code as it is never used and if anyone accidentally
> turned it on, it would be broken.
>
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 07d03447cdfd..1b4077006744 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1894,10 +1894,6 @@ static void gsm_queue(struct gsm_mux *gsm)
> case UI|PF:
> case UIH:
> case UIH|PF:
> -#if 0
> - if (cr)
> - goto invalid;
> -#endif
> if (dlci == NULL || dlci->state != DLCI_OPEN) {
> gsm_command(gsm, address, DM|PF);
> return;
> --
> 2.25.1
>
As I have already taken the majority of this series, just send a new
series with the remaining changes, and properly document what is
different here from the original version, as the documentation asks you
to.
thanks,
greg k-h
> > From: Daniel Starke <[email protected]>
> >
> > gsm_carrier_raised() returns always 1 if the kernel module parameter
> > 'debug' has its second least significant bit set. This changes the
> > behavior of the module instead of just adding some debug output.
> > Remove this 'debug' gated early out to avoid debug settings from
> > changing the program flow.
> >
> > Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> > Cc: [email protected]
>
> Why is this relevant for stable backporting? It's a debugging feature
> only, and you are changing how it previously worked :(
You are right. This was a mistake on my side. Please ignore this commit
completely.
Best regards,
Daniel Starke
On Tue, Apr 19, 2022 at 01:17:13AM -0700, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> Remove commented out code as it is never used and if anyone accidentally
> turned it on, it would be broken.
>
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 07d03447cdfd..1b4077006744 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1894,10 +1894,6 @@ static void gsm_queue(struct gsm_mux *gsm)
> case UI|PF:
> case UIH:
> case UIH|PF:
> -#if 0
> - if (cr)
> - goto invalid;
> -#endif
> if (dlci == NULL || dlci->state != DLCI_OPEN) {
> gsm_command(gsm, address, DM|PF);
> return;
> --
> 2.25.1
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for what needs to be done
here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
> 42:21AM -0700, D. Starke wrote:
> > From: Daniel Starke <[email protected]>
> >
> > n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> > See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> > The changes from 07.010 to 27.010 are non-functional. Therefore, I
> > refer to the newer 27.010 here. Chapter 5.4.3.1 states the CR bit in
> > UI and UIH frames shall always be set 1 by the initiator and 0 by the responder.
>
> This has nothing to do with the change you made here.
>
>
> > Currently, gsm_queue() has a pre-processor gated (excluded) check
> > which treats all frames that conform to the standard as malformed frames.
> > Remove this optional code to avoid confusion and possible breaking
> > changes in case that someone includes it.
>
> Again, nothing to do with the code change.
Including this code (i.e. with #if 1) will treat every correct UI/UIH frame
as invalid, because the cr flag is always set to 1 for those frames
(as mentioned in chapter 5.4.3.1 of the standard). This is obviously wrong.
> >
> > Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
>
> This "fixes" nothing :(
What is the correct way to handle the removal of such dead and obviously
wrong code?
> > Cc: [email protected]
>
> How is commenting out unused code a stable backport requirement?
True, it does not change the behavior but it fixes a commit which is also
present in the current stable release. I was unsure how to handle this
case. I will remove the backport remark.
> > Signed-off-by: Daniel Starke <[email protected]>
> > ---
> > drivers/tty/n_gsm.c | 4 ----
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index
> > e9a7d9483c1f..f4ec48c0d6d7 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
> > case UI|PF:
> > case UIH:
> > case UIH|PF:
> > -#if 0
> > - if (cr)
> > - goto invalid;
> > -#endif
>
> All you are doing is cleaning up dead code. Not a big deal, and not
> worth all the text above to confuse people :(
As mentioned above, this is not only dead but also wrong code. I tried to
elaborate the reason for it being wrong code in the text above.
Best regards,
Daniel Starke