Re: [PATCH v3 1/5] drm/xe/hwmon: Detect unavailable temperature sensors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Repeated fallible PCODE mailbox reads during sysfs attribute visibility checks block driver probe and can cause flaky attribute registration.
- [Medium] Data race on `hwmon->temp.value` shared scratchpad buffer when `get_mc_temp()` is called lockless from sysfs callbacks.
--

commit daf20f08a8a78b5a377223b407b9b3a08568942e
Author: Karthik Poosa <karthik.poosa@intel.com>
Subject: drm/xe/hwmon: Detect unavailable temperature sensors

This commit introduces is_temp_available() to validate sensor presence during
sysfs attribute registration. It uses hwmon_temp_is_visible() to hide
attributes for sensors that report invalid values, avoiding exposing 
unavailable sensors to userspace.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5284cab6703d8..faed2f5da3947 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
[ ... ]
> -static inline bool is_vram_ch_available(struct xe_hwmon *hwmon, int channel)
> +static bool is_temp_available(struct xe_hwmon *hwmon, int channel)
>  {
>  	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
[ ... ]
> +	case CHANNEL_MCTRL:
> +		return hwmon->temp.count && !get_mc_temp(hwmon, NULL);
> +	case CHANNEL_PCIE:
> +		return hwmon->temp.count && !get_pcie_temp(hwmon, NULL);
> +	default:
> +		return false;
> +	}
>  }
>  
>  static umode_t
> @@ -973,14 +1019,13 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  	case hwmon_temp_emergency:
>  		switch (channel) {
>  		case CHANNEL_PKG:
> -			return hwmon->temp.limit[TEMP_LIMIT_PKG_SHUTDOWN] ? 0444 : 0;
> -		case CHANNEL_VRAM:
> -			return hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN] ? 0444 : 0;
>  		case CHANNEL_MCTRL:
>  		case CHANNEL_PCIE:
> -			return hwmon->temp.count ? 0444 : 0;
> +			return (is_temp_available(hwmon, channel) &&
> +				hwmon->temp.limit[TEMP_LIMIT_PKG_SHUTDOWN]) ? 0444 : 0;

[Severity: Medium]
Is it safe to execute lockless hardware I/O from the sysfs visibility callback?

The xe_hwmon_temp_is_visible callback executes get_mc_temp without holding
hwmon->hwmon_lock. Because the sysfs core creates attributes one by one, an
aggressive userspace process can open and read an early attribute as soon as
it appears. 

This could trigger a concurrent lockless execution of get_mc_temp from
xe_hwmon_temp_read while the sysfs core is still evaluating is_visible 
for subsequent attributes. Both threads would concurrently execute 
xe_pcode_read and perform unsynchronized writes to the shared 
hwmon->temp.value array. Could this lead to torn or corrupted temperature 
readings?

[Severity: Medium]
Could these repeated hardware reads cause flaky attribute registration and slow
down device probe?

During hwmon_device_register_with_info, the sysfs core sequentially evaluates
the is_visible callback for every attribute in a group. For CHANNEL_MCTRL and
CHANNEL_PCIE, this now unconditionally executes blocking hardware mailbox reads
(get_mc_temp and get_pcie_temp). 

If the PCU mailbox is busy and times out for a single attribute's check, that
specific attribute might be silently omitted while others in the same channel
are successfully created. Should the sensor availability be probed once and
cached during initialization instead of evaluating it dynamically for every
sysfs attribute?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910182756.638830-1-karthik.poosa@intel.com?part=1