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

[Ilpo Järvinen]

On Fri, 21 Oct 2022, D. Starke wrote:

> From: Daniel Starke <daniel.starke@xxxxxxxxxxx>
> 
> 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_UNIT_SIZE to make this value more obvious.
> 
> Signed-off-by: Daniel Starke <daniel.starke@xxxxxxxxxxx>
> ---
>  drivers/tty/n_gsm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 5e516f5cac5a..8e039f2a0427 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_UNIT_SIZE 8
>  /* SOF, ADDR, CTRL, LEN1, LEN2, ..., FCS, EOF */
>  #define PROT_OVERHEAD 7

Why not call it just MIN_MTU?

I know you check it also against MRU but that seems so minor problem to me 
it's not worth even noting because of course MRU is related MTU, it's just 
the other end of the pipe. To be honest, I don't understand why MAX_MRU is 
even defined there when is just the same as MAX_MTU :-).

You could do this btw:

#define MIN_MTU	(PROT_OVERHEAD + 1)

To make it even more obvious where it comes from (matching to what you 
describe in your commit message).

The change looks otherwise ok.

-- 
 i.


>  #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_UNIT_SIZE || c->mtu < MIN_UNIT_SIZE)
> +		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_UNIT_SIZE;
>  	net->max_mtu = dlci->gsm->mtu;
>  	mux_net = netdev_priv(net);
>  	mux_net->dlci = dlci;
>