Re: [PATCH v4 2/4] tty: n_gsm: add keep alive support

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Feb 09, 2023 at 12:09:04PM +0000, Starke, Daniel wrote:
> > > +	if (gsm->ka_num && gsm->ka_retries == 0) {
> > > +		/* Keep-alive expired -> close the link */
> > > +		if (debug & DBG_ERRORS)
> > > +			pr_info("%s keep-alive timed out\n", __func__);
> > 
> > info for a debugging error?  no, please don't do that.  Please fix up
> > the debugging mess in this driver, don't add to it.
> 
> I am aware that the current debugging concept of the driver does not align
> with the kernel philosophy. However, this is the established way it is
> handled in n_gsm right now. Cleaning this up should be done before adding
> new concepts here. But not printing out any information in case of errors
> does not help during use and development of/for this driver. Also note that
> all these outputs are only enabled if explicitly set via kernel module
> parameter. That means syslog does not get polluted if not intentionally
> set so. Unfortunately, I do not have a better proposal for now as neither
> ftrace nor dynamic debug are available to the normal Linux user.

dynamic debug _is_ availble to the normal Linux user, and it's _the_
kernel debug facility and interface.  To not use it is to somehow state
that this one tiny .c file is unique and special over the whole of the
rest of the kernel :)

I recommend just moving to the dynamic debug interface now and then
going forward, it's not going to be an issue at all.

> > > +struct gsm_config_ext {
> > > +	__u32 keep_alive;	/* Control channel keep-alive in 1/100th of a
> > > +				 * second (0 to disable)
> > > +				 */
> > > +	__u32 reserved[7];	/* For future use */
> > 
> > say "must be set to 0"?
> 
> Right, I will add ", needs to be initialized to zero".

Use "must", that means more.

> > Where are you documenting this ioctl so userspace knows how to use it?
> > Where is the userspace tool that uses it?
> 
> I will extend the description and code example in
> Documentation/driver-api/tty/n_gsm.rst. Should this go CC to
> linux-man@vger.kernel.org?

I don't know, if there is a man page for it, yes, if not, no need.

thanks,

greg k-h