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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> > > > > --- > > > > 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.