Re: [PATCH] drm/amdkfd: Release the topology_lock in error case

[Felix Kuehling <felix.kuehling@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

Am 2022-11-16 um 03:04 schrieb Ma Jun:
> Release the topology_lock in error case
>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
Dan, did you change your email address, is this one correct?

Ma Jun, thanks for looking into this. Some of this problem predates your 
patch that was flagged by Dan. I would prefer a more consistent and 
robust handling of these error cases. I think everything inside the 
topology lock could be moved into another function to simplify the error 
handling. I'm attaching a completely untested patch to illustrate the idea.

Regards,
   Felix


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index ef9c6fdfb88d..5ea737337658 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1841,6 +1841,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>   			pr_err("Error creating VCRAT for GPU (ID: 0x%x)\n",
>   			       gpu_id);
>   			topology_crat_proximity_domain--;
> +			up_write(&topology_lock);
>   			return res;
>   		}
>   
> @@ -1851,6 +1852,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>   			pr_err("Error parsing VCRAT for GPU (ID: 0x%x)\n",
>   			       gpu_id);
>   			topology_crat_proximity_domain--;
> +			up_write(&topology_lock);
>   			goto err;
>   		}
>   
> @@ -1860,6 +1862,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>   		dev = kfd_assign_gpu(gpu);
>   		if (WARN_ON(!dev)) {
>   			res = -ENODEV;
> +			up_write(&topology_lock);
>   			goto err;
>   		}
>   

[-- Attachment #2: 0001-drm-amdkfd-Release-the-topology_lock-in-error-case.patch --]
[-- Type: text/x-patch, Size: 4282 bytes --]

From ceb79972cdd490de181a6895836e40bf4e93c631 Mon Sep 17 00:00:00 2001
From: Felix Kuehling <felix.kuehling@gmail.com>
Date: Wed, 16 Nov 2022 15:38:44 -0500
Subject: [PATCH] drm/amdkfd: Release the topology_lock in error case

Move the topology-locked part of kfd_topology_add_device into a separate
function to simlpify error handling and release the topology lock
consistently.

Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Felix Kuehling <felix.kuehling@gmail.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 109 ++++++++++++----------
 1 file changed, 58 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index ef9c6fdfb88d..b56beab71c5b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1805,16 +1805,66 @@ static void kfd_fill_cache_non_crat_info(struct kfd_topology_device *dev, struct
 	pr_debug("Added [%d] GPU cache entries\n", num_of_entries);
 }
 
+static int kfd_topology_add_device_locked(struct kfd_dev *gpu, uint32_t gpu_id,
+					  struct kfd_topology_device **dev)
+{
+	int proximity_domain = ++topology_crat_proximity_domain;
+	struct list_head temp_topology_device_list;
+	void *crat_image = NULL;
+	size_t image_size = 0;
+	int res;
+
+	res = kfd_create_crat_image_virtual(&crat_image, &image_size,
+					    COMPUTE_UNIT_GPU, gpu,
+					    proximity_domain);
+	if (res) {
+		pr_err("Error creating VCRAT for GPU (ID: 0x%x)\n",
+		       gpu_id);
+		topology_crat_proximity_domain--;
+		return res;
+	}
+
+	res = kfd_parse_crat_table(crat_image,
+				   &temp_topology_device_list,
+				   proximity_domain);
+	if (res) {
+		pr_err("Error parsing VCRAT for GPU (ID: 0x%x)\n",
+		       gpu_id);
+		topology_crat_proximity_domain--;
+		return res;
+	}
+
+	kfd_topology_update_device_list(&temp_topology_device_list,
+					&topology_device_list);
+
+	*dev = kfd_assign_gpu(gpu);
+	if (WARN_ON(!*dev))
+		return -ENODEV;
+
+	/* Fill the cache affinity information here for the GPUs
+	 * using VCRAT
+	 */
+	kfd_fill_cache_non_crat_info(*dev, gpu);
+
+	/* Update the SYSFS tree, since we added another topology
+	 * device
+	 */
+	res = kfd_topology_update_sysfs();
+	if (!res)
+		sys_props.generation_count++;
+	else
+		pr_err("Failed to update GPU (ID: 0x%x) to sysfs topology. res=%d\n",
+		       gpu_id, res);
+
+	return res;
+}
+
 int kfd_topology_add_device(struct kfd_dev *gpu)
 {
 	uint32_t gpu_id;
 	struct kfd_topology_device *dev;
 	struct kfd_cu_info cu_info;
 	int res = 0;
-	struct list_head temp_topology_device_list;
-	void *crat_image = NULL;
-	size_t image_size = 0;
-	int proximity_domain;
 	int i;
 	const char *asic_name = amdgpu_asic_name[gpu->adev->asic_type];
 
@@ -1831,54 +1881,11 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
 	 */
 	down_write(&topology_lock);
 	dev = kfd_assign_gpu(gpu);
-	if (!dev) {
-		proximity_domain = ++topology_crat_proximity_domain;
-
-		res = kfd_create_crat_image_virtual(&crat_image, &image_size,
-						    COMPUTE_UNIT_GPU, gpu,
-						    proximity_domain);
-		if (res) {
-			pr_err("Error creating VCRAT for GPU (ID: 0x%x)\n",
-			       gpu_id);
-			topology_crat_proximity_domain--;
-			return res;
-		}
-
-		res = kfd_parse_crat_table(crat_image,
-					   &temp_topology_device_list,
-					   proximity_domain);
-		if (res) {
-			pr_err("Error parsing VCRAT for GPU (ID: 0x%x)\n",
-			       gpu_id);
-			topology_crat_proximity_domain--;
-			goto err;
-		}
-
-		kfd_topology_update_device_list(&temp_topology_device_list,
-			&topology_device_list);
-
-		dev = kfd_assign_gpu(gpu);
-		if (WARN_ON(!dev)) {
-			res = -ENODEV;
-			goto err;
-		}
-
-		/* Fill the cache affinity information here for the GPUs
-		 * using VCRAT
-		 */
-		kfd_fill_cache_non_crat_info(dev, gpu);
-
-		/* Update the SYSFS tree, since we added another topology
-		 * device
-		 */
-		res = kfd_topology_update_sysfs();
-		if (!res)
-			sys_props.generation_count++;
-		else
-			pr_err("Failed to update GPU (ID: 0x%x) to sysfs topology. res=%d\n",
-						gpu_id, res);
-	}
+	if (!dev)
+		res = kfd_topology_add_device_locked(gpu, gpu_id, &dev);
 	up_write(&topology_lock);
+	if (res)
+		goto err;
 
 	dev->gpu_id = gpu_id;
 	gpu->id = gpu_id;
-- 
2.34.1