Re: [PATCH 03/12] drm/xe/hwmon: Report errors using structured xe_log API

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `xe_log_err()` API is invoked with a hardcoded `-EIO` error value instead of the actual error code, corrupting structured telemetry.
- [Low] Missing newline character in the format string for `xe_log_err()` will cause malformed dmesg output.
--

--- Patch [3]: [PATCH 03/12] drm/xe/hwmon: Report errors using structured xe_log API ---
commit d3f9f82c222b6fd15cd76563436ef83288bc6453
Author: Dnyaneshwar Bhadane <[email protected]>

drm/xe/hwmon: Report errors using structured xe_log API
    
Replace drm_err() calls with xe_log_err() using the HWMON component
tag to enable structured error reporting with SIGID for hardware
monitor failures:

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5284cab6703d8..9ff351e87e5a2 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
[ ... ]
> @@ -563,9 +564,9 @@ xe_hwmon_power_max_interval_show(struct device *dev, struct device_attribute *at
>  	if (hwmon->xe->info.has_mbx_power_limits) {
>  		ret = xe_hwmon_pcode_read_power_limit(hwmon, power_attr, channel, &reg_val);
>  		if (ret) {
> -			drm_err(&hwmon->xe->drm,
> -				"power interval read fail, ch %d, attr %d, val 0x%08x, ret %d\n",
> -				channel, power_attr, reg_val, ret);
> +			xe_log_err(hwmon->xe, HWMON, -EIO,
> +				   "power interval read fail, ch %d, attr %d, val 0x%08x, ret %d\n",
> +				   channel, power_attr, reg_val, ret);

[Severity: Medium]
In xe_hwmon_power_max_interval_show(), should we be passing the actual ret
error code instead of hardcoding -EIO here? By passing -EIO, the structured
error payload will log an incorrect telemetry value instead of the actual
failure code from xe_hwmon_pcode_read_power_limit() when it fails.

>  			reg_val = 0;
>  		}
>  	} else {
[ ... ]
> @@ -1599,7 +1600,8 @@ int xe_hwmon_register(struct xe_device *xe)
>  								&hwmon_chip_info,
>  								hwmon_groups);
>  	if (IS_ERR(hwmon->hwmon_dev)) {
> -		drm_err(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
> +		xe_log_err(xe, HWMON, -EIO,
> +			   "Failed to register xe hwmon (%pe)", hwmon->hwmon_dev);

[Severity: Medium]
In xe_hwmon_register(), should this use PTR_ERR(hwmon->hwmon_dev) instead
of hardcoded -EIO to ensure the correct error is captured in telemetry when
devm_hwmon_device_register_with_info() fails?

[Severity: Low]
It looks like the \n terminator was accidentally dropped from the format
string during the conversion in xe_hwmon_register(). Since xe_log_err()
maps to dev_printk(), will this missing newline cause subsequent kernel
messages to be appended to the same line when hwmon registration fails?

>  		xe->hwmon = NULL;
>  		return PTR_ERR(hwmon->hwmon_dev);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3