RE: [bug report] drm/amdgpu: add function to creat all ras debugfs node

2020-03-12 Thread Yang, Stanley 
[AMD Official Use Only - Internal Distribution Only]

Hi Carpenter,

Thanks for your report and advice, I will update the code.

Regards,
Stanley

-Original Message-
From: amd-gfx  On Behalf Of Dan Carpenter
Sent: Thursday, March 12, 2020 3:34 PM
To: Zhou1, Tao 
Cc: amd-gfx@lists.freedesktop.org
Subject: [bug report] drm/amdgpu: add function to creat all ras debugfs node

Hello Tao Zhou,

The patch f9317014ea51: "drm/amdgpu: add function to creat all ras debugfs 
node" from Mar 6, 2020, leads to the following static checker
warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1132 
amdgpu_ras_debugfs_create_all()
warn: variable dereferenced before check 'obj' (see line 1131)

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
  1116  void amdgpu_ras_debugfs_create_all(struct amdgpu_device *adev)
  1117  {
  1118  struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
  1119  struct ras_manager *obj, *tmp;
  1120  struct ras_fs_if fs_info;
  1121  
  1122  /*
  1123   * it won't be called in resume path, no need to check
  1124   * suspend and gpu reset status
  1125   */
  1126  if (!con)
  1127  return;
  1128  
  1129  amdgpu_ras_debugfs_create_ctrl_node(adev);
  1130  
  1131  list_for_each_entry_safe(obj, tmp, &con->head, node) {
  1132  if (!obj)

There is no need to check for NULL here, so just remove the check.  The other 
question is why is this using list_for_each_entry_safe() instead of vanilla 
list_for_each_entry()?  It doesn't seem to be freeing "obj"
or removing "obj" from the list which are basically the only reasons why 
_safe() is used.  Some people think _safe() has something to do with locking 
but it doesn't.

Please remove the test and use vanilla list_for_each_entry().

  1133  continue;
  1134  
  1135  if (amdgpu_ras_is_supported(adev, obj->head.block) &&
  1136  (obj->attr_inuse == 1)) {
  1137  sprintf(fs_info.debugfs_name, "%s_err_inject",
  1138  ras_block_str(obj->head.block));
  1139  fs_info.head = obj->head;
  1140  amdgpu_ras_debugfs_create(adev, &fs_info);
  1141  }
  1142  }
  1143  }

regards,
dan carpenter
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CStanley.Yang%40amd.com%7C53ba68a9a148488cc80208d7c657bce8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637195953186661183&sdata=d8nkKXcNy6Jyg3SyTdlUpe%2B28WsltOmkCMbeNPXksCg%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[bug report] drm/amdgpu: add function to creat all ras debugfs node

2020-03-12 Thread Dan Carpenter 
Hello Tao Zhou,

The patch f9317014ea51: "drm/amdgpu: add function to creat all ras
debugfs node" from Mar 6, 2020, leads to the following static checker
warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1132 
amdgpu_ras_debugfs_create_all()
warn: variable dereferenced before check 'obj' (see line 1131)

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
  1116  void amdgpu_ras_debugfs_create_all(struct amdgpu_device *adev)
  1117  {
  1118  struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
  1119  struct ras_manager *obj, *tmp;
  1120  struct ras_fs_if fs_info;
  1121  
  1122  /*
  1123   * it won't be called in resume path, no need to check
  1124   * suspend and gpu reset status
  1125   */
  1126  if (!con)
  1127  return;
  1128  
  1129  amdgpu_ras_debugfs_create_ctrl_node(adev);
  1130  
  1131  list_for_each_entry_safe(obj, tmp, &con->head, node) {
  1132  if (!obj)

There is no need to check for NULL here, so just remove the check.  The
other question is why is this using list_for_each_entry_safe() instead
of vanilla list_for_each_entry()?  It doesn't seem to be freeing "obj"
or removing "obj" from the list which are basically the only reasons
why _safe() is used.  Some people think _safe() has something to do with
locking but it doesn't.

Please remove the test and use vanilla list_for_each_entry().

  1133  continue;
  1134  
  1135  if (amdgpu_ras_is_supported(adev, obj->head.block) &&
  1136  (obj->attr_inuse == 1)) {
  1137  sprintf(fs_info.debugfs_name, "%s_err_inject",
  1138  ras_block_str(obj->head.block));
  1139  fs_info.head = obj->head;
  1140  amdgpu_ras_debugfs_create(adev, &fs_info);
  1141  }
  1142  }
  1143  }

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