On Fri, 21 Oct 2022, D. Starke wrote: > From: Daniel Starke <daniel.starke@xxxxxxxxxxx> > > 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 <daniel.starke@xxxxxxxxxxx> > --- > drivers/tty/n_gsm.c | 308 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 300 insertions(+), 8 deletions(-) > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c > index b6813a134c18..c6fe00afe1b2 100644 > --- a/drivers/tty/n_gsm.c > +++ b/drivers/tty/n_gsm.c > @@ -123,6 +123,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 */ > @@ -406,6 +407,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, > @@ -528,6 +530,55 @@ 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 > + * @data: 8 byte buffer for encoded data > + * @dlen: length of buffer > + * > + * 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, u8 *data, > + unsigned int dlen) > +{ > + struct gsm_mux *gsm = dlci->gsm; > + > + if (dlen < 8) > + return -EINVAL; Should this be MIN_MTU? > + data[0] = dlci->addr; > + data[1] = 0x00; /* UIH, convergence layer type 1 */ > + data[2] = dlci->prio; > + data[3] = gsm->t1; > + data[4] = dlci->mtu & 0xFF; > + data[5] = (dlci->mtu >> 8) & 0xFF; > + data[6] = gsm->n2; > + data[7] = dlci->k; Magic offsets, shouldn't you define a struct and assign to the named fields (and use the correct endian type + accessors for that two-byte field). > + if (dlci->ftype == UI) { > + data[1] = 0x01; /* UI */ > + } else if (dlci->ftype != UIH) { > + pr_err("%s: unsupported frame type %d\n", __func__, > + dlci->ftype); > + return -EINVAL; > + } > + > + switch (dlci->adaption) { > + case 1: /* Unstructured */ > + break; > + case 2: /* Unstructured with modem bits. */ > + data[1] |= 0x10; /* convergence layer type 2 */ > + break; > + default: > + pr_err("%s: unsupported adaption %d\n", __func__, > + dlci->adaption); > + return -EINVAL; > + } > + > + return 0; > +} > + > /** > * gsm_register_devices - register all tty devices for a given mux index > * > @@ -1445,6 +1496,122 @@ 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 > + * @data: data following command > + * @clen: length of data > + * > + * 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 u8 *data, > + unsigned int clen) > +{ > + struct gsm_dlci *dlci = gsm->dlci[addr]; > + unsigned int ftype, i, adaption, prio, n1, k; > + > + if (clen < 8) > + return -EINVAL; > + > + i = data[1] & 0x0F; > + adaption = ((data[1] >> 4) & 0x0F) + 1; > + prio = data[2] & 0x3F; > + /* t1 = data[3]; */ > + n1 = data[4] | (data[5] << 8); > + /* n2 = data[6]; */ > + k = data[7] & 0x07; Magic offsets. Define these fields properly too with names and GENMASK. Use FIELD_GET() to extract values. > + if (n1 < MIN_UNIT_SIZE) { > + 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 > @@ -1498,6 +1665,62 @@ 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; > + u8 params[8]; > + struct gsm_dlci *dlci; > + > + if (dlen < 8) > + return; MIN_MTU? > + > + /* Invalid DLCI? */ > + addr = data[0] & 0x3F; #define + FIELD_GET() > + 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, data, dlen) != 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, params, sizeof(params)) == 0) > + gsm_control_reply(gsm, CMD_PN, params, sizeof(params)); > + 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 > @@ -1607,8 +1830,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: > @@ -1641,8 +1868,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); > @@ -1652,6 +1879,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); > } > @@ -1829,6 +2059,31 @@ 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; > + u8 params[8]; > + int ret; > + > + ret = gsm_encode_params(dlci, params, sizeof(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, params, sizeof(params)); > + > + return ret; > +} > + > /** > * gsm_dlci_t1 - T1 timer expiry > * @t: timer contained in the DLCI that opened > @@ -1850,6 +2105,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); I'd tend to think a helper for this wouldn't hurt. There are already a few of them. > + } else { > + gsm_dlci_begin_close(dlci); /* prevent half open link */ > + } > + break; > case DLCI_OPENING: > if (dlci->retries) { > dlci->retries--; > @@ -1888,17 +2151,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 != ((((dlci->addr / 8) + 1) * 8) - 1)) roundup(addr, 8) - 1 again? -- i. > + 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; > + } > } > > /** >
