Re: [PATCH i-g-t v2 1/1] tests/intel/xe_exec_sip_eudebug: skip eudebug tests on VF

["Manszewski, Christoph" <christoph.manszewski@intel.com>]

Hi Jan,

On 3.04.2025 13:53, Jan Sokolowski wrote:
> Tests using eudebug are not compatible with VF functionality.
> A way to check for that scenario is to check eudebug toggle
> capability.
> 
> Skip a test that tries to use eudebug toggle if it doesn't
> exist.

I think that the commit message is not quite appropriate. The title 
should be something like:
'lib/xe/xe_eudebug: Skip when disabling eudebug with no eudebug support.'

And the description then should somehow capture what we have currently 
and what this change brings.

> 
> v2: Changed the idea from disabling a single test to disabling
> all possible tests.

Personally I am not a fan of having versions in a commit message. IMO it 
should describe the commit itself. Cover letters are a better place to 
track something like this for the sake of discussion. Ofc this is simple 
change so just skipping the version history and cover letter should be ok.

> 
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
> Reviewed-by: Lukasz Laguna <lukasz.laguna@intel.com>
> Cc: Lukasz Laguna <lukasz.laguna@intel.com>
> Reviewed-by: Christoph Manszewski <christoph.manszewski@intel.com>

Please don't include r-b without explicit consent.

Regards,
Christoph

> Cc: Christoph Manszewski <christoph.manszewski@intel.com>
> ---
>   lib/xe/xe_eudebug.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/lib/xe/xe_eudebug.c b/lib/xe/xe_eudebug.c
> index ec946058b..667c31c6e 100644
> --- a/lib/xe/xe_eudebug.c
> +++ b/lib/xe/xe_eudebug.c
> @@ -1794,10 +1794,7 @@ bool xe_eudebug_enable(int fd, bool enable)
>   	bool old = false;
>   	int ret = __xe_eudebug_enable_getset(fd, &old, &enable);
>   
> -	if (ret) {
> -		igt_skip_on(enable);
> -		old = false;
> -	}
> +	igt_skip_on(ret);
>   
>   	return old;
>   }