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

[Ilpo Järvinen]

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;
> +	}
>  }
>  
>  /**
>