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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

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