[PATCH v3 13/13] drm/xe/hwmon: Stop ignoring errors on probe

[Nilawar, Badal]

Hi Lucas,

On 08-02-2025 03:49, Lucas De Marchi wrote:
> Not registering hwmon because it's not available (SRIOV_VF and DGFX) is
> different from failing the initialization. Handle the errors
> appropriately.
>
> Cc: Badal Nilawar <badal.nilawar at intel.com>
> Cc: Karthik Poosa <karthik.poosa at intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device.c |  4 +++-
>   drivers/gpu/drm/xe/xe_hwmon.c  | 31 ++++++++++++++++---------------
>   drivers/gpu/drm/xe/xe_hwmon.h  |  4 ++--
>   3 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 002e066f5f288..2e6f42d1b1d01 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -889,7 +889,9 @@ int xe_device_probe(struct xe_device *xe)
>   
>   	xe_debugfs_register(xe);
>   
> -	xe_hwmon_register(xe);
> +	err = xe_hwmon_register(xe);
> +	if (err)
> +		goto err_unregister_display;
>   
>   	for_each_gt(gt, xe, id)
>   		xe_gt_sanitize_freq(gt);
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 7f327e3342123..48d80ffdf7bb9 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -839,10 +839,9 @@ static const struct hwmon_chip_info hwmon_chip_info = {
>   };
>   
>   static void
> -xe_hwmon_get_preregistration_info(struct xe_device *xe)
> +xe_hwmon_get_preregistration_info(struct xe_hwmon *hwmon)
>   {
> -	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> -	struct xe_hwmon *hwmon = xe->hwmon;
> +	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>   	long energy;
>   	u64 val_sku_unit = 0;
>   	int channel;
> @@ -876,33 +875,34 @@ static void xe_hwmon_mutex_destroy(void *arg)
>   	mutex_destroy(&hwmon->hwmon_lock);
>   }
>   
> -void xe_hwmon_register(struct xe_device *xe)
> +int xe_hwmon_register(struct xe_device *xe)
>   {
>   	struct device *dev = xe->drm.dev;
>   	struct xe_hwmon *hwmon;
> +	int ret;
>   
>   	/* hwmon is available only for dGfx */
>   	if (!IS_DGFX(xe))
> -		return;
> +		return 0;
>   
>   	/* hwmon is not available on VFs */
>   	if (IS_SRIOV_VF(xe))
> -		return;
> +		return 0;
>   
>   	hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
>   	if (!hwmon)
> -		return;
> -
> -	xe->hwmon = hwmon;
> +		return -ENOMEM;
>   
>   	mutex_init(&hwmon->hwmon_lock);
> -	if (devm_add_action_or_reset(dev, xe_hwmon_mutex_destroy, hwmon))
> -		return;
> +	ret = devm_add_action_or_reset(dev, xe_hwmon_mutex_destroy, hwmon);
> +	if (ret)
> +		return ret;
>   
>   	/* There's only one instance of hwmon per device */
>   	hwmon->xe = xe;
> +	xe->hwmon = hwmon;
>   
> -	xe_hwmon_get_preregistration_info(xe);
> +	xe_hwmon_get_preregistration_info(hwmon);
>   
>   	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>   
> @@ -910,11 +910,12 @@ void xe_hwmon_register(struct xe_device *xe)
>   	hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev, "xe", hwmon,
>   								&hwmon_chip_info,
>   								hwmon_groups);
> -
>   	if (IS_ERR(hwmon->hwmon_dev)) {
> -		drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
> +		drm_err(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
>   		xe->hwmon = NULL;
> -		return;
> +		return PTR_ERR(hwmon->hwmon_dev);

Hwmon is not part of drm subsystem so we didn't tie hwmon registration 
failure with xe probe.

Regards,
Badal

>   	}
> +
> +	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
> index c42a1de2cd7a2..d02c1bfe8c0a0 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.h
> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
> @@ -11,9 +11,9 @@
>   struct xe_device;
>   
>   #if IS_REACHABLE(CONFIG_HWMON)
> -void xe_hwmon_register(struct xe_device *xe);
> +int xe_hwmon_register(struct xe_device *xe);
>   #else
> -static inline void xe_hwmon_register(struct xe_device *xe) { };
> +static inline int xe_hwmon_register(struct xe_device *xe) { return 0; };
>   #endif
>   
>   #endif /* _XE_HWMON_H_ */