Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 25 Feb 2022 09:29:03 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Daniel Starke <daniel.starke@siemens.com>
Cc:     linux-serial@vger.kernel.org, jirislaby@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC 1/1] tty: n_gsm: improve standard compliance and feature
 completeness
Message-ID: <YhiTT2F5NJqFNrwM@kroah.com>
References: <20220225080758.2869-1-daniel.starke@siemens.com>
 <20220225080758.2869-2-daniel.starke@siemens.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220225080758.2869-2-daniel.starke@siemens.com>
Precedence: bulk

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