[PATCH 7/9] drm/xe: Stop ignoring errors from xe_heci_gsc_init()

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Lucas De Marchi
Sent: Friday, February 14, 2025 1:23 PM
To: intel-xe at lists.freedesktop.org
Cc: De Marchi, Lucas <lucas.demarchi at intel.com>; Tauro, Riana <riana.tauro at intel.com>; Usyskin, Alexander <alexander.usyskin at intel.com>
Subject: [PATCH 7/9] drm/xe: Stop ignoring errors from xe_heci_gsc_init()
> 
> Do not ignore errors from xe_heci_gsc_init(). For example, it shouldn't
> be fine to report successfully entering survivability mode when there's
> no communication with gsc working. The driver should also not be
> half-initialized in the normal case neither.
> 
> Cc: Riana Tauro <riana.tauro at intel.com>
> Cc: Alexander Usyskin <alexander.usyskin at intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>

LGTM.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

> ---
>  drivers/gpu/drm/xe/xe_device.c             |  6 ++--
>  drivers/gpu/drm/xe/xe_heci_gsc.c           | 35 +++++++++-------------
>  drivers/gpu/drm/xe/xe_heci_gsc.h           |  3 +-
>  drivers/gpu/drm/xe/xe_survivability_mode.c |  5 ++--
>  4 files changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 782ad564d0ba4..4af4b526a2801 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -851,7 +851,9 @@ int xe_device_probe(struct xe_device *xe)
>  			return err;
>  	}
>  
> -	xe_heci_gsc_init(xe);
> +	err = xe_heci_gsc_init(xe);
> +	if (err)
> +		return err;
>  
>  	err = xe_oa_init(xe);
>  	if (err)
> @@ -903,8 +905,6 @@ void xe_device_remove(struct xe_device *xe)
>  	xe_display_unregister(xe);
>  
>  	drm_dev_unplug(&xe->drm);
> -
> -	xe_heci_gsc_fini(xe);
>  }
>  
>  void xe_device_shutdown(struct xe_device *xe)
> diff --git a/drivers/gpu/drm/xe/xe_heci_gsc.c b/drivers/gpu/drm/xe/xe_heci_gsc.c
> index 992ee47abcdb7..3ea325d3db99d 100644
> --- a/drivers/gpu/drm/xe/xe_heci_gsc.c
> +++ b/drivers/gpu/drm/xe/xe_heci_gsc.c
> @@ -89,12 +89,9 @@ static void heci_gsc_release_dev(struct device *dev)
>  	kfree(adev);
>  }
>  
> -void xe_heci_gsc_fini(struct xe_device *xe)
> +static void xe_heci_gsc_fini(void *arg)
>  {
> -	struct xe_heci_gsc *heci_gsc = &xe->heci_gsc;
> -
> -	if (!xe->info.has_heci_gscfi && !xe->info.has_heci_cscfi)
> -		return;
> +	struct xe_heci_gsc *heci_gsc = arg;
>  
>  	if (heci_gsc->adev) {
>  		struct auxiliary_device *aux_dev = &heci_gsc->adev->aux_dev;
> @@ -106,6 +103,7 @@ void xe_heci_gsc_fini(struct xe_device *xe)
>  
>  	if (heci_gsc->irq >= 0)
>  		irq_free_desc(heci_gsc->irq);
> +
>  	heci_gsc->irq = -1;
>  }
>  
> @@ -172,14 +170,14 @@ static int heci_gsc_add_device(struct xe_device *xe, const struct heci_gsc_def *
>  	return ret;
>  }
>  
> -void xe_heci_gsc_init(struct xe_device *xe)
> +int xe_heci_gsc_init(struct xe_device *xe)
>  {
>  	struct xe_heci_gsc *heci_gsc = &xe->heci_gsc;
>  	const struct heci_gsc_def *def;
>  	int ret;
>  
>  	if (!xe->info.has_heci_gscfi && !xe->info.has_heci_cscfi)
> -		return;
> +		return 0;
>  
>  	heci_gsc->irq = -1;
>  
> @@ -191,29 +189,24 @@ void xe_heci_gsc_init(struct xe_device *xe)
>  		def = &heci_gsc_def_dg2;
>  	} else if (xe->info.platform == XE_DG1) {
>  		def = &heci_gsc_def_dg1;
> -	} else {
> -		drm_warn_once(&xe->drm, "Unknown platform\n");
> -		return;
>  	}
>  
> -	if (!def->name) {
> -		drm_warn_once(&xe->drm, "HECI is not implemented!\n");
> -		return;
> +	if (!def || !def->name) {
> +		drm_warn(&xe->drm, "HECI is not implemented!\n");
> +		return 0;
>  	}
>  
> +	ret = devm_add_action_or_reset(xe->drm.dev, xe_heci_gsc_fini, heci_gsc);
> +	if (ret)
> +		return ret;
> +
>  	if (!def->use_polling && !xe_survivability_mode_is_enabled(xe)) {
>  		ret = heci_gsc_irq_setup(xe);
>  		if (ret)
> -			goto fail;
> +			return ret;
>  	}
>  
> -	ret = heci_gsc_add_device(xe, def);
> -	if (ret)
> -		goto fail;
> -
> -	return;
> -fail:
> -	xe_heci_gsc_fini(xe);
> +	return heci_gsc_add_device(xe, def);
>  }
>  
>  void xe_heci_gsc_irq_handler(struct xe_device *xe, u32 iir)
> diff --git a/drivers/gpu/drm/xe/xe_heci_gsc.h b/drivers/gpu/drm/xe/xe_heci_gsc.h
> index 48b3b18380453..745eb6783942d 100644
> --- a/drivers/gpu/drm/xe/xe_heci_gsc.h
> +++ b/drivers/gpu/drm/xe/xe_heci_gsc.h
> @@ -33,8 +33,7 @@ struct xe_heci_gsc {
>  	int irq;
>  };
>  
> -void xe_heci_gsc_init(struct xe_device *xe);
> -void xe_heci_gsc_fini(struct xe_device *xe);
> +int xe_heci_gsc_init(struct xe_device *xe);
>  void xe_heci_gsc_irq_handler(struct xe_device *xe, u32 iir);
>  void xe_heci_csc_irq_handler(struct xe_device *xe, u32 iir);
>  
> diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c b/drivers/gpu/drm/xe/xe_survivability_mode.c
> index 7ba02e085b5b1..d939ce70e6fa8 100644
> --- a/drivers/gpu/drm/xe/xe_survivability_mode.c
> +++ b/drivers/gpu/drm/xe/xe_survivability_mode.c
> @@ -134,7 +134,6 @@ static void xe_survivability_mode_fini(void *arg)
>  	struct device *dev = &pdev->dev;
>  
>  	sysfs_remove_file(&dev->kobj, &dev_attr_survivability_mode.attr);
> -	xe_heci_gsc_fini(xe);
>  }
>  
>  static int enable_survivability_mode(struct pci_dev *pdev)
> @@ -156,7 +155,9 @@ static int enable_survivability_mode(struct pci_dev *pdev)
>  	if (ret)
>  		return ret;
>  
> -	xe_heci_gsc_init(xe);
> +	ret = xe_heci_gsc_init(xe);
> +	if (ret)
> +		return ret;
>  
>  	xe_vsec_init(xe);
>  
> -- 
> 2.48.1
> 
>