Date:   Thu, 9 Feb 2023 12:09:04 +0000
From:   "Starke, Daniel" <daniel.starke@...mens.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v4 2/4] tty: n_gsm: add keep alive support

Thank you for this detailed review.

> > +	u8 ka_num;			/* Keep-alive match pattern */
> 
> What do you mean by "pattern"?

Keep alive uses the test command which expects a byte pattern. This pattern
is simply echoed by the peer. We use a different pattern / byte value for
each keep-alive packet to distinguish between interval and re-sent
attempts. I will add ka_num to the commit message to make it clear that the
"single incrementing octet" refers to this field.

> > +	int ka_retries;			/* Keep-alive retry counter */
> 
> I know padding doesn't really matter much here, but you are adding holes
> here to this structure, is that intentional?

This was not intentional. The same is already true for the ftype field and
I need ka_num to be 8-bit. Changing the field order does not really make
this any better. Therefore, I would like to keep it as it is.

> And why "int"?  What is the range here?  And shouldn't this be "signed
> int" to be explicit you set this to -1 in places (and what does -1
> mean?)

ka_retries takes the value from the field n2, which also happens to be
"int". I will add -1 to the field description as "not yet initialized".
I will also change it to "signed int" and add an appropriate cast from n2.

> > +	/* Or did we receive the TEST response to our TEST command */
> > +	} else if (command == CMD_TEST && clen == 1 && *data == gsm->ka_num) {
> > +		gsm->ka_retries = -1; /* trigger new keep-alive message */
> > +		if (dlci && !dlci->dead)
> > +			mod_timer(&gsm->ka_timer,
> > +				  jiffies + gsm->keep_alive * HZ / 100);
> 
> We can use 100 columns now if you want to.

I will change this for better readability.

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

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

> 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@...r.kernel.org?

Best regards,
Daniel Starke

Powered by blists - more mailing lists