[PATCH 0/4] gpiolib: cdev: directionless line reconfiguration

[PATCH 0/4] gpiolib: cdev: directionless line reconfiguration

[Kent Gibson]

The behaviour of request reconfiguration without a direction flag set is
ill-defined and badly behaved, for both uAPI v1 and v2.  I'll will refer
to such a configuration as 'directionless' here.  That refers to the
configuration requested, not the actual state of the line.

The configuration validation used during reconfiguration is borrowed from
the line request operation, where, to verify the intent of the user, the
direction must be set to in order to effect a change to the electrical
configuration of a line. But that validation does not allow for the
directionless case, making it possible to clear flags set previously
without specifying the line direction.

Adding to the inconsistency, those changes are not immediately applied,
but will take effect when the line value is next get or set.

For example, by requesting a reconfiguration with no flags set, an output
line requested with active low and open drain flags set could have those
flags cleared, inverting the sense of the line and changing the line drive
to push-pull on the next line value set.

This series addresses directionless reconfiguration behaviour for both
uAPI versions.

Patch 1 disallows reconfiguration without direction for uAPI v1.

Patch 2 ignores reconfiguration of a line without direction for uAPI v2.
A different approach is used, compared to uAPI v1, as v2 allows for
reconfiguration of multiple lines with different configurations.
It is more useful to skip directionless lines rather than returning an
error, as it allows for reconfiguration of a subset of requested lines.

Patches 3 and 4 update the documentation for uAPI v1 and v2, respectively,
to describe the updated behaviour.

Cheers,
Kent.

Kent Gibson (4):
  gpiolib: cdev: Disallow reconfiguration without direction (uAPI v1)
  gpiolib: cdev: Ignore reconfiguration without direction
  Documentation: gpio: Reconfiguration with unset direction (uAPI v1)
  Documentation: gpio: Reconfiguration with unset direction (uAPI v2)

 .../gpio/gpio-handle-set-config-ioctl.rst     |  5 +++-
 .../gpio/gpio-v2-line-set-config-ioctl.rst    |  7 +++--
 drivers/gpio/gpiolib-cdev.c                   | 28 +++++++++++--------
 3 files changed, 26 insertions(+), 14 deletions(-)

-- 
2.39.2

[PATCH 1/4] gpiolib: cdev: Disallow reconfiguration without direction (uAPI v1)

[Kent Gibson]

linehandle_set_config() behaves badly when direction is not set.
The configuration validation is borrowed from linehandle_create(), where,
to verify the intent of the user, the direction must be set to in order
to effect a change to the electrical configuration of a line. But, when
applied to reconfiguration, that validation does not allow for the unset
direction case, making it possible to clear flags set previously without
specifying the line direction.

Adding to the inconsistency, those changes are not immediately applied by
linehandle_set_config(), but will take effect when the line value is next
get or set.

For example, by requesting a configuration with no flags set, an output
line with GPIOHANDLE_REQUEST_ACTIVE_LOW and GPIOHANDLE_REQUEST_OPEN_DRAIN
requested could have those flags cleared, inverting the sense of the line
and changing the line drive to push-pull on the next line value set.

Ensure the intent of the user by disallowing configurations which do not
have direction set, returning an error to userspace to indicate that the
configuration is invalid.

And, for clarity, use lflags, a local copy of gcnf.flags, throughout when
dealing with the requested flags, rather than a mixture of both.

Fixes: e588bb1eae31 ("gpio: add new SET_CONFIG ioctl() to gpio chardev")
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 1cb952daacfb..f7a129d67b7d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -89,6 +89,10 @@ struct linehandle_state {
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
 	GPIOHANDLE_REQUEST_OPEN_SOURCE)
 
+#define GPIOHANDLE_REQUEST_DIRECTION_FLAGS \
+	(GPIOHANDLE_REQUEST_INPUT | \
+	 GPIOHANDLE_REQUEST_OUTPUT)
+
 static int linehandle_validate_flags(u32 flags)
 {
 	/* Return an error if an unknown flag is set */
@@ -169,21 +173,21 @@ static long linehandle_set_config(struct linehandle_state *lh,
 	if (ret)
 		return ret;
 
+	/* Lines must be reconfigured explicitly as input or output. */
+	if (!(lflags & GPIOHANDLE_REQUEST_DIRECTION_FLAGS))
+		return -EINVAL;
+
 	for (i = 0; i < lh->num_descs; i++) {
 		desc = lh->descs[i];
-		linehandle_flags_to_desc_flags(gcnf.flags, &desc->flags);
+		linehandle_flags_to_desc_flags(lflags, &desc->flags);
 
-		/*
-		 * Lines have to be requested explicitly for input
-		 * or output, else the line will be treated "as is".
-		 */
 		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
 			int val = !!gcnf.default_values[i];
 
 			ret = gpiod_direction_output(desc, val);
 			if (ret)
 				return ret;
-		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
+		} else {
 			ret = gpiod_direction_input(desc);
 			if (ret)
 				return ret;
-- 
2.39.2

[PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

[Kent Gibson]

linereq_set_config() behaves badly when direction is not set.
The configuration validation is borrowed from linereq_create(), where,
to verify the intent of the user, the direction must be set to in order to
effect a change to the electrical configuration of a line. But, when
applied to reconfiguration, that validation does not allow for the unset
direction case, making it possible to clear flags set previously without
specifying the line direction.

Adding to the inconsistency, those changes are not immediately applied by
linereq_set_config(), but will take effect when the line value is next get
or set.

For example, by requesting a configuration with no flags set, an output
line with GPIO_V2_LINE_FLAG_ACTIVE_LOW and GPIO_V2_LINE_FLAG_OPEN_DRAIN
set could have those flags cleared, inverting the sense of the line and
changing the line drive to push-pull on the next line value set.

Skip the reconfiguration of lines for which the direction is not set, and
only reconfigure the lines for which direction is set.

Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f7a129d67b7d..ef08b23a56e2 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1534,12 +1534,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
 		line = &lr->lines[i];
 		desc = lr->lines[i].desc;
 		flags = gpio_v2_line_config_flags(&lc, i);
+		/*
+		 * Lines not explicitly reconfigured as input or output
+		 * are left unchanged.
+		 */
+		if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
+			continue;
 		gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
 		edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS;
-		/*
-		 * Lines have to be requested explicitly for input
-		 * or output, else the line will be treated "as is".
-		 */
 		if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
 			int val = gpio_v2_line_config_output_value(&lc, i);
 
@@ -1547,7 +1549,7 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
 			ret = gpiod_direction_output(desc, val);
 			if (ret)
 				return ret;
-		} else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
+		} else {
 			ret = gpiod_direction_input(desc);
 			if (ret)
 				return ret;
-- 
2.39.2

[PATCH 3/4] Documentation: gpio: Reconfiguration with unset direction (uAPI v1)

[Kent Gibson]

Update description of reconfiguration rules, adding requirement that a
direction flag be set or the configuration is considered invalid.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 .../userspace-api/gpio/gpio-handle-set-config-ioctl.rst      | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/gpio/gpio-handle-set-config-ioctl.rst b/Documentation/userspace-api/gpio/gpio-handle-set-config-ioctl.rst
index d002a84681ac..a03f30db63ab 100644
--- a/Documentation/userspace-api/gpio/gpio-handle-set-config-ioctl.rst
+++ b/Documentation/userspace-api/gpio/gpio-handle-set-config-ioctl.rst
@@ -43,7 +43,10 @@ The configuration applies to all requested lines.
 
 The same :ref:`gpio-get-linehandle-config-rules` and
 :ref:`gpio-get-linehandle-config-support` that apply when requesting the
-lines also apply when updating the line configuration.
+lines also apply when updating the line configuration, with the additional
+restriction that a direction flag must be set. Requesting an invalid
+configuration, including without a direction flag set, is an error
+(**EINVAL**).
 
 The motivating use case for this command is changing direction of
 bi-directional lines between input and output, but it may be used more
-- 
2.39.2

[PATCH 4/4] Documentation: gpio: Reconfiguration with unset direction (uAPI v2)

[Kent Gibson]

Update description of reconfiguration rules, adding requirement that a
direction flag be set to enable changing configuration for a line.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 .../userspace-api/gpio/gpio-v2-line-set-config-ioctl.rst   | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/gpio/gpio-v2-line-set-config-ioctl.rst b/Documentation/userspace-api/gpio/gpio-v2-line-set-config-ioctl.rst
index 9b942a8a53ca..cfaab801556c 100644
--- a/Documentation/userspace-api/gpio/gpio-v2-line-set-config-ioctl.rst
+++ b/Documentation/userspace-api/gpio/gpio-v2-line-set-config-ioctl.rst
@@ -35,11 +35,14 @@ Description
 Update the configuration of previously requested lines, without releasing the
 line or introducing potential glitches.
 
-The new configuration must specify the configuration of all requested lines.
+The new configuration must specify a configuration for all requested lines.
 
 The same :ref:`gpio-v2-get-line-config-rules` and
 :ref:`gpio-v2-get-line-config-support` that apply when requesting the lines
-also apply when updating the line configuration.
+also apply when updating the line configuration, with the additional
+restriction that a direction flag must be set to enable reconfiguration.
+If no direction flag is set in the configuration for a given line then the
+configuration for that line is left unchanged.
 
 The motivating use case for this command is changing direction of
 bi-directional lines between input and output, but it may also be used to
-- 
2.39.2

Re: [PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

[Bartosz Golaszewski]

On Wed, Jun 26, 2024 at 7:29 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> linereq_set_config() behaves badly when direction is not set.
> The configuration validation is borrowed from linereq_create(), where,
> to verify the intent of the user, the direction must be set to in order to
> effect a change to the electrical configuration of a line. But, when
> applied to reconfiguration, that validation does not allow for the unset
> direction case, making it possible to clear flags set previously without
> specifying the line direction.
>
> Adding to the inconsistency, those changes are not immediately applied by
> linereq_set_config(), but will take effect when the line value is next get
> or set.
>
> For example, by requesting a configuration with no flags set, an output
> line with GPIO_V2_LINE_FLAG_ACTIVE_LOW and GPIO_V2_LINE_FLAG_OPEN_DRAIN
> set could have those flags cleared, inverting the sense of the line and
> changing the line drive to push-pull on the next line value set.
>
> Skip the reconfiguration of lines for which the direction is not set, and
> only reconfigure the lines for which direction is set.
>
> Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index f7a129d67b7d..ef08b23a56e2 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1534,12 +1534,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
>                 line = &lr->lines[i];
>                 desc = lr->lines[i].desc;
>                 flags = gpio_v2_line_config_flags(&lc, i);
> +               /*
> +                * Lines not explicitly reconfigured as input or output
> +                * are left unchanged.
> +                */
> +               if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> +                       continue;

Series looks good, thanks. I'd say that this bit here calls for at
least a debug-level message since we don't return an error unlike v1.
What do you think?

Bart

>                 gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
>                 edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS;
> -               /*
> -                * Lines have to be requested explicitly for input
> -                * or output, else the line will be treated "as is".
> -                */
>                 if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
>                         int val = gpio_v2_line_config_output_value(&lc, i);
>
> @@ -1547,7 +1549,7 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
>                         ret = gpiod_direction_output(desc, val);
>                         if (ret)
>                                 return ret;
> -               } else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
> +               } else {
>                         ret = gpiod_direction_input(desc);
>                         if (ret)
>                                 return ret;
> --
> 2.39.2
>

Re: [PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

[Kent Gibson]

On Thu, Jun 27, 2024 at 04:06:21PM +0200, Bartosz Golaszewski wrote:
> On Wed, Jun 26, 2024 at 7:29 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > linereq_set_config() behaves badly when direction is not set.
> > The configuration validation is borrowed from linereq_create(), where,
> > to verify the intent of the user, the direction must be set to in order to
> > effect a change to the electrical configuration of a line. But, when
> > applied to reconfiguration, that validation does not allow for the unset
> > direction case, making it possible to clear flags set previously without
> > specifying the line direction.
> >
> > Adding to the inconsistency, those changes are not immediately applied by
> > linereq_set_config(), but will take effect when the line value is next get
> > or set.
> >
> > For example, by requesting a configuration with no flags set, an output
> > line with GPIO_V2_LINE_FLAG_ACTIVE_LOW and GPIO_V2_LINE_FLAG_OPEN_DRAIN
> > set could have those flags cleared, inverting the sense of the line and
> > changing the line drive to push-pull on the next line value set.
> >
> > Skip the reconfiguration of lines for which the direction is not set, and
> > only reconfigure the lines for which direction is set.
> >
> > Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index f7a129d67b7d..ef08b23a56e2 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -1534,12 +1534,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> >                 line = &lr->lines[i];
> >                 desc = lr->lines[i].desc;
> >                 flags = gpio_v2_line_config_flags(&lc, i);
> > +               /*
> > +                * Lines not explicitly reconfigured as input or output
> > +                * are left unchanged.
> > +                */
> > +               if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> > +                       continue;
>
> Series looks good, thanks. I'd say that this bit here calls for at
> least a debug-level message since we don't return an error unlike v1.
> What do you think?
>

The change to the libgpiod Python bindings makes use of this to support
reconfiguration of subsets, so on its own it isn't an abnormal path and
I'm not sure it warrants even a debug.

OTOH, I did consider if there should be a check that at least one line
in the reconfig has a direction, returning an error if there are none, but
was on the fence about it and left it out as it added complexity.

Would that make more sense?
Or do you have a problem with reconfiguring subsets?

Cheers,
Kent.

Re: [PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

[Bartosz Golaszewski]

On Thu, Jun 27, 2024 at 4:22 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Jun 27, 2024 at 04:06:21PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Jun 26, 2024 at 7:29 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > linereq_set_config() behaves badly when direction is not set.
> > > The configuration validation is borrowed from linereq_create(), where,
> > > to verify the intent of the user, the direction must be set to in order to
> > > effect a change to the electrical configuration of a line. But, when
> > > applied to reconfiguration, that validation does not allow for the unset
> > > direction case, making it possible to clear flags set previously without
> > > specifying the line direction.
> > >
> > > Adding to the inconsistency, those changes are not immediately applied by
> > > linereq_set_config(), but will take effect when the line value is next get
> > > or set.
> > >
> > > For example, by requesting a configuration with no flags set, an output
> > > line with GPIO_V2_LINE_FLAG_ACTIVE_LOW and GPIO_V2_LINE_FLAG_OPEN_DRAIN
> > > set could have those flags cleared, inverting the sense of the line and
> > > changing the line drive to push-pull on the next line value set.
> > >
> > > Skip the reconfiguration of lines for which the direction is not set, and
> > > only reconfigure the lines for which direction is set.
> > >
> > > Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > ---
> > >  drivers/gpio/gpiolib-cdev.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > index f7a129d67b7d..ef08b23a56e2 100644
> > > --- a/drivers/gpio/gpiolib-cdev.c
> > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > @@ -1534,12 +1534,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > >                 line = &lr->lines[i];
> > >                 desc = lr->lines[i].desc;
> > >                 flags = gpio_v2_line_config_flags(&lc, i);
> > > +               /*
> > > +                * Lines not explicitly reconfigured as input or output
> > > +                * are left unchanged.
> > > +                */
> > > +               if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> > > +                       continue;
> >
> > Series looks good, thanks. I'd say that this bit here calls for at
> > least a debug-level message since we don't return an error unlike v1.
> > What do you think?
> >
>
> The change to the libgpiod Python bindings makes use of this to support
> reconfiguration of subsets, so on its own it isn't an abnormal path and
> I'm not sure it warrants even a debug.
>
> OTOH, I did consider if there should be a check that at least one line
> in the reconfig has a direction, returning an error if there are none, but
> was on the fence about it and left it out as it added complexity.
>
> Would that make more sense?
> Or do you have a problem with reconfiguring subsets?
>
> Cheers,
> Kent.

I see. Ok, I'll take it as is interpreting it as a feature.

Bart

Re: [PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

[Kent Gibson]

On Thu, Jun 27, 2024 at 04:44:02PM +0200, Bartosz Golaszewski wrote:
> On Thu, Jun 27, 2024 at 4:22 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Jun 27, 2024 at 04:06:21PM +0200, Bartosz Golaszewski wrote:
> > > On Wed, Jun 26, 2024 at 7:29 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > linereq_set_config() behaves badly when direction is not set.
> > > > The configuration validation is borrowed from linereq_create(), where,
> > > > to verify the intent of the user, the direction must be set to in order to
> > > > effect a change to the electrical configuration of a line. But, when
> > > > applied to reconfiguration, that validation does not allow for the unset
> > > > direction case, making it possible to clear flags set previously without
> > > > specifying the line direction.
> > > >
> > > > Adding to the inconsistency, those changes are not immediately applied by
> > > > linereq_set_config(), but will take effect when the line value is next get
> > > > or set.
> > > >
> > > > For example, by requesting a configuration with no flags set, an output
> > > > line with GPIO_V2_LINE_FLAG_ACTIVE_LOW and GPIO_V2_LINE_FLAG_OPEN_DRAIN
> > > > set could have those flags cleared, inverting the sense of the line and
> > > > changing the line drive to push-pull on the next line value set.
> > > >
> > > > Skip the reconfiguration of lines for which the direction is not set, and
> > > > only reconfigure the lines for which direction is set.
> > > >
> > > > Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> > > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > > ---
> > > >  drivers/gpio/gpiolib-cdev.c | 12 +++++++-----
> > > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > > index f7a129d67b7d..ef08b23a56e2 100644
> > > > --- a/drivers/gpio/gpiolib-cdev.c
> > > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > > @@ -1534,12 +1534,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > >                 line = &lr->lines[i];
> > > >                 desc = lr->lines[i].desc;
> > > >                 flags = gpio_v2_line_config_flags(&lc, i);
> > > > +               /*
> > > > +                * Lines not explicitly reconfigured as input or output
> > > > +                * are left unchanged.
> > > > +                */
> > > > +               if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> > > > +                       continue;
> > >
> > > Series looks good, thanks. I'd say that this bit here calls for at
> > > least a debug-level message since we don't return an error unlike v1.
> > > What do you think?
> > >
> >
> > The change to the libgpiod Python bindings makes use of this to support
> > reconfiguration of subsets, so on its own it isn't an abnormal path and
> > I'm not sure it warrants even a debug.
> >
> > OTOH, I did consider if there should be a check that at least one line
> > in the reconfig has a direction, returning an error if there are none, but
> > was on the fence about it and left it out as it added complexity.
> >
> > Would that make more sense?
> > Or do you have a problem with reconfiguring subsets?
> >
> > Cheers,
> > Kent.
>
> I see. Ok, I'll take it as is interpreting it as a feature.
>

I'm totally ok with adding a check that direction is set at least once,
if you would like that. Can be done with a reasonably minor change to
gpio_v2_line_config_validate().  Though that would probably still double
the size of this patch.

Cheers,
Kent.

Re: [PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

[Bartosz Golaszewski]

On Thu, Jun 27, 2024 at 4:49 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Jun 27, 2024 at 04:44:02PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Jun 27, 2024 at 4:22 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Jun 27, 2024 at 04:06:21PM +0200, Bartosz Golaszewski wrote:
> > > > On Wed, Jun 26, 2024 at 7:29 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > linereq_set_config() behaves badly when direction is not set.
> > > > > The configuration validation is borrowed from linereq_create(), where,
> > > > > to verify the intent of the user, the direction must be set to in order to
> > > > > effect a change to the electrical configuration of a line. But, when
> > > > > applied to reconfiguration, that validation does not allow for the unset
> > > > > direction case, making it possible to clear flags set previously without
> > > > > specifying the line direction.
> > > > >
> > > > > Adding to the inconsistency, those changes are not immediately applied by
> > > > > linereq_set_config(), but will take effect when the line value is next get
> > > > > or set.
> > > > >
> > > > > For example, by requesting a configuration with no flags set, an output
> > > > > line with GPIO_V2_LINE_FLAG_ACTIVE_LOW and GPIO_V2_LINE_FLAG_OPEN_DRAIN
> > > > > set could have those flags cleared, inverting the sense of the line and
> > > > > changing the line drive to push-pull on the next line value set.
> > > > >
> > > > > Skip the reconfiguration of lines for which the direction is not set, and
> > > > > only reconfigure the lines for which direction is set.
> > > > >
> > > > > Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > > > ---
> > > > >  drivers/gpio/gpiolib-cdev.c | 12 +++++++-----
> > > > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > > > index f7a129d67b7d..ef08b23a56e2 100644
> > > > > --- a/drivers/gpio/gpiolib-cdev.c
> > > > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > > > @@ -1534,12 +1534,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > >                 line = &lr->lines[i];
> > > > >                 desc = lr->lines[i].desc;
> > > > >                 flags = gpio_v2_line_config_flags(&lc, i);
> > > > > +               /*
> > > > > +                * Lines not explicitly reconfigured as input or output
> > > > > +                * are left unchanged.
> > > > > +                */
> > > > > +               if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> > > > > +                       continue;
> > > >
> > > > Series looks good, thanks. I'd say that this bit here calls for at
> > > > least a debug-level message since we don't return an error unlike v1.
> > > > What do you think?
> > > >
> > >
> > > The change to the libgpiod Python bindings makes use of this to support
> > > reconfiguration of subsets, so on its own it isn't an abnormal path and
> > > I'm not sure it warrants even a debug.
> > >
> > > OTOH, I did consider if there should be a check that at least one line
> > > in the reconfig has a direction, returning an error if there are none, but
> > > was on the fence about it and left it out as it added complexity.
> > >
> > > Would that make more sense?
> > > Or do you have a problem with reconfiguring subsets?
> > >
> > > Cheers,
> > > Kent.
> >
> > I see. Ok, I'll take it as is interpreting it as a feature.
> >
>
> I'm totally ok with adding a check that direction is set at least once,
> if you would like that. Can be done with a reasonably minor change to
> gpio_v2_line_config_validate().  Though that would probably still double
> the size of this patch.
>
> Cheers,
> Kent.
>

No, it's fine. Docs are quite explicit about the behavior and there's
a comment in place.

Bart

Re: [PATCH 0/4] gpiolib: cdev: directionless line reconfiguration

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 26 Jun 2024 13:29:21 +0800, Kent Gibson wrote:
> The behaviour of request reconfiguration without a direction flag set is
> ill-defined and badly behaved, for both uAPI v1 and v2.  I'll will refer
> to such a configuration as 'directionless' here.  That refers to the
> configuration requested, not the actual state of the line.
> 
> The configuration validation used during reconfiguration is borrowed from
> the line request operation, where, to verify the intent of the user, the
> direction must be set to in order to effect a change to the electrical
> configuration of a line. But that validation does not allow for the
> directionless case, making it possible to clear flags set previously
> without specifying the line direction.
> 
> [...]

I applied the first two for fixes and will apply the remaining two for the
upcoming merge window once this gets upstream.

[1/4] gpiolib: cdev: Disallow reconfiguration without direction (uAPI v1)
      commit: 9919cce62f68e6ab68dc2a975b5dc670f8ca7d40
[2/4] gpiolib: cdev: Ignore reconfiguration without direction
      commit: b440396387418fe2feaacd41ca16080e7a8bc9ad

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 0/4] gpiolib: cdev: directionless line reconfiguration

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 26 Jun 2024 13:29:21 +0800, Kent Gibson wrote:
> The behaviour of request reconfiguration without a direction flag set is
> ill-defined and badly behaved, for both uAPI v1 and v2.  I'll will refer
> to such a configuration as 'directionless' here.  That refers to the
> configuration requested, not the actual state of the line.
> 
> The configuration validation used during reconfiguration is borrowed from
> the line request operation, where, to verify the intent of the user, the
> direction must be set to in order to effect a change to the electrical
> configuration of a line. But that validation does not allow for the
> directionless case, making it possible to clear flags set previously
> without specifying the line direction.
> 
> [...]

Applied, thanks!

[3/4] Documentation: gpio: Reconfiguration with unset direction (uAPI v1)
      commit: e48fe75afa539d110753f7420aa398ef89f8e383
[4/4] Documentation: gpio: Reconfiguration with unset direction (uAPI v2)
      commit: 6a9c15083b1662a4b7b36e787272deb696d72c24

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>