[PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[Chunming Zhou]

On 2017年10月09日 21:49, John Einar Reitan wrote:
> sync_file_ioctl_fence_info has a race between filling the status
> of the underlying fences and the overall status of the sync_file.
> If fence transitions in the time frame between its sync_fill_fence_info
> and the later dma_fence_is_signaled for the sync_file, the returned
> information is inconsistent showing non-signaled underlying fences but
> an overall signaled state.
>
> This patch changes sync_file_ioctl_fence_info to track what has been
> encoded and using that as the overall sync_file status.
>
> Tested-by: Vamsidhar Reddy Gaddam <vamsidhar.gaddam at arm.com>
> Signed-off-by: John Einar Reitan <john.reitan at arm.com>
> Cc: Sumit Semwal <sumit.semwal at linaro.org>
> Cc: Gustavo Padovan <gustavo at padovan.org>
> Cc: dri-devel at lists.freedesktop.org
> ---
>
> Changes since v1 (thanks Chris Wilson):
> - Replaced an unneeded local variable by writing directly to the struct
>
>   drivers/dma-buf/sync_file.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 66fb40d0ebdb..03830634e141 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -383,7 +383,7 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
>   	return err;
>   }
>   
> -static void sync_fill_fence_info(struct dma_fence *fence,
> +static int sync_fill_fence_info(struct dma_fence *fence,
>   				 struct sync_fence_info *info)
Out of curious, why name to sync_fill_xxx not sync_file_xxx?

Regards,
David Zhou
>   {
>   	strlcpy(info->obj_name, fence->ops->get_timeline_name(fence),
> @@ -399,6 +399,8 @@ static void sync_fill_fence_info(struct dma_fence *fence,
>   		test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags) ?
>   		ktime_to_ns(fence->timestamp) :
>   		ktime_set(0, 0);
> +
> +	return info->status;
>   }
>   
>   static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> @@ -424,8 +426,12 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>   	 * sync_fence_info and return the actual number of fences on
>   	 * info->num_fences.
>   	 */
> -	if (!info.num_fences)
> +	if (!info.num_fences) {
> +		info.status = dma_fence_is_signaled(sync_file->fence);
>   		goto no_fences;
> +	} else {
> +		info.status = 1;
> +	}
>   
>   	if (info.num_fences < num_fences)
>   		return -EINVAL;
> @@ -435,8 +441,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>   	if (!fence_info)
>   		return -ENOMEM;
>   
> -	for (i = 0; i < num_fences; i++)
> -		sync_fill_fence_info(fences[i], &fence_info[i]);
> +	for (i = 0; i < num_fences; i++) {
> +		int status = sync_fill_fence_info(fences[i], &fence_info[i]);
> +		info.status = info.status <= 0 ? info.status : status;
> +	}
>   
>   	if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info,
>   			 size)) {
> @@ -446,7 +454,6 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>   
>   no_fences:
>   	sync_file_get_name(sync_file, info.name, sizeof(info.name));
> -	info.status = dma_fence_is_signaled(sync_file->fence);
>   	info.num_fences = num_fences;
>   
>   	if (copy_to_user((void __user *)arg, &info, sizeof(info)))