Re: [PATCH v3] drm/sched: Fix kernel NULL pointer dereference error

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Am 18.10.22 um 14:20 schrieb Yadav, Arvind:
[SNIP]

+    drm_sched_fence_finished(s_fence);
+    dma_fence_put(&s_fence->finished);
+    wake_up_interruptible(&sched->wake_up_worker);
+}
+
+int drm_sched_fence_add_parent_cb(struct dma_fence *fence,
+                  struct drm_sched_fence *s_fence)
+{
+    return dma_fence_add_callback(fence, &s_fence->cb,
+                      drm_sched_job_done_cb);
+}
+
+bool drm_sched_fence_remove_parent_cb(struct drm_sched_fence *s_fence)
+{
+    return dma_fence_remove_callback(s_fence->parent,
+                     &s_fence->cb);
+}

Do we really need separate functions for that?

We can use  'drm_sched_fence_set_parent' but we need to add extra NULL check before

adding in the callback otherwise add-callback will throw the warning message every time.

If I add NULL check then will never get any callback warning message for setting NULL parent fence.

So I have kept separate functions.

I rather prefer having a single function and allowing the parent fence to be set to NULL.

Alternatively we could have a drm_sched_fence_set_parent() and drm_sched_fence_clear_parent() function if you really think it's cleaner that way.

atomic_dec(&sched->hw_rq_count);
@@ -576,15 +562,14 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
              continue;
            if (fence) {
-            r = dma_fence_add_callback(fence, &s_job->cb,
-                           drm_sched_job_done_cb);
+            r = drm_sched_fence_add_parent_cb(fence, s_job->s_fence);
              if (r == -ENOENT)
-                drm_sched_job_done(s_job);
+                drm_sched_job_done(s_job->s_fence);
              else if (r)
                  DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",

Completely nuke that here. All of this should be done in the single drm_sched_fence_set_parent() function.

And an error message is completely superfluous. We just need to handle the case that the callback can't be installed because the fence is already signaled.

I will do the changes as per your review comments, Thank you for the review.

Just to clarify, you should nuke the error message. Error handling is rather pointless here.

Thanks,
Christian.


Thanks,

~Arvind

Regards,
Christian.

                        r);
          } else
-            drm_sched_job_done(s_job);
+            drm_sched_job_done(s_job->s_fence);
      }
        if (full_recovery) {
@@ -1049,14 +1034,9 @@ static int drm_sched_main(void *param)
          drm_sched_fence_scheduled(s_fence);
            if (!IS_ERR_OR_NULL(fence)) {
-            s_fence->parent = dma_fence_get(fence);
-            /* Drop for original kref_init of the fence */
-            dma_fence_put(fence);
-
-            r = dma_fence_add_callback(fence, &sched_job->cb,
-                           drm_sched_job_done_cb);
+            r = drm_sched_fence_set_parent(fence, s_fence);
              if (r == -ENOENT)
-                drm_sched_job_done(sched_job);
+                drm_sched_job_done(s_fence);
              else if (r)
                  DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
                        r);
@@ -1064,7 +1044,7 @@ static int drm_sched_main(void *param)
              if (IS_ERR(fence))
dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
  -            drm_sched_job_done(sched_job);
+            drm_sched_job_done(s_fence);
          }
            wake_up(&sched->job_scheduled);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 1f7d9dd1a444..7258e2fa195f 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -281,6 +281,10 @@ struct drm_sched_fence {
           * @owner: job owner for debugging
           */
      void                *owner;
+    /**
+     * @cb: callback
+     */
+    struct dma_fence_cb cb;
  };
    struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
@@ -300,7 +304,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
   *         be scheduled further.
   * @s_priority: the priority of the job.
   * @entity: the entity to which this job belongs.
- * @cb: the callback for the parent fence in s_fence.
   *
   * A job is created by the driver using drm_sched_job_init(), and
   * should call drm_sched_entity_push_job() once it wants the scheduler
@@ -325,7 +328,6 @@ struct drm_sched_job {
      atomic_t            karma;
      enum drm_sched_priority        s_priority;
      struct drm_sched_entity         *entity;
-    struct dma_fence_cb        cb;
      /**
       * @dependencies:
       *
@@ -559,6 +561,12 @@ void drm_sched_fence_free(struct drm_sched_fence *fence);
  void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
  void drm_sched_fence_finished(struct drm_sched_fence *fence);
  +int drm_sched_fence_add_parent_cb(struct dma_fence *fence,
+                  struct drm_sched_fence *s_fence);
+bool drm_sched_fence_remove_parent_cb(struct drm_sched_fence *s_fence);
+int drm_sched_fence_set_parent(struct dma_fence *fence,
+                   struct drm_sched_fence *s_fence);
+
  unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
  void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
                          unsigned long remaining);





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux