[PATCH 0/2] Input: tests - miscellaneous fixes

[Geert Uytterhoeven]

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(-)

--
2.34.1

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[Geert Uytterhoeven]

While KUnit tests that cannot be built as a loadable module must depend
on "KUNIT=y", this is not true for modular tests, where it adds an
unnecessary limitation.

Fix this by relaxing the dependency to "KUNIT".

Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
Signed-off-by: Geert Uytterhoeven <geert+...@glider.be>
---
drivers/input/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index 735f90b74ee5ad44..3bdbd34314b3495a 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -168,7 +168,7 @@ config INPUT_EVBUG

config INPUT_KUNIT_TEST
tristate "KUnit tests for Input" if !KUNIT_ALL_TESTS
- depends on INPUT && KUNIT=y
+ depends on INPUT && KUNIT
default KUNIT_ALL_TESTS
help
Say Y here if you want to build the KUnit tests for the input
--
2.34.1

[Javier Martinez Canillas]

Hello Geert,

I've only been Cc'ed in patch #2.

Geert Uytterhoeven <geert+...@glider.be> writes:

> While KUnit tests that cannot be built as a loadable module must depend
> on "KUNIT=y", this is not true for modular tests, where it adds an
> unnecessary limitation.
>
> Fix this by relaxing the dependency to "KUNIT".
>
> Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
> Signed-off-by: Geert Uytterhoeven <geert+...@glider.be>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@redhat.com>

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[Geert Uytterhoeven]

Hi Javier,

On Tue, May 2, 2023 at 1:19 PM Javier Martinez Canillas
<jav...@redhat.com> wrote:
> I've only been Cc'ed in patch #2.

Not really, you're in the To-header on the full series?
https://lore.kernel.org/all/cover.1683022164....@glider.be

> Geert Uytterhoeven <geert+...@glider.be> writes:
> > While KUnit tests that cannot be built as a loadable module must depend
> > on "KUNIT=y", this is not true for modular tests, where it adds an
> > unnecessary limitation.
> >
> > Fix this by relaxing the dependency to "KUNIT".
> >
> > Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
> > Signed-off-by: Geert Uytterhoeven <geert+...@glider.be>
> > ---
>
> Reviewed-by: Javier Martinez Canillas <jav...@redhat.com>

Thanks!

[Javier Martinez Canillas]

Geert Uytterhoeven <ge...@linux-m68k.org> writes:

> Hi Javier,
>
> On Tue, May 2, 2023 at 1:19 PM Javier Martinez Canillas
> <jav...@redhat.com> wrote:
>> I've only been Cc'ed in patch #2.
>
> Not really, you're in the To-header on the full series?
> https://lore.kernel.org/all/cover.1683022164....@glider.be
>

Strange... I only got patch #2, neither patch #1 nor the cover letter.

For patch #1 as well:

Reviewed-by: Javier Martinez Canillas <jav...@redhat.com>

Sorry for missing that bug and thanks a lot for fixing it!

[Geert Uytterhoeven]

Hi Javier,

On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven
<geert+...@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

[Javier Martinez Canillas]

Geert Uytterhoeven <ge...@linux-m68k.org> writes:

Hello Geert,

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!

[Dmitry Torokhov]

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.

--
Dmitry

[Geert Uytterhoeven]

Hi Dmitry,

Thanks, that did the trick! 3/3 tests pass.

[Javier Martinez Canillas]

Oh, silly me. Are you going to post that as a proper patch ?

[David Gow]

On Tue, 2 May 2023 at 18:17, Geert Uytterhoeven <geert+...@glider.be> wrote:
>
> While KUnit tests that cannot be built as a loadable module must depend
> on "KUNIT=y", this is not true for modular tests, where it adds an
> unnecessary limitation.
>
> Fix this by relaxing the dependency to "KUNIT".
>
> Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
> Signed-off-by: Geert Uytterhoeven <geert+...@glider.be>
> ---

Works here, thanks!

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David