AMD General
> -----Original Message-----
> From: amd-gfx <[email protected]> On Behalf Of Joshi,
> Mukul
> Sent: Wednesday, September 9, 2026 1:14 PM
> To: Lazar, Lijo <[email protected]>; [email protected]
> Cc: Zhang, Hawking <[email protected]>; Deucher, Alexander
> <[email protected]>; Kuehling, Felix <[email protected]>;
> Liu, Xiang(Dean) <[email protected]>
> Subject: RE: [PATCH 1/2] drm/amdgpu: Fix kfd device lock during partition
>
> AMD General
>
> > -----Original Message-----
> > From: Lazar, Lijo <[email protected]>
> > Sent: Wednesday, September 9, 2026 3:51 AM
> > To: [email protected]
> > Cc: Zhang, Hawking <[email protected]>; Deucher, Alexander
> > <[email protected]>; Kuehling, Felix
> <[email protected]>;
> > Joshi, Mukul <[email protected]>; Liu, Xiang(Dean)
> > <[email protected]>
> > Subject: [PATCH 1/2] drm/amdgpu: Fix kfd device lock during partition
> >
> > The compute partition switch locked the kfd device, freed it during
> > the pre- switch teardown, and then unlocked at the end - by which
> > point
> > adev->kfd.dev referenced the newly reinitialized device, so the unlock
> > landed on the wrong object and the per-device lock was never balanced.
> >
> > Add a dedicated prepare function to unwind and clean up kfd device.
> > Don't use the global lock during teardown. A new device is created
> > after partition switch; hence there is no need to do 'unlock' on a
> > different object after the switch.
> >
> > User is not supposed to run any process on the device during partition
> > switch. Keep acquire/release semantics for init_complete so that the
> > device remains unavailable for a process till its reinitialization is
> > complete.
> >
> > Signed-off-by: Lijo Lazar <[email protected]>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 19
> > +++++++++++++++++++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 6 ++++++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 6 ++++--
> > drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c | 11 +----------
> > drivers/gpu/drm/amd/amdgpu/soc_v1_0.c | 11 +----------
> > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 10 ++++++++--
> > drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c | 6 ++++++
> > 7 files changed, 45 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > index 09feb45584ba..a2a368f1e290 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > @@ -245,6 +245,25 @@ void amdgpu_amdkfd_device_fini_sw(struct
> > amdgpu_device *adev)
> > }
> > }
> >
> > +int amdgpu_amdkfd_prepare_partition_switch(struct amdgpu_device
> > *adev)
> > +{
> > + struct kfd_dev *kfd = adev->kfd.dev;
> > + int r;
> > +
> > + if (!kfd)
> > + return 0;
> > + r = kgd2kfd_check_and_lock_kfd(kfd);
> > + if (r)
> > + return r;
> > +
> > + adev->kfd.init_complete = false;
> > + kgd2kfd_device_fini(kfd);
> > + adev->kfd.dev = NULL;
> > + amdgpu_amdkfd_total_mem_size -= adev->gmc.real_vram_size;
> > +
> > + return 0;
> > +}
> > +
> > void amdgpu_amdkfd_interrupt(struct amdgpu_device *adev,
> > const void *ih_ring_entry) { diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > index 1b7dc0d3963b..36e222f3ac5b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > @@ -170,6 +170,7 @@ void amdgpu_amdkfd_interrupt(struct
> amdgpu_device
> > *adev, void amdgpu_amdkfd_device_probe(struct amdgpu_device *adev);
> > void amdgpu_amdkfd_device_init(struct amdgpu_device *adev); void
> > amdgpu_amdkfd_device_fini_sw(struct
> > amdgpu_device *adev);
> > +int amdgpu_amdkfd_prepare_partition_switch(struct amdgpu_device
> > *adev);
> > int amdgpu_amdkfd_check_and_lock_kfd(struct amdgpu_device *adev);
> > void amdgpu_amdkfd_unlock_kfd(struct amdgpu_device *adev); int
> > amdgpu_amdkfd_submit_ib(struct amdgpu_device *adev, @@ -431,6
> +432,7
> > @@ struct kfd_dev *kgd2kfd_probe(struct amdgpu_device *adev, bool vf);
> > bool kgd2kfd_device_init(struct kfd_dev *kfd,
> > const struct kgd2kfd_shared_resources
> > *gpu_resources); void kgd2kfd_device_exit(struct kfd_dev *kfd);
> > +void kgd2kfd_device_fini(struct kfd_dev *kfd);
> > void kgd2kfd_suspend(struct kfd_dev *kfd, bool suspend_proc); int
> > kgd2kfd_resume(struct kfd_dev *kfd, bool resume_proc); void
> > kgd2kfd_suspend_process(struct kfd_dev *kfd); @@ -485,6 +487,10 @@
> > static inline void kgd2kfd_device_exit(struct kfd_dev *kfd) { }
> >
> > +static inline void kgd2kfd_device_fini(struct kfd_dev *kfd) { }
> > +
>
> Maybe follow the pattern of having open and close branch on new lines as
> done for the other functions.
>
Sorry I meant open and close brace here.
Regards,
Mukul
> > static inline void kgd2kfd_suspend(struct kfd_dev *kfd, bool
> > suspend_proc) { } diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> > index a8c79a66df1d..92fa15ace438 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> > @@ -694,14 +694,16 @@ void
> > amdgpu_xcp_update_supported_modes(struct amdgpu_xcp_mgr
> *xcp_mgr)
> >
> > int amdgpu_xcp_pre_partition_switch(struct amdgpu_xcp_mgr *xcp_mgr,
> > u32 flags) {
> > + int ret = 0;
> > +
> > /* TODO:
> > * Stop user queues and threads, and make sure GPU is empty of
> > work.
> > */
> >
> > if (flags & AMDGPU_XCP_OPS_KFD)
> > - amdgpu_amdkfd_device_fini_sw(xcp_mgr->adev);
> > + ret = amdgpu_amdkfd_prepare_partition_switch(xcp_mgr-
> > >adev);
> >
> > - return 0;
> > + return ret;
> > }
> >
> > int amdgpu_xcp_post_partition_switch(struct amdgpu_xcp_mgr *xcp_mgr,
> > u32 flags) diff --git a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
> > b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
> > index cddfe4015f53..699e797679ec 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
> > @@ -391,15 +391,9 @@ static int
> > aqua_vanjaram_switch_partition_mode(struct amdgpu_xcp_mgr *xcp_mgr,
> > !adev->in_suspend)
> > flags |= AMDGPU_XCP_OPS_KFD;
> >
> > - if (flags & AMDGPU_XCP_OPS_KFD) {
> > - ret = amdgpu_amdkfd_check_and_lock_kfd(adev);
> > - if (ret)
> > - goto out;
> > - }
> > -
> > ret = amdgpu_xcp_pre_partition_switch(xcp_mgr, flags);
> > if (ret)
> > - goto unlock;
> > + goto out;
> >
> > num_xcc_per_xcp = __aqua_vanjaram_get_xcc_per_xcp(xcp_mgr,
> > mode);
> > if (adev->gfx.funcs->switch_partition_mode)
> > @@ -413,9 +407,6 @@ static int
> > aqua_vanjaram_switch_partition_mode(struct amdgpu_xcp_mgr *xcp_mgr,
> > ret = amdgpu_xcp_post_partition_switch(xcp_mgr, flags);
> > if (!ret)
> >
> > __aqua_vanjaram_update_available_partition_mode(xcp_mgr);
> > -unlock:
> > - if (flags & AMDGPU_XCP_OPS_KFD)
> > - amdgpu_amdkfd_unlock_kfd(adev);
> > out:
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
> > b/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
> > index 757da0a30fdc..28eca48c6596 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/soc_v1_0.c
> > @@ -1097,15 +1097,9 @@ static int
> > soc_v1_0_switch_partition_mode(struct
> > amdgpu_xcp_mgr *xcp_mgr,
> > if (adev->kfd.init_complete && !amdgpu_in_reset(adev))
> > flags |= AMDGPU_XCP_OPS_KFD;
> >
> > - if (flags & AMDGPU_XCP_OPS_KFD) {
> > - ret = amdgpu_amdkfd_check_and_lock_kfd(adev);
> > - if (ret)
> > - goto out;
> > - }
> > -
> > ret = amdgpu_xcp_pre_partition_switch(xcp_mgr, flags);
> > if (ret)
> > - goto unlock;
> > + goto out;
> >
> > num_xcc_per_xcp = __soc_v1_0_get_xcc_per_xcp(xcp_mgr, mode);
> > if (adev->gfx.imu.funcs &&
> > @@ -1126,9 +1120,6 @@ static int soc_v1_0_switch_partition_mode(struct
> > amdgpu_xcp_mgr *xcp_mgr,
> > ret = amdgpu_xcp_post_partition_switch(xcp_mgr, flags);
> > if (!ret)
> > __soc_v1_0_update_available_partition_mode(xcp_mgr);
> > -unlock:
> > - if (flags & AMDGPU_XCP_OPS_KFD)
> > - amdgpu_amdkfd_unlock_kfd(adev);
> > out:
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > index 1c1e7878a2a7..76b308286095 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > @@ -952,7 +952,8 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
> >
> > svm_range_set_max_pages(kfd->adev);
> >
> > - kfd->init_complete = true;
> > + /* Release pairs with the acquire in kfd_init_apertures(). */
> > + smp_store_release(&kfd->init_complete, true);
> > dev_info(kfd_device, "added device %x:%x\n", kfd->adev->pdev-
> > >vendor,
> > kfd->adev->pdev->device);
> >
> > @@ -977,7 +978,7 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
> > return kfd->init_complete;
> > }
> >
> > -void kgd2kfd_device_exit(struct kfd_dev *kfd)
> > +void kgd2kfd_device_fini(struct kfd_dev *kfd)
> > {
> > if (kfd->init_complete) {
> > /* Cleanup KFD nodes */
> > @@ -991,6 +992,11 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
> > }
> >
> > kfree(kfd);
> > +}
> > +
> > +void kgd2kfd_device_exit(struct kfd_dev *kfd) {
>
> Open brace should be on the next line.
> Maybe run it through checkpatch once if not already done.
>
> > + kgd2kfd_device_fini(kfd);
> >
> > /* after remove a kfd device unlock kfd driver */
> > kgd2kfd_unlock_kfd(NULL);
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
> > index 04c5e26f01ed..da61d939b35c 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
> > @@ -396,6 +396,12 @@ int kfd_init_apertures(struct kfd_process *process)
> > continue;
> > }
> >
> > + /* acquire pairs with the release in kgd2kfd_device_init() */
> > + if (!smp_load_acquire(&dev->kfd->init_complete)) {
>
> Do we need similar smp_load_acquire() at other call sites of this kfd-
> >init_complete?
>
> With the minor nit-picks above fixed, this series is:
> Reviewed-by: Mukul Joshi <[email protected]>
>
> > + id++;
> > + continue;
> > + }
> > +
> > pdd = kfd_create_process_device_data(dev, process);
> > if (!pdd) {
> > dev_err(dev->adev->dev,
> > --
> > 2.49.0
