On 26/09/2025 13:36, Philipp Stanner wrote:
drm_sched_job_cleanup()'s documentation so far uses relatively soft
language, only "recommending" usage of the function. To avoid memory
leaks and, potentiall, other bugs, however, the function has to be used.
Demand usage of the function explicitly.
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/gpu/drm/scheduler/sched_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 46119aacb809..0a9df9e61963 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1030,7 +1030,7 @@ EXPORT_SYMBOL(drm_sched_job_has_dependency);
*
* Cleans up the resources allocated with drm_sched_job_init().
*
- * Drivers should call this from their error unwind code if @job is aborted
+ * Drivers need to call this from their error unwind code if @job is aborted
Should is indeed wrong. I think it could be better to go with "shall" or
"must" though. Since those two are more standard language for this.
* before drm_sched_job_arm() is called.
*
* drm_sched_job_arm() is a point of no return since it initializes the fences
@@ -1038,7 +1038,7 @@ EXPORT_SYMBOL(drm_sched_job_has_dependency);
* submit it with drm_sched_entity_push_job() and cannot simply abort it by
* calling drm_sched_job_cleanup().
*
- * This function should be called in the &drm_sched_backend_ops.free_job
callback.
+ * This function must be called in the &drm_sched_backend_ops.free_job
callback.
*/
void drm_sched_job_cleanup(struct drm_sched_job *job)
{
Hm, having read the thread so far the situation seems not so easy to
untangle.
On one hand I see Matt's point that free_job callback is a bit
misleadingly named and that there isn't anything really requiring
drm_sched_job_cleanup() to be called exactly from there. Free_job
semantics seem to be "job is done, *scheduler* is done with it".
Drm_sched_job_cleanup() already says it has to be called after the point
of no return (arm) so it could be left at drivers discretion (de facto
state today) to choose how and when to do it.
What I did not get is what is actually the perceived problem with
letting this stay? (Ie. "should" indicating it is recommended, but not a
must/shall.)
Regards,
Tvrtko
