Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Oct 2022 14:09:19 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     "D. Starke" <daniel.starke@siemens.com>
cc:     linux-serial <linux-serial@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support
In-Reply-To: <20221024130114.2070-3-daniel.starke@siemens.com>
Message-ID: <403445fd-fc99-290-2a5d-cd7c18fb715c@linux.intel.com>
References: <20221024130114.2070-1-daniel.starke@siemens.com> <20221024130114.2070-3-daniel.starke@siemens.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 24 Oct 2022, 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)

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.