Re: [PATCH v2 11/14] drm/xe/oa: Handle errors in xe_oa_register()

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 06 Feb 2025 15:23:29 -0800, Lucas De Marchi wrote:
>
> Let xe_oa_unregister() be handled by devm infra since it's only putting
> the kobject. Also, since kobject_create_and_add may fail, handle the
> error accordingly.

LGTM,

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c | 14 ++++++++------
>  drivers/gpu/drm/xe/xe_oa.c     | 30 +++++++++++++++---------------
>  drivers/gpu/drm/xe/xe_oa.h     |  3 +--
>  3 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 5e4c50a70df35..6f90d74330524 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -871,15 +871,17 @@ int xe_device_probe(struct xe_device *xe)
>
>	err = xe_pxp_init(xe);
>	if (err)
> -		goto err_fini_display;
> +		goto err_remove_display;
>
>	err = drm_dev_register(&xe->drm, 0);
>	if (err)
> -		goto err_fini_display;
> +		goto err_remove_display;
>
>	xe_display_register(xe);
>
> -	xe_oa_register(xe);
> +	err = xe_oa_register(xe);
> +	if (err)
> +		goto err_unregister_display;
>
>	xe_pmu_register(&xe->pmu);
>
> @@ -896,7 +898,9 @@ int xe_device_probe(struct xe_device *xe)
>
>	return devm_add_action_or_reset(xe->drm.dev, xe_device_sanitize, xe);
>
> -err_fini_display:
> +err_unregister_display:
> +	xe_display_unregister(xe);
> +err_remove_display:
>	xe_display_driver_remove(xe);
>
>	return err;
> @@ -962,8 +966,6 @@ void xe_device_remove(struct xe_device *xe)
>
>	xe_display_driver_remove(xe);
>
> -	xe_oa_unregister(xe);
> -
>	xe_heci_gsc_fini(xe);
>
>	xe_device_call_remove_actions(xe);
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 2c640185bdeca..d89e6cabf5a56 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -2423,36 +2423,36 @@ int xe_oa_remove_config_ioctl(struct drm_device *dev, u64 data, struct drm_file
>	return ret;
>  }
>
> +static void xe_oa_unregister(void *arg)
> +{
> +	struct xe_oa *oa = arg;
> +
> +	if (!oa->metrics_kobj)
> +		return;
> +
> +	kobject_put(oa->metrics_kobj);
> +	oa->metrics_kobj = NULL;
> +}
> +
>  /**
>   * xe_oa_register - Xe OA registration
>   * @xe: @xe_device
>   *
>   * Exposes the metrics sysfs directory upon completion of module initialization
>   */
> -void xe_oa_register(struct xe_device *xe)
> +int xe_oa_register(struct xe_device *xe)
>  {
>	struct xe_oa *oa = &xe->oa;
>
>	if (!oa->xe)
> -		return;
> +		return 0;
>
>	oa->metrics_kobj = kobject_create_and_add("metrics",
>						  &xe->drm.primary->kdev->kobj);
> -}
> -
> -/**
> - * xe_oa_unregister - Xe OA de-registration
> - * @xe: @xe_device
> - */
> -void xe_oa_unregister(struct xe_device *xe)
> -{
> -	struct xe_oa *oa = &xe->oa;
> -
>	if (!oa->metrics_kobj)
> -		return;
> +		return -ENOMEM;
>
> -	kobject_put(oa->metrics_kobj);
> -	oa->metrics_kobj = NULL;
> +	return devm_add_action_or_reset(xe->drm.dev, xe_oa_unregister, oa);
>  }
>
>  static u32 num_oa_units_per_gt(struct xe_gt *gt)
> diff --git a/drivers/gpu/drm/xe/xe_oa.h b/drivers/gpu/drm/xe/xe_oa.h
> index eb36ce250c615..e510826f9efc6 100644
> --- a/drivers/gpu/drm/xe/xe_oa.h
> +++ b/drivers/gpu/drm/xe/xe_oa.h
> @@ -15,8 +15,7 @@ struct xe_gt;
>  struct xe_hw_engine;
>
>  int xe_oa_init(struct xe_device *xe);
> -void xe_oa_register(struct xe_device *xe);
> -void xe_oa_unregister(struct xe_device *xe);
> +int xe_oa_register(struct xe_device *xe);
>  int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *file);
>  int xe_oa_add_config_ioctl(struct drm_device *dev, u64 data, struct drm_file *file);
>  int xe_oa_remove_config_ioctl(struct drm_device *dev, u64 data, struct drm_file *file);
> --
> 2.48.1
>