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

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

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.

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

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