Re: [PATCH] drm/i915/huc: Fix fence not released on early probe errors

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 4/8/2025 7:07 AM, Janusz Krzysztofik wrote:
> Hi Daniele,
>
> On Thursday, 3 April 2025 11:12:12 CEST Daniele Ceraolo Spurio wrote:
>> On 4/2/2025 10:20 AM, Janusz Krzysztofik wrote:
>>> HuC delayed loading fence, introduced with commit 27536e03271da
>>> ("drm/i915/huc: track delayed HuC load with a fence"), is registered with
>>> object tracker early on driver probe but unregistered only from driver
>>> remove, which is not called on early probe errors.  Since its memory is
>>> allocated under devres, then released anyway, it may happen to be
>>> allocated again to the fence and reused on future driver probes, resulting
>>> in kernel warnings that taint the kernel:
>>>
>>> <4> [309.731371] ------------[ cut here ]------------
>>> <3> [309.731373] ODEBUG: init destroyed (active state 0) object: ffff88813d7dd2e0 object type: i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x20 [i915]
>>> <4> [309.731575] WARNING: CPU: 2 PID: 3161 at lib/debugobjects.c:612 debug_print_object+0x93/0xf0
>>> ...
>>> <4> [309.731693] CPU: 2 UID: 0 PID: 3161 Comm: i915_module_loa Tainted: G     U             6.14.0-CI_DRM_16362-gf0fd77956987+ #1
>>> ...
>>> <4> [309.731700] RIP: 0010:debug_print_object+0x93/0xf0
>>> ...
>>> <4> [309.731728] Call Trace:
>>> <4> [309.731730]  <TASK>
>>> ...
>>> <4> [309.731949]  __debug_object_init+0x17b/0x1c0
>>> <4> [309.731957]  debug_object_init+0x34/0x50
>>> <4> [309.732126]  __i915_sw_fence_init+0x34/0x60 [i915]
>>> <4> [309.732256]  intel_huc_init_early+0x4b/0x1d0 [i915]
>>> <4> [309.732468]  intel_uc_init_early+0x61/0x680 [i915]
>>> <4> [309.732667]  intel_gt_common_init_early+0x105/0x130 [i915]
>>> <4> [309.732804]  intel_root_gt_init_early+0x63/0x80 [i915]
>>> <4> [309.732938]  i915_driver_probe+0x1fa/0xeb0 [i915]
>>> <4> [309.733075]  i915_pci_probe+0xe6/0x220 [i915]
>>> <4> [309.733198]  local_pci_probe+0x44/0xb0
>>> <4> [309.733203]  pci_device_probe+0xf4/0x270
>>> <4> [309.733209]  really_probe+0xee/0x3c0
>>> <4> [309.733215]  __driver_probe_device+0x8c/0x180
>>> <4> [309.733219]  driver_probe_device+0x24/0xd0
>>> <4> [309.733223]  __driver_attach+0x10f/0x220
>>> <4> [309.733230]  bus_for_each_dev+0x7d/0xe0
>>> <4> [309.733236]  driver_attach+0x1e/0x30
>>> <4> [309.733239]  bus_add_driver+0x151/0x290
>>> <4> [309.733244]  driver_register+0x5e/0x130
>>> <4> [309.733247]  __pci_register_driver+0x7d/0x90
>>> <4> [309.733251]  i915_pci_register_driver+0x23/0x30 [i915]
>>> <4> [309.733413]  i915_init+0x34/0x120 [i915]
>>> <4> [309.733655]  do_one_initcall+0x62/0x3f0
>>> <4> [309.733667]  do_init_module+0x97/0x2a0
>>> <4> [309.733671]  load_module+0x25ff/0x2890
>>> <4> [309.733688]  init_module_from_file+0x97/0xe0
>>> <4> [309.733701]  idempotent_init_module+0x118/0x330
>>> <4> [309.733711]  __x64_sys_finit_module+0x77/0x100
>>> <4> [309.733715]  x64_sys_call+0x1f37/0x2650
>>> <4> [309.733719]  do_syscall_64+0x91/0x180
>>> <4> [309.733763]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>> <4> [309.733792]  </TASK>
>>> ...
>>> <4> [309.733806] ---[ end trace 0000000000000000 ]---
>>>
>>> That scenario is most easily reproducible with
>>> igt@i915_module_load@reload-with-fault-injection.
>>>
>>> Fix the issue by moving the cleanup step to driver release path.
>>>
>>> Fixes: 27536e03271da ("drm/i915/huc: track delayed HuC load with a fence")
>>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13592
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> May I ask you to push this change?

Sure, I've pushed the patch to drm-intel-gt-next.

Daniele

>
> Thanks,
> Janusz
>
>
>> I'm kind of surprised that this issue only showed up in the last few
>> months, given that the code has been there for 2+ years. Did we change
>> something that caused this to show up? I had a quick look at the patches
>> merged around the time this issue was first reported but I didn't spot
>> anything that could impact this.
>>
>> Daniele
>>
>>> ---
>>>    drivers/gpu/drm/i915/gt/uc/intel_huc.c | 11 +++++------
>>>    drivers/gpu/drm/i915/gt/uc/intel_huc.h |  1 +
>>>    drivers/gpu/drm/i915/gt/uc/intel_uc.c  |  1 +
>>>    3 files changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> index f30c90650b7ec..9659e6a301e19 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> @@ -317,6 +317,11 @@ void intel_huc_init_early(struct intel_huc *huc)
>>>    	}
>>>    }
>>>    
>>> +void intel_huc_fini_late(struct intel_huc *huc)
>>> +{
>>> +	delayed_huc_load_fini(huc);
>>> +}
>>> +
>>>    #define HUC_LOAD_MODE_STRING(x) (x ? "GSC" : "legacy")
>>>    static int check_huc_loading_mode(struct intel_huc *huc)
>>>    {
>>> @@ -414,12 +419,6 @@ int intel_huc_init(struct intel_huc *huc)
>>>    
>>>    void intel_huc_fini(struct intel_huc *huc)
>>>    {
>>> -	/*
>>> -	 * the fence is initialized in init_early, so we need to clean it up
>>> -	 * even if HuC loading is off.
>>> -	 */
>>> -	delayed_huc_load_fini(huc);
>>> -
>>>    	if (huc->heci_pkt)
>>>    		i915_vma_unpin_and_release(&huc->heci_pkt, 0);
>>>    
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>> index d5e441b9e08d6..921ad4b1687f0 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>> @@ -55,6 +55,7 @@ struct intel_huc {
>>>    
>>>    int intel_huc_sanitize(struct intel_huc *huc);
>>>    void intel_huc_init_early(struct intel_huc *huc);
>>> +void intel_huc_fini_late(struct intel_huc *huc);
>>>    int intel_huc_init(struct intel_huc *huc);
>>>    void intel_huc_fini(struct intel_huc *huc);
>>>    int intel_huc_auth(struct intel_huc *huc, enum intel_huc_authentication_type type);
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index 90ba1b0b4c9d2..4a3493e8d4333 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -136,6 +136,7 @@ void intel_uc_init_late(struct intel_uc *uc)
>>>    
>>>    void intel_uc_driver_late_release(struct intel_uc *uc)
>>>    {
>>> +	intel_huc_fini_late(&uc->huc);
>>>    }
>>>    
>>>    /**
>>
>
>
>