| Message ID | 20230424075251.5216-1-daniel.starke@siemens.com |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp2583481vqo;
Mon, 24 Apr 2023 01:21:35 -0700 (PDT)
X-Google-Smtp-Source:
AKy350agmIf/icObjcKr9w6wlFhg2vI32cxU649Ah7ypeS/DrHDElBYD1ZEPkjcw4axa3hUPWosQ
X-Received: by 2002:a05:6a20:4413:b0:f3:3810:8a89 with SMTP id
ce19-20020a056a20441300b000f338108a89mr7245659pzb.51.1682324495400;
Mon, 24 Apr 2023 01:21:35 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1682324495; cv=none;
d=google.com; s=arc-20160816;
b=VaUsetuNWSR65iOPq6cJ6Dn4QGjukqE175cD1qUuC+hPj5EJtyiKg/fqp+KLxRel3M
rZd6MI4FfYLRIDiqJb54hVH2vOjTk7KsMzLWHH19gb9S19eFiuLKN6kJykYXVgX4YThJ
sXYrmHX0VRzhu8KLbdHjUACjiE6qQ3xDoCG5r6aCZBLjsRgyykjek8i6RVH8Jk4vhp9n
TCTP4h/6sdHITHF9yUwuaVOAGOaW4ZecMaKqxipxkTDnwlbjN0+2t+5Ygtcnn5iQP2hJ
fpWRLk/3avaBpHBTBlca7bwK3Cvc90KCDV0+le0Qw+rF5j3ifaNhK6eVpEgepwx3EmAx
PM4w==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:feedback-id:content-transfer-encoding
:mime-version:message-id:date:subject:cc:to:from:dkim-signature;
bh=+v3MIb8PNZ4ZgxyCgk4r+KodnCwVlLhLxXYg0SWiEHo=;
b=VdgrbctQDkzLDrbK9DQrEMgsCVoJ6xHIF/IWNWoaSlGrhJ93LA2WFv2SuqJVkoaDIM
ZfR80LgO/0NdNPkURxNXQKmKi1ceCxx/mzBsr7FvewWeOrtCcIbr8UOaa5GTY0waONXw
/0MnQqkDYsTFmJKBvsdT4wisDuQpWwc3UvUwGQ5CRc9X+lePOnD3UNXNBtldZpTiKYKD
zGv4nBuK80eyE6MU3Qap6+mu1jN9IkbFGhKBGK6VVoQO8EkE2R1HwYd9XNLw030VP6Q+
1C6vGdG8RzFo4pX7ALqiVuo76N+CtY+aKnx9nPS7cvrbhQVnZ6aFgJO5Q0fhshiw1U/L
a8nQ==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@siemens.com header.s=fm1 header.b="PUI5S/qZ";
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=siemens.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
j5-20020a63e745000000b00517f0c53072si10707884pgk.170.2023.04.24.01.21.19;
Mon, 24 Apr 2023 01:21:35 -0700 (PDT)
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;
Authentication-Results: mx.google.com;
dkim=pass header.i=@siemens.com header.s=fm1 header.b="PUI5S/qZ";
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=siemens.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S231280AbjDXH4V (ORCPT <rfc822;zxc52fgh@gmail.com> + 99 others);
Mon, 24 Apr 2023 03:56:21 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40988 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S230311AbjDXH4R (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Mon, 24 Apr 2023 03:56:17 -0400
X-Greylist: delayed 78 seconds by postgrey-1.37 at lindbergh.monkeyblade.net;
Mon, 24 Apr 2023 00:55:55 PDT
Received: from mta-65-226.siemens.flowmailer.net
(mta-65-226.siemens.flowmailer.net [185.136.65.226])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 111C31737
for <linux-kernel@vger.kernel.org>;
Mon, 24 Apr 2023 00:55:54 -0700 (PDT)
Received: by mta-65-226.siemens.flowmailer.net with ESMTPSA id
202304240754343fbeb7fb65f76aa97e
for <linux-kernel@vger.kernel.org>;
Mon, 24 Apr 2023 09:54:34 +0200
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; s=fm1;
d=siemens.com; i=daniel.starke@siemens.com;
h=Date:From:Subject:To:Message-ID:MIME-Version:Content-Type:Content-Transfer-Encoding:Cc;
bh=+v3MIb8PNZ4ZgxyCgk4r+KodnCwVlLhLxXYg0SWiEHo=;
b=PUI5S/qZHtiME/0oNhuHpCviJ3YzaxRqU25OBtOu7Lw8ANfD1ludQuEAyGuIB9nVHrgBnY
57GBs63dQxYNL+wabT8LutbGG6scReu7HhKMrAx7zaesgTN01oLka4qmDKpWsMfhRI/oerRo
wmXDyiGtRY5wUe5WrIdxB2lu1qhVk=;
From: "D. Starke" <daniel.starke@siemens.com>
To: linux-serial@vger.kernel.org, gregkh@linuxfoundation.org,
jirislaby@kernel.org, ilpo.jarvinen@linux.intel.com
Cc: linux-kernel@vger.kernel.org,
Daniel Starke <daniel.starke@siemens.com>
Subject: [PATCH v3 1/8] tty: n_gsm: add restart parameter to DLC specific
ioctl config
Date: Mon, 24 Apr 2023 09:52:44 +0200
Message-Id: <20230424075251.5216-1-daniel.starke@siemens.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-Flowmailer-Platform: Siemens
Feedback-ID: 519:519-314044:519-21489:flowmailer
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_MED,
DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_MSPIKE_H2,
SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1764045090141278507?=
X-GMAIL-MSGID: =?utf-8?q?1764045090141278507?=
|
| Series |
[v3,1/8] tty: n_gsm: add restart parameter to DLC specific ioctl config
|
|
Commit Message
D. Starke
April 24, 2023, 7:52 a.m. UTC
From: Daniel Starke <daniel.starke@siemens.com> Currently, changing the parameters of a DLCI gives no direct control to the user whether this should trigger a channel reset or not. The decision is solely made by the driver based on the assumption which parameter changes are compatible or not. Therefore, the user has no means to perform an automatic channel reset after parameter configuration for non-conflicting changes. Add the parameter 'restart' to 'gsm_dlci_config' to force a channel reset after ioctl setting regardless of whether the changes made require this or not. Note that the parameter is limited to the values 0 and 1 to allow future additions here. Signed-off-by: Daniel Starke <daniel.starke@siemens.com> --- drivers/tty/n_gsm.c | 4 ++++ include/uapi/linux/gsmmux.h | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) v2 -> v3: No changes. Link: https://lore.kernel.org/all/20230420085017.7314-2-daniel.starke@siemens.com/
Comments
On Mon, Apr 24, 2023 at 09:52:44AM +0200, D. Starke wrote: > From: Daniel Starke <daniel.starke@siemens.com> > > Currently, changing the parameters of a DLCI gives no direct control to the > user whether this should trigger a channel reset or not. The decision is > solely made by the driver based on the assumption which parameter changes > are compatible or not. Therefore, the user has no means to perform an > automatic channel reset after parameter configuration for non-conflicting > changes. > > Add the parameter 'restart' to 'gsm_dlci_config' to force a channel reset > after ioctl setting regardless of whether the changes made require this or > not. > > Note that the parameter is limited to the values 0 and 1 to allow future > additions here. > > Signed-off-by: Daniel Starke <daniel.starke@siemens.com> > --- > drivers/tty/n_gsm.c | 4 ++++ > include/uapi/linux/gsmmux.h | 3 ++- > 2 files changed, 6 insertions(+), 1 deletion(-) > > v2 -> v3: > No changes. > > Link: https://lore.kernel.org/all/20230420085017.7314-2-daniel.starke@siemens.com/ > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c > index b411a26cc092..00f692e2e810 100644 > --- a/drivers/tty/n_gsm.c > +++ b/drivers/tty/n_gsm.c > @@ -2532,6 +2532,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in > return -EINVAL; > if (dc->k > 7) > return -EINVAL; > + if (dc->restart > 1) /* allow future extensions */ > + return -EINVAL; > > /* > * See what is needed for reconfiguration > @@ -2546,6 +2548,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in > /* Requires care */ > if (dc->priority != dlci->prio) > need_restart = true; > + if (dc->restart) > + need_restart = true; > > if ((open && gsm->wait_config) || need_restart) > need_open = true; > diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h > index eb67884e5f38..33ee7b857c52 100644 > --- a/include/uapi/linux/gsmmux.h > +++ b/include/uapi/linux/gsmmux.h > @@ -58,7 +58,8 @@ struct gsm_dlci_config { > __u32 priority; /* Priority (0 for default value) */ > __u32 i; /* Frame type (1 = UIH, 2 = UI) */ > __u32 k; /* Window size (0 for default value) */ > - __u32 reserved[8]; /* For future use, must be initialized to zero */ > + __u32 restart; /* Force DLCI channel reset? */ Why are you using a full 32 bits for just 1 bit of data here? Why not use a bitfield? And what happened to the request to turn the documentation for this structure into proper kerneldoc format? thanks, greg k-h
> > --- a/include/uapi/linux/gsmmux.h > > +++ b/include/uapi/linux/gsmmux.h > > @@ -58,7 +58,8 @@ struct gsm_dlci_config { > > __u32 priority; /* Priority (0 for default value) */ > > __u32 i; /* Frame type (1 = UIH, 2 = UI) */ > > __u32 k; /* Window size (0 for default value) */ > > - __u32 reserved[8]; /* For future use, must be initialized to zero */ > > + __u32 restart; /* Force DLCI channel reset? */ > > Why are you using a full 32 bits for just 1 bit of data here? Why not > use a bitfield? The ioctrl guide states: Bitfields and enums generally work as one would expect them to, but some properties of them are implementation-defined, so it is better to avoid them completely in ioctl interfaces. Therefore, I tried to avoid them here. > And what happened to the request to turn the documentation for this > structure into proper kerneldoc format? That applied to patch 2/8 and is unrelated to this patch. Another patch will need to fix this. Link: https://lore.kernel.org/all/20230424075251.5216-2-daniel.starke@siemens.com/ Best regards, Daniel Starke
On Mon, Apr 24, 2023 at 11:03:26AM +0000, Starke, Daniel wrote: > > > --- a/include/uapi/linux/gsmmux.h > > > +++ b/include/uapi/linux/gsmmux.h > > > @@ -58,7 +58,8 @@ struct gsm_dlci_config { > > > __u32 priority; /* Priority (0 for default value) */ > > > __u32 i; /* Frame type (1 = UIH, 2 = UI) */ > > > __u32 k; /* Window size (0 for default value) */ > > > - __u32 reserved[8]; /* For future use, must be initialized to zero */ > > > + __u32 restart; /* Force DLCI channel reset? */ > > > > Why are you using a full 32 bits for just 1 bit of data here? Why not > > use a bitfield? > > The ioctrl guide states: > Bitfields and enums generally work as one would expect them to, > but some properties of them are implementation-defined, so it is better > to avoid them completely in ioctl interfaces. > > Therefore, I tried to avoid them here. Then use a u8? > > And what happened to the request to turn the documentation for this > > structure into proper kerneldoc format? > > That applied to patch 2/8 and is unrelated to this patch. Another patch > will need to fix this. > > Link: https://lore.kernel.org/all/20230424075251.5216-2-daniel.starke@siemens.com/ It's kind of related in that the format is not right :) As it's a few weeks before I am allowed to even apply this, please rework the series a bit. thanks, greg k-h
On Mon, 24 Apr 2023, Greg KH wrote: > On Mon, Apr 24, 2023 at 11:03:26AM +0000, Starke, Daniel wrote: > > > > --- a/include/uapi/linux/gsmmux.h > > > > +++ b/include/uapi/linux/gsmmux.h > > > > @@ -58,7 +58,8 @@ struct gsm_dlci_config { > > > > __u32 priority; /* Priority (0 for default value) */ > > > > __u32 i; /* Frame type (1 = UIH, 2 = UI) */ > > > > __u32 k; /* Window size (0 for default value) */ > > > > - __u32 reserved[8]; /* For future use, must be initialized to zero */ > > > > + __u32 restart; /* Force DLCI channel reset? */ > > > > > > Why are you using a full 32 bits for just 1 bit of data here? Why not > > > use a bitfield? > > > > The ioctrl guide states: > > Bitfields and enums generally work as one would expect them to, > > but some properties of them are implementation-defined, so it is better > > to avoid them completely in ioctl interfaces. > > > > Therefore, I tried to avoid them here. > > Then use a u8? To add further, I think that the ioctl guidance tries to say that C bitfields using :number postfix are not a good idea, not that much to disallow flag like content within an integer type.
On Mon, Apr 24, 2023 at 04:20:00PM +0300, Ilpo Järvinen wrote: > On Mon, 24 Apr 2023, Greg KH wrote: > > > On Mon, Apr 24, 2023 at 11:03:26AM +0000, Starke, Daniel wrote: > > > > > --- a/include/uapi/linux/gsmmux.h > > > > > +++ b/include/uapi/linux/gsmmux.h > > > > > @@ -58,7 +58,8 @@ struct gsm_dlci_config { > > > > > __u32 priority; /* Priority (0 for default value) */ > > > > > __u32 i; /* Frame type (1 = UIH, 2 = UI) */ > > > > > __u32 k; /* Window size (0 for default value) */ > > > > > - __u32 reserved[8]; /* For future use, must be initialized to zero */ > > > > > + __u32 restart; /* Force DLCI channel reset? */ > > > > > > > > Why are you using a full 32 bits for just 1 bit of data here? Why not > > > > use a bitfield? > > > > > > The ioctrl guide states: > > > Bitfields and enums generally work as one would expect them to, > > > but some properties of them are implementation-defined, so it is better > > > to avoid them completely in ioctl interfaces. > > > > > > Therefore, I tried to avoid them here. > > > > Then use a u8? > > To add further, I think that the ioctl guidance tries to say that C > bitfields using :number postfix are not a good idea, not that much to > disallow flag like content within an integer type. Agreed.
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index b411a26cc092..00f692e2e810 100644 --- a/drivers/tty/n_gsm.c +++ b/drivers/tty/n_gsm.c @@ -2532,6 +2532,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in return -EINVAL; if (dc->k > 7) return -EINVAL; + if (dc->restart > 1) /* allow future extensions */ + return -EINVAL; /* * See what is needed for reconfiguration @@ -2546,6 +2548,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in /* Requires care */ if (dc->priority != dlci->prio) need_restart = true; + if (dc->restart) + need_restart = true; if ((open && gsm->wait_config) || need_restart) need_open = true; diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h index eb67884e5f38..33ee7b857c52 100644 --- a/include/uapi/linux/gsmmux.h +++ b/include/uapi/linux/gsmmux.h @@ -58,7 +58,8 @@ struct gsm_dlci_config { __u32 priority; /* Priority (0 for default value) */ __u32 i; /* Frame type (1 = UIH, 2 = UI) */ __u32 k; /* Window size (0 for default value) */ - __u32 reserved[8]; /* For future use, must be initialized to zero */ + __u32 restart; /* Force DLCI channel reset? */ + __u32 reserved[7]; /* For future use, must be initialized to zero */ }; #define GSMIOC_GETCONF_DLCI _IOWR('G', 7, struct gsm_dlci_config)