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

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Oct 24, 2022 at 03:01:14PM +0200, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> 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@siemens.com>
> ---
>  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/8c2b9492-caf4-7a48-3a7b-da939a4ac8b6@linux.intel.com/
> 
> 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