Re: [RFC 1/1] tty: n_gsm: improve standard compliance and feature completeness

Re: [RFC 1/1] tty: n_gsm: improve standard compliance and feature completeness

[Greg KH]

On Fri, Feb 25, 2022 at 12:07:58AM -0800, Daniel Starke wrote:
> The complete patch based on tty-next is included here for reference.
> A proper series of patches will follow based on the comments to this RFC.
> Please refer to the cover letter for more details.
> 
> Changes included:
> - added: proper n_gsm Kernel module information fields and module version
> - added: optional start-of-frame flag skipping in advanced option mode
> - added: UI (unnumbered information) frame support
> - added: PN (parameter negotiation) message handling and function support
> - added: optional keep-alive control link supervision via Test messages
> - added: proc fs files to provide meta data for the n_gsm instances
> - added: various parameter gated kernel debug messages
> - added: option for strict mode to reject non-standard compliant behavior
> - added: TIOCM_OUT1 and TIOCM_OUT2 to allow responder to operate as modem
> - added: TIOCMIWAIT support on virtual ttys
> - added: additional ioctls and parameters to configure the new functions
> - added: more function comments and aligned all to the coding guidelines
> - changed: overall locking mechanism to avoid data race conditions
> - changed: outgoing data flow to decouple physical from virtual tty
>   handling for better performance and to avoid dead-locks
> - fixed: advanced option mode implementation
> - fixed: convergence layer type 2 implementation
> - fixed: handling of CLD (multiplexer close down) messages
> - fixed: broken muxer close down procedure
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c         | 4523 +++++++++++++++++++++++++----------
>  include/uapi/linux/gsmmux.h |   70 +-
>  2 files changed, 3364 insertions(+), 1229 deletions(-)

I'm sorry, but there is nothing we can do with such a large patch here
at all.

Please break this up into "one logical change per patch" and we will be
glad to review it.

Put your "fixes" at the beginning of the patch series, so they can be
properly backported to older kernels as needed.  Don't mix up new
features with fixes as that means that the fixes can never be
backported.

One note, please drop the useless MODULE_VERSION() that never belongs in
an in-kernel driver as it makes no sense (the version is the kernel
version).  Same for the license "boiler-plate" text, that should not be
added back, we removed it for a reason.

Also, no module parameters should be added.  Especially pointless ones
like debugging flags, use the built-in kernel debugging options that the
whole rest of the kernel uses.  There is no need for a special case for
just one tiny line discipline module :)

thanks,

greg k-h

Re: [RFC 1/1] tty: n_gsm: improve standard compliance and feature completeness

[Starke, Daniel]

Thank you for this quick response.

> I'm sorry, but there is nothing we can do with such a large patch here at
> all.
> 
> Please break this up into "one logical change per patch" and we will be
> glad to review it.

You are absolutely right but this is still work in progress. The attached
patch was only for reference for all those who would like to look at the
changes in detail or try out the proposed work. This was not a request for
code review yet. Also, it seems that the patch was too large for the
linux-serial mailing list so I have uploaded it here for reference:
https://github.com/siemens/linux/tree/dstarke-siemens/n_gsm_rfc

> Put your "fixes" at the beginning of the patch series, so they can be
> properly backported to older kernels as needed. Don't mix up new features
> with fixes as that means that the fixes can never be backported.

All fixes that have no dependency on the upcoming features have been
submitted by me already. I will double check the remaining fixes but I
could not find a way to separate those from the new features so far.

> One note, please drop the useless MODULE_VERSION() that never belongs in
> an in-kernel driver as it makes no sense (the version is the kernel
> version).

This helped us during our internal development and test procedure. But I
will remove it before submitting the actual patches.

> Same for the license "boiler-plate" text, that should not be added back,
> we removed it for a reason.

Will be removed.

> Also, no module parameters should be added. Especially pointless ones
> like debugging flags, use the built-in kernel debugging options that the
> whole rest of the kernel uses. There is no need for a special case for
> just one tiny line discipline module :)

The module parameter "debug" is not new. It is included in all kernel
version. And I did not plan to remove it. Especially, since this protocol
is complex and hard to debug with the provided kernel debugging options
only.
My addition was the parameter "strict" to verify standard conformance of
connected devices with the underlying protocol. Otherwise, the protocol
gracefully handles invalid values. Would you suggest to make this parameter
an ioctl instead?

With best regards,
Daniel Starke

Re: [RFC 1/1] tty: n_gsm: improve standard compliance and feature completeness

[Greg KH]

On Wed, Mar 02, 2022 at 02:47:20PM +0000, Starke, Daniel wrote:
> Thank you for this quick response.
> 
> > I'm sorry, but there is nothing we can do with such a large patch here at
> > all.
> > 
> > Please break this up into "one logical change per patch" and we will be
> > glad to review it.
> 
> You are absolutely right but this is still work in progress. The attached
> patch was only for reference for all those who would like to look at the
> changes in detail or try out the proposed work. This was not a request for
> code review yet. Also, it seems that the patch was too large for the
> linux-serial mailing list so I have uploaded it here for reference:
> https://github.com/siemens/linux/tree/dstarke-siemens/n_gsm_rfc

You need to provide a set of patches for people to review.  Random code
dumps and random github links are impossible to review cleanly, would
you want to to so?

thanks,

greg k-h