All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH v3 6/9] tty: n_gsm: fix deadlock and link starvation in outgoing data path
@ 2022-06-28  9:41 Starke, Daniel
  2022-06-30 14:59 ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Starke, Daniel @ 2022-06-28  9:41 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-serial, jirislaby, linux-kernel

> Please read the stable kernel rules for what is, and is not allowed.
> Generally a patch that does:
>  drivers/tty/n_gsm.c | 410 ++++++++++++++++++++++++++++++--------------
> is not allowed.
> 
> Please sort by what is stable fixes, and what is not.  Given that you
> don't seem to want to backport patches to older stable kernels when they
> fail to apply, why are any of these needed in stable kernels if the older
> ones are not also going to be merged there?

Indeed, the documentation disallows such big backports as I have just found
out.
Initially, I added the stable backport remark as suggested by you for my
very first patch.
We have split our complete change set in small patches, re-ordered the
changes to submit bug fixes first and documented all changes pedantically.
Supporting all the backports turns out to come with much higher effort as
we have anticipated. We do not have the resources to support this.
For us it is sufficient to keep all our changes on the n_gsm driver in the
mainline branch. Also for all upcoming patches.
We have already done various integration tests with the final version of
the n_gsm driver with Triorail and Funkwerk mobiles. It is part of our
commercial product which has an official approval. The quality and feature
level (3GPP standard conformance) of the n_gsm driver has been
significantly improved by our changes. And we would like to share this with
the community. This would bring the n_gsm driver out of its experimental
state.
Hence, I would like to resubmit this patch series without any stable
backport remark to avoid unmanageable effort.

Best regards,
Daniel Starke

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH v3 6/9] tty: n_gsm: fix deadlock and link starvation in outgoing data path
@ 2022-06-23 12:26 Starke, Daniel
  2022-06-27 11:54 ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Starke, Daniel @ 2022-06-23 12:26 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-serial, jirislaby, linux-kernel

> This is a bit huge for stable backports, especially given that a huge
> number of the previous stable backports have totally failed and no one
> has submitted new versions.
> 
> So why is this needed for stable?  Same for all of these in the series...

Given the fact that these are all bug fixes I assumed that these are also
relevant for backporting. Maybe only in the more recent stable releases
if there are issues with the merges.
I do not mind removing the stable kernel reference and keep these changes
only for mainline. So please let me know your preference.
Should I resubmit the patches without stable reference?

Best regards,
Daniel Starke

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v3 1/9] tty: n_gsm: fix user open not possible at responder until initiator open
@ 2022-05-30 14:45 D. Starke
  2022-05-30 14:45 ` [PATCH v3 6/9] tty: n_gsm: fix deadlock and link starvation in outgoing data path D. Starke
  0 siblings, 1 reply; 6+ messages in thread
From: D. Starke @ 2022-05-30 14:45 UTC (permalink / raw)
  To: linux-serial, gregkh, jirislaby; +Cc: linux-kernel, Daniel Starke

From: Daniel Starke <daniel.starke@siemens.com>

After setting up the control channel on both sides the responder side may
want to open a virtual tty to listen on until the initiator starts an
application on a user channel. The current implementation allows the
open() but no other operation, like termios. These fail with EINVAL.
The responder sided application has no means to detect an open by the
initiator sided application this way. And the initiator sided applications
usually expect the responder sided application to listen on the user
channel upon open.
Set the user channel into half-open state on responder side once a user
application opens the virtual tty to allow IO operations on it.
Furthermore, keep the user channel constipated until the initiator side
opens it to give the responder sided application the chance to detect the
new connection and to avoid data loss if the responder sided application
starts sending before the user channel is open.

Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

Renamed gsm_dlci_wait_open() to gsm_dlci_set_opening() and reworded its
documentation accordingly to reflect its function. This was done due to the
review comment on v2.

Link: https://lore.kernel.org/all/YoZtlq3RkNU56xFx@kroah.com/
Link: https://lore.kernel.org/all/20220519070757.2096-1-daniel.starke@siemens.com/

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index fd8b86dde525..63314fe5e43b 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1493,6 +1493,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
 	if (debug & 8)
 		pr_debug("DLCI %d goes closed.\n", dlci->addr);
 	dlci->state = DLCI_CLOSED;
+	/* Prevent us from sending data before the link is up again */
+	dlci->constipated = true;
 	if (dlci->addr != 0) {
 		tty_port_tty_hangup(&dlci->port, false);
 		spin_lock_irqsave(&dlci->lock, flags);
@@ -1522,6 +1524,7 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
 	del_timer(&dlci->t1);
 	/* This will let a tty open continue */
 	dlci->state = DLCI_OPEN;
+	dlci->constipated = false;
 	if (debug & 8)
 		pr_debug("DLCI %d goes open.\n", dlci->addr);
 	/* Send current modem state */
@@ -1602,6 +1605,25 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
 	mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
 }
 
+/**
+ *	gsm_dlci_set_opening	-	change state to opening
+ *	@dlci: DLCI to open
+ *
+ *	Change internal state to wait for DLCI open from initiator side.
+ *	We set off timers and responses upon reception of an SABM.
+ */
+static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
+{
+	switch (dlci->state) {
+	case DLCI_CLOSED:
+	case DLCI_CLOSING:
+		dlci->state = DLCI_OPENING;
+		break;
+	default:
+		break;
+	}
+}
+
 /**
  *	gsm_dlci_begin_close	-	start channel open procedure
  *	@dlci: DLCI to open
@@ -1745,10 +1767,13 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 	dlci->addr = addr;
 	dlci->adaption = gsm->adaption;
 	dlci->state = DLCI_CLOSED;
-	if (addr)
+	if (addr) {
 		dlci->data = gsm_dlci_data;
-	else
+		/* Prevent us from sending data before the link is up */
+		dlci->constipated = true;
+	} else {
 		dlci->data = gsm_dlci_command;
+	}
 	gsm->dlci[addr] = dlci;
 	return dlci;
 }
@@ -3163,6 +3188,8 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
 	/* Start sending off SABM messages */
 	if (gsm->initiator)
 		gsm_dlci_begin_open(dlci);
+	else
+		gsm_dlci_set_opening(dlci);
 	/* And wait for virtual carrier */
 	return tty_port_block_til_ready(port, tty, filp);
 }
-- 
2.34.1


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

end of thread, other threads:[~2022-06-30 14:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-28  9:41 [PATCH v3 6/9] tty: n_gsm: fix deadlock and link starvation in outgoing data path Starke, Daniel
2022-06-30 14:59 ` Greg KH
  -- strict thread matches above, loose matches on Subject: below --
2022-06-23 12:26 Starke, Daniel
2022-06-27 11:54 ` Greg KH
2022-05-30 14:45 [PATCH v3 1/9] tty: n_gsm: fix user open not possible at responder until initiator open D. Starke
2022-05-30 14:45 ` [PATCH v3 6/9] tty: n_gsm: fix deadlock and link starvation in outgoing data path D. Starke
2022-06-10 11:55   ` Greg KH

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.