[PATCH 1/2] android: fix reference leak in sync_fence_create

[PATCH 1/2] android: fix reference leak in sync_fence_create

[Maarten Lankhorst]

According to the documentation sync_fence_create takes ownership of the point,
not a reference on the point.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Cc: Colin Cross <ccross@google.com>
---
 drivers/staging/android/sync.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index e7b2e0234196..69139ce7420d 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -199,7 +199,6 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
 	fence->num_fences = 1;
 	atomic_set(&fence->status, 1);
 
-	fence_get(&pt->base);
 	fence->cbs[0].sync_pt = &pt->base;
 	fence->cbs[0].fence = fence;
 	if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
-- 
2.0.4

[PATCH 2/2] android: add sync_fence_create_dma

[Maarten Lankhorst]

This allows users of dma fences to create a android fence.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/staging/android/sync.c | 24 ++++++++++++++++++++----
 drivers/staging/android/sync.h | 11 +++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index 69139ce7420d..c9331250ac26 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
 }
 
 /* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+static struct sync_fence *sync_fence_create_noref(const char *name, struct fence *pt)
 {
 	struct sync_fence *fence;
 
@@ -199,16 +199,32 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
 	fence->num_fences = 1;
 	atomic_set(&fence->status, 1);
 
-	fence->cbs[0].sync_pt = &pt->base;
+	fence->cbs[0].sync_pt = pt;
 	fence->cbs[0].fence = fence;
-	if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
-			       fence_check_cb_func))
+	if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func))
 		atomic_dec(&fence->status);
 
 	sync_fence_debug_add(fence);
 
 	return fence;
 }
+
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt)
+{
+	struct sync_fence *fence;
+
+	fence = sync_fence_create_noref(name, fence_get(pt));
+	if (!fence)
+		fence_put(pt);
+
+	return fence;
+}
+EXPORT_SYMBOL(sync_fence_create_dma);
+
+struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+{
+	return sync_fence_create_noref(name, &pt->base);
+}
 EXPORT_SYMBOL(sync_fence_create);
 
 struct sync_fence *sync_fence_fdget(int fd)
diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
index 66b0f431f63e..7b3bf560790c 100644
--- a/drivers/staging/android/sync.h
+++ b/drivers/staging/android/sync.h
@@ -254,6 +254,17 @@ void sync_pt_free(struct sync_pt *pt);
  */
 struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
 
+/**
+ * sync_fence_create_dma() - creates a sync fence from a dma fence
+ * @name:	name of fence to create
+ * @pt:		dma fence to add to the sync fence
+ *
+ * Creates a fence containg @pt.  Once this is called, the fence takes
+ * a reference on @pt, unlike sync_fence_create which doesn't add one.
+ */
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt);
+
+
 /*
  * API for sync_fence consumers
  */
-- 
2.0.4

Re: [PATCH 2/2] android: add sync_fence_create_dma

[Jesse Barnes]

On Thu, 14 Aug 2014 11:54:52 +0200
Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote:

> This allows users of dma fences to create a android fence.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/staging/android/sync.c | 24 ++++++++++++++++++++----
>  drivers/staging/android/sync.h | 11 +++++++++++
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index 69139ce7420d..c9331250ac26 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
>  }
>  
>  /* TODO: implement a create which takes more that one sync_pt */
> -struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
> +static struct sync_fence *sync_fence_create_noref(const char *name, struct fence *pt)
>  {
>  	struct sync_fence *fence;
>  
> @@ -199,16 +199,32 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
>  	fence->num_fences = 1;
>  	atomic_set(&fence->status, 1);
>  
> -	fence->cbs[0].sync_pt = &pt->base;
> +	fence->cbs[0].sync_pt = pt;
>  	fence->cbs[0].fence = fence;
> -	if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
> -			       fence_check_cb_func))
> +	if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func))
>  		atomic_dec(&fence->status);
>  
>  	sync_fence_debug_add(fence);
>  
>  	return fence;
>  }
> +
> +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt)
> +{
> +	struct sync_fence *fence;

I ran into the same naming trouble in my implementation; I think I
ended up with sfence for sync fence declarations.

> +
> +	fence = sync_fence_create_noref(name, fence_get(pt));
> +	if (!fence)
> +		fence_put(pt);
> +
> +	return fence;
> +}
> +EXPORT_SYMBOL(sync_fence_create_dma);
> +
> +struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
> +{
> +	return sync_fence_create_noref(name, &pt->base);
> +}
>  EXPORT_SYMBOL(sync_fence_create);
>  
>  struct sync_fence *sync_fence_fdget(int fd)
> diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
> index 66b0f431f63e..7b3bf560790c 100644
> --- a/drivers/staging/android/sync.h
> +++ b/drivers/staging/android/sync.h
> @@ -254,6 +254,17 @@ void sync_pt_free(struct sync_pt *pt);
>   */
>  struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
>  
> +/**
> + * sync_fence_create_dma() - creates a sync fence from a dma fence
> + * @name:	name of fence to create
> + * @pt:		dma fence to add to the sync fence
> + *
> + * Creates a fence containg @pt.  Once this is called, the fence takes
> + * a reference on @pt, unlike sync_fence_create which doesn't add one.
> + */
> +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt);
> +
> +
>  /*
>   * API for sync_fence consumers
>   */

Yeah, I've been using this, looks good.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] android: add sync_fence_create_dma

[Greg Kroah-Hartman]

On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote:
> This allows users of dma fences to create a android fence.

Who is going to use these functions?  I need an in-kernel user before I
can add new api calls.

thanks,

greg k-h

Re: [PATCH 2/2] android: add sync_fence_create_dma

[Jesse Barnes]

On Fri, 15 Aug 2014 14:46:56 +0800
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote:
> > This allows users of dma fences to create a android fence.
> 
> Who is going to use these functions?  I need an in-kernel user before I
> can add new api calls.

I have some code that uses them, but I need a userspace user to push my
stuff. :)

I'm working on the latter bit too in Mesa, and we have demand from
Android, so we should get some users shortly.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] android: fix reference leak in sync_fence_create

[Dan Carpenter]

On Thu, Aug 14, 2014 at 11:53:38AM +0200, Maarten Lankhorst wrote:
> According to the documentation sync_fence_create takes ownership of the point,
> not a reference on the point.
> 

What are the user visible effects of this bug?  I assume this is a real
bug but judging solely based on your patch description, it sounds like
you could just update the documentation instead of changing the code.

regards,
dan carpenter

Re: [PATCH 1/2] android: fix reference leak in sync_fence_create

[Maarten Lankhorst]

Hey,

Op 18-08-14 om 14:57 schreef Dan Carpenter:
> On Thu, Aug 14, 2014 at 11:53:38AM +0200, Maarten Lankhorst wrote:
>> According to the documentation sync_fence_create takes ownership of the point,
>> not a reference on the point.
>>
> What are the user visible effects of this bug?  I assume this is a real
> bug but judging solely based on your patch description, it sounds like
> you could just update the documentation instead of changing the code.
>
Small memory leak on every created android fence when you run out of tree android drivers.

But because it happens every frame (or possibly even more often) it's worth fixing.

~Maarten

Re: [PATCH 2/2] android: add sync_fence_create_dma

[Maarten Lankhorst]

Hey,

On 15-08-14 08:46, Greg Kroah-Hartman wrote:
> On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote:
>> This allows users of dma fences to create a android fence.
> 
> Who is going to use these functions?  I need an in-kernel user before I
> can add new api calls.

So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync.
Will you apply these patches?

~Maarten

Re: [PATCH 2/2] android: add sync_fence_create_dma

[Dan Carpenter]

On Thu, Aug 28, 2014 at 08:54:05AM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> On 15-08-14 08:46, Greg Kroah-Hartman wrote:
> > On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote:
> >> This allows users of dma fences to create a android fence.
> > 
> > Who is going to use these functions?  I need an in-kernel user before I
> > can add new api calls.
> 
> So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync.
> Will you apply these patches?

Can you resend the patches?  Fix the changelog of the first one to
mention that it is a bugfix.  Send a [patch 3/3] which uses the new
functions in [patch 2/3].

regards,
dan carpenter

Re: [PATCH 2/2] android: add sync_fence_create_dma

[Maarten Lankhorst]

Hey,

Op 28-08-14 om 13:57 schreef Dan Carpenter:
> On Thu, Aug 28, 2014 at 08:54:05AM +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>> On 15-08-14 08:46, Greg Kroah-Hartman wrote:
>>> On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote:
>>>> This allows users of dma fences to create a android fence.
>>> Who is going to use these functions?  I need an in-kernel user before I
>>> can add new api calls.
>> So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync.
>> Will you apply these patches?
> Can you resend the patches?  Fix the changelog of the first one to
> mention that it is a bugfix.  Send a [patch 3/3] which uses the new
> functions in [patch 2/3].
The second patch will have to be applied without an in-kernel user because it
will be used in the drm subsystem, by someone other than me. Their code is not
ready yet,  but will likely will be for the 3.18 merge window.

~Maarten

Re: [PATCH 2/2] android: add sync_fence_create_dma

[Dan Carpenter]

On Mon, Sep 01, 2014 at 02:33:59PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 28-08-14 om 13:57 schreef Dan Carpenter:
> > On Thu, Aug 28, 2014 at 08:54:05AM +0200, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> On 15-08-14 08:46, Greg Kroah-Hartman wrote:
> >>> On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote:
> >>>> This allows users of dma fences to create a android fence.
> >>> Who is going to use these functions?  I need an in-kernel user before I
> >>> can add new api calls.
> >> So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync.
> >> Will you apply these patches?
> > Can you resend the patches?  Fix the changelog of the first one to
> > mention that it is a bugfix.  Send a [patch 3/3] which uses the new
> > functions in [patch 2/3].
> The second patch will have to be applied without an in-kernel user because it
> will be used in the drm subsystem, by someone other than me. Their code is not
> ready yet,  but will likely will be for the 3.18 merge window.

Let's just wait until the user is ready.  It might be easiest if they
push your patch?

Anyway, please resend the first patch so we can apply that right away.

regards,
dan carpenter