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

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

[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>
---
 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;
 		}
 
-- 
2.25.1

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

[Felix Kuehling]

[-- 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

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

[Dan Carpenter]

On Wed, Nov 16, 2022 at 03:49:18PM -0500, Felix Kuehling wrote:
> 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?
> 

Yep.

I'm still around doing Smatch stuff though:
https://lore.kernel.org/all/Y1qf7w%2Fjo8FH5I8G@kadam/

regards,
dan carpenter

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

[Ma, Jun]

Hi Felix,

I just tested your patch. It works fine on my test set with the following little fix.

Regards,
Ma Jun

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 7ea3ec1e9e75..7d6fbfbfeb79 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1954,9 +1954,11 @@ static int kfd_topology_add_device_locked(struct kfd_dev *gpu, uint32_t gpu_id,
                pr_err("Error creating VCRAT for GPU (ID: 0x%x)\n",
                       gpu_id);
                topology_crat_proximity_domain--;
-               return res;
+               goto err;
        }
 
+       INIT_LIST_HEAD(&temp_topology_device_list);
+
        res = kfd_parse_crat_table(crat_image,
                                   &temp_topology_device_list,
                                   proximity_domain);
@@ -1964,15 +1966,17 @@ static int kfd_topology_add_device_locked(struct kfd_dev *gpu, uint32_t gpu_id,
                pr_err("Error parsing VCRAT for GPU (ID: 0x%x)\n",
                       gpu_id);
                topology_crat_proximity_domain--;
-               return res;
+               goto err;
        }
 
        kfd_topology_update_device_list(&temp_topology_device_list,
                                        &topology_device_list);
 
        *dev = kfd_assign_gpu(gpu);
-       if (WARN_ON(!*dev))
-               return -ENODEV;
+       if (WARN_ON(!*dev)) {
+               res = -ENODEV;
+               goto err;
+       }
 
        /* Fill the cache affinity information here for the GPUs
         * using VCRAT
@@ -1989,6 +1993,8 @@ static int kfd_topology_add_device_locked(struct kfd_dev *gpu, uint32_t gpu_id,
                pr_err("Failed to update GPU (ID: 0x%x) to sysfs topology. res=%d\n",
                       gpu_id, res);
 
+err:
+       kfd_destroy_crat_image(crat_image);
        return res;
 }
 
@@ -2001,8 +2007,6 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
        int i;
        const char *asic_name = amdgpu_asic_name[gpu->adev->asic_type];
 
-       INIT_LIST_HEAD(&temp_topology_device_list);
-
        gpu_id = kfd_generate_gpu_id(gpu);
        pr_debug("Adding new GPU (ID: 0x%x) to topology\n", gpu_id);
 
@@ -2018,7 +2022,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
                res = kfd_topology_add_device_locked(gpu, gpu_id, &dev);
        up_write(&topology_lock);
        if (res)
-               goto err;
+               return res;
 
        dev->gpu_id = gpu_id;
        gpu->id = gpu_id;
@@ -2141,8 +2145,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
 
        if (!res)
                kfd_notify_gpu_change(gpu_id, 1);
-err:
-       kfd_destroy_crat_image(crat_image);
+
        return res;
 }



On 11/17/2022 4:49 AM, Felix Kuehling wrote:
> 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;
>>   		}
>>   

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

[Felix Kuehling]

Looks good. Feel free to send the revised patch to amd-gfx. I'll review it.

Thanks,
   Felix


Am 2022-11-17 um 02:33 schrieb Ma, Jun:
> Hi Felix,
>
> I just tested your patch. It works fine on my test set with the following little fix.
>
> Regards,
> Ma Jun
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index 7ea3ec1e9e75..7d6fbfbfeb79 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1954,9 +1954,11 @@ static int kfd_topology_add_device_locked(struct kfd_dev *gpu, uint32_t gpu_id,
>                  pr_err("Error creating VCRAT for GPU (ID: 0x%x)\n",
>                         gpu_id);
>                  topology_crat_proximity_domain--;
> -               return res;
> +               goto err;
>          }
>   
> +       INIT_LIST_HEAD(&temp_topology_device_list);
> +
>          res = kfd_parse_crat_table(crat_image,
>                                     &temp_topology_device_list,
>                                     proximity_domain);
> @@ -1964,15 +1966,17 @@ static int kfd_topology_add_device_locked(struct kfd_dev *gpu, uint32_t gpu_id,
>                  pr_err("Error parsing VCRAT for GPU (ID: 0x%x)\n",
>                         gpu_id);
>                  topology_crat_proximity_domain--;
> -               return res;
> +               goto err;
>          }
>   
>          kfd_topology_update_device_list(&temp_topology_device_list,
>                                          &topology_device_list);
>   
>          *dev = kfd_assign_gpu(gpu);
> -       if (WARN_ON(!*dev))
> -               return -ENODEV;
> +       if (WARN_ON(!*dev)) {
> +               res = -ENODEV;
> +               goto err;
> +       }
>   
>          /* Fill the cache affinity information here for the GPUs
>           * using VCRAT
> @@ -1989,6 +1993,8 @@ static int kfd_topology_add_device_locked(struct kfd_dev *gpu, uint32_t gpu_id,
>                  pr_err("Failed to update GPU (ID: 0x%x) to sysfs topology. res=%d\n",
>                         gpu_id, res);
>   
> +err:
> +       kfd_destroy_crat_image(crat_image);
>          return res;
>   }
>   
> @@ -2001,8 +2007,6 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>          int i;
>          const char *asic_name = amdgpu_asic_name[gpu->adev->asic_type];
>   
> -       INIT_LIST_HEAD(&temp_topology_device_list);
> -
>          gpu_id = kfd_generate_gpu_id(gpu);
>          pr_debug("Adding new GPU (ID: 0x%x) to topology\n", gpu_id);
>   
> @@ -2018,7 +2022,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>                  res = kfd_topology_add_device_locked(gpu, gpu_id, &dev);
>          up_write(&topology_lock);
>          if (res)
> -               goto err;
> +               return res;
>   
>          dev->gpu_id = gpu_id;
>          gpu->id = gpu_id;
> @@ -2141,8 +2145,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>   
>          if (!res)
>                  kfd_notify_gpu_change(gpu_id, 1);
> -err:
> -       kfd_destroy_crat_image(crat_image);
> +
>          return res;
>   }
>
>
>
> On 11/17/2022 4:49 AM, Felix Kuehling wrote:
>> 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;
>>>    		}
>>>    

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

[Ma Jun]

From: Felix Kuehling <felix.kuehling@gmail.com>

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>
Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 120 ++++++++++++----------
 1 file changed, 65 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 8c555c32ea70..7d6fbfbfeb79 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1938,21 +1938,75 @@ 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--;
+		goto err;
+	}
+
+	INIT_LIST_HEAD(&temp_topology_device_list);
+
+	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);
+
+err:
+	kfd_destroy_crat_image(crat_image);
+	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];
 
-	INIT_LIST_HEAD(&temp_topology_device_list);
-
 	gpu_id = kfd_generate_gpu_id(gpu);
 	pr_debug("Adding new GPU (ID: 0x%x) to topology\n", gpu_id);
 
@@ -1964,54 +2018,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)
+		return res;
 
 	dev->gpu_id = gpu_id;
 	gpu->id = gpu_id;
@@ -2134,8 +2145,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
 
 	if (!res)
 		kfd_notify_gpu_change(gpu_id, 1);
-err:
-	kfd_destroy_crat_image(crat_image);
+
 	return res;
 }
 
-- 
2.25.1

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

[Felix Kuehling]

Am 2022-11-21 um 00:13 schrieb Ma Jun:
> From: Felix Kuehling <felix.kuehling@gmail.com>
>
> 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>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 120 ++++++++++++----------
>   1 file changed, 65 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index 8c555c32ea70..7d6fbfbfeb79 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1938,21 +1938,75 @@ 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--;
> +		goto err;
> +	}
> +
> +	INIT_LIST_HEAD(&temp_topology_device_list);
> +
> +	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);
> +
> +err:
> +	kfd_destroy_crat_image(crat_image);
> +	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];
>   
> -	INIT_LIST_HEAD(&temp_topology_device_list);
> -
>   	gpu_id = kfd_generate_gpu_id(gpu);
>   	pr_debug("Adding new GPU (ID: 0x%x) to topology\n", gpu_id);
>   
> @@ -1964,54 +2018,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)
> +		return res;
>   
>   	dev->gpu_id = gpu_id;
>   	gpu->id = gpu_id;
> @@ -2134,8 +2145,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>   
>   	if (!res)
>   		kfd_notify_gpu_change(gpu_id, 1);
> -err:
> -	kfd_destroy_crat_image(crat_image);
> +
>   	return res;
>   }
>