Re: [libgpiod][PATCH v2 retry] core: fix deselection of output direction when edge detection is selected

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Jan 4, 2024 at 2:35 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Thu, Jan 04, 2024 at 05:13:59AM -0800, Bartosz Golaszewski wrote:
> > On Thu, 4 Jan 2024 11:51:50 +0100, Kent Gibson <warthog618@xxxxxxxxx> said:
> > > On Thu, Jan 04, 2024 at 01:13:35AM -0800, Bartosz Golaszewski wrote:
> > >> On Wed, 3 Jan 2024 20:18:27 +0100, "J.A. Bezemer"
> > >> <j.a.bezemer@xxxxxxxxxxxxxxxxxxxxx> said:
> > >> > Fix deselection of output direction when edge detection is selected in
> > >> > make_kernel_flags(). Use correct flag to perform deselection rather than
> > >> > a library enum.
> > >> >
> > >> > For correct usage, there are no visible side-effects. The wrongly reset
> > >> > kernel flags are always zero already.
> > >> >
> > >> > For incorrect usage of edge detection combined with output direction,
> > >> > both output and input directions would have been requested from the
> > >> > kernel, causing a confusing error. Such usage will now be sanitized, as
> > >> > intended, into a working configuration with only input direction.
> > >> >
> > >> > Signed-off-by: Anne Bezemer <j.a.bezemer@xxxxxxxxxxxxxxxxxxxxx>
> > >> > ---
> > >> >  lib/line-config.c | 6 +++---
> > >> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >> >
> > >> > diff --git a/lib/line-config.c b/lib/line-config.c
> > >> > index 2749a2a..9bf7734 100644
> > >> > --- a/lib/line-config.c
> > >> > +++ b/lib/line-config.c
> > >> > @@ -381,18 +381,18 @@ static uint64_t make_kernel_flags(struct gpiod_line_settings *settings)
> > >> >          case GPIOD_LINE_EDGE_FALLING:
> > >> >                  flags |= (GPIO_V2_LINE_FLAG_EDGE_FALLING |
> > >> >                            GPIO_V2_LINE_FLAG_INPUT);
> > >> > -                flags &= ~GPIOD_LINE_DIRECTION_OUTPUT;
> > >> > +                flags &= ~GPIO_V2_LINE_FLAG_OUTPUT;
> > >> >                  break;
> > >> >          case GPIOD_LINE_EDGE_RISING:
> > >> >                  flags |= (GPIO_V2_LINE_FLAG_EDGE_RISING |
> > >> >                            GPIO_V2_LINE_FLAG_INPUT);
> > >> > -                flags &= ~GPIOD_LINE_DIRECTION_OUTPUT;
> > >> > +                flags &= ~GPIO_V2_LINE_FLAG_OUTPUT;
> > >> >                  break;
> > >> >          case GPIOD_LINE_EDGE_BOTH:
> > >> >                  flags |= (GPIO_V2_LINE_FLAG_EDGE_FALLING |
> > >> >                            GPIO_V2_LINE_FLAG_EDGE_RISING |
> > >> >                            GPIO_V2_LINE_FLAG_INPUT);
> > >> > -                flags &= ~GPIOD_LINE_DIRECTION_OUTPUT;
> > >> > +                flags &= ~GPIO_V2_LINE_FLAG_OUTPUT;
> > >> >                  break;
> > >> >          default:
> > >> >                  break;
> > >> > --
> > >> > 2.30.2
> > >> >
> > >> >
> > >> >
> > >>
> > >> It doesn't seem like you ran the test suite because it breaks one of
> > >> the test cases:
> > >>
> > >> **
> > >> gpiod-test:ERROR:tests-edge-event.c:80:_gpiod_test_func_cannot_request_lines_in_output_mode_with_edge_detection:
> > >> 'request' should be NULL
> > >> # gpiod-test:ERROR:tests-edge-event.c:80:_gpiod_test_func_cannot_request_lines_in_output_mode_with_edge_detection:
> > >> 'request' should be NULL
> > >> **
> > >> gpiod-test:ERROR:tests-edge-event.c:81:_gpiod_test_func_cannot_request_lines_in_output_mode_with_edge_detection:
> > >> assertion failed (22 == errno): (22 == 0)
> > >> # gpiod-test:ERROR:tests-edge-event.c:81:_gpiod_test_func_cannot_request_lines_in_output_mode_with_edge_detection:
> > >> assertion failed (22 == errno): (22 == 0)
> > >> not ok 19 /gpiod/edge-event/cannot_request_lines_in_output_mode_with_edge_detection
> > >>
> > >
> > > Interesting.  So the actual bug is that make_kernel_flags() shouldm't be
> > > sanatizing the flags at all?
> > >
> >
> > Yes, I think so because we don't want to silently drop the output flag but
> > rather complain the user tried to use it together with edge detection.
> >
>
> That is what I thought you meant - it just seems at odds with what you
> were thinking when you wrote it, cos the intent does look to be sanitizing.
>
> I'm fine either approach way, btw, just verifiying which way you wanted
> to go - the other option being changing the test.
>

I no longer remember what I was thinking at the time but it makes more
sense to me now to not sanitize and complain loudly about a clearly
wrong configuration.

Bartosz