[PATCH] drm/amdkfd: Initialize dqm earlier

[PATCH] drm/amdkfd: Initialize dqm earlier

[Zeng, Oak]

dqm is referenced in function kfd_toplogy_add_device.
Move dqm initialization up to avoid NULL pointer reference.

Change-Id: Id6cb2541af129826b7621ceaa8e06e638c7bb122
Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_device.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 9d1b026..e7e24fe 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -603,6 +603,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
 	if (kfd->kfd2kgd->get_hive_id)
 		kfd->hive_id = kfd->kfd2kgd->get_hive_id(kfd->kgd);
 
+	kfd->dqm = device_queue_manager_init(kfd);
+	if (!kfd->dqm) {
+		dev_err(kfd_device, "Error initializing queue manager\n");
+		goto device_queue_manager_error;
+	}
+
 	if (kfd_topology_add_device(kfd)) {
 		dev_err(kfd_device, "Error adding device to topology\n");
 		goto kfd_topology_add_device_error;
@@ -613,12 +619,6 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
 		goto kfd_interrupt_error;
 	}
 
-	kfd->dqm = device_queue_manager_init(kfd);
-	if (!kfd->dqm) {
-		dev_err(kfd_device, "Error initializing queue manager\n");
-		goto device_queue_manager_error;
-	}
-
 	if (kfd_iommu_device_init(kfd)) {
 		dev_err(kfd_device, "Error initializing iommuv2\n");
 		goto device_iommu_error;
@@ -642,12 +642,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
 
 kfd_resume_error:
 device_iommu_error:
-	device_queue_manager_uninit(kfd->dqm);
-device_queue_manager_error:
 	kfd_interrupt_exit(kfd);
 kfd_interrupt_error:
 	kfd_topology_remove_device(kfd);
 kfd_topology_add_device_error:
+	device_queue_manager_uninit(kfd->dqm);
+device_queue_manager_error:
 	kfd_doorbell_fini(kfd);
 kfd_doorbell_error:
 	kfd_gtt_sa_fini(kfd);
-- 
2.7.4

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

Re: [PATCH] drm/amdkfd: Initialize dqm earlier

[Kuehling, Felix]

On 2019-06-06 5:51 p.m., Zeng, Oak wrote:
> dqm is referenced in function kfd_toplogy_add_device.
> Move dqm initialization up to avoid NULL pointer reference.

This addresses a pretty unlikely race condition where someone looks at 
/sys/kernel/debug/kfd/hqds during the device initialization.

We add devices do the topology before their initialization is 
successfully completed. If it fails, we remove the device again. Having 
devices in the topology that are not completely initialized yet seems to 
be the real issue. A cleaner solution would move 
kfd_topoglogy_add_device to the end of kgd2kfd_device_init, so that we 
only add a device to the topology after they are successfully and 
completely initialized. Not sure if there are any dependencies in the 
init sequence that would be broken by this, though.

Regards,
   Felix


>
> Change-Id: Id6cb2541af129826b7621ceaa8e06e638c7bb122
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_device.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 9d1b026..e7e24fe 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -603,6 +603,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>   	if (kfd->kfd2kgd->get_hive_id)
>   		kfd->hive_id = kfd->kfd2kgd->get_hive_id(kfd->kgd);
>   
> +	kfd->dqm = device_queue_manager_init(kfd);
> +	if (!kfd->dqm) {
> +		dev_err(kfd_device, "Error initializing queue manager\n");
> +		goto device_queue_manager_error;
> +	}
> +
>   	if (kfd_topology_add_device(kfd)) {
>   		dev_err(kfd_device, "Error adding device to topology\n");
>   		goto kfd_topology_add_device_error;
> @@ -613,12 +619,6 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>   		goto kfd_interrupt_error;
>   	}
>   
> -	kfd->dqm = device_queue_manager_init(kfd);
> -	if (!kfd->dqm) {
> -		dev_err(kfd_device, "Error initializing queue manager\n");
> -		goto device_queue_manager_error;
> -	}
> -
>   	if (kfd_iommu_device_init(kfd)) {
>   		dev_err(kfd_device, "Error initializing iommuv2\n");
>   		goto device_iommu_error;
> @@ -642,12 +642,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>   
>   kfd_resume_error:
>   device_iommu_error:
> -	device_queue_manager_uninit(kfd->dqm);
> -device_queue_manager_error:
>   	kfd_interrupt_exit(kfd);
>   kfd_interrupt_error:
>   	kfd_topology_remove_device(kfd);
>   kfd_topology_add_device_error:
> +	device_queue_manager_uninit(kfd->dqm);
> +device_queue_manager_error:
>   	kfd_doorbell_fini(kfd);
>   kfd_doorbell_error:
>   	kfd_gtt_sa_fini(kfd);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdkfd: Initialize dqm earlier

[Zeng, Oak]

Hi Felix,

Kfd_iommu_device_init depends on kfd topology. I will do as you suggested below. Also I will move the setting of HSA_CAP_ATS_PRESENT from kfd_iommu_device_init to kfd_topology_add_device, to avoid the dependency.

Regards,
Oak

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@amd.com> 
Sent: Thursday, June 6, 2019 6:09 PM
To: Zeng, Oak <Oak.Zeng@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdkfd: Initialize dqm earlier

On 2019-06-06 5:51 p.m., Zeng, Oak wrote:
> dqm is referenced in function kfd_toplogy_add_device.
> Move dqm initialization up to avoid NULL pointer reference.

This addresses a pretty unlikely race condition where someone looks at /sys/kernel/debug/kfd/hqds during the device initialization.

We add devices do the topology before their initialization is successfully completed. If it fails, we remove the device again. Having devices in the topology that are not completely initialized yet seems to be the real issue. A cleaner solution would move kfd_topoglogy_add_device to the end of kgd2kfd_device_init, so that we only add a device to the topology after they are successfully and completely initialized. Not sure if there are any dependencies in the init sequence that would be broken by this, though.

Regards,
   Felix


>
> Change-Id: Id6cb2541af129826b7621ceaa8e06e638c7bb122
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_device.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 9d1b026..e7e24fe 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -603,6 +603,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>   	if (kfd->kfd2kgd->get_hive_id)
>   		kfd->hive_id = kfd->kfd2kgd->get_hive_id(kfd->kgd);
>   
> +	kfd->dqm = device_queue_manager_init(kfd);
> +	if (!kfd->dqm) {
> +		dev_err(kfd_device, "Error initializing queue manager\n");
> +		goto device_queue_manager_error;
> +	}
> +
>   	if (kfd_topology_add_device(kfd)) {
>   		dev_err(kfd_device, "Error adding device to topology\n");
>   		goto kfd_topology_add_device_error; @@ -613,12 +619,6 @@ bool 
> kgd2kfd_device_init(struct kfd_dev *kfd,
>   		goto kfd_interrupt_error;
>   	}
>   
> -	kfd->dqm = device_queue_manager_init(kfd);
> -	if (!kfd->dqm) {
> -		dev_err(kfd_device, "Error initializing queue manager\n");
> -		goto device_queue_manager_error;
> -	}
> -
>   	if (kfd_iommu_device_init(kfd)) {
>   		dev_err(kfd_device, "Error initializing iommuv2\n");
>   		goto device_iommu_error;
> @@ -642,12 +642,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>   
>   kfd_resume_error:
>   device_iommu_error:
> -	device_queue_manager_uninit(kfd->dqm);
> -device_queue_manager_error:
>   	kfd_interrupt_exit(kfd);
>   kfd_interrupt_error:
>   	kfd_topology_remove_device(kfd);
>   kfd_topology_add_device_error:
> +	device_queue_manager_uninit(kfd->dqm);
> +device_queue_manager_error:
>   	kfd_doorbell_fini(kfd);
>   kfd_doorbell_error:
>   	kfd_gtt_sa_fini(kfd);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx