Date:   Wed, 1 Feb 2023 10:27:53 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "Starke, Daniel" <daniel.starke@siemens.com>
Cc:     "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "jirislaby@kernel.org" <jirislaby@kernel.org>,
        "ilpo.jarvinen@linux.intel.com" <ilpo.jarvinen@linux.intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] tty: n_gsm: add keep alive support
Message-ID: <Y9owmfZwKn9+1xBf@kroah.com>
References: <20230201080151.2068-1-daniel.starke@siemens.com>
 <Y9olcm0PiSCSikri@kroah.com>
 <DB9PR10MB58814F4711FC3403F7AF089BE0D19@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DB9PR10MB58814F4711FC3403F7AF089BE0D19@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
Precedence: bulk

On Wed, Feb 01, 2023 at 09:17:31AM +0000, Starke, Daniel wrote:
> > > 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.

Will it?  It shouldn't that's why using the correct data types is
essencial.

> 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.

If the value was not checked previously, then you can not use the field
now, otherwise things will break, sorry.  Those are useless fields and
should be marked as such :(

sorry,

greg k-h