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

["Poosa, Karthik" <karthik.poosa@intel.com>]

On 11-09-2026 00:12, sashiko-bot@kernel.org wrote:
> 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?
Yes, that could be an issue for get_mc_temp(). It uses hwmon->temp.value 
as shared scratch storage and can be reached from both 
xe_hwmon_temp_is_visible() and xe_hwmon_temp_read(), so those paths 
could overlap and observe a mixed snapshot.

xe_pcode_read() already serializes the mailbox transaction itself, but 
it does not protect the full get_mc_temp() sequence or the shared buffer.

We can address this by taking hwmon->hwmon_lock inside get_mc_temp().

>
> [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?
Agree, for this we probe the temperature sensor availability once during 
registration and cache its state and use is it visible to avoid slow 
device probe.
>