[i-g-t,v3] tests/core_setmaster: simplify device handling

[Kamil Konieczny]

Hi Bommu,
On 2024-10-04 at 22:27:45 +0530, Bommu Krishnaiah wrote:
> Refactored test setup to dynamically determine the correct drm card
> for testing. Instead of changing permissions for all cards,
> change only one that will be tested.
> 
> Problem Description:
> The test fails after running the `core_hotunplug at hot*` subtest, where
> Card0 is populated as card1, This leads to a failure in the subsequent
> `master-drop-set-user` test.
> 
> Command sequence for reproducing the issue:
> - Check available cards: `ls /dev/dri/`
>         - Output: by-path  card0  renderD128
> - Run the test: `# ./core_hotunplug --r hotrebind-lateclose`
> - Check again: ‘ls /dev/dri/’
>         - Output after core_hotunplug : by-path  card1  renderD129
> - Run the test: `# ./core_setmaster --r master-drop-set-user`
>         - Output: gta at core_setmaster@master-drop-set-user failure
> 
> This failures on both XE and i915 for above sequence.
> 
> v2: corrected the e-mail address
> 

LGTM,
Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>


> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
> Cc: Emil Velikov <emil.l.velikov at gmail.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> Cc: Kamil Konieczny <kamil.konieczny at intel.com>
> ---
>  tests/core_setmaster.c | 82 ++++++++++++++++++++++++------------------
>  1 file changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c
> index 9c2083f66..fe9993515 100644
> --- a/tests/core_setmaster.c
> +++ b/tests/core_setmaster.c
> @@ -107,40 +107,31 @@ static void check_drop_set(void)
>  	drm_close_driver(master);
>  }
>  
> -static unsigned tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
> +static void tweak_perm(uint8_t *saved_perm, char *path, bool save)
>  {
> -	char path[256];
>  	struct stat st;
> -	unsigned i;
> -
> -	for (i = 0; i < max_perm; i++) {
> -		snprintf(path, sizeof(path), "/dev/dri/card%u", i);
> -
> -		/* Existing userspace assumes there's no gaps, do the same. */
> -		if (stat(path, &st) != 0)
> -			break;
> -
> -		if (save) {
> -			/* Save and toggle */
> -			saved_perm[i] = st.st_mode & (S_IROTH | S_IWOTH);
> -			st.st_mode |= S_IROTH | S_IWOTH;
> -		} else {
> -			/* Clear and restore */
> -			st.st_mode &= ~(S_IROTH | S_IWOTH);
> -			st.st_mode |= saved_perm[i];
> -		}
> -
> -		/* There's only one way for chmod to fail - race vs rmmod.
> -		 * In that case, do _not_ error/skip, since:
> -		 * - we need to restore the [correct] permissions
> -		 * - __drm_open_driver() can open another device, aka the
> -		 * failure may be irrelevant.
> -		 */
> -		chmod(path, st.st_mode);
> +	int ret;
> +
> +	if (path[0] == 0)
> +		return;
> +
> +	ret = stat(path, &st);
> +	igt_assert_f(!ret, "stat failed with %d path=%s\n", errno, path);
> +
> +	if (save) {
> +		/* Save and toggle */
> +		*saved_perm = st.st_mode & (S_IROTH | S_IWOTH);
> +		st.st_mode |= S_IROTH | S_IWOTH;
> +	} else {
> +		/* Clear and restore */
> +		st.st_mode &= ~(S_IROTH | S_IWOTH);
> +		st.st_mode |= *saved_perm;
>  	}
> -	return i;
> -}
>  
> +	/* There's only one way for chmod to fail - race vs rmmod. */
> +	ret = chmod(path, st.st_mode);
> +	igt_assert_f(!ret, "chmod failed with %d path=%s\n", errno, path);
> +}
>  
>  igt_main
>  {
> @@ -160,8 +151,8 @@ igt_main
>  
>  
>  	igt_subtest_group {
> -		uint8_t saved_perm[255];
> -		unsigned num;
> +		uint8_t saved_perm;
> +		char buf[255];
>  
>  		/* Upon dropping root we end up as random user, which
>  		 * a) is not in the video group, and
> @@ -176,8 +167,29 @@ igt_main
>  		 * restored on skip or failure.
>  		 */
>  		igt_fixture {
> -			num = tweak_perm(saved_perm, ARRAY_SIZE(saved_perm),
> -					 true);
> +			char path[255];
> +			int len;
> +			int fd;
> +
> +			memset(buf, 0, sizeof(buf));
> +			saved_perm = 0;
> +
> +			fd = __drm_open_driver(DRIVER_ANY);
> +			igt_assert_fd(fd);
> +
> +			snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
> +
> +			len = readlink(path, buf, sizeof(buf) - 1);
> +			igt_assert_f(len > 0, "readlink failed with %d path=%s\n", errno, path);
> +
> +			buf[len] = '\0';
> +			if (!(strstr(buf, "/dev/dri/card") == buf ||
> +					strstr(buf, "/dev/dri/renderD") == buf))
> +				igt_assert_f(0, "Not a card nor render, path=%s\n", buf);
> +
> +			igt_assert_eq(__drm_close_driver(fd), 0);
> +
> +			tweak_perm(&saved_perm, buf, true);
>  		}
>  
>  		igt_describe("Ensure first normal user can Set/DropMaster");
> @@ -191,7 +203,7 @@ igt_main
>  
>  		/* Restore the original permissions */
>  		igt_fixture {
> -			tweak_perm(saved_perm, num, false);
> +			tweak_perm(&saved_perm, buf, false);
>  		}
>  	}
>