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.3.7 describes the encoding of the
control signal octet used by the MSC (modem status command). The same
encoding is also used in convergence layer type 2 as described in chapter
5.5.2. Table 7 and 24 both require the DV (data valid) bit to be set 1 for
outgoing control signal octets sent by the DTE (data terminal equipment),
i.e. for the initiator side.
Currently, the DV bit is only set if CD (carrier detect) is on, regardless
of the side.
This patch fixes this behavior by setting the DV bit on the initiator side
unconditionally.
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 0b1808e3a912..e199315a158e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -439,7 +439,7 @@ static u8 gsm_encode_modem(const struct gsm_dlci *dlci)
modembits |= MDM_RTR;
if (dlci->modem_tx & TIOCM_RI)
modembits |= MDM_IC;
- if (dlci->modem_tx & TIOCM_CD)
+ if (dlci->modem_tx & TIOCM_CD || dlci->gsm->initiator)
modembits |= MDM_DV;
return modembits;
}
--
2.25.1
The here fixed commit made the tty hangup asynchronous to avoid a circular
locking warning. I could not reproduce this warning. Furthermore, due to
the asynchronous hangup the function call now gets queued up while the
underlying tty is being freed. Depending on the timing this results in a
NULL pointer access in the global work queue scheduler. To be precise in
process_one_work(). Therefore, the previous commit made the issue worse
which it tried to fix.
This patch fixes this by falling back to the old behavior which uses a
blocking tty hangup call before freeing up the associated tty.
Fixes: 7030082a7415 ("tty: n_gsm: avoid recursive locking with async port hangup")
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 0b1808e3a912..e63154ef0b6c 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1748,7 +1748,12 @@ static void gsm_dlci_release(struct gsm_dlci *dlci)
gsm_destroy_network(dlci);
mutex_unlock(&dlci->mutex);
- tty_hangup(tty);
+ /* We cannot use tty_hangup() because in tty_kref_put() the tty
+ * driver assumes that the hangup queue is free and reuses it to
+ * queue release_one_tty() -> NULL pointer panic in
+ * process_one_work().
+ */
+ tty_vhangup(tty);
tty_port_tty_set(&dlci->port, NULL);
tty_kref_put(tty);
--
2.25.1
Trying to open a DLCI by sending a SABM frame may fail with a timeout.
The link is closed on the initiator side without informing the responder
about this event. The responder assumes the link is open after sending a
UA frame to answer the SABM frame. The link gets stuck in a half open
state.
This patch fixes this by initiating the proper link termination procedure
after link setup timeout instead of silently closing it down.
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 0b1808e3a912..52224a3494a0 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1514,7 +1514,7 @@ static void gsm_dlci_t1(struct timer_list *t)
dlci->mode = DLCI_MODE_ADM;
gsm_dlci_open(dlci);
} else {
- gsm_dlci_close(dlci);
+ gsm_dlci_begin_close(dlci); /* prevent half open link */
}
break;
--
2.25.1
> All of these are patch 1/1, which is odd :(
>
> Please renumber them properly and resend.
All these patches are based on the current version of tty-next and are
completely independent from each other. The only common part is the file
they apply to. Therefore, this is not a patch series. Would you still
suggest to apply a different numbering?
With best regards,
Daniel Starke
tty flow control is handled via gsmtty_throttle() and gsmtty_unthrottle().
Both functions propagate the outgoing hardware flow control state to the
remote side via MSC (modem status command) frames. The local state is taken
from the RTS (ready to send) flag of the tty. However, RTS gets mapped to
DTR (data terminal ready), which is wrong.
This patch corrects this by mapping RTS to RTS.
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, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 0b1808e3a912..d57fd055b489 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3234,9 +3234,9 @@ static void gsmtty_throttle(struct tty_struct *tty)
if (dlci->state == DLCI_CLOSED)
return;
if (C_CRTSCTS(tty))
- dlci->modem_tx &= ~TIOCM_DTR;
+ dlci->modem_tx &= ~TIOCM_RTS;
dlci->throttled = true;
- /* Send an MSC with DTR cleared */
+ /* Send an MSC with RTS cleared */
gsmtty_modem_update(dlci, 0);
}
@@ -3246,9 +3246,9 @@ static void gsmtty_unthrottle(struct tty_struct *tty)
if (dlci->state == DLCI_CLOSED)
return;
if (C_CRTSCTS(tty))
- dlci->modem_tx |= TIOCM_DTR;
+ dlci->modem_tx |= TIOCM_RTS;
dlci->throttled = false;
- /* Send an MSC with DTR set */
+ /* Send an MSC with RTS set */
gsmtty_modem_update(dlci, 0);
}
--
2.25.1
On Thu, Feb 17, 2022 at 12:05:49AM -0800, [email protected] wrote:
> 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.3.7 describes the encoding of the
> control signal octet used by the MSC (modem status command). The same
> encoding is also used in convergence layer type 2 as described in chapter
> 5.5.2. Table 7 and 24 both require the DV (data valid) bit to be set 1 for
> outgoing control signal octets sent by the DTE (data terminal equipment),
> i.e. for the initiator side.
> Currently, the DV bit is only set if CD (carrier detect) is on, regardless
> of the side.
>
> This patch fixes this behavior by setting the DV bit on the initiator side
> unconditionally.
>
> 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 0b1808e3a912..e199315a158e 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -439,7 +439,7 @@ static u8 gsm_encode_modem(const struct gsm_dlci *dlci)
> modembits |= MDM_RTR;
> if (dlci->modem_tx & TIOCM_RI)
> modembits |= MDM_IC;
> - if (dlci->modem_tx & TIOCM_CD)
> + if (dlci->modem_tx & TIOCM_CD || dlci->gsm->initiator)
> modembits |= MDM_DV;
> return modembits;
> }
> --
> 2.25.1
>
All of these are patch 1/1, which is odd :(
Please renumber them properly and resend.
thanks,
greg k-h
> > > All of these are patch 1/1, which is odd :(
> > >
> > > Please renumber them properly and resend.
> >
> > All these patches are based on the current version of tty-next and are
> > completely independent from each other. The only common part is the
> > file they apply to. Therefore, this is not a patch series. Would you
> > still suggest to apply a different numbering?
>
> Yes, please send them as a patch series, numbered correctly, as a whole
> series of patches, all listed as 1/1 does not do good things for our
> tools.
I have noticed that all patches have been sent in one thread. This was not
my intention. I see two options here now:
1. Resend all patches one by one as they are completely independent.
2. Create a patch series.
And the following two variants in case of option 2:
a. Just renumber the subjects and resend all (i.e. without a cover letter).
b. Create a patch series from commits that depend on each other. Send this
together with a cover letter.
Please let me know what you prefer.
With best regards,
Daniel Starke
In the current implementation the user may open a virtual tty which then
could fail to establish the underlying DLCI. The function gsmtty_open()
gets stuck in tty_port_block_til_ready() while waiting for a carrier rise.
This happens if the remote side fails to acknowledge the link establishment
request in time or completely. At some point gsm_dlci_close() is called
to abort the link establishment attempt. The function tries to inform the
associated virtual tty by performing a hangup. But the blocking loop within
tty_port_block_til_ready() is not informed about this event.
The patch proposed here fixes this by resetting the initialization state of
the virtual tty to ensure the loop exits and triggering it to make
tty_port_block_til_ready() return.
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 insertions(+)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 0b1808e3a912..e61a47c349e3 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1451,6 +1451,9 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
if (dlci->addr != 0) {
tty_port_tty_hangup(&dlci->port, false);
kfifo_reset(&dlci->fifo);
+ /* Ensure that gsmtty_open() can return. */
+ tty_port_set_initialized(&dlci->port, 0);
+ wake_up_interruptible(&dlci->port.open_wait);
} else
dlci->gsm->dead = true;
/* Unregister gsmtty driver,report gsmtty dev remove uevent for user */
--
2.25.1
On Thu, Feb 17, 2022 at 10:45:14AM +0000, Starke, Daniel wrote:
> > All of these are patch 1/1, which is odd :(
> >
> > Please renumber them properly and resend.
>
> All these patches are based on the current version of tty-next and are
> completely independent from each other. The only common part is the file
> they apply to. Therefore, this is not a patch series. Would you still
> suggest to apply a different numbering?
Yes, please send them as a patch series, numbered correctly, as a whole
series of patches, all listed as 1/1 does not do good things for our
tools.
thanks,
greg k-h
On Thu, Feb 17, 2022 at 02:03:37PM +0000, Starke, Daniel wrote:
> > > > All of these are patch 1/1, which is odd :(
> > > >
> > > > Please renumber them properly and resend.
> > >
> > > All these patches are based on the current version of tty-next and are
> > > completely independent from each other. The only common part is the
> > > file they apply to. Therefore, this is not a patch series. Would you
> > > still suggest to apply a different numbering?
> >
> > Yes, please send them as a patch series, numbered correctly, as a whole
> > series of patches, all listed as 1/1 does not do good things for our
> > tools.
>
> I have noticed that all patches have been sent in one thread. This was not
> my intention. I see two options here now:
> 1. Resend all patches one by one as they are completely independent.
> 2. Create a patch series.
>
> And the following two variants in case of option 2:
> a. Just renumber the subjects and resend all (i.e. without a cover letter).
This is fine, and the easiest for everyone involved, as git makes it
automatic.
thanks,
greg k-h