Date:   Wed, 5 Apr 2023 13:03:31 -0600
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Ivan Orlov <ivan.orlov0322@...il.com>
Cc:     linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        himadrispandya@...il.com,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] selftests: media_tests: Add new subtest to
 video_device_test

On 4/1/23 03:22, Ivan Orlov wrote:
> Add new subtest to video_device_test to cover the VIDIOC_G_PRIORITY
> and VIDIOC_S_PRIORITY ioctl calls. This test tries to set the priority
> associated with the file descriptior via ioctl VIDIOC_S_PRIORITY
> command from V4L2 API. After that, the test tries to get the new
> priority via VIDIOC_G_PRIORITY ioctl command and compares the result
> with the v4l2_priority it set before. At the end, the test restores the
> old priority.
> 
> This test will increase the code coverage for video_device_test, so
> I think it might be useful. Additionally, this patch will refactor the
> video_device_test a little bit, according to the new functionality.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@...il.com>
> ---
>   .../selftests/media_tests/video_device_test.c | 131 +++++++++++++-----
>   1 file changed, 93 insertions(+), 38 deletions(-)
> 
> diff --git a/tools/testing/selftests/media_tests/video_device_test.c b/tools/testing/selftests/media_tests/video_device_test.c
> index 0f6aef2e2593..5e6f65ad2ca3 100644
> --- a/tools/testing/selftests/media_tests/video_device_test.c
> +++ b/tools/testing/selftests/media_tests/video_device_test.c
> @@ -13,18 +13,9 @@
>    * in the Kselftest run. This test should be run when hardware and driver
>    * that makes use of V4L2 API is present.
>    *
> - * This test opens user specified Video Device and calls video ioctls in a
> - * loop once every 10 seconds.
> - *
>    * Usage:
>    *	sudo ./video_device_test -d /dev/videoX
> - *
> - *	While test is running, remove the device or unbind the driver and
> - *	ensure there are no use after free errors and other Oops in the
> - *	dmesg.
> - *	When possible, enable KaSan kernel config option for use-after-free
> - *	error detection.
> -*/
> + */

Leave this text here so it is easy to spot. There is no need to move this
down.

>   
>   #include <stdio.h>
>   #include <unistd.h>
> @@ -37,45 +28,67 @@
>   #include <time.h>
>   #include <linux/videodev2.h>
>   
> -int main(int argc, char **argv)
> +#define PRIORITY_MAX 4
> +
> +int priority_test(int fd)
>   {
> -	int opt;
> -	char video_dev[256];
> -	int count;
> -	struct v4l2_tuner vtuner;
> -	struct v4l2_capability vcap;
> +	/* This test will try to update the priority associated with a file descriptor */
> +
> +	enum v4l2_priority old_priority, new_priority, priority_to_compare;
>   	int ret;
> -	int fd;
> +	int result = 0;
>   
> -	if (argc < 2) {
> -		printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
> -		exit(-1);
> +	ret = ioctl(fd, VIDIOC_G_PRIORITY, &old_priority);
> +	if (ret < 0) {
> +		printf("Failed to get priority: %s\n", strerror(errno));
> +		return -1;
> +	}
> +	new_priority = (old_priority + 1) % PRIORITY_MAX;
> +	ret = ioctl(fd, VIDIOC_S_PRIORITY, &new_priority);
> +	if (ret < 0) {
> +		printf("Failed to set priority: %s\n", strerror(errno));
> +		return -1;
> +	}
> +	ret = ioctl(fd, VIDIOC_G_PRIORITY, &priority_to_compare);
> +	if (ret < 0) {
> +		printf("Failed to get new priority: %s\n", strerror(errno));
> +		result = -1;
> +		goto cleanup;
> +	}
> +	if (priority_to_compare != new_priority) {
> +		printf("Priority wasn't set - test failed\n");
> +		result = -1;
>   	}
>   
> -	/* Process arguments */
> -	while ((opt = getopt(argc, argv, "d:")) != -1) {
> -		switch (opt) {
> -		case 'd':
> -			strncpy(video_dev, optarg, sizeof(video_dev) - 1);
> -			video_dev[sizeof(video_dev)-1] = '\0';
> -			break;
> -		default:
> -			printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
> -			exit(-1);
> -		}
> +cleanup:
> +	ret = ioctl(fd, VIDIOC_S_PRIORITY, &old_priority);
> +	if (ret < 0) {
> +		printf("Failed to restore priority: %s\n", strerror(errno));
> +		return -1;
>   	}
> +	return result;
> +}
> +
> +int loop_test(int fd)
> +{
> +	/*
> +	 * This test opens user specified Video Device and calls video ioctls in a
> +	 * loop once every 10 seconds.
> +	 * While test is running, remove the device or unbind the driver and
> +	 * ensure there are no use after free errors and other Oops in the
> +	 * dmesg.
> +	 * When possible, enable KaSan kernel config option for use-after-free
> +	 * error detection.
> +	 */
> +	int count;
> +	struct v4l2_tuner vtuner;
> +	struct v4l2_capability vcap;
> +	int ret;
>   
>   	/* Generate random number of interations */
>   	srand((unsigned int) time(NULL));
>   	count = rand();
>   
> -	/* Open Video device and keep it open */
> -	fd = open(video_dev, O_RDWR);
> -	if (fd == -1) {
> -		printf("Video Device open errno %s\n", strerror(errno));
> -		exit(-1);
> -	}
> -
>   	printf("\nNote:\n"
>   	       "While test is running, remove the device or unbind\n"
>   	       "driver and ensure there are no use after free errors\n"
> @@ -98,4 +111,46 @@ int main(int argc, char **argv)
>   		sleep(10);
>   		count--;
>   	}
> +	return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int opt;
> +	char video_dev[256];
> +	int fd;
> +	int test_result;
> +
> +	if (argc < 2) {
> +		printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
> +		exit(-1);
> +	}
> +
> +	/* Process arguments */
> +	while ((opt = getopt(argc, argv, "d:")) != -1) {
> +		switch (opt) {
> +		case 'd':
> +			strncpy(video_dev, optarg, sizeof(video_dev) - 1);
> +			video_dev[sizeof(video_dev)-1] = '\0';
> +			break;
> +		default:
> +			printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
> +			exit(-1);
> +		}
> +	}
> +
> +	/* Open Video device and keep it open */
> +	fd = open(video_dev, O_RDWR);
> +	if (fd == -1) {
> +		printf("Video Device open errno %s\n", strerror(errno));
> +		exit(-1);
> +	}
> +
> +	test_result = priority_test(fd);
> +	if (!test_result)
> +		printf("Priority test - PASSED\n");
> +	else
> +		printf("Priority test - FAILED\n");
> +
> +	loop_test(fd);
>   }

Otherwise looks good to me.

thanks,
-- Shuah

Powered by blists - more mailing lists