From: Daniel Starke <[email protected]>
Currently, changing the parameters of a DLCI gives no direct control to the
user whether this should trigger a channel reset or not. The decision is
solely made by the driver based on the assumption which parameter changes
are compatible or not. Therefore, the user has no means to perform an
automatic channel reset after parameter configuration for non-conflicting
changes.
Add the parameter 'flags' to 'gsm_dlci_config' to force a channel reset
after ioctl setting regardless of whether the changes made require this or
not by setting this to 'GSM_FL_RESTART'.
Note that 'GSM_FL_RESTART' is currently the only allow flag to allow
additions here.
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 4 ++++
include/uapi/linux/gsmmux.h | 13 ++++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
v3 -> v4:
Changed gsm_dlci_config field name from 'restart' to 'flags' and introduced
'GSM_FL_RESTART' to set the restart flag. The patch description was changed
accordingly. This was done as suggested during the review.
The remarked kernel doc compatible field comment is done in patch 2/8.
Link: https://lore.kernel.org/all/2023042453-dubbed-botany-2ed9@gregkh/
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index b411a26cc092..66edcf65a4dd 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2532,6 +2532,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
return -EINVAL;
if (dc->k > 7)
return -EINVAL;
+ if (dc->flags & ~GSM_FL_RESTART) /* allow future extensions */
+ return -EINVAL;
/*
* See what is needed for reconfiguration
@@ -2546,6 +2548,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
/* Requires care */
if (dc->priority != dlci->prio)
need_restart = true;
+ if (dc->flags & GSM_FL_RESTART)
+ need_restart = true;
if ((open && gsm->wait_config) || need_restart)
need_open = true;
diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index eb67884e5f38..958257af05ab 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -2,10 +2,20 @@
#ifndef _LINUX_GSMMUX_H
#define _LINUX_GSMMUX_H
+#include <linux/const.h>
#include <linux/if.h>
#include <linux/ioctl.h>
#include <linux/types.h>
+/*
+ * flags definition for n_gsm
+ *
+ * Used by:
+ * struct gsm_dlci_config.flags
+ */
+/* Force DLCI channel reset? Always cleared on retrieval. */
+#define GSM_FL_RESTART _BITUL(0)
+
struct gsm_config
{
unsigned int adaption;
@@ -58,7 +68,8 @@ struct gsm_dlci_config {
__u32 priority; /* Priority (0 for default value) */
__u32 i; /* Frame type (1 = UIH, 2 = UI) */
__u32 k; /* Window size (0 for default value) */
- __u32 reserved[8]; /* For future use, must be initialized to zero */
+ __u32 flags; /* DLCI specific flags. */
+ __u32 reserved[7]; /* For future use, must be initialized to zero */
};
#define GSMIOC_GETCONF_DLCI _IOWR('G', 7, struct gsm_dlci_config)
--
2.34.1
From: Daniel Starke <[email protected]>
Add counters for the number of data bytes received/transmitted per DLCI in
for preparation for an upcoming patch which will expose these values to the
user.
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
v3 -> v4:
No changes.
Link: https://lore.kernel.org/all/[email protected]/
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 62bff4474b57..2e2e1dafcf40 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -186,6 +186,9 @@ struct gsm_dlci {
void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
struct net_device *net; /* network interface, if created */
+ /* Statistics (not currently exposed) */
+ u64 tx; /* Data bytes sent on this DLCI */
+ u64 rx; /* Data bytes received on this DLCI */
};
/*
@@ -1216,6 +1219,7 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
tty_port_tty_wakeup(&dlci->port);
__gsm_data_queue(dlci, msg);
+ dlci->tx += len;
/* Bytes of data we used up */
return size;
}
@@ -1283,6 +1287,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
memcpy(dp, dlci->skb->data, len);
skb_pull(dlci->skb, len);
__gsm_data_queue(dlci, msg);
+ dlci->tx += len;
if (last) {
dev_kfree_skb_any(dlci->skb);
dlci->skb = NULL;
@@ -1461,6 +1466,7 @@ static int gsm_control_command(struct gsm_mux *gsm, int cmd, const u8 *data,
msg->data[1] = (dlen << 1) | EA;
memcpy(msg->data + 2, data, dlen);
gsm_data_queue(dlci, msg);
+ dlci->tx += dlen;
return 0;
}
@@ -1488,6 +1494,7 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
msg->data[1] = (dlen << 1) | EA;
memcpy(msg->data + 2, data, dlen);
gsm_data_queue(dlci, msg);
+ dlci->tx += dlen;
}
/**
@@ -1852,10 +1859,13 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
const u8 *data, int clen)
{
u8 buf[1];
+ struct gsm_dlci *dlci = gsm->dlci[0];
+
+ if (dlci)
+ dlci->rx += clen;
switch (command) {
case CMD_CLD: {
- struct gsm_dlci *dlci = gsm->dlci[0];
/* Modem wishes to close down */
if (dlci) {
dlci->dead = true;
@@ -1934,6 +1944,8 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
ctrl = gsm->pending_cmd;
dlci = gsm->dlci[0];
+ if (dlci)
+ dlci->rx += clen;
command |= 1;
/* Does the reply match our command */
if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
@@ -2298,6 +2310,9 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
need_pn = true;
}
+ dlci->tx = 0;
+ dlci->rx = 0;
+
switch (dlci->state) {
case DLCI_CLOSED:
case DLCI_WAITING_CONFIG:
@@ -2330,6 +2345,9 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
*/
static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
{
+ dlci->tx = 0;
+ dlci->rx = 0;
+
switch (dlci->state) {
case DLCI_CLOSED:
case DLCI_WAITING_CONFIG:
@@ -2349,6 +2367,9 @@ static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
*/
static void gsm_dlci_set_wait_config(struct gsm_dlci *dlci)
{
+ dlci->tx = 0;
+ dlci->rx = 0;
+
switch (dlci->state) {
case DLCI_CLOSED:
case DLCI_CLOSING:
@@ -2425,6 +2446,7 @@ static void gsm_dlci_data(struct gsm_dlci *dlci, const u8 *data, int clen)
fallthrough;
case 1: /* Line state will go via DLCI 0 controls only */
default:
+ dlci->rx += clen;
tty_insert_flip_string(port, data, clen);
tty_flip_buffer_push(port);
}
@@ -2785,6 +2807,7 @@ static void gsm_queue(struct gsm_mux *gsm)
gsm->open_error++;
return;
}
+ dlci->rx += gsm->len;
if (dlci->dead)
gsm_response(gsm, address, DM|PF);
else {
--
2.34.1
From: Daniel Starke <[email protected]>
The malformed counter in gsm_mux is already increased in case of errors
detected in gsm_queue() and gsm1_receive(). gsm_dlci_command() also
detects a case for a malformed frame but does not increase the malformed
counter yet.
Fix this by also increasing the gsm_mux malformed counter in case of a
malformed frame in gsm_dlci_command().
Note that the malformed counter is not yet exposed and only set internally.
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
v3 -> v4:
No changes.
Link: https://lore.kernel.org/all/[email protected]/
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 186f463f0f11..5b6a03668c78 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2455,8 +2455,10 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len)
data += dlen;
/* Malformed command? */
- if (clen > len)
+ if (clen > len) {
+ dlci->gsm->malformed++;
return;
+ }
if (command & 1)
gsm_control_message(dlci->gsm, command, data, clen);
--
2.34.1
From: Daniel Starke <[email protected]>
There are multiple places in gsm_control_command and gsm_control_reply that
derive the specific DLCI handle directly out of the DLCI table in gsm.
Add a local variable which holds this handle and use it instead to improve
code readability.
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
v3 -> v4:
No changes.
Link: https://lore.kernel.org/all/[email protected]/
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 42a8507aae4a..62bff4474b57 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1451,15 +1451,16 @@ static int gsm_control_command(struct gsm_mux *gsm, int cmd, const u8 *data,
int dlen)
{
struct gsm_msg *msg;
+ struct gsm_dlci *dlci = gsm->dlci[0];
- msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
+ msg = gsm_data_alloc(gsm, 0, dlen + 2, dlci->ftype);
if (msg == NULL)
return -ENOMEM;
msg->data[0] = (cmd << 1) | CR | EA; /* Set C/R */
msg->data[1] = (dlen << 1) | EA;
memcpy(msg->data + 2, data, dlen);
- gsm_data_queue(gsm->dlci[0], msg);
+ gsm_data_queue(dlci, msg);
return 0;
}
@@ -1478,14 +1479,15 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
int dlen)
{
struct gsm_msg *msg;
+ struct gsm_dlci *dlci = gsm->dlci[0];
- msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
+ msg = gsm_data_alloc(gsm, 0, dlen + 2, dlci->ftype);
if (msg == NULL)
return;
msg->data[0] = (cmd & 0xFE) << 1 | EA; /* Clear C/R */
msg->data[1] = (dlen << 1) | EA;
memcpy(msg->data + 2, data, dlen);
- gsm_data_queue(gsm->dlci[0], msg);
+ gsm_data_queue(dlci, msg);
}
/**
--
2.34.1
On Wed, Apr 26, 2023 at 10:03:08AM +0200, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> Currently, changing the parameters of a DLCI gives no direct control to the
> user whether this should trigger a channel reset or not. The decision is
> solely made by the driver based on the assumption which parameter changes
> are compatible or not. Therefore, the user has no means to perform an
> automatic channel reset after parameter configuration for non-conflicting
> changes.
>
> Add the parameter 'flags' to 'gsm_dlci_config' to force a channel reset
> after ioctl setting regardless of whether the changes made require this or
> not by setting this to 'GSM_FL_RESTART'.
>
> Note that 'GSM_FL_RESTART' is currently the only allow flag to allow
> additions here.
>
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 4 ++++
> include/uapi/linux/gsmmux.h | 13 ++++++++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> v3 -> v4:
> Changed gsm_dlci_config field name from 'restart' to 'flags' and introduced
> 'GSM_FL_RESTART' to set the restart flag. The patch description was changed
> accordingly. This was done as suggested during the review.
> The remarked kernel doc compatible field comment is done in patch 2/8.
>
> Link: https://lore.kernel.org/all/2023042453-dubbed-botany-2ed9@gregkh/
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index b411a26cc092..66edcf65a4dd 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2532,6 +2532,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
> return -EINVAL;
> if (dc->k > 7)
> return -EINVAL;
> + if (dc->flags & ~GSM_FL_RESTART) /* allow future extensions */
> + return -EINVAL;
>
> /*
> * See what is needed for reconfiguration
> @@ -2546,6 +2548,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
> /* Requires care */
> if (dc->priority != dlci->prio)
> need_restart = true;
> + if (dc->flags & GSM_FL_RESTART)
> + need_restart = true;
>
> if ((open && gsm->wait_config) || need_restart)
> need_open = true;
> diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
> index eb67884e5f38..958257af05ab 100644
> --- a/include/uapi/linux/gsmmux.h
> +++ b/include/uapi/linux/gsmmux.h
> @@ -2,10 +2,20 @@
> #ifndef _LINUX_GSMMUX_H
> #define _LINUX_GSMMUX_H
>
> +#include <linux/const.h>
> #include <linux/if.h>
> #include <linux/ioctl.h>
> #include <linux/types.h>
>
> +/*
> + * flags definition for n_gsm
> + *
> + * Used by:
> + * struct gsm_dlci_config.flags
> + */
> +/* Force DLCI channel reset? Always cleared on retrieval. */
I do not understand this comment, sorry. What question are you asking?
What happens if it is set? What happens if it is not set? More
documentation is always good, especially for new user/kernel apis that
are not documented anywhere else.
thanks,
greg k-h
On Wed, Apr 26, 2023 at 10:03:15AM +0200, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> Add counters for the number of data bytes received/transmitted per DLCI in
> for preparation for an upcoming patch which will expose these values to the
> user.
As this is patch 8/8, "upcoming patch" makes no sense, sorry. Please
either drop this and add it as part of the series that provides this
functionality, or add the functionality to the patch series as the next
patch in it.
>
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> v3 -> v4:
> No changes.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 62bff4474b57..2e2e1dafcf40 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -186,6 +186,9 @@ struct gsm_dlci {
> void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
> void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
> struct net_device *net; /* network interface, if created */
> + /* Statistics (not currently exposed) */
No blank line before this?
And why isn't this structure documented in kerneldoc to make it more
obvious what is happening?
thanks,
greg k-h
> > +/* Force DLCI channel reset? Always cleared on retrieval. */
>
> I do not understand this comment, sorry. What question are you asking?
> What happens if it is set? What happens if it is not set? More
> documentation is always good, especially for new user/kernel apis that
> are not documented anywhere else.
Ok, I will rephrase it to be more explicit about it.
Best regards,
Daniel Starke
> > From: Daniel Starke <[email protected]>
> >
> > Add counters for the number of data bytes received/transmitted per DLCI in
> > for preparation for an upcoming patch which will expose these values to the
> > user.
>
> As this is patch 8/8, "upcoming patch" makes no sense, sorry. Please
> either drop this and add it as part of the series that provides this
> functionality, or add the functionality to the patch series as the next
> patch in it.
I will add the remaining two outstanding patches on my side to this patch
series. The functional addition which exposes these values to the user is
included there.
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index 62bff4474b57..2e2e1dafcf40 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -186,6 +186,9 @@ struct gsm_dlci {
> > void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
> > void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
> > struct net_device *net; /* network interface, if created */
> > + /* Statistics (not currently exposed) */
>
> No blank line before this?
I will add a blank line before this one.
>
> And why isn't this structure documented in kerneldoc to make it more
> obvious what is happening?
True, it would be better if this was the case. However, there was no such
cleanup of this internal structure yet and this is out of scope of this
patch/patch series. A future patch needs to fix this.
Best regards,
Daniel Starke