| Message ID | Pine.LNX.4.64.2401032017390.31157@wormhole.robuust.nl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [libgpiod,v2,retry] core: fix deselection of output direction when edge detection is selected | expand |
On Wed, 3 Jan 2024 20:18:27 +0100, "J.A. Bezemer" <j.a.bezemer@opensourcepartners.nl> 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@opensourcepartners.nl> > --- > 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 Bartosz
On Thu, 4 Jan 2024 11:51:50 +0100, Kent Gibson <warthog618@gmail.com> 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@opensourcepartners.nl> 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@opensourcepartners.nl> >> > --- >> > 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. The actual fix should be something like: diff --git a/lib/line-config.c b/lib/line-config.c index 2749a2a..9302c1b 100644 --- a/lib/line-config.c +++ b/lib/line-config.c @@ -381,18 +381,15 @@ 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; break; case GPIOD_LINE_EDGE_RISING: flags |= (GPIO_V2_LINE_FLAG_EDGE_RISING | GPIO_V2_LINE_FLAG_INPUT); - flags &= ~GPIOD_LINE_DIRECTION_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; break; default: break; Bartosz
On Thu, Jan 4, 2024 at 2:35 PM Kent Gibson <warthog618@gmail.com> 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@gmail.com> 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@opensourcepartners.nl> 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@opensourcepartners.nl> > > >> > --- > > >> > 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
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;
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@opensourcepartners.nl> --- lib/line-config.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)