Re: [PATCH 3/3] drm/xe/xe_oa: Add a lag to the reports that is exported to user

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Readers can block indefinitely on disabled streams because `stream->enabled` is set to false too late, failing to flush the final OA reports.
- [Medium] Data race on `stream->enabled` between `xe_oa_buffer_check_unlocked` and `xe_oa_disable_locked`/`xe_oa_enable_locked`.
--

--- Patch [3]: [PATCH 3/3] drm/xe/xe_oa: Add a lag to the reports that is exported to user ---
commit 4ec3e1c770bffdf39a86c1db169059b4bc510058
Author: Umesh Nerlige Ramappa <[email protected]>

drm/xe/xe_oa: Add a lag to the reports that is exported to user

This commit delays the visibility of OA reports to user space by holding
back the tail by 32 reports to ensure they have landed in memory. It also
attempts to flush all remaining reports when the stream is disabled.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c

[ ... ]

> @@ -234,17 +234,23 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
>  	/* Subtract partial amount off the hw_tail */
>  	hw_tail = xe_oa_circ_diff(stream, hw_tail, partial_report_size);
>  
> -	stream->oa_buffer.tail = hw_tail;
> +#define LAG_REPORTS 32
> +	lag = xe_oa_circ_diff(stream, hw_tail, stream->oa_buffer.tail);
> +	if (!stream->enabled)

[Severity: Medium]
Is it safe to read stream->enabled here? It appears to be read while holding
the stream->oa_buffer.ptr_lock spinlock, but modified in
xe_oa_disable_locked() and xe_oa_enable_locked() while holding the
stream->stream_lock mutex. Could this cause data races?

[Severity: High]
Will readers waiting on a disabled stream block indefinitely?

When a thread calls DRM_XE_OBSERVATION_IOCTL_DISABLE, xe_oa_disable_locked()
is executed:

xe_oa_disable_locked() {
    xe_oa_stream_disable(stream);
    ...
    stream->enabled = false;
}

xe_oa_stream_disable() cancels the poll timer and calls
xe_oa_buffer_check_unlocked() to flush the final reports. However, since
stream->enabled is still true when xe_oa_buffer_check_unlocked() is called,
this bypass check will evaluate to false.

If the tail is held back by the lag, pollin evaluates to false, and
wake_up() is never called, could this cause userspace threads blocking in
read() or poll() to hang indefinitely with stranded reports?

> +		stream->oa_buffer.tail = hw_tail;
> +	else if (lag > LAG_REPORTS * report_size)
> +		stream->oa_buffer.tail = xe_oa_circ_diff(stream, hw_tail,
> +							 LAG_REPORTS * report_size);
>  
>  	available = xe_oa_circ_diff(stream, stream->oa_buffer.tail, stream->oa_buffer.head);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3