LinuxLists.cc - [PATCH v2 1/3] tty: n_gsm: introduce macro for minimal unit size

2022-10-24 21:34:42

Subject: [PATCH v2 1/3] tty: n_gsm: introduce macro for minimal unit size

From: Daniel Starke <[email protected]>

n_gsm has a minimal protocol overhead of 7 bytes. The current code already
checks whether the configured MRU/MTU size is at least one byte more than
this.

Introduce the macro MIN_MTU to make this value more obvious.

Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

v1 -> v2:
Incorporated review comments.

Link: https://lore.kernel.org/all/[email protected]/

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5e516f5cac5a..570c40a3d78f 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -89,6 +89,7 @@ module_param(debug, int, 0600);
*/
#define MAX_MRU 1500
#define MAX_MTU 1500
+#define MIN_MTU (PROT_OVERHEAD + 1)
/* SOF, ADDR, CTRL, LEN1, LEN2, ..., FCS, EOF */
#define PROT_OVERHEAD 7
#define GSM_NET_TX_TIMEOUT (HZ*10)
@@ -2712,7 +2713,9 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
if ((c->adaption != 1 && c->adaption != 2) || c->k)
return -EOPNOTSUPP;
/* Check the MRU/MTU range looks sane */
- if (c->mru > MAX_MRU || c->mtu > MAX_MTU || c->mru < 8 || c->mtu < 8)
+ if (c->mru < MIN_MTU || c->mtu < MIN_MTU)
+ return -EINVAL;
+ if (c->mru > MAX_MRU || c->mtu > MAX_MTU)
return -EINVAL;
if (c->n2 > 255)
return -EINVAL;
@@ -3296,7 +3299,7 @@ static int gsm_create_network(struct gsm_dlci *dlci, struct gsm_netconfig *nc)
return -ENOMEM;
}
net->mtu = dlci->gsm->mtu;
- net->min_mtu = 8;
+ net->min_mtu = MIN_MTU;
net->max_mtu = dlci->gsm->mtu;
mux_net = netdev_priv(net);
mux_net->dlci = dlci;
--
2.34.1

2022-10-24 21:39:27

by D. Starke

[permalink] [raw]

Subject: [PATCH v2 2/3] tty: n_gsm: add parameters used with parameter negotiation

2022-10-25 01:30:45

by D. Starke

[permalink] [raw]

Subject: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

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.1.8.1.1 describes the parameter negotiation
messages and parameters. Chapter 5.4.1 states that the default parameters
are to be used if no negotiation is performed. Chapter 5.4.6.3.1 describes
the encoding of the parameter negotiation message. The meaning of the
parameters and allowed value ranges can be found in chapter 5.7.

Add parameter negotiation support accordingly. DLCI specific parameter
configuration by the user requires additional ioctls. This is subject to
another patch.

Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 335 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 327 insertions(+), 8 deletions(-)

v1 -> v2:
Incorporated review comments.
Simplification of command retry handling remains subject to future patches.

Link: https://lore.kernel.org/all/[email protected]/

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c217013b3e16..11d3730bf436 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -38,6 +38,7 @@
#include <linux/sched/signal.h>
#include <linux/interrupt.h>
#include <linux/tty.h>
+#include <linux/bitfield.h>
#include <linux/ctype.h>
#include <linux/mm.h>
#include <linux/math.h>
@@ -127,6 +128,7 @@ struct gsm_msg {

enum gsm_dlci_state {
DLCI_CLOSED,
+ DLCI_CONFIGURE, /* Sending PN (for adaption > 1) */
DLCI_OPENING, /* Sending SABM not seen UA */
DLCI_OPEN, /* SABM/UA complete */
DLCI_CLOSING, /* Sending DISC not seen UA/DM */
@@ -184,6 +186,30 @@ struct gsm_dlci {
struct net_device *net; /* network interface, if created */
};

+/*
+ * Parameter bits used for parameter negotiation according to 3GPP 27.010
+ * chapter 5.4.6.3.1.
+ */
+
+struct gsm_dlci_param_bits {
+ u8 d_bits;
+ u8 i_cl_bits;
+ u8 p_bits;
+ u8 t_bits;
+ __le16 n_bits;
+ u8 na_bits;
+ u8 k_bits;
+} __packed;
+
+#define PN_D_FIELD_DLCI GENMASK(5, 0)
+#define PN_I_CL_FIELD_FTYPE GENMASK(3, 0)
+#define PN_I_CL_FIELD_ADAPTION GENMASK(7, 4)
+#define PN_P_FIELD_PRIO GENMASK(5, 0)
+#define PN_T_FIELD_T1 GENMASK(7, 0)
+#define PN_N_FIELD_N1 GENMASK(15, 0)
+#define PN_NA_FIELD_N2 GENMASK(7, 0)
+#define PN_K_FIELD_K GENMASK(2, 0)
+
/* Total number of supported devices */
#define GSM_TTY_MINORS 256

@@ -411,6 +437,7 @@ static const u8 gsm_fcs8[256] = {
#define INIT_FCS 0xFF
#define GOOD_FCS 0xCF

+static void gsm_dlci_close(struct gsm_dlci *dlci);
static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
@@ -533,6 +560,59 @@ static void gsm_hex_dump_bytes(const char *fname, const u8 *data,
kfree(prefix);
}

+/**
+ * gsm_encode_params - encode DLCI parameters
+ * @dlci: DLCI to encode from
+ * @params: buffer to fill with the encoded parameters
+ *
+ * Encodes the parameters according to GSM 07.10 section 5.4.6.3.1
+ * table 3.
+ */
+static int gsm_encode_params(const struct gsm_dlci *dlci,
+ struct gsm_dlci_param_bits *params)
+{
+ const struct gsm_mux *gsm = dlci->gsm;
+ unsigned int i, cl;
+
+ switch (dlci->ftype) {
+ case UIH:
+ i = 0; /* UIH */
+ break;
+ case UI:
+ i = 1; /* UI */
+ break;
+ default:
+ pr_err("%s: unsupported frame type %d\n", __func__,
+ dlci->ftype);
+ return -EINVAL;
+ }
+
+ switch (dlci->adaption) {
+ case 1: /* Unstructured */
+ cl = 0; /* convergence layer type 1 */
+ break;
+ case 2: /* Unstructured with modem bits. */
+ cl = 1; /* convergence layer type 2 */
+ break;
+ default:
+ pr_err("%s: unsupported adaption %d\n", __func__,
+ dlci->adaption);
+ return -EINVAL;
+ }
+
+ params->d_bits = FIELD_PREP(PN_D_FIELD_DLCI, dlci->addr);
+ /* UIH, convergence layer type 1 */
+ params->i_cl_bits = FIELD_PREP(PN_I_CL_FIELD_FTYPE, i) |
+ FIELD_PREP(PN_I_CL_FIELD_ADAPTION, cl);
+ params->p_bits = FIELD_PREP(PN_P_FIELD_PRIO, dlci->prio);
+ params->t_bits = FIELD_PREP(PN_T_FIELD_T1, gsm->t1);
+ params->n_bits = cpu_to_le16(FIELD_PREP(PN_N_FIELD_N1, dlci->mtu));
+ params->na_bits = FIELD_PREP(PN_NA_FIELD_N2, gsm->n2);
+ params->k_bits = FIELD_PREP(PN_K_FIELD_K, dlci->k);
+
+ return 0;
+}
+
/**
* gsm_register_devices - register all tty devices for a given mux index
*
@@ -1450,6 +1530,116 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
dlci->modem_rx = mlines;
}

+/**
+ * gsm_process_negotiation - process received parameters
+ * @gsm: GSM channel
+ * @addr: DLCI address
+ * @cr: command/response
+ * @params: encoded parameters from the parameter negotiation message
+ *
+ * Used when the response for our parameter negotiation command was
+ * received.
+ */
+static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
+ unsigned int cr,
+ const struct gsm_dlci_param_bits *params)
+{
+ struct gsm_dlci *dlci = gsm->dlci[addr];
+ unsigned int ftype, i, adaption, prio, n1, k;
+
+ i = FIELD_GET(PN_I_CL_FIELD_FTYPE, params->i_cl_bits);
+ adaption = FIELD_GET(PN_I_CL_FIELD_ADAPTION, params->i_cl_bits) + 1;
+ prio = FIELD_GET(PN_P_FIELD_PRIO, params->p_bits);
+ n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));
+ k = FIELD_GET(PN_K_FIELD_K, params->k_bits);
+
+ if (n1 < MIN_MTU) {
+ if (debug & DBG_ERRORS)
+ pr_info("%s N1 out of range in PN\n", __func__);
+ return -EINVAL;
+ }
+
+ switch (i) {
+ case 0x00:
+ ftype = UIH;
+ break;
+ case 0x01:
+ ftype = UI;
+ break;
+ case 0x02: /* I frames are not supported */
+ if (debug & DBG_ERRORS)
+ pr_info("%s unsupported I frame request in PN\n",
+ __func__);
+ return -EINVAL;
+ default:
+ if (debug & DBG_ERRORS)
+ pr_info("%s i out of range in PN\n", __func__);
+ return -EINVAL;
+ }
+
+ if (!cr && gsm->initiator) {
+ if (adaption != dlci->adaption) {
+ if (debug & DBG_ERRORS)
+ pr_info("%s invalid adaption %d in PN\n",
+ __func__, adaption);
+ return -EINVAL;
+ }
+ if (prio != dlci->prio) {
+ if (debug & DBG_ERRORS)
+ pr_info("%s invalid priority %d in PN",
+ __func__, prio);
+ return -EINVAL;
+ }
+ if (n1 > gsm->mru || n1 > dlci->mtu) {
+ /* We requested a frame size but the other party wants
+ * to send larger frames. The standard allows only a
+ * smaller response value than requested (5.4.6.3.1).
+ */
+ if (debug & DBG_ERRORS)
+ pr_info("%s invalid N1 %d in PN\n", __func__,
+ n1);
+ return -EINVAL;
+ }
+ dlci->mtu = n1;
+ if (ftype != dlci->ftype) {
+ if (debug & DBG_ERRORS)
+ pr_info("%s invalid i %d in PN\n", __func__, i);
+ return -EINVAL;
+ }
+ if (ftype != UI && ftype != UIH && k > dlci->k) {
+ if (debug & DBG_ERRORS)
+ pr_info("%s invalid k %d in PN\n", __func__, k);
+ return -EINVAL;
+ }
+ dlci->k = k;
+ } else if (cr && !gsm->initiator) {
+ /* Only convergence layer type 1 and 2 are supported. */
+ if (adaption != 1 && adaption != 2) {
+ if (debug & DBG_ERRORS)
+ pr_info("%s invalid adaption %d in PN\n",
+ __func__, adaption);
+ return -EINVAL;
+ }
+ dlci->adaption = adaption;
+ if (n1 > gsm->mru) {
+ /* Propose a smaller value */
+ dlci->mtu = gsm->mru;
+ } else if (n1 > MAX_MTU) {
+ /* Propose a smaller value */
+ dlci->mtu = MAX_MTU;
+ } else {
+ dlci->mtu = n1;
+ }
+ dlci->prio = prio;
+ dlci->ftype = ftype;
+ dlci->k = k;
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
/**
* gsm_control_modem - modem status received
* @gsm: GSM channel
@@ -1503,6 +1693,65 @@ static void gsm_control_modem(struct gsm_mux *gsm, const u8 *data, int clen)
gsm_control_reply(gsm, CMD_MSC, data, clen);
}

+/**
+ * gsm_control_negotiation - parameter negotiation received
+ * @gsm: GSM channel
+ * @cr: command/response flag
+ * @data: data following command
+ * @dlen: data length
+ *
+ * We have received a parameter negotiation message. This is used by
+ * the GSM mux protocol to configure protocol parameters for a new DLCI.
+ */
+static void gsm_control_negotiation(struct gsm_mux *gsm, unsigned int cr,
+ const u8 *data, unsigned int dlen)
+{
+ unsigned int addr;
+ struct gsm_dlci_param_bits pn_reply;
+ struct gsm_dlci *dlci;
+ struct gsm_dlci_param_bits *params;
+
+ if (dlen < sizeof(struct gsm_dlci_param_bits))
+ return;
+
+ /* Invalid DLCI? */
+ params = (struct gsm_dlci_param_bits *)data;
+ addr = FIELD_GET(PN_D_FIELD_DLCI, params->d_bits);
+ if (addr == 0 || addr >= NUM_DLCI || !gsm->dlci[addr])
+ return;
+ dlci = gsm->dlci[addr];
+
+ /* Too late for parameter negotiation? */
+ if ((!cr && dlci->state == DLCI_OPENING) || dlci->state == DLCI_OPEN)
+ return;
+
+ /* Process the received parameters */
+ if (gsm_process_negotiation(gsm, addr, cr, params) != 0) {
+ /* Negotiation failed. Close the link. */
+ if (debug & DBG_ERRORS)
+ pr_info("%s PN failed\n", __func__);
+ gsm_dlci_close(dlci);
+ return;
+ }
+
+ if (cr) {
+ /* Reply command with accepted parameters. */
+ if (gsm_encode_params(dlci, &pn_reply) == 0)
+ gsm_control_reply(gsm, CMD_PN, (const u8 *)&pn_reply,
+ sizeof(pn_reply));
+ else if (debug & DBG_ERRORS)
+ pr_info("%s PN invalid\n", __func__);
+ } else if (dlci->state == DLCI_CONFIGURE) {
+ /* Proceed with link setup by sending SABM before UA */
+ dlci->state = DLCI_OPENING;
+ gsm_command(gsm, dlci->addr, SABM|PF);
+ mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+ } else {
+ if (debug & DBG_ERRORS)
+ pr_info("%s PN in invalid state\n", __func__);
+ }
+}
+
/**
* gsm_control_rls - remote line status
* @gsm: GSM channel
@@ -1612,8 +1861,12 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
/* Modem wishes to enter power saving state */
gsm_control_reply(gsm, CMD_PSC, NULL, 0);
break;
+ /* Optional commands */
+ case CMD_PN:
+ /* Modem sends a parameter negotiation command */
+ gsm_control_negotiation(gsm, 1, data, clen);
+ break;
/* Optional unsupported commands */
- case CMD_PN: /* Parameter negotiation */
case CMD_RPN: /* Remote port negotiation */
case CMD_SNC: /* Service negotiation command */
default:
@@ -1646,8 +1899,8 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
spin_lock_irqsave(&gsm->control_lock, flags);

ctrl = gsm->pending_cmd;
- /* Does the reply match our command */
command |= 1;
+ /* Does the reply match our command */
if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
/* Our command was replied to, kill the retry timer */
del_timer(&gsm->t2_timer);
@@ -1657,6 +1910,9 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
ctrl->error = -EOPNOTSUPP;
ctrl->done = 1;
wake_up(&gsm->event);
+ /* Or did we receive the PN response to our PN command */
+ } else if (command == CMD_PN) {
+ gsm_control_negotiation(gsm, 0, data, clen);
}
spin_unlock_irqrestore(&gsm->control_lock, flags);
}
@@ -1834,6 +2090,32 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
wake_up(&dlci->gsm->event);
}

+/**
+ * gsm_dlci_negotiate - start parameter negotiation
+ * @dlci: DLCI to open
+ *
+ * Starts the parameter negotiation for the new DLCI. This needs to be done
+ * before the DLCI initialized the channel via SABM.
+ */
+static int gsm_dlci_negotiate(struct gsm_dlci *dlci)
+{
+ struct gsm_mux *gsm = dlci->gsm;
+ struct gsm_dlci_param_bits params;
+ int ret;
+
+ ret = gsm_encode_params(dlci, &params);
+ if (ret != 0)
+ return ret;
+
+ /* We cannot asynchronous wait for the command response with
+ * gsm_command() and gsm_control_wait() at this point.
+ */
+ ret = gsm_control_command(gsm, CMD_PN, (const u8 *)&params,
+ sizeof(params));
+
+ return ret;
+}
+
/**
* gsm_dlci_t1 - T1 timer expiry
* @t: timer contained in the DLCI that opened
@@ -1855,6 +2137,14 @@ static void gsm_dlci_t1(struct timer_list *t)
struct gsm_mux *gsm = dlci->gsm;

switch (dlci->state) {
+ case DLCI_CONFIGURE:
+ if (dlci->retries && gsm_dlci_negotiate(dlci) == 0) {
+ dlci->retries--;
+ mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+ } else {
+ gsm_dlci_begin_close(dlci); /* prevent half open link */
+ }
+ break;
case DLCI_OPENING:
if (dlci->retries) {
dlci->retries--;
@@ -1893,17 +2183,46 @@ static void gsm_dlci_t1(struct timer_list *t)
* to the modem which should then reply with a UA or ADM, at which point
* we will move into open state. Opening is done asynchronously with retry
* running off timers and the responses.
+ * Parameter negotiation is performed before SABM if required.
*/

static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
{
- struct gsm_mux *gsm = dlci->gsm;
- if (dlci->state == DLCI_OPEN || dlci->state == DLCI_OPENING)
+ struct gsm_mux *gsm = dlci ? dlci->gsm : NULL;
+ bool need_pn = false;
+
+ if (!gsm)
return;
- dlci->retries = gsm->n2;
- dlci->state = DLCI_OPENING;
- gsm_command(dlci->gsm, dlci->addr, SABM|PF);
- mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+
+ if (dlci->addr != 0) {
+ if (gsm->adaption != 1 || gsm->adaption != dlci->adaption)
+ need_pn = true;
+ if (dlci->prio != (roundup(dlci->addr + 1, 8) - 1))
+ need_pn = true;
+ if (gsm->ftype != dlci->ftype)
+ need_pn = true;
+ }
+
+ switch (dlci->state) {
+ case DLCI_CLOSED:
+ case DLCI_CLOSING:
+ dlci->retries = gsm->n2;
+ if (!need_pn) {
+ dlci->state = DLCI_OPENING;
+ gsm_command(gsm, dlci->addr, SABM|PF);
+ } else {
+ /* Configure DLCI before setup */
+ dlci->state = DLCI_CONFIGURE;
+ if (gsm_dlci_negotiate(dlci) != 0) {
+ gsm_dlci_close(dlci);
+ return;
+ }
+ }
+ mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+ break;
+ default:
+ break;
+ }
}

/**
--
2.34.1

2022-10-25 10:52:59

by Ilpo Järvinen

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] tty: n_gsm: add parameters used with parameter negotiation

2022-10-25 11:25:51

by Ilpo Järvinen

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] tty: n_gsm: introduce macro for minimal unit size

On Mon, 24 Oct 2022, D. Starke wrote:

> From: Daniel Starke <[email protected]>
>
> n_gsm has a minimal protocol overhead of 7 bytes. The current code already
> checks whether the configured MRU/MTU size is at least one byte more than
> this.
>
> Introduce the macro MIN_MTU to make this value more obvious.
>
> Signed-off-by: Daniel Starke <[email protected]>

Reviewed-by: Ilpo J?rvinen <[email protected]>

--
i.

2022-10-25 11:26:55

by Ilpo Järvinen

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

On Mon, 24 Oct 2022, 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.1.8.1.1 describes the parameter negotiation
> messages and parameters. Chapter 5.4.1 states that the default parameters
> are to be used if no negotiation is performed. Chapter 5.4.6.3.1 describes
> the encoding of the parameter negotiation message. The meaning of the
> parameters and allowed value ranges can be found in chapter 5.7.
>
> Add parameter negotiation support accordingly. DLCI specific parameter
> configuration by the user requires additional ioctls. This is subject to
> another patch.
>
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 335 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 327 insertions(+), 8 deletions(-)
>
> v1 -> v2:
> Incorporated review comments.
> Simplification of command retry handling remains subject to future patches.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c217013b3e16..11d3730bf436 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -38,6 +38,7 @@
> #include <linux/sched/signal.h>
> #include <linux/interrupt.h>
> #include <linux/tty.h>
> +#include <linux/bitfield.h>
> #include <linux/ctype.h>
> #include <linux/mm.h>
> #include <linux/math.h>
> @@ -127,6 +128,7 @@ struct gsm_msg {
>
> enum gsm_dlci_state {
> DLCI_CLOSED,
> + DLCI_CONFIGURE, /* Sending PN (for adaption > 1) */
> DLCI_OPENING, /* Sending SABM not seen UA */
> DLCI_OPEN, /* SABM/UA complete */
> DLCI_CLOSING, /* Sending DISC not seen UA/DM */
> @@ -184,6 +186,30 @@ struct gsm_dlci {
> struct net_device *net; /* network interface, if created */
> };
>
> +/*
> + * Parameter bits used for parameter negotiation according to 3GPP 27.010
> + * chapter 5.4.6.3.1.
> + */
> +
> +struct gsm_dlci_param_bits {
> + u8 d_bits;
> + u8 i_cl_bits;
> + u8 p_bits;
> + u8 t_bits;
> + __le16 n_bits;
> + u8 na_bits;
> + u8 k_bits;
> +} __packed;
> +
> +#define PN_D_FIELD_DLCI GENMASK(5, 0)
> +#define PN_I_CL_FIELD_FTYPE GENMASK(3, 0)
> +#define PN_I_CL_FIELD_ADAPTION GENMASK(7, 4)
> +#define PN_P_FIELD_PRIO GENMASK(5, 0)

> +#define PN_T_FIELD_T1 GENMASK(7, 0)
> +#define PN_N_FIELD_N1 GENMASK(15, 0)
> +#define PN_NA_FIELD_N2 GENMASK(7, 0)

I guess there three would not be strictly necessary since they match to
the full type size but then they're not harmful either.

> +#define PN_K_FIELD_K GENMASK(2, 0)
> +
> /* Total number of supported devices */
> #define GSM_TTY_MINORS 256
>
> @@ -411,6 +437,7 @@ static const u8 gsm_fcs8[256] = {
> #define INIT_FCS 0xFF
> #define GOOD_FCS 0xCF
>
> +static void gsm_dlci_close(struct gsm_dlci *dlci);
> static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
> static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
> static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
> @@ -533,6 +560,59 @@ static void gsm_hex_dump_bytes(const char *fname, const u8 *data,
> kfree(prefix);
> }
>
> +/**
> + * gsm_encode_params - encode DLCI parameters
> + * @dlci: DLCI to encode from
> + * @params: buffer to fill with the encoded parameters
> + *
> + * Encodes the parameters according to GSM 07.10 section 5.4.6.3.1
> + * table 3.
> + */
> +static int gsm_encode_params(const struct gsm_dlci *dlci,
> + struct gsm_dlci_param_bits *params)
> +{
> + const struct gsm_mux *gsm = dlci->gsm;
> + unsigned int i, cl;
> +
> + switch (dlci->ftype) {
> + case UIH:
> + i = 0; /* UIH */
> + break;
> + case UI:
> + i = 1; /* UI */
> + break;
> + default:
> + pr_err("%s: unsupported frame type %d\n", __func__,
> + dlci->ftype);
> + return -EINVAL;
> + }
> +
> + switch (dlci->adaption) {
> + case 1: /* Unstructured */
> + cl = 0; /* convergence layer type 1 */
> + break;
> + case 2: /* Unstructured with modem bits. */
> + cl = 1; /* convergence layer type 2 */
> + break;
> + default:
> + pr_err("%s: unsupported adaption %d\n", __func__,
> + dlci->adaption);
> + return -EINVAL;
> + }
> +
> + params->d_bits = FIELD_PREP(PN_D_FIELD_DLCI, dlci->addr);
> + /* UIH, convergence layer type 1 */
> + params->i_cl_bits = FIELD_PREP(PN_I_CL_FIELD_FTYPE, i) |
> + FIELD_PREP(PN_I_CL_FIELD_ADAPTION, cl);
> + params->p_bits = FIELD_PREP(PN_P_FIELD_PRIO, dlci->prio);
> + params->t_bits = FIELD_PREP(PN_T_FIELD_T1, gsm->t1);
> + params->n_bits = cpu_to_le16(FIELD_PREP(PN_N_FIELD_N1, dlci->mtu));
> + params->na_bits = FIELD_PREP(PN_NA_FIELD_N2, gsm->n2);
> + params->k_bits = FIELD_PREP(PN_K_FIELD_K, dlci->k);
> +
> + return 0;
> +}
> +
> /**
> * gsm_register_devices - register all tty devices for a given mux index
> *
> @@ -1450,6 +1530,116 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
> dlci->modem_rx = mlines;
> }
>
> +/**
> + * gsm_process_negotiation - process received parameters
> + * @gsm: GSM channel
> + * @addr: DLCI address
> + * @cr: command/response
> + * @params: encoded parameters from the parameter negotiation message
> + *
> + * Used when the response for our parameter negotiation command was
> + * received.
> + */
> +static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
> + unsigned int cr,
> + const struct gsm_dlci_param_bits *params)
> +{
> + struct gsm_dlci *dlci = gsm->dlci[addr];
> + unsigned int ftype, i, adaption, prio, n1, k;
> +
> + i = FIELD_GET(PN_I_CL_FIELD_FTYPE, params->i_cl_bits);
> + adaption = FIELD_GET(PN_I_CL_FIELD_ADAPTION, params->i_cl_bits) + 1;
> + prio = FIELD_GET(PN_P_FIELD_PRIO, params->p_bits);
> + n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));

Should this be using get_unaligned...()?

--
i.

2022-10-25 11:45:20

by D. Starke

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

> > + n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));
>
> Should this be using get_unaligned...()?

Is this really necessary if the structure is already __packed? I did not
receive any warning by the compiler.

Best regards,
Daniel Starke

2022-10-25 11:58:23

by Ilpo Järvinen

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

On Tue, 25 Oct 2022, Starke, Daniel wrote:

> > > + n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));
> >
> > Should this be using get_unaligned...()?
>
> Is this really necessary if the structure is already __packed? I did not
> receive any warning by the compiler.

It would be arch dependent to begin with. But honestly, I'm not entirely
certain here myself.

Documentation/core-api/unaligned-memory-access.rst claims compiler would
indeed do extra work to ensure access of unaligned member in a packed
struct is handled ok. But then you call le16_to_cpu() for the member field
which is full of cast magic so I'd be a bit hesitant to claim the
knowledge about the unalignment is carried all the way down there through
those casts.

Other subtle detail is the reply side struct which is allocated from stack
and with packed compiler is allowed (I don't know if it does that or not)
to make the struct unaligned as well (so perhaps put_unaligned would be
necessary there too if packed is retained).

If you want my recommendation, I'd just remove the packed altogether from
the struct because there seems to be no natural holes in it, use
get_unaligned for the receive side, and add this build time check:

static_assert(sizeof(struct gsm_dlci_param_bits) == 8);

If lkp builds all its current archs fine with that static_assert(), I'd be
pretty sure the struct that the unpacked struct is ok on all archs. Would
it ever stop being true on any arch/compiler setting, the assert would
catch it right away.

--
i.

2022-10-25 12:14:59

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

On Mon, Oct 24, 2022 at 03:01:14PM +0200, 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.1.8.1.1 describes the parameter negotiation
> messages and parameters. Chapter 5.4.1 states that the default parameters
> are to be used if no negotiation is performed. Chapter 5.4.6.3.1 describes
> the encoding of the parameter negotiation message. The meaning of the
> parameters and allowed value ranges can be found in chapter 5.7.
>
> Add parameter negotiation support accordingly. DLCI specific parameter
> configuration by the user requires additional ioctls. This is subject to
> another patch.
>
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> drivers/tty/n_gsm.c | 335 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 327 insertions(+), 8 deletions(-)
>
> v1 -> v2:
> Incorporated review comments.
> Simplification of command retry handling remains subject to future patches.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c217013b3e16..11d3730bf436 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -38,6 +38,7 @@
> #include <linux/sched/signal.h>
> #include <linux/interrupt.h>
> #include <linux/tty.h>
> +#include <linux/bitfield.h>
> #include <linux/ctype.h>
> #include <linux/mm.h>
> #include <linux/math.h>
> @@ -127,6 +128,7 @@ struct gsm_msg {
>
> enum gsm_dlci_state {
> DLCI_CLOSED,
> + DLCI_CONFIGURE, /* Sending PN (for adaption > 1) */
> DLCI_OPENING, /* Sending SABM not seen UA */
> DLCI_OPEN, /* SABM/UA complete */
> DLCI_CLOSING, /* Sending DISC not seen UA/DM */
> @@ -184,6 +186,30 @@ struct gsm_dlci {
> struct net_device *net; /* network interface, if created */
> };
>
> +/*
> + * Parameter bits used for parameter negotiation according to 3GPP 27.010
> + * chapter 5.4.6.3.1.
> + */
> +
> +struct gsm_dlci_param_bits {
> + u8 d_bits;
> + u8 i_cl_bits;
> + u8 p_bits;
> + u8 t_bits;
> + __le16 n_bits;
> + u8 na_bits;
> + u8 k_bits;
> +} __packed;
> +
> +#define PN_D_FIELD_DLCI GENMASK(5, 0)
> +#define PN_I_CL_FIELD_FTYPE GENMASK(3, 0)
> +#define PN_I_CL_FIELD_ADAPTION GENMASK(7, 4)
> +#define PN_P_FIELD_PRIO GENMASK(5, 0)
> +#define PN_T_FIELD_T1 GENMASK(7, 0)
> +#define PN_N_FIELD_N1 GENMASK(15, 0)
> +#define PN_NA_FIELD_N2 GENMASK(7, 0)
> +#define PN_K_FIELD_K GENMASK(2, 0)
> +
> /* Total number of supported devices */
> #define GSM_TTY_MINORS 256
>
> @@ -411,6 +437,7 @@ static const u8 gsm_fcs8[256] = {
> #define INIT_FCS 0xFF
> #define GOOD_FCS 0xCF
>
> +static void gsm_dlci_close(struct gsm_dlci *dlci);
> static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
> static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
> static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
> @@ -533,6 +560,59 @@ static void gsm_hex_dump_bytes(const char *fname, const u8 *data,
> kfree(prefix);
> }
>
> +/**
> + * gsm_encode_params - encode DLCI parameters
> + * @dlci: DLCI to encode from
> + * @params: buffer to fill with the encoded parameters
> + *
> + * Encodes the parameters according to GSM 07.10 section 5.4.6.3.1
> + * table 3.
> + */
> +static int gsm_encode_params(const struct gsm_dlci *dlci,
> + struct gsm_dlci_param_bits *params)
> +{
> + const struct gsm_mux *gsm = dlci->gsm;
> + unsigned int i, cl;
> +
> + switch (dlci->ftype) {
> + case UIH:
> + i = 0; /* UIH */
> + break;
> + case UI:
> + i = 1; /* UI */
> + break;
> + default:
> + pr_err("%s: unsupported frame type %d\n", __func__,
> + dlci->ftype);

This needs to be dev_err(), right?

And why is it not just dev_dbg()/

> + return -EINVAL;
> + }
> +
> + switch (dlci->adaption) {
> + case 1: /* Unstructured */
> + cl = 0; /* convergence layer type 1 */
> + break;
> + case 2: /* Unstructured with modem bits. */
> + cl = 1; /* convergence layer type 2 */
> + break;
> + default:
> + pr_err("%s: unsupported adaption %d\n", __func__,
> + dlci->adaption);

Again, dev_dbg()?

Do not yet userspace, or external devices, spam kernel logs with
messages.

> + return -EINVAL;
> + }
> +
> + params->d_bits = FIELD_PREP(PN_D_FIELD_DLCI, dlci->addr);
> + /* UIH, convergence layer type 1 */
> + params->i_cl_bits = FIELD_PREP(PN_I_CL_FIELD_FTYPE, i) |
> + FIELD_PREP(PN_I_CL_FIELD_ADAPTION, cl);
> + params->p_bits = FIELD_PREP(PN_P_FIELD_PRIO, dlci->prio);
> + params->t_bits = FIELD_PREP(PN_T_FIELD_T1, gsm->t1);
> + params->n_bits = cpu_to_le16(FIELD_PREP(PN_N_FIELD_N1, dlci->mtu));
> + params->na_bits = FIELD_PREP(PN_NA_FIELD_N2, gsm->n2);
> + params->k_bits = FIELD_PREP(PN_K_FIELD_K, dlci->k);
> +
> + return 0;
> +}
> +
> /**
> * gsm_register_devices - register all tty devices for a given mux index
> *
> @@ -1450,6 +1530,116 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
> dlci->modem_rx = mlines;
> }
>
> +/**
> + * gsm_process_negotiation - process received parameters
> + * @gsm: GSM channel
> + * @addr: DLCI address
> + * @cr: command/response
> + * @params: encoded parameters from the parameter negotiation message
> + *
> + * Used when the response for our parameter negotiation command was
> + * received.
> + */
> +static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
> + unsigned int cr,
> + const struct gsm_dlci_param_bits *params)
> +{
> + struct gsm_dlci *dlci = gsm->dlci[addr];
> + unsigned int ftype, i, adaption, prio, n1, k;
> +
> + i = FIELD_GET(PN_I_CL_FIELD_FTYPE, params->i_cl_bits);
> + adaption = FIELD_GET(PN_I_CL_FIELD_ADAPTION, params->i_cl_bits) + 1;
> + prio = FIELD_GET(PN_P_FIELD_PRIO, params->p_bits);
> + n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));
> + k = FIELD_GET(PN_K_FIELD_K, params->k_bits);
> +
> + if (n1 < MIN_MTU) {
> + if (debug & DBG_ERRORS)

Please use the proper debug code in the kernel, don't roll your own.

> + pr_info("%s N1 out of range in PN\n", __func__);

This should be dev_dbg().

And never use __func__ in a dev_dbg() call, it's there automatically.

thanks,

greg k-h

2022-10-25 12:43:08

by D. Starke

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

> > > > + n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));
> > >
> > > Should this be using get_unaligned...()?
> >
> > Is this really necessary if the structure is already __packed? I did not
> > receive any warning by the compiler.
>
> It would be arch dependent to begin with. But honestly, I'm not entirely
> certain here myself.

I have checked the code in include/asm-generic/unaligned.h.
An extract:

#define __get_unaligned_t(type, ptr) ({ \
const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr); \
__pptr->x; \
})

#define get_unaligned(ptr) __get_unaligned_t(typeof(*(ptr)), (ptr))

static inline u16 get_unaligned_le16(const void *p)
{
return le16_to_cpu(__get_unaligned_t(__le16, p));
}

Looking at this I would assume that the use of get_unaligned_le16() makes
no difference compared to the current implementation. My assumption is
that the compiler makes sure the 16-bit value is accessed at the correct
address and le16_to_cpu() converts the intermediate value correctly.

> static_assert(sizeof(struct gsm_dlci_param_bits) == 8);
>
> If lkp builds all its current archs fine with that static_assert(), I'd be
> pretty sure the struct that the unpacked struct is ok on all archs. Would
> it ever stop being true on any arch/compiler setting, the assert would
> catch it right away.

That is an uncertainty I would like to avoid. And what should be the
solution if the assertion fails? Nevertheless, I do not mind implementing
it in this way to move forward.

Best regards,
Daniel Starke

2022-10-31 13:48:24

by D. Starke

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

Thank you for your review.

> > + pr_err("%s: unsupported frame type %d\n", __func__,
> > + dlci->ftype);
>
> This needs to be dev_err(), right?

There is no place within this driver that uses dev_*() at the moment except
for the timeout function of the network stack (gsm_mux_net_tx_timeout()).
I do not mind either way, but I would prefer a consistent variant within
this driver. Therefore, I suggest switching from pr_*() to dev_*() in a
separate patch.

> And why is it not just dev_dbg()/

I used pr_err() instead of pr_dbg() due to the fact that this mismatch will
most likely make it impossible to use the n_gsm driver with the connected
device as it stands. I am okay to replace it with pr_info() though.

> > + pr_err("%s: unsupported adaption %d\n", __func__,
> > + dlci->adaption);
>
> Again, dev_dbg()?
>
> Do not yet userspace, or external devices, spam kernel logs with
> messages.

These are related to the user API. Therefore, I do not mind changing these
to debug level.

> > + if (n1 < MIN_MTU) {
> > + if (debug & DBG_ERRORS)
>
> Please use the proper debug code in the kernel, don't roll your own.

Again, I am only reusing what is already there in the driver. To avoid
segmentation I suggest cleaning this up in a future patch.

> > + pr_info("%s N1 out of range in PN\n", __func__);
>
> This should be dev_dbg().

Same as above. The connected device appears to be incompatible with the
based standard. It will most likely not work. Should it be debug level,
nevertheless?

> And never use __func__ in a dev_dbg() call, it's there automatically.

I could not find a hint that __func__ is included in dev_dbg(). What is
included is the subsystem name and the device name but not the function
name within the driver according to include/linux/dev_printk.h. Other
device drivers like usb/dwc2/core.c also include __func__ here. But it
appears to be possible to automate this by re-defining dev_fmt().

Please let me know if you prefer me to segment the current printk-related
code by introducing dev_*() or keep this change open for a later patch.

Best regards,
Daniel Starke

2022-11-01 06:40:21

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

On Mon, Oct 31, 2022 at 01:26:50PM +0000, Starke, Daniel wrote:
> Thank you for your review.
>
> > > + pr_err("%s: unsupported frame type %d\n", __func__,
> > > + dlci->ftype);
> >
> > This needs to be dev_err(), right?
>
> There is no place within this driver that uses dev_*() at the moment except
> for the timeout function of the network stack (gsm_mux_net_tx_timeout()).
> I do not mind either way, but I would prefer a consistent variant within
> this driver. Therefore, I suggest switching from pr_*() to dev_*() in a
> separate patch.

Yes, that's a good idea.

> > And why is it not just dev_dbg()/
>
> I used pr_err() instead of pr_dbg() due to the fact that this mismatch will
> most likely make it impossible to use the n_gsm driver with the connected
> device as it stands. I am okay to replace it with pr_info() though.
>
> > > + pr_err("%s: unsupported adaption %d\n", __func__,
> > > + dlci->adaption);
> >
> > Again, dev_dbg()?
> >
> > Do not yet userspace, or external devices, spam kernel logs with
> > messages.
>
> These are related to the user API. Therefore, I do not mind changing these
> to debug level.

Please do, userspace should not be able to spam kernel logs, bad things
can happen if that is possible.

> > And never use __func__ in a dev_dbg() call, it's there automatically.
>
> I could not find a hint that __func__ is included in dev_dbg(). What is
> included is the subsystem name and the device name but not the function
> name within the driver according to include/linux/dev_printk.h. Other
> device drivers like usb/dwc2/core.c also include __func__ here. But it
> appears to be possible to automate this by re-defining dev_fmt().

You can dynamically add the function location at runtime when using
pr_dbg() or dev_dbg(), see the documentation. So any addition of
__func__ in a string for those calls is just wrong and should be
removed.

Can you fix this up to not spam the log for errors (move to debug
level), and resend the updated series?

thanks,

greg k-h