[PATCH v2] Input: tests: add test to cover all input_grab_device() function

[PATCH v2] Input: tests: add test to cover all input_grab_device() function

[Dana Elfassy]

Currently input_grab_device() isn't covered by any tests
Thus, adding a test to cover the cases:
1. The device is grabbed successfully
2. Trying to grab a device that is already grabbed by another input
   handle

Signed-off-by: Dana Elfassy <dangel101@gmail.com>
Tested-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Use input_put_device() to decrement the refcount increased by get().
- Remove unnecessary struct input_handle test_handle variable.

 drivers/input/tests/input_test.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c
index 25bbf51b5c87..cea0167a74d2 100644
--- a/drivers/input/tests/input_test.c
+++ b/drivers/input/tests/input_test.c
@@ -124,10 +124,33 @@ static void input_test_match_device_id(struct kunit *test)
 	KUNIT_ASSERT_FALSE(test, input_match_device_id(input_dev, &id));
 }
 
+static void input_test_grab(struct kunit *test)
+{
+	struct input_dev *input_dev = test->priv;
+	struct input_handler handler;
+	struct input_handle handle;
+	struct input_device_id id;
+	int res;
+
+	handler.name = "handler";
+	handler.id_table = &id;
+
+	handle.dev = input_get_device(input_dev);
+	handle.name = dev_name(&input_dev->dev);
+	handle.handler = &handler;
+	res = input_grab_device(&handle);
+	KUNIT_ASSERT_TRUE(test, input_grab_device(&handle));
+
+	res = input_grab_device(&handle);
+	KUNIT_ASSERT_EQ(test, res, -EBUSY);
+	input_put_device(input_dev);
+}
+
 static struct kunit_case input_tests[] = {
 	KUNIT_CASE(input_test_polling),
 	KUNIT_CASE(input_test_timestamp),
 	KUNIT_CASE(input_test_match_device_id),
+	KUNIT_CASE(input_test_grab),
 	{ /* sentinel */ }
 };
 
-- 
2.40.1

Re: [PATCH v2] Input: tests: add test to cover all input_grab_device() function

[Dmitry Torokhov]

Hi Dana,

On Wed, May 17, 2023 at 06:31:45PM +0300, Dana Elfassy wrote:
> Currently input_grab_device() isn't covered by any tests
> Thus, adding a test to cover the cases:
> 1. The device is grabbed successfully
> 2. Trying to grab a device that is already grabbed by another input
>    handle
> 
> Signed-off-by: Dana Elfassy <dangel101@gmail.com>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v2:
> - Use input_put_device() to decrement the refcount increased by get().
> - Remove unnecessary struct input_handle test_handle variable.

So this tests something different than what patch description states.
You are testing that there is no "recursive" grabbing happening (an API
could be designed to allow the same handle grab device several times).
This is a good and useful test, but you do want to also use 2nd separate
handle to see that it gets -EBUSY as well. And ideally we should have
another test verifying that the 2nd handle can successfully grab the
device once the first handle releases it.

Thanks.

-- 
Dmitry

Re: [PATCH v2] Input: tests: add test to cover all input_grab_device() function

[Javier Martinez Canillas]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

Hello Dmitry,

> Hi Dana,
>
> On Wed, May 17, 2023 at 06:31:45PM +0300, Dana Elfassy wrote:
>> Currently input_grab_device() isn't covered by any tests
>> Thus, adding a test to cover the cases:
>> 1. The device is grabbed successfully
>> 2. Trying to grab a device that is already grabbed by another input
>>    handle
>> 
>> Signed-off-by: Dana Elfassy <dangel101@gmail.com>
>> Tested-by: Javier Martinez Canillas <javierm@redhat.com>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>> 
>> Changes in v2:
>> - Use input_put_device() to decrement the refcount increased by get().
>> - Remove unnecessary struct input_handle test_handle variable.
>
> So this tests something different than what patch description states.
> You are testing that there is no "recursive" grabbing happening (an API
> could be designed to allow the same handle grab device several times).
> This is a good and useful test, but you do want to also use 2nd separate
> handle to see that it gets -EBUSY as well. And ideally we should have

That was my fault since v1 had two different handles but since it wasn't
releasing it, didn't add any value really so I asked Dana to just drop it.

> another test verifying that the 2nd handle can successfully grab the
> device once the first handle releases it.
>

That's the correct approach indeed and would make the test more useful.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] Input: tests: add test to cover all input_grab_device() function

[Javier Martinez Canillas]

Dana Elfassy <delfassy@redhat.com> writes:

Hello Dana,

> Currently input_grab_device() isn't covered by any tests
> Thus, adding a test to cover the cases:
> 1. The device is grabbed successfully
> 2. Trying to grab a device that is already grabbed by another input
>    handle
>
> Signed-off-by: Dana Elfassy <dangel101@gmail.com>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---

[...]

> +
> +	handle.dev = input_get_device(input_dev);
> +	handle.name = dev_name(&input_dev->dev);
> +	handle.handler = &handler;
> +	res = input_grab_device(&handle);

Another thing I noticed is that your test will try to grab the same
input_handle twice. So you need to remove the line above I believe.

> +	KUNIT_ASSERT_TRUE(test, input_grab_device(&handle));
> +

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat