Re: [PATCH v2 1/1] drm/xe/gsc: Fix GSC proxy cleanup on early initialization failure

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2026-01-27 6:32 p.m., Daniele Ceraolo Spurio wrote:
> 
> 
> On 1/26/2026 6:43 PM, Zhanjun Dong wrote:
>> xe_gsc_proxy_remove undoes what is done in both xe_gsc_proxy_init and
>> xe_gsc_proxy_start; however, if we fail between those 2 calls, it is
>> possible that the HW forcewake access hasn't been initialized yet and so
>> we hit errors when the cleanup code tries to write GSC register. To
>> avoid that, split the cleanup in 2 functions so that the HW cleanup is
>> only called if the HW setup was completed successfully.
>>
>> Additionally, fix error handling in xe_gsc_proxy_start to properly
>> disable interrupts on failure paths before returning, ensuring cleanup
>> is performed correctly when xe_gsc_proxy_request_handler() or
>> xe_gsc_proxy_init_done() fails.
> 
> This second one is not a fix. In the current behavior, if 
> xe_gsc_proxy_start fails then xe_gsc_proxy_remove is called and the 
> cleanup is performed correctly. Since you're removing that part of the 
> cleanup from xe_gsc_proxy_remove then this new change to 
> xe_gsc_proxy_start becomes necessary.
Yes, to be updated in next rev.
> 
>>
>> Fixes: ff6cd29b690b ("drm/xe: Cleanup unwind of gt initialization")
>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>> v2:
>> - Split cleanup into two functions: xe_gsc_proxy_remove() for SW cleanup
>>    and xe_gsc_proxy_stop() for HW cleanup that requires forcewake access.
>> - Add error handling in xe_gsc_proxy_start to disable interrupts on
>>    early error exits.
>> ---
>>   drivers/gpu/drm/xe/xe_gsc_proxy.c | 42 ++++++++++++++++++++-----------
>>   1 file changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.c b/drivers/gpu/drm/xe/ 
>> xe_gsc_proxy.c
>> index 42438b21f235..0f62ee7dab4a 100644
>> --- a/drivers/gpu/drm/xe/xe_gsc_proxy.c
>> +++ b/drivers/gpu/drm/xe/xe_gsc_proxy.c
>> @@ -444,16 +444,6 @@ static void xe_gsc_proxy_remove(void *arg)
>>       if (!gsc->proxy.component_added)
>>           return;
>> -    /* disable HECI2 IRQs */
>> -    scoped_guard(xe_pm_runtime, xe) {
>> -        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GSC);
>> -        if (!fw_ref.domains)
>> -            xe_gt_err(gt, "failed to get forcewake to disable GSC 
>> interrupts\n");
>> -
>> -        /* try do disable irq even if forcewake failed */
>> -        gsc_proxy_irq_toggle(gsc, false);
>> -    }
>> -
>>       xe_gsc_wait_for_worker_completion(gsc);
> 
> in v1 you had xe_gsc_wait_for_worker_completion moved to 
> xe_gsc_proxy_stop(). I think that works better, since the worker 
> shouldn't be queued anymore after we disable interrupts.
Sure, will follow this in next rev.

Regards,
Zhanjun Dong
> 
> Daniele
> 
>>       component_del(xe->drm.dev, &xe_gsc_proxy_component_ops);
>> @@ -502,6 +492,23 @@ int xe_gsc_proxy_init(struct xe_gsc *gsc)
>>       return devm_add_action_or_reset(xe->drm.dev, 
>> xe_gsc_proxy_remove, gsc);
>>   }
>> +static void xe_gsc_proxy_stop(void *arg)
>> +{
>> +    struct xe_gsc *gsc = arg;
>> +    struct xe_gt *gt = gsc_to_gt(gsc);
>> +    struct xe_device *xe = gt_to_xe(gt);
>> +
>> +    /* disable HECI2 IRQs */
>> +    scoped_guard(xe_pm_runtime, xe) {
>> +        CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GSC);
>> +        if (!fw_ref.domains)
>> +            xe_gt_err(gt, "failed to get forcewake to disable GSC 
>> interrupts\n");
>> +
>> +        /* try do disable irq even if forcewake failed */
>> +        gsc_proxy_irq_toggle(gsc, false);
>> +    }
>> +}
>> +
>>   /**
>>    * xe_gsc_proxy_start() - start the proxy by submitting the first 
>> request
>>    * @gsc: the GSC uC
>> @@ -510,6 +517,8 @@ int xe_gsc_proxy_init(struct xe_gsc *gsc)
>>    */
>>   int xe_gsc_proxy_start(struct xe_gsc *gsc)
>>   {
>> +    struct xe_gt *gt = gsc_to_gt(gsc);
>> +    struct xe_device *xe = gt_to_xe(gt);
>>       int err;
>>       /* enable the proxy interrupt in the GSC shim layer */
>> @@ -521,12 +530,17 @@ int xe_gsc_proxy_start(struct xe_gsc *gsc)
>>        */
>>       err = xe_gsc_proxy_request_handler(gsc);
>>       if (err)
>> -        return err;
>> +        goto err_irq_disable;
>>       if (!xe_gsc_proxy_init_done(gsc)) {
>> -        xe_gt_err(gsc_to_gt(gsc), "GSC FW reports proxy init not 
>> completed\n");
>> -        return -EIO;
>> +        xe_gt_err(gt, "GSC FW reports proxy init not completed\n");
>> +        err = -EIO;
>> +        goto err_irq_disable;
>>       }
>> -    return 0;
>> +    return devm_add_action_or_reset(xe->drm.dev, xe_gsc_proxy_stop, 
>> gsc);
>> +
>> +err_irq_disable:
>> +    gsc_proxy_irq_toggle(gsc, false);
>> +    return err;
>>   }
>