| Message ID | cover.1683022164.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| Headers | show |
| Series | Input: tests - miscellaneous fixes | expand |
Hi Javier, On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven <geert+renesas@glider.be> wrote: > This patch series fixes a crash in the new input selftest, and makes the > test available when the KUnit framework is modular. > > Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W) > and ARAnyM): > > KTAP version 1 > # Subtest: input_core > 1..3 > input: Test input device as /devices/virtual/input/input1 > ok 1 input_test_polling > input: Test input device as /devices/virtual/input/input2 > ok 2 input_test_timestamp > input: Test input device as /devices/virtual/input/input3 > # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99 > Expected input_match_device_id(input_dev, &id) to be true, but is false > not ok 3 input_test_match_device_id > # input_core: pass:2 fail:1 skip:0 total:3 > # Totals: pass:2 fail:1 skip:0 total:3 > not ok 1 input_core Adding more debug code shows that it's the test on evbit [1] in input_match_device_id() that fails. Looking at your input_test_match_device_id(), I think you expect the checks for the various bitmaps to be gated by "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the other checks? [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021 Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, > > On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven > <geert+renesas@glider.be> wrote: >> This patch series fixes a crash in the new input selftest, and makes the >> test available when the KUnit framework is modular. >> >> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W) >> and ARAnyM): >> >> KTAP version 1 >> # Subtest: input_core >> 1..3 >> input: Test input device as /devices/virtual/input/input1 >> ok 1 input_test_polling >> input: Test input device as /devices/virtual/input/input2 >> ok 2 input_test_timestamp >> input: Test input device as /devices/virtual/input/input3 >> # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99 >> Expected input_match_device_id(input_dev, &id) to be true, but is false >> not ok 3 input_test_match_device_id >> # input_core: pass:2 fail:1 skip:0 total:3 >> # Totals: pass:2 fail:1 skip:0 total:3 >> not ok 1 input_core > > Adding more debug code shows that it's the test on evbit [1] in > input_match_device_id() that fails. > Looking at your input_test_match_device_id(), I think you expect > the checks for the various bitmaps to be gated by > "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the > other checks? > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021 > That's correct. In input_test_init(), the input dev is marked as capable of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to check this. That is, check if matches by the input dev capabilities in which case the __set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..) would make the condition false. But maybe I misunderstood how the input_set_capability() and __set_bit() functions work ? I'll take a look to this tomorrow, thanks a lot for your report!
On Tue, May 02, 2023 at 06:31:51PM +0200, Javier Martinez Canillas wrote: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > Hello Geert, > > > Hi Javier, > > > > On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven > > <geert+renesas@glider.be> wrote: > >> This patch series fixes a crash in the new input selftest, and makes the > >> test available when the KUnit framework is modular. > >> > >> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W) > >> and ARAnyM): > >> > >> KTAP version 1 > >> # Subtest: input_core > >> 1..3 > >> input: Test input device as /devices/virtual/input/input1 > >> ok 1 input_test_polling > >> input: Test input device as /devices/virtual/input/input2 > >> ok 2 input_test_timestamp > >> input: Test input device as /devices/virtual/input/input3 > >> # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99 > >> Expected input_match_device_id(input_dev, &id) to be true, but is false > >> not ok 3 input_test_match_device_id > >> # input_core: pass:2 fail:1 skip:0 total:3 > >> # Totals: pass:2 fail:1 skip:0 total:3 > >> not ok 1 input_core > > > > Adding more debug code shows that it's the test on evbit [1] in > > input_match_device_id() that fails. > > Looking at your input_test_match_device_id(), I think you expect > > the checks for the various bitmaps to be gated by > > "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the > > other checks? > > > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021 > > > > That's correct. In input_test_init(), the input dev is marked as capable > of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to > check this. > > That is, check if matches by the input dev capabilities in which case the > __set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..) > would make the condition false. > > But maybe I misunderstood how the input_set_capability() and __set_bit() > functions work ? > > I'll take a look to this tomorrow, thanks a lot for your report! Unfortunately (?) INPUT_DEVICE_ID_MATCH_*BIT have never had any effect, the kernel always used bitmaps when matching (with the assumption that if one does not care about given bitmap they can simply pass empty one), so I think what we need to change is: diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c index 8b8ac3412a70..0540225f0288 100644 --- a/drivers/input/tests/input_test.c +++ b/drivers/input/tests/input_test.c @@ -87,7 +87,7 @@ static void input_test_timestamp(struct kunit *test) static void input_test_match_device_id(struct kunit *test) { struct input_dev *input_dev = test->priv; - struct input_device_id id; + struct input_device_id id = { 0 }; /* * Must match when the input device bus, vendor, product, version to avoid having garbage in the match data. I suppose we should remove INPUT_DEVICE_ID_MATCH_*BIT from mod_devicetable.h to avoid this confusion. Thanks.
Hi Dmitry, On Tue, May 2, 2023 at 7:05 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Tue, May 02, 2023 at 06:31:51PM +0200, Javier Martinez Canillas wrote: > > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > > On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven > > > <geert+renesas@glider.be> wrote: > > >> This patch series fixes a crash in the new input selftest, and makes the > > >> test available when the KUnit framework is modular. > > >> > > >> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W) > > >> and ARAnyM): > > >> > > >> KTAP version 1 > > >> # Subtest: input_core > > >> 1..3 > > >> input: Test input device as /devices/virtual/input/input1 > > >> ok 1 input_test_polling > > >> input: Test input device as /devices/virtual/input/input2 > > >> ok 2 input_test_timestamp > > >> input: Test input device as /devices/virtual/input/input3 > > >> # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99 > > >> Expected input_match_device_id(input_dev, &id) to be true, but is false > > >> not ok 3 input_test_match_device_id > > >> # input_core: pass:2 fail:1 skip:0 total:3 > > >> # Totals: pass:2 fail:1 skip:0 total:3 > > >> not ok 1 input_core > > > > > > Adding more debug code shows that it's the test on evbit [1] in > > > input_match_device_id() that fails. > > > Looking at your input_test_match_device_id(), I think you expect > > > the checks for the various bitmaps to be gated by > > > "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the > > > other checks? > > > > > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021 > > > > > > > That's correct. In input_test_init(), the input dev is marked as capable > > of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to > > check this. > > > > That is, check if matches by the input dev capabilities in which case the > > __set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..) > > would make the condition false. > > > > But maybe I misunderstood how the input_set_capability() and __set_bit() > > functions work ? > > > > I'll take a look to this tomorrow, thanks a lot for your report! > > Unfortunately (?) INPUT_DEVICE_ID_MATCH_*BIT have never had any effect, > the kernel always used bitmaps when matching (with the assumption that > if one does not care about given bitmap they can simply pass empty one), > so I think what we need to change is: > > diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c > index 8b8ac3412a70..0540225f0288 100644 > --- a/drivers/input/tests/input_test.c > +++ b/drivers/input/tests/input_test.c > @@ -87,7 +87,7 @@ static void input_test_timestamp(struct kunit *test) > static void input_test_match_device_id(struct kunit *test) > { > struct input_dev *input_dev = test->priv; > - struct input_device_id id; > + struct input_device_id id = { 0 }; > > /* > * Must match when the input device bus, vendor, product, version > > to avoid having garbage in the match data. Thanks, that did the trick! 3/3 tests pass. Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: > Hi Dmitry, > > On Tue, May 2, 2023 at 7:05 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: >> On Tue, May 02, 2023 at 06:31:51PM +0200, Javier Martinez Canillas wrote: >> > Geert Uytterhoeven <geert@linux-m68k.org> writes: >> > > On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven >> > > <geert+renesas@glider.be> wrote: >> > >> This patch series fixes a crash in the new input selftest, and makes the >> > >> test available when the KUnit framework is modular. >> > >> >> > >> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W) >> > >> and ARAnyM): >> > >> >> > >> KTAP version 1 >> > >> # Subtest: input_core >> > >> 1..3 >> > >> input: Test input device as /devices/virtual/input/input1 >> > >> ok 1 input_test_polling >> > >> input: Test input device as /devices/virtual/input/input2 >> > >> ok 2 input_test_timestamp >> > >> input: Test input device as /devices/virtual/input/input3 >> > >> # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99 >> > >> Expected input_match_device_id(input_dev, &id) to be true, but is false >> > >> not ok 3 input_test_match_device_id >> > >> # input_core: pass:2 fail:1 skip:0 total:3 >> > >> # Totals: pass:2 fail:1 skip:0 total:3 >> > >> not ok 1 input_core >> > > >> > > Adding more debug code shows that it's the test on evbit [1] in >> > > input_match_device_id() that fails. >> > > Looking at your input_test_match_device_id(), I think you expect >> > > the checks for the various bitmaps to be gated by >> > > "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the >> > > other checks? >> > > >> > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021 >> > > >> > >> > That's correct. In input_test_init(), the input dev is marked as capable >> > of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to >> > check this. >> > >> > That is, check if matches by the input dev capabilities in which case the >> > __set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..) >> > would make the condition false. >> > >> > But maybe I misunderstood how the input_set_capability() and __set_bit() >> > functions work ? >> > >> > I'll take a look to this tomorrow, thanks a lot for your report! >> >> Unfortunately (?) INPUT_DEVICE_ID_MATCH_*BIT have never had any effect, >> the kernel always used bitmaps when matching (with the assumption that >> if one does not care about given bitmap they can simply pass empty one), >> so I think what we need to change is: >> >> diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c >> index 8b8ac3412a70..0540225f0288 100644 >> --- a/drivers/input/tests/input_test.c >> +++ b/drivers/input/tests/input_test.c >> @@ -87,7 +87,7 @@ static void input_test_timestamp(struct kunit *test) >> static void input_test_match_device_id(struct kunit *test) >> { >> struct input_dev *input_dev = test->priv; >> - struct input_device_id id; >> + struct input_device_id id = { 0 }; >> >> /* >> * Must match when the input device bus, vendor, product, version >> >> to avoid having garbage in the match data. > > Thanks, that did the trick! 3/3 tests pass. > Oh, silly me. Are you going to post that as a proper patch ?
Hi all, This patch series fixes a crash in the new input selftest, and makes the test available when the KUnit framework is modular. Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W) and ARAnyM): KTAP version 1 # Subtest: input_core 1..3 input: Test input device as /devices/virtual/input/input1 ok 1 input_test_polling input: Test input device as /devices/virtual/input/input2 ok 2 input_test_timestamp input: Test input device as /devices/virtual/input/input3 # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99 Expected input_match_device_id(input_dev, &id) to be true, but is false not ok 3 input_test_match_device_id # input_core: pass:2 fail:1 skip:0 total:3 # Totals: pass:2 fail:1 skip:0 total:3 not ok 1 input_core Thanks! Geert Uytterhoeven (2): Input: tests - fix use-after-free and refcount underflow in input_test_exit() Input: tests - modular KUnit tests should not depend on KUNIT=y drivers/input/Kconfig | 2 +- drivers/input/tests/input_test.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)