From: Daniel Starke <[email protected]>
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 <[email protected]>
---
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/[email protected]/
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)
--
2.34.1
From: Daniel Starke <[email protected]>
The variable 'ret' is not used before assignment from gsm_activate_mux().
Still it gets initialized to zero at declaration.
Fix this as remarked in the link below by moving the declaration to the
first assignment.
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Daniel Starke <[email protected]>
---
drivers/tty/n_gsm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
v2 -> v3:
No changes.
Link: https://lore.kernel.org/all/[email protected]/
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 00f692e2e810..9a2cf3e14678 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3277,7 +3277,6 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
{
- int ret = 0;
int need_close = 0;
int need_restart = 0;
@@ -3356,7 +3355,7 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
* and removing from the mux array
*/
if (gsm->dead) {
- ret = gsm_activate_mux(gsm);
+ int ret = gsm_activate_mux(gsm);
if (ret)
return ret;
if (gsm->initiator)
--
2.34.1
On Mon, Apr 24, 2023 at 09:52:44AM +0200, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> 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 <[email protected]>
> ---
> 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/[email protected]/
>
> 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/[email protected]/
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/[email protected]/
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.
--
i.
> > > 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/[email protected]/
>
> 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, 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.