* [PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO
@ 2017-10-03 13:51 John Einar Reitan
2017-10-04 10:43 ` Chris Wilson
2017-10-09 13:49 ` [PATCH v2] " John Einar Reitan
0 siblings, 2 replies; 7+ messages in thread
From: John Einar Reitan @ 2017-10-03 13:51 UTC (permalink / raw)
To: dri-devel
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@arm.com>
Signed-off-by: John Einar Reitan <john.reitan@arm.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: dri-devel@lists.freedesktop.org
---
drivers/dma-buf/sync_file.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 66fb40d0ebdb..ebf5764b868c 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)
{
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,
@@ -408,7 +410,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
struct sync_fence_info *fence_info = NULL;
struct dma_fence **fences;
__u32 size;
- int num_fences, ret, i;
+ int num_fences, ret, i, fences_status = 1;
if (copy_from_user(&info, (void __user *)arg, sizeof(info)))
return -EFAULT;
@@ -424,8 +426,10 @@ 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) {
+ fences_status = dma_fence_is_signaled(sync_file->fence);
goto no_fences;
+ }
if (info.num_fences < num_fences)
return -EINVAL;
@@ -435,8 +439,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]);
+ fences_status = fences_status <= 0 ? fences_status : status;
+ }
if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info,
size)) {
@@ -446,7 +452,7 @@ 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.status = fences_status;
info.num_fences = num_fences;
if (copy_to_user((void __user *)arg, &info, sizeof(info)))
--
2.13.6
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO
2017-10-03 13:51 [PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO John Einar Reitan
@ 2017-10-04 10:43 ` Chris Wilson
2017-10-04 12:15 ` John Einar Reitan
2017-10-09 13:49 ` [PATCH v2] " John Einar Reitan
1 sibling, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2017-10-04 10:43 UTC (permalink / raw)
To: John Einar Reitan, dri-devel
Quoting John Einar Reitan (2017-10-03 14:51:00)
> 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@arm.com>
> Signed-off-by: John Einar Reitan <john.reitan@arm.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: dri-devel@lists.freedesktop.org
> ---
> drivers/dma-buf/sync_file.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 66fb40d0ebdb..ebf5764b868c 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)
> {
> 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;
In hindsight, having both the per-fence and global variable be called
info was a mistake. Certainly made this diff a little harder to parse
than required!
> }
>
> static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> @@ -408,7 +410,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> struct sync_fence_info *fence_info = NULL;
> struct dma_fence **fences;
> __u32 size;
> - int num_fences, ret, i;
> + int num_fences, ret, i, fences_status = 1;
>
> if (copy_from_user(&info, (void __user *)arg, sizeof(info)))
> return -EFAULT;
> @@ -424,8 +426,10 @@ 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) {
> + fences_status = dma_fence_is_signaled(sync_file->fence);
Personally I would have set info.status directly rather than add a new
fences_status. But that's immaterial to the bug fix,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO
2017-10-04 10:43 ` Chris Wilson
@ 2017-10-04 12:15 ` John Einar Reitan
0 siblings, 0 replies; 7+ messages in thread
From: John Einar Reitan @ 2017-10-04 12:15 UTC (permalink / raw)
To: Chris Wilson; +Cc: dri-devel
On Wed, Oct 04, 2017 at 11:43:23AM +0100, Chris Wilson wrote:
> In hindsight, having both the per-fence and global variable be called
> info was a mistake. Certainly made this diff a little harder to parse
> than required!
I agree, I had to re-read the code a few times to find & fix the bug.
> Personally I would have set info.status directly rather than add a new
> fences_status. But that's immaterial to the bug fix,
I can clean that up if there is a need for a v2.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks!
John
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO
2017-10-03 13:51 [PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO John Einar Reitan
2017-10-04 10:43 ` Chris Wilson
@ 2017-10-09 13:49 ` John Einar Reitan
2017-10-09 13:53 ` Chris Wilson
2017-10-10 6:09 ` Chunming Zhou
1 sibling, 2 replies; 7+ messages in thread
From: John Einar Reitan @ 2017-10-09 13:49 UTC (permalink / raw)
To: sumit.semwal; +Cc: dri-devel
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@arm.com>
Signed-off-by: John Einar Reitan <john.reitan@arm.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: dri-devel@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)
{
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)))
--
2.13.6
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO
2017-10-09 13:49 ` [PATCH v2] " John Einar Reitan
@ 2017-10-09 13:53 ` Chris Wilson
2017-10-09 17:28 ` Gustavo Padovan
2017-10-10 6:09 ` Chunming Zhou
1 sibling, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2017-10-09 13:53 UTC (permalink / raw)
To: John Einar Reitan, sumit.semwal; +Cc: dri-devel
Quoting John Einar Reitan (2017-10-09 14:49:36)
> 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@arm.com>
> Signed-off-by: John Einar Reitan <john.reitan@arm.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO
2017-10-09 13:53 ` Chris Wilson
@ 2017-10-09 17:28 ` Gustavo Padovan
0 siblings, 0 replies; 7+ messages in thread
From: Gustavo Padovan @ 2017-10-09 17:28 UTC (permalink / raw)
To: Chris Wilson; +Cc: John Einar Reitan, dri-devel
2017-10-09 Chris Wilson <chris@chris-wilson.co.uk>:
> Quoting John Einar Reitan (2017-10-09 14:49:36)
> > 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@arm.com>
> > Signed-off-by: John Einar Reitan <john.reitan@arm.com>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Pushed to drm-misc-fixes.
Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO
2017-10-09 13:49 ` [PATCH v2] " John Einar Reitan
2017-10-09 13:53 ` Chris Wilson
@ 2017-10-10 6:09 ` Chunming Zhou
1 sibling, 0 replies; 7+ messages in thread
From: Chunming Zhou @ 2017-10-10 6:09 UTC (permalink / raw)
To: John Einar Reitan, sumit.semwal; +Cc: dri-devel
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@arm.com>
> Signed-off-by: John Einar Reitan <john.reitan@arm.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: dri-devel@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)))
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-10-10 6:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-03 13:51 [PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO John Einar Reitan
2017-10-04 10:43 ` Chris Wilson
2017-10-04 12:15 ` John Einar Reitan
2017-10-09 13:49 ` [PATCH v2] " John Einar Reitan
2017-10-09 13:53 ` Chris Wilson
2017-10-09 17:28 ` Gustavo Padovan
2017-10-10 6:09 ` Chunming Zhou
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.