Skip to contentSkip to conversation

: Mux n_gsm: Add a DLCI hangup state and callback

PATCH In review Gwenn Bourrée 3 participants 3 messages Jun 29, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 : Mux n_gsm: Add a DLCI hangup state and callback

Dear kernel tty maintainers,

Please review the following patch:

From 6e006bd522124d0e8a2f6075099a21f7051a426f Mon Sep 17 00:00:00 2001
From: Gwenn Bourree <[email protected]>
Date: Mon, 29 Jun 2015 17:26:01 +0200
Subject: [PATCH] Mux n_gsm: Add a DLCI hangup state and callback

Use of asynchronous hangup instead of vhangup.

---
 drivers/tty/n_gsm.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)
drivers/tty/n_gsm.c +1 -0
index d6e0ea0..762f555 100644
@@ -135,6 +135,7 @@ struct gsm_dlci {
 #define DLCI_OPENING		1	/* Sending SABM not seen UA */
 #define DLCI_OPEN		2	/* SABM/UA complete */
 #define DLCI_CLOSING		3	/* Sending DISC not seen UA/DM */
+#define DLCI_HANGUP		4	/*HANGUP received  */
 	struct mutex mutex;
 
 	/* Link layer */
@@ -1530,7 +1531,8 @@ static void gsm_dlci_begin_open(struct gsm_dlci

*dlci)
static void gsm_dlci_begin_close(struct gsm_dlci *dlci) {
struct gsm_mux *gsm = dlci->gsm;

-	if (dlci->state == DLCI_CLOSED || dlci->state == DLCI_CLOSING)
+	if (dlci->state == DLCI_CLOSED || dlci->state == DLCI_CLOSING ||

+ dlci->state == DLCI_HANGUP)
return;
dlci->retries = gsm->n2;
dlci->state = DLCI_CLOSING;
@@ -1717,7 +1719,7 @@ static void gsm_dlci_release(struct gsm_dlci *dlci)
gsm_destroy_network(dlci); mutex_unlock(&dlci->mutex);

- tty_vhangup(tty);
+ tty_hangup(tty);

tty_port_tty_set(&dlci->port, NULL); tty_kref_put(tty);
@@ -2339,6 +2341,26 @@ static void gsmld_flush_buffer(struct tty_struct *tty)
}

/**
+ * gsmld_hangup - hangup the ldisc for this tty
+ * @tty: device
+ */
+
+static int gsmld_hangup(struct tty_struct *tty)
+{
+ struct gsm_mux *gsm = tty->disc_data;
+ int i;
+ struct gsm_dlci *dlci;
+
+ for (i = NUM_DLCI-1; i >= 0; i--) {
+ dlci = gsm->dlci[i];
+ if (dlci)
+ dlci->state = DLCI_HANGUP;
+ }
+
+ return 0;
+}
+
+/**
* gsmld_close - close the ldisc for this tty
* @tty: device
*
@@ -2836,6 +2858,7 @@ static struct tty_ldisc_ops tty_ldisc_packet = {

 	.name            = "n_gsm",
 	.open            = gsmld_open,
 	.close           = gsmld_close,
+	.hangup          = gsmld_hangup,
 	.flush_buffer    = gsmld_flush_buffer,

.chars_in_buffer = gsmld_chars_in_buffer,
.read = gsmld_read,

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company) Registered headquarters: "Les Montalets"- 2, rue de Paris, 92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

maintainer
#1 lore raw
Quoted 18 lines · Gwenn Bourrée

Dear kernel tty maintainers,

Please review the following patch:

From 6e006bd522124d0e8a2f6075099a21f7051a426f Mon Sep 17 00:00:00 2001
From: Gwenn Bourree <[email protected]>
Date: Mon, 29 Jun 2015 17:26:01 +0200
Subject: [PATCH] Mux n_gsm: Add a DLCI hangup state and callback

Use of asynchronous hangup instead of vhangup.

Quoted 3 lines
---
 drivers/tty/n_gsm.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

Same comments on this one, but as you are wanting to send a series of patches, send a series of patches, each one numbered properly, otherwise I have no idea what order to apply them in.

greg k-h

#2 lore raw
In reply to Gwenn Bourrée

Hi Gwenn,

Quoted 6 lines

Please review the following patch:

From 6e006bd522124d0e8a2f6075099a21f7051a426f Mon Sep 17 00:00:00 2001
From: Gwenn Bourree <[email protected]>
Date: Mon, 29 Jun 2015 17:26:01 +0200
Subject: [PATCH] Mux n_gsm: Add a DLCI hangup state and callback

this is borked. Consider using git format-patch and git send-email.

Quoted 2 lines

Use of asynchronous hangup instead of vhangup.

In general it is useful to have a bit more explanation in your patch on what it does, why it does it and how.

Quoted 3 lines

You do not need to have yourself twice here.

Quoted 3 lines

Please make sure reviewed-by are correct. I would use first name last name <email>. And make sure to close the email with >

Quoted 10 lines

---
drivers/tty/n_gsm.c | 27 +++++++++++++++++++++++++--

1 file changed, 25 insertions(+), 2 deletions(-)
drivers/tty/n_gsm.c +0 -0
index d6e0ea0..762f555 100644

drivers/tty/n_gsm.c struct gsm_dlci { · line 138

Quoted 4 lines
drivers/tty/n_gsm.c +1 -0
@@ -135,6 +135,7 @@ struct gsm_dlci {
 #define DLCI_OPENING		1	/* Sending SABM not seen UA */
 #define DLCI_OPEN		2	/* SABM/UA complete */
 #define DLCI_CLOSING		3	/* Sending DISC not seen UA/DM */
+#define DLCI_HANGUP		4	/*HANGUP received  */

Please follow coding style. The example of the comment is above.

drivers/tty/n_gsm.c static void gsm_dlci_begin_open(struct gsm_dlci · line 1536

Quoted 11 lines
drivers/tty/n_gsm.c +2 -1
 	struct mutex mutex;
 
 	/* Link layer */
@@ -1530,7 +1531,8 @@ static void gsm_dlci_begin_open(struct gsm_dlci
 *dlci)
 static void gsm_dlci_begin_close(struct gsm_dlci *dlci)
 {
 	struct gsm_mux *gsm = dlci->gsm;
-	if (dlci->state == DLCI_CLOSED || dlci->state == DLCI_CLOSING)
+	if (dlci->state == DLCI_CLOSED || dlci->state == DLCI_CLOSING ||
+		dlci->state == DLCI_HANGUP)

I am pretty sure this indentation is wrong. You can not tell the actual code block apart from the condition. So either align with the dlci->state above or use two tabs. Check what coding style is common in this code and choose the one used in similar multiline if conditions.

Quoted 20 lines
drivers/tty/n_gsm.c +0 -0
 		return;
 	dlci->retries = gsm->n2;
	dlci->state = DLCI_CLOSING;
drivers/tty/n_gsm.c +1 -1
@@ -1717,7 +1719,7 @@ static void gsm_dlci_release(struct gsm_dlci
 *dlci)
 		gsm_destroy_network(dlci);
 		mutex_unlock(&dlci->mutex);
 
-		tty_vhangup(tty);
+		tty_hangup(tty);
 
 		tty_port_tty_set(&dlci->port, NULL);
		tty_kref_put(tty);
drivers/tty/n_gsm.c +2 -0
@@ -2339,6 +2341,26 @@ static void gsmld_flush_buffer(struct tty_struct
 *tty)
 }
 
 /**
+ *	gsmld_hangup		-	hangup the ldisc for this tty
+ *	@tty: device

drivers/tty/n_gsm.c static void gsmld_flush_buffer(struct tty_struct · line 2356

Quoted 10 lines
drivers/tty/n_gsm.c +10 -0
+ */
+
+static int gsmld_hangup(struct tty_struct *tty)
+{
+	struct gsm_mux *gsm = tty->disc_data;
+	int i;
+	struct gsm_dlci *dlci;
+
+	for (i = NUM_DLCI-1; i >= 0; i--) {
+		dlci = gsm->dlci[i];

I would have moved the struct gsm_dlci declaration into the body of the for loop.

Quoted 12 lines
drivers/tty/n_gsm.c +8 -0
+		if (dlci)
+			dlci->state = DLCI_HANGUP;
+	}
+
+	return 0;
+}
+
+/**
 *	gsmld_close		-	close the ldisc for this tty
 *	@tty: device

*

drivers/tty/n_gsm.c static struct tty_ldisc_ops tty_ldisc_packet = { · line 2864

Quoted 7 lines
drivers/tty/n_gsm.c +1 -0
@@ -2836,6 +2858,7 @@ static struct tty_ldisc_ops tty_ldisc_packet = {
 	.name            = "n_gsm",
 	.open            = gsmld_open,
 	.close           = gsmld_close,
+	.hangup          = gsmld_hangup,
 	.flush_buffer    = gsmld_flush_buffer,
 	.chars_in_buffer = gsmld_chars_in_buffer,
 	.read            = gsmld_read,

Regards

Marcel