Re: [PATCH 2/2] drm/xe/oa: Fix locking for stream->pollin

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jan 21, 2025 at 10:03:54AM -0800, Ashutosh Dixit wrote:
> Previously locking was not implemented for stream->pollin. Now
> stream->pollin should be accessed under stream->oa_buffer.ptr_lock.

This commit message fails to explain why. Why it was not needed
before and why it is needed now?

Also, please make sure that the lock really makes sense and
we are not just increasing the scope of a locking that was
designed for something else... and that the locking guidelines
are followed... [1]

[1] - https://blog.ffwll.ch/2022/07/locking-engineering.html



> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_oa.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index fa873f3d0a9d1..9de62ce4b9e42 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -530,6 +530,7 @@ static ssize_t xe_oa_read(struct file *file, char __user *buf,
>  			  size_t count, loff_t *ppos)
>  {
>  	struct xe_oa_stream *stream = file->private_data;
> +	unsigned long flags;
>  	size_t offset = 0;
>  	int ret;
>  
> @@ -562,8 +563,10 @@ static ssize_t xe_oa_read(struct file *file, char __user *buf,
>  	 * Also in case of -EIO, we have already waited for data before returning
>  	 * -EIO, so need to wait again
>  	 */
> +	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
>  	if (ret != -ENOSPC && ret != -EIO)
>  		stream->pollin = false;
> +	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
>  
>  	/* Possible values for ret are 0, -EFAULT, -ENOSPC, -EIO, -EINVAL, ... */
>  	return offset ?: (ret ?: -EAGAIN);
> @@ -573,6 +576,7 @@ static __poll_t xe_oa_poll_locked(struct xe_oa_stream *stream,
>  				  struct file *file, poll_table *wait)
>  {
>  	__poll_t events = 0;
> +	unsigned long flags;
>  
>  	poll_wait(file, &stream->poll_wq, wait);
>  
> @@ -582,8 +586,10 @@ static __poll_t xe_oa_poll_locked(struct xe_oa_stream *stream,
>  	 * in use. We rely on hrtimer xe_oa_poll_check_timer_cb to notify us when there
>  	 * are samples to read
>  	 */
> +	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
>  	if (stream->pollin)
>  		events |= EPOLLIN;
> +	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
>  
>  	return events;
>  }
> -- 
> 2.47.1
>