[bug report] drm/amdkfd: Add procfs-style information for KFD processes

[bug report] drm/amdkfd: Add procfs-style information for KFD processes

[Dan Carpenter]

Hello Kent Russell,

The patch de9f26bbd384: "drm/amdkfd: Add procfs-style information for
KFD processes" from Jun 13, 2019, leads to the following static
checker warning:

	drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c:297 kfd_create_process()
	error: 'process' dereferencing possible ERR_PTR()

drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c
   284           */
   285          mutex_lock(&kfd_processes_mutex);
   286  
   287          /* A prior open of /dev/kfd could have already created the process. */
   288          process = find_process(thread);
   289          if (process) {
   290                  pr_debug("Process already found\n");
   291          } else {
   292                  process = create_process(thread, filep);
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This returns error pointers.

   293  
   294                  if (!procfs.kobj)
                             ^^^^^^^^^^^
This is a global.  Can we check it earlier?


   295                          goto out;
   296  
   297                  process->kobj = kfd_alloc_struct(process->kobj);
   298                  if (!process->kobj) {
   299                          pr_warn("Creating procfs kobject failed");
   300                          goto out;

We return success on this path.

   301                  }
   302                  ret = kobject_init_and_add(process->kobj, &procfs_type,
   303                                             procfs.kobj, "%d",
   304                                             (int)process->lead_thread->pid);
   305                  if (ret) {
   306                          pr_warn("Creating procfs pid directory failed");
   307                          goto out;

No error handling.  Basically whenever there is a goto out the error
handling is suspect.  It's better to pick a name which says what the
error label does...

   308                  }
   309  
   310                  process->attr_pasid.name = "pasid";
   311                  process->attr_pasid.mode = KFD_SYSFS_FILE_MODE;
   312                  sysfs_attr_init(&process->attr_pasid);
   313                  ret = sysfs_create_file(process->kobj, &process->attr_pasid);
   314                  if (ret)
   315                          pr_warn("Creating pasid for pid %d failed",
   316                                          (int)process->lead_thread->pid);

Error handling and error code missing.

   317          }
   318  out:
   319          mutex_unlock(&kfd_processes_mutex);
   320  
   321          return process;
   322  }


regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [bug report] drm/amdkfd: Add procfs-style information for KFD processes

[Russell, Kent]

HI Dan,

I'll comment inline with [KR] for the most part.

Most of these come from the fact that "failure" here is not critical. If we can't make the procfs, we just comment about it in dmesg and carry on. If we fail to make the procfs structure here, we just report and carry on. The rest of the kernel can function without it, so failure isn't critical. But I should make this clear in the comments. There is no real error handling required, but we can clarify that with comments. I'll try to address this in a coming patch.

 Kent

-----Original Message-----
From: Dan Carpenter <dan.carpenter@oracle.com> 
Sent: Monday, June 24, 2019 8:27 AM
To: Russell, Kent <Kent.Russell@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Subject: [bug report] drm/amdkfd: Add procfs-style information for KFD processes

Hello Kent Russell,

The patch de9f26bbd384: "drm/amdkfd: Add procfs-style information for KFD processes" from Jun 13, 2019, leads to the following static checker warning:

	drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c:297 kfd_create_process()
	error: 'process' dereferencing possible ERR_PTR()

drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c
   284           */
   285          mutex_lock(&kfd_processes_mutex);
   286  
   287          /* A prior open of /dev/kfd could have already created the process. */
   288          process = find_process(thread);
   289          if (process) {
   290                  pr_debug("Process already found\n");
   291          } else {
   292                  process = create_process(thread, filep);
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This returns error pointers.
[KR] This was the original code. Not saying it's right though, I'll see if we can address it.
   293  
   294                  if (!procfs.kobj)
                             ^^^^^^^^^^^ This is a global.  Can we check it earlier?
[KR] The big thing for this is that if the kobj doesn't exist, we can skip the rest. We've basically got 2 things here and I combined them into 1, getting the process and making the procfs entry. I should make comments to clarify.

   295                          goto out;
   296  
   297                  process->kobj = kfd_alloc_struct(process->kobj);
   298                  if (!process->kobj) {
   299                          pr_warn("Creating procfs kobject failed");
   300                          goto out;

We return success on this path.
   301                  }
   302                  ret = kobject_init_and_add(process->kobj, &procfs_type,
   303                                             procfs.kobj, "%d",
   304                                             (int)process->lead_thread->pid);
   305                  if (ret) {
   306                          pr_warn("Creating procfs pid directory failed");
   307                          goto out;

No error handling.  Basically whenever there is a goto out the error handling is suspect.  It's better to pick a name which says what the error label does...
   308                  }
   309  
   310                  process->attr_pasid.name = "pasid";
   311                  process->attr_pasid.mode = KFD_SYSFS_FILE_MODE;
   312                  sysfs_attr_init(&process->attr_pasid);
   313                  ret = sysfs_create_file(process->kobj, &process->attr_pasid);
   314                  if (ret)
   315                          pr_warn("Creating pasid for pid %d failed",
   316                                          (int)process->lead_thread->pid);

Error handling and error code missing.

   317          }
   318  out:
   319          mutex_unlock(&kfd_processes_mutex);
   320  
   321          return process;
   322  }


regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [bug report] drm/amdkfd: Add procfs-style information for KFD processes

[Dan Carpenter]

On Mon, Jun 24, 2019 at 06:06:36PM +0000, Russell, Kent wrote:
> HI Dan,
> 
> I'll comment inline with [KR] for the most part.
> 
> Most of these come from the fact that "failure" here is not critical. If we can't make the procfs, we just comment about it in dmesg and carry on. If we fail to make the procfs structure here, we just report and carry on. The rest of the kernel can function without it, so failure isn't critical. But I should make this clear in the comments. There is no real error handling required, but we can clarify that with comments. I'll try to address this in a coming patch.
> 

Part of the reason for error handling is just to silence static checker
warnings because otherwise everyone has to review it to see if the
resource leaks are important.  It's easier in the long run to just fix
everything no matter how trivial.

regards,
dan carpenter

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