Re: [PATCH i-g-t v1] /tests/intel/xe_eudebug_online: Remove vm_open_trigger on debugger reattachment.

["Grzegorzek, Dominik" <dominik.grzegorzek@intel.com>]

Hi Jan!

On Thu, 2024-10-24 at 11:29 +0000, Jan Sokolowski wrote:
> On debugger detach-and-reattach, discovery worker will replay events
> that have occurred, which leads to a vm event being sent and
> vm_open_trigger being re-run, which would lead to us trying
> to open a removed vm.
> 
> On debugger reattach trigger, remove vm open trigger from list of
> triggers.
> 
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> ---
>  lib/xe/xe_eudebug.c             | 25 +++++++++++++++++++++++++
>  lib/xe/xe_eudebug.h             |  2 ++
>  tests/intel/xe_eudebug_online.c |  6 ++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/lib/xe/xe_eudebug.c b/lib/xe/xe_eudebug.c
> index 36763a184..62f7e7361 100644
> --- a/lib/xe/xe_eudebug.c
> +++ b/lib/xe/xe_eudebug.c
> @@ -1226,6 +1226,31 @@ void xe_eudebug_debugger_add_trigger(struct xe_eudebug_debugger *d,
>  	igt_debug("added trigger %p\n", t);
>  }
>  
> +/**
> + * xe_eudebug_debugger_remove_trigger:
> + * @d: pointer to the debugger
> + * @type: the type of the event which activates the trigger.
> + * @fn: function to be removed when event of @type was read by the debugger.
> + *
> + * Removes function @fn from the list of triggers activated when event of
> + * @type has been read by worker.
> + */
> +void xe_eudebug_debugger_remove_trigger(struct xe_eudebug_debugger *d,
> +					int type, xe_eudebug_trigger_fn fn)
> +{
> +	struct event_trigger *t;
> +
> +	igt_list_for_each_entry(t, &d->triggers, link) {
igt_list_for_each_entry_safe 
> +		if (type == t->type && fn == t->fn) {
> +			igt_list_del(&t->link);
> +			break;
> +		}
> +	}
> +
> +	igt_debug("removed trigger %p\n", t);
> +	free(t);
What if it has not been found? You would then free the head. 
Also igt_warn if it was not found would be welcome.
> +}
> +
>  /**
>   * xe_eudebug_debugger_start_worker:
>   * @d: pointer to the debugger
> diff --git a/lib/xe/xe_eudebug.h b/lib/xe/xe_eudebug.h
> index 6e4666333..9aadbe847 100644
> --- a/lib/xe/xe_eudebug.h
> +++ b/lib/xe/xe_eudebug.h
> @@ -150,6 +150,8 @@ void xe_eudebug_debugger_detach(struct xe_eudebug_debugger *d);
>  void xe_eudebug_debugger_set_data(struct xe_eudebug_debugger *c, void *ptr);
>  void xe_eudebug_debugger_add_trigger(struct xe_eudebug_debugger *d, int type,
>  				     xe_eudebug_trigger_fn fn);
> +void xe_eudebug_debugger_remove_trigger(struct xe_eudebug_debugger *d, int type,
> +				     xe_eudebug_trigger_fn fn);
>  void xe_eudebug_debugger_signal_stage(struct xe_eudebug_debugger *d, uint64_t stage);
>  void xe_eudebug_debugger_wait_stage(struct xe_eudebug_session *s, uint64_t stage);
>  
> diff --git a/tests/intel/xe_eudebug_online.c b/tests/intel/xe_eudebug_online.c
> index 01ccf45eb..0ef0d8093 100644
> --- a/tests/intel/xe_eudebug_online.c
> +++ b/tests/intel/xe_eudebug_online.c
> @@ -1785,6 +1785,12 @@ static void eu_attention_debugger_detach_trigger(struct xe_eudebug_debugger *d,
>  	d->fd = ret;
>  	d->target_pid = c_pid;
>  
> +	/* Discovery worker will replay events that have occurred, which leads to
> +	 * a vm event being sent and vm_open_trigger being re-run, which would lead
> +	 * to us trying to open a removed vm. Thus, remove this trigger from list.
> +	 */
> +	xe_eudebug_debugger_remove_trigger(d, DRM_XE_EUDEBUG_EVENT_VM, vm_open_trigger);
> +
Sounds fine for me. Let split this change into two patches, one for lib other for the test. 

Regards, 
Dominik
>  	/* Let the discovery worker discover resources */
>  	sleep(2);
>