Re: [PATCH v5 13/19] drm: writeback: Fix leak of writeback job

From: Brian Starkey <Brian.Starkey@arm.com>
To: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>, nd <nd@arm.com>,
	"james qian wang (Arm Technology China)"
	<james.qian.wang@arm.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v5 13/19] drm: writeback: Fix leak of writeback job
Date: Thu, 21 Feb 2019 17:48:09 +0000	[thread overview]
Message-ID: <20190221174810.3bhshikduornhs22@DESKTOP-E1NTVVP.localdomain> (raw)
In-Reply-To: <20190221103212.28764-14-laurent.pinchart+renesas@ideasonboard.com>

Hi,

On Thu, Feb 21, 2019 at 12:32:06PM +0200, Laurent Pinchart wrote:
> Writeback jobs are allocated when the WRITEBACK_FB_ID is set, and
> deleted when the jobs complete. This results in both a memory leak of
> the job and a leak of the framebuffer if the atomic commit returns
> before the job is queued for processing, for instance if the atomic
> check fails or if the commit runs in test-only mode.
> 
> Fix this by implementing the drm_writeback_cleanup_job() function and
> calling it from __drm_atomic_helper_connector_destroy_state(). As
> writeback jobs are removed from the state when they're queued for
> processing, any job left in the state when the state gets destroyed
> needs to be cleaned up.
> 
> The existing declaration of the drm_writeback_cleanup_job() function
> without an implementation hints that this problem was considered, but
> never addressed.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Thanks for fixing this, it looks like it got dropped in one of the
rework/rebases.

Reviewed-by: Brian Starkey <brian.starkey@arm.com>

> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c |  4 ++++
>  drivers/gpu/drm/drm_writeback.c           | 13 ++++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 4985384e51f6..59ffb6b9c745 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_connector.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_writeback.h>
>  
>  #include <linux/slab.h>
>  #include <linux/dma-fence.h>
> @@ -412,6 +413,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>  
>  	if (state->commit)
>  		drm_crtc_commit_put(state->commit);
> +
> +	if (state->writeback_job)
> +		drm_writeback_cleanup_job(state->writeback_job);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 338b993d7c9f..afb1ae6e0ecb 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -273,6 +273,14 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
>  }
>  EXPORT_SYMBOL(drm_writeback_queue_job);
>  
> +void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> +{
> +	if (job->fb)
> +		drm_framebuffer_put(job->fb);
> +
> +	kfree(job);
> +}
> +
>  /*
>   * @cleanup_work: deferred cleanup of a writeback job
>   *
> @@ -285,10 +293,9 @@ static void cleanup_work(struct work_struct *work)
>  	struct drm_writeback_job *job = container_of(work,
>  						     struct drm_writeback_job,
>  						     cleanup_work);
> -	drm_framebuffer_put(job->fb);
> -	kfree(job);
> -}
>  
> +	drm_writeback_cleanup_job(job);
> +}
>  
>  /**
>   * drm_writeback_signal_completion - Signal the completion of a writeback job
> -- 
> Regards,
> 
> Laurent Pinchart
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel