Re: [PATCH] drm/scheduler: add NULL pointer check for run queue (v2)

[Zhang, Jerry (Junwei)]

On 07/16/2018 05:54 PM, Michel Dänzer wrote:

On 2018-07-16 11:23 AM, Junwei Zhang wrote:

To check rq pointer before adding entity into it.
That avoids NULL pointer access in some case.

v2: move the check to caller

Suggested-by: Christian König 
Signed-off-by: Junwei Zhang 
---
  drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 16bf446..dac71e3 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -547,6 +547,11 @@ void drm_sched_entity_push_job(struct drm_sched_job 
*sched_job,
if (first) {
/* Add the entity to the run queue */
spin_lock(>rq_lock);
+   if (!entity->rq) {
+   DRM_ERROR("Trying to push to a killed entity\n");


This could result in spamming dmesg with this error message. I suggest


Thanks for your comments.
Actually it will cause App or GPU hang, as the job is unexpected to push to a 
non-existing entity run queue.
Now we're trying to predict that before kernel panic or BUG_ON.

Regards,
Jerry



if (WARN_ON_ONCE(!entity->rq)) {

instead, no DRM_ERROR. That will produce a single warning with a backtrace.



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue (v2)

[Michel Dänzer]

On 2018-07-16 11:23 AM, Junwei Zhang wrote:
> To check rq pointer before adding entity into it.
> That avoids NULL pointer access in some case.
> 
> v2: move the check to caller
> 
> Suggested-by: Christian König 
> Signed-off-by: Junwei Zhang 
> ---
>  drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index 16bf446..dac71e3 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -547,6 +547,11 @@ void drm_sched_entity_push_job(struct drm_sched_job 
> *sched_job,
>   if (first) {
>   /* Add the entity to the run queue */
>   spin_lock(>rq_lock);
> + if (!entity->rq) {
> + DRM_ERROR("Trying to push to a killed entity\n");

This could result in spamming dmesg with this error message. I suggest

if (WARN_ON_ONCE(!entity->rq)) {

instead, no DRM_ERROR. That will produce a single warning with a backtrace.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue (v2)

[Christian König]

Am 16.07.2018 um 11:23 schrieb Junwei Zhang:

To check rq pointer before adding entity into it.
That avoids NULL pointer access in some case.

v2: move the check to caller

Suggested-by: Christian König 
Signed-off-by: Junwei Zhang 


Reviewed-by: Christian König 


---
  drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 16bf446..dac71e3 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -547,6 +547,11 @@ void drm_sched_entity_push_job(struct drm_sched_job 
*sched_job,
if (first) {
/* Add the entity to the run queue */
spin_lock(>rq_lock);
+   if (!entity->rq) {
+   DRM_ERROR("Trying to push to a killed entity\n");
+   spin_unlock(>rq_lock);
+   return;
+   }
drm_sched_rq_add_entity(entity->rq, entity);
spin_unlock(>rq_lock);
drm_sched_wakeup(sched);


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Zhang, Jerry (Junwei)]

On 07/16/2018 05:11 PM, Christian König wrote:

Am 16.07.2018 um 11:01 schrieb Zhang, Jerry (Junwei):

On 07/16/2018 04:31 PM, Christian König wrote:

Am 16.07.2018 um 04:59 schrieb Junwei Zhang:

To check rq pointer before adding entity into it.
That avoids NULL pointer access in some case.

Suggested-by: Christian König 
Signed-off-by: Junwei Zhang 
---
  drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 16bf446..5e5268d 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
  {
  if (!list_empty(>list))
  return;
+if (!rq) {
+DRM_ERROR("rq is NULL!\n");
+return;
+}


Better put that into drm_sched_entity_push_job(), e.g. something like:


Considered that as well.
Just be afraid of someone else could call it in another place without rq 
checking in the future.


Well that's exactly the reason why I wanted to have the check in 
drm_sched_rq_add_entity().

Calling drm_sched_rq_add_entity() will a NULL rq is illegal and that should be 
avoided in the caller instead of more or less silently dropped in the function.


Fine, to expose the error in place explicitly.

Regards,
Jerry



Regards,
Christian.



Regards,
Jerry



/* first job wakes up scheduler */
if (first) {
 /* Add the entity to the run queue */
 spin_lock(>rq_lock);
 if (!entity->rq) {
 DRM_ERROR("Trying to push to killed entity!\n");


Regards,
Christian.


  spin_lock(>lock);
  list_add_tail(>list, >entities);
  spin_unlock(>lock);




___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Christian König]

Am 16.07.2018 um 11:01 schrieb Zhang, Jerry (Junwei):

On 07/16/2018 04:31 PM, Christian König wrote:

Am 16.07.2018 um 04:59 schrieb Junwei Zhang:

To check rq pointer before adding entity into it.
That avoids NULL pointer access in some case.

Suggested-by: Christian König 
Signed-off-by: Junwei Zhang 
---
  drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/scheduler/gpu_scheduler.c

index 16bf446..5e5268d 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct 
drm_sched_rq *rq,

  {
  if (!list_empty(>list))
  return;
+    if (!rq) {
+    DRM_ERROR("rq is NULL!\n");
+    return;
+    }


Better put that into drm_sched_entity_push_job(), e.g. something like:


Considered that as well.
Just be afraid of someone else could call it in another place without 
rq checking in the future.


Well that's exactly the reason why I wanted to have the check in 
drm_sched_rq_add_entity().


Calling drm_sched_rq_add_entity() will a NULL rq is illegal and that 
should be avoided in the caller instead of more or less silently dropped 
in the function.


Regards,
Christian.



Regards,
Jerry



/* first job wakes up scheduler */
if (first) {
 /* Add the entity to the run queue */
 spin_lock(>rq_lock);
 if (!entity->rq) {
 DRM_ERROR("Trying to push to killed entity!\n");


Regards,
Christian.


  spin_lock(>lock);
  list_add_tail(>list, >entities);
  spin_unlock(>lock);




___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Zhang, Jerry (Junwei)]

On 07/16/2018 04:31 PM, Christian König wrote:

Am 16.07.2018 um 04:59 schrieb Junwei Zhang:

To check rq pointer before adding entity into it.
That avoids NULL pointer access in some case.

Suggested-by: Christian König 
Signed-off-by: Junwei Zhang 
---
  drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 16bf446..5e5268d 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
  {
  if (!list_empty(>list))
  return;
+if (!rq) {
+DRM_ERROR("rq is NULL!\n");
+return;
+}


Better put that into drm_sched_entity_push_job(), e.g. something like:


Considered that as well.
Just be afraid of someone else could call it in another place without rq 
checking in the future.

Regards,
Jerry



/* first job wakes up scheduler */
if (first) {
 /* Add the entity to the run queue */
 spin_lock(>rq_lock);
 if (!entity->rq) {
 DRM_ERROR("Trying to push to killed entity!\n");


Regards,
Christian.


  spin_lock(>lock);
  list_add_tail(>list, >entities);
  spin_unlock(>lock);



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/scheduler: add NULL pointer check for run queue

[Christian König]

Am 16.07.2018 um 04:59 schrieb Junwei Zhang:

To check rq pointer before adding entity into it.
That avoids NULL pointer access in some case.

Suggested-by: Christian König 
Signed-off-by: Junwei Zhang 
---
  drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 16bf446..5e5268d 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -91,6 +91,10 @@ static void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
  {
if (!list_empty(>list))
return;
+   if (!rq) {
+   DRM_ERROR("rq is NULL!\n");
+   return;
+   }


Better put that into drm_sched_entity_push_job(), e.g. something like:

/* first job wakes up scheduler */
if (first) {
    /* Add the entity to the run queue */
    spin_lock(>rq_lock);
    if (!entity->rq) {
        DRM_ERROR("Trying to push to killed entity!\n");


Regards,
Christian.


spin_lock(>lock);
list_add_tail(>list, >entities);
spin_unlock(>lock);


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx