2022-10-21 09:11:38

by D. Starke

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

From: Daniel Starke <[email protected]>

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 <[email protected]>
---
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
#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;
--
2.34.1

2022-10-21 10:36:42

by Ilpo Järvinen

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

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

> From: Daniel Starke <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
>