| Message ID | 20190221111304.846-1-james.qian.wang@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix writeback_job leak when state is check only or check failed | expand |
Hey Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China): > The writeback job will not be added to writeback queue if the state is > check only or check failed, to avoid leak, need to cleanup writeback job > in connector_destroy_state if the job existed. > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com> Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success. ~Maarten > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++ > drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++--- > 2 files changed, 22 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..6a8e414233de 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -28,6 +28,7 @@ > #include <drm/drm_crtc.h> > #include <drm/drm_plane.h> > #include <drm/drm_connector.h> > +#include <drm/drm_writeback.h> > #include <drm/drm_atomic.h> > #include <drm/drm_device.h> > > @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); > void > __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) > { > + if (state->writeback_job) > + drm_writeback_cleanup_job(state->writeback_job); > + > if (state->crtc) > drm_connector_put(state->connector); > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index c20e6fe00cb3..486121150eaa 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, > } > EXPORT_SYMBOL(drm_writeback_queue_job); > > +/** > + * drm_writeback_cleanup_job - cleanup a writeback job > + * @job: The job to cleanup > + */ > +void drm_writeback_cleanup_job(struct drm_writeback_job *job) > +{ > + if (job->fb) > + drm_framebuffer_put(job->fb); > + > + if (job->out_fence) > + dma_fence_put(job->out_fence); > + > + kfree(job); > +} > +EXPORT_SYMBOL(drm_writeback_cleanup_job); > + > /* > * @cleanup_work: deferred cleanup of a writeback job > * > @@ -280,11 +296,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 > * @wb_connector: The writeback connector whose job is complete > @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector, > dma_fence_set_error(job->out_fence, status); > dma_fence_signal(job->out_fence); > dma_fence_put(job->out_fence); > + job->out_fence = NULL; > } > } > spin_unlock_irqrestore(&wb_connector->job_lock, flags);
On Thu, Feb 21, 2019 at 02:56:56PM +0100, Maarten Lankhorst wrote: > Hey > > Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China): > > The writeback job will not be added to writeback queue if the state is > > check only or check failed, to avoid leak, need to cleanup writeback job > > in connector_destroy_state if the job existed. > > > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com> > > Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success. > > ~Maarten Yes, the state->writeback_job need to be set to null once the job has been queued. Maybe we can refine the queue_job function and add this set NULL into it. void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, struct drm_writeback_job **new_job) { struct drm_writeback_job *job = *new_job; unsigned long flags; *new_job = NULL; spin_lock_irqsave(&wb_connector->job_lock, flags); list_add_tail(&job->list_entry, &wb_connector->job_queue); spin_unlock_irqrestore(&wb_connector->job_lock, flags); } Thanks James > > > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++ > > drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++--- > > 2 files changed, 22 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..6a8e414233de 100644 > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > @@ -28,6 +28,7 @@ > > #include <drm/drm_crtc.h> > > #include <drm/drm_plane.h> > > #include <drm/drm_connector.h> > > +#include <drm/drm_writeback.h> > > #include <drm/drm_atomic.h> > > #include <drm/drm_device.h> > > > > @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); > > void > > __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) > > { > > + if (state->writeback_job) > > + drm_writeback_cleanup_job(state->writeback_job); > > + > > if (state->crtc) > > drm_connector_put(state->connector); > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > index c20e6fe00cb3..486121150eaa 100644 > > --- a/drivers/gpu/drm/drm_writeback.c > > +++ b/drivers/gpu/drm/drm_writeback.c > > @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, > > } > > EXPORT_SYMBOL(drm_writeback_queue_job); > > > > +/** > > + * drm_writeback_cleanup_job - cleanup a writeback job > > + * @job: The job to cleanup > > + */ > > +void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > +{ > > + if (job->fb) > > + drm_framebuffer_put(job->fb); > > + > > + if (job->out_fence) > > + dma_fence_put(job->out_fence); > > + > > + kfree(job); > > +} > > +EXPORT_SYMBOL(drm_writeback_cleanup_job); > > + > > /* > > * @cleanup_work: deferred cleanup of a writeback job > > * > > @@ -280,11 +296,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 > > * @wb_connector: The writeback connector whose job is complete > > @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector, > > dma_fence_set_error(job->out_fence, status); > > dma_fence_signal(job->out_fence); > > dma_fence_put(job->out_fence); > > + job->out_fence = NULL; > > } > > } > > spin_unlock_irqrestore(&wb_connector->job_lock, flags); >
Hi James, On Fri, Feb 22, 2019 at 07:39:55AM +0000, james qian wang (Arm Technology China) wrote: > On Thu, Feb 21, 2019 at 02:56:56PM +0100, Maarten Lankhorst wrote: > > Hey > > > > Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China): > > > The writeback job will not be added to writeback queue if the state is > > > check only or check failed, to avoid leak, need to cleanup writeback job > > > in connector_destroy_state if the job existed. > > > > > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com> > > > > Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success. > > > > ~Maarten > > Yes, the state->writeback_job need to be set to null once the job has been > queued. > Maybe we can refine the queue_job function and add this set NULL into > it. > Laurent has submitted patches for both of these issues, please check [PATCH v5 12/19] drm: writeback: Cleanup job ownership handling when queuing job [PATCH v5 13/19] drm: writeback: Fix leak of writeback job Also note they will impact the komeda implementation Thanks, -Brian > void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, > struct drm_writeback_job **new_job) > { > struct drm_writeback_job *job = *new_job; > unsigned long flags; > > *new_job = NULL; > > spin_lock_irqsave(&wb_connector->job_lock, flags); > list_add_tail(&job->list_entry, &wb_connector->job_queue); > spin_unlock_irqrestore(&wb_connector->job_lock, flags); > } > > Thanks > James > > > > > > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++ > > > drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++--- > > > 2 files changed, 22 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..6a8e414233de 100644 > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > @@ -28,6 +28,7 @@ > > > #include <drm/drm_crtc.h> > > > #include <drm/drm_plane.h> > > > #include <drm/drm_connector.h> > > > +#include <drm/drm_writeback.h> > > > #include <drm/drm_atomic.h> > > > #include <drm/drm_device.h> > > > > > > @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); > > > void > > > __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) > > > { > > > + if (state->writeback_job) > > > + drm_writeback_cleanup_job(state->writeback_job); > > > + > > > if (state->crtc) > > > drm_connector_put(state->connector); > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > index c20e6fe00cb3..486121150eaa 100644 > > > --- a/drivers/gpu/drm/drm_writeback.c > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, > > > } > > > EXPORT_SYMBOL(drm_writeback_queue_job); > > > > > > +/** > > > + * drm_writeback_cleanup_job - cleanup a writeback job > > > + * @job: The job to cleanup > > > + */ > > > +void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > +{ > > > + if (job->fb) > > > + drm_framebuffer_put(job->fb); > > > + > > > + if (job->out_fence) > > > + dma_fence_put(job->out_fence); > > > + > > > + kfree(job); > > > +} > > > +EXPORT_SYMBOL(drm_writeback_cleanup_job); > > > + > > > /* > > > * @cleanup_work: deferred cleanup of a writeback job > > > * > > > @@ -280,11 +296,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 > > > * @wb_connector: The writeback connector whose job is complete > > > @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector, > > > dma_fence_set_error(job->out_fence, status); > > > dma_fence_signal(job->out_fence); > > > dma_fence_put(job->out_fence); > > > + job->out_fence = NULL; > > > } > > > } > > > spin_unlock_irqrestore(&wb_connector->job_lock, flags); > >
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 4985384e51f6..6a8e414233de 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -28,6 +28,7 @@ #include <drm/drm_crtc.h> #include <drm/drm_plane.h> #include <drm/drm_connector.h> +#include <drm/drm_writeback.h> #include <drm/drm_atomic.h> #include <drm/drm_device.h> @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); void __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) { + if (state->writeback_job) + drm_writeback_cleanup_job(state->writeback_job); + if (state->crtc) drm_connector_put(state->connector); diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index c20e6fe00cb3..486121150eaa 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, } EXPORT_SYMBOL(drm_writeback_queue_job); +/** + * drm_writeback_cleanup_job - cleanup a writeback job + * @job: The job to cleanup + */ +void drm_writeback_cleanup_job(struct drm_writeback_job *job) +{ + if (job->fb) + drm_framebuffer_put(job->fb); + + if (job->out_fence) + dma_fence_put(job->out_fence); + + kfree(job); +} +EXPORT_SYMBOL(drm_writeback_cleanup_job); + /* * @cleanup_work: deferred cleanup of a writeback job * @@ -280,11 +296,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 * @wb_connector: The writeback connector whose job is complete @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector, dma_fence_set_error(job->out_fence, status); dma_fence_signal(job->out_fence); dma_fence_put(job->out_fence); + job->out_fence = NULL; } } spin_unlock_irqrestore(&wb_connector->job_lock, flags);
The writeback job will not be added to writeback queue if the state is check only or check failed, to avoid leak, need to cleanup writeback job in connector_destroy_state if the job existed. Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com> --- drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++ drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-)