RE: [PATCH 1/3] tty: n_gsm: add keep alive support

From: Starke, Daniel
Date: Wed Feb 01 2023 - 04:17:40 EST


> > index cb8693b39cb7..b64360aca1f9 100644
> > --- a/include/uapi/linux/gsmmux.h
> > +++ b/include/uapi/linux/gsmmux.h
> > @@ -19,7 +19,8 @@ struct gsm_config
> > unsigned int mtu;
> > unsigned int k;
> > unsigned int i;
> > - unsigned int unused[8]; /* Padding for expansion without
> > + unsigned int keep_alive;
> > + unsigned int unused[7]; /* Padding for expansion without
>
> "unsigned int" is not really a valid uapi variable type.
>
> Shouldn't this be __u32 instead?

I know but changing it to a fixed size data type may break compatibility
as this may change the overall size of the structure. This is why I
took a field out of the "unused" array for the "keep_alive" parameter.
A value of zero disables keep-alive polling.

> Should you document this field as to what the value is and the units as
> you are creating a new user/kernel api here.

I will add a comment here. Comments for the other fields remain subject to
another patch.

> And finally, "unused" here is being properly checked to be all 0, right?
> If not, then this change can't happen for obvious reasons :(

This was not the case until now. I assumed there was some coding guideline
that unused fields need to be initialized to zero. Obviously, checking it
prevents misuse here. I will add relevant checks for this.

Best regards,
Daniel Starke