| Message ID | 20240626052925.174272-3-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: cdev: directionless line reconfiguration | expand |
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 >
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.
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
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.
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
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;
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(-)