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:

New issues:
- [High] The newly introduced 32-report lag reduces the maximum possible available reports, but `wait_num_reports` validation still allows values up to full buffer capacity, causing a permanent hang and `-EIO` on overflow.

Pre-existing issues:
- [High] Readers blocked in `read()` or `poll()` will hang indefinitely when the OA stream is disabled due to conditional wakeups and incomplete wait conditions.
--

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

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

This commit adds a 32-report lag to the OA stream reports exposed to the user.
It updates the buffer check logic to ensure the software tail stays behind the
hardware tail unless the stream is disabled, in which case all remaining reports
can be read.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 020b27c642268..a025e6c2753f2 100644
> --- 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)
>  	hw_tail -= gtt_offset;
>  
>  	/*
> -	 * The tail pointer increases in 64 byte (cacheline size), not in report_size
> +	 * The hw_tail pointer increases in 64 byte (cacheline size), not in report_size
>  	 * increments. Also report size may not be a power of 2. Compute potential
>  	 * partially landed report in OA buffer.
>  	 */
>  	partial_report_size = xe_oa_circ_diff(stream, hw_tail, stream->oa_buffer.tail);
>  	partial_report_size %= report_size;
>  
> -	/* Subtract partial amount off the tail */
> +	/* 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)
> +		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);

[Severity: High]
Does this logic effectively reduce the maximum number of available reports
in the buffer by 32?

If so, userspace could now specify a wait_num_reports in
xe_oa_stream_open_ioctl() that is impossible to satisfy. The ioctl validation
still allows values up to the full buffer capacity:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_stream_open_ioctl() {
    if (param.wait_num_reports > param.oa_buffer_size / f->size) {
        ...
        ret = -EINVAL;
    }
}

If userspace requests a wait amount greater than (capacity - 32),
the available amount will never reach wait_num_reports. The hardware
tail will wrap around and overwrite unread data, causing read() or poll()
to hang indefinitely until the overflow is processed and returns -EIO.

Should the validation limit in xe_oa_stream_open_ioctl() be updated to
subtract LAG_REPORTS?

>  
>  	available = xe_oa_circ_diff(stream, stream->oa_buffer.tail, stream->oa_buffer.head);
>  	stream->pollin = available >= stream->wait_num_reports * report_size;
[ ... ]
> @@ -1490,6 +1492,11 @@ static int xe_oa_disable_locked(struct xe_oa_stream *stream)
>  		ret = xe_oa_enable_preempt_timeslice(stream);
>  
>  	stream->enabled = false;
> +
> +	/* Update stream->oa_buffer.tail to allow any final reports to be read */
> +	if (xe_oa_buffer_check_unlocked(stream))
> +		wake_up(&stream->poll_wq);
> +

[Severity: High]
This is a pre-existing issue, but can readers blocked in read() or poll() hang
indefinitely when the OA stream is disabled here?

Because xe_oa_buffer_check_unlocked() returns true only when
available >= wait_num_reports, the wakeup is skipped entirely if there are
fewer pending reports than the wait threshold.

Furthermore, looking at xe_oa_wait_unlocked(), it doesn't check
!stream->enabled in its wait condition:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_wait_unlocked() {
    return wait_event_interruptible(stream->poll_wq,
                                    xe_oa_buffer_check_unlocked(stream));
}

Even if an unconditional wakeup was performed here, wouldn't the thread
re-evaluate the condition and go back to sleep forever since the hardware
is stopped and timers are canceled?

>  	return ret;
>  }
>

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