Re: [PATCH 1/3] 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:
- [High] Incorrect bitmask in `is_vram_ch_available()` evaluates the wrong bits, improperly hiding valid VRAM sensors.
- [Medium] Incorrect short-circuit logic in `is_visible` causes unconditional hardware polling, leading to permanently hidden attributes on transient errors and excessive `drm_warn` log spam.
--

--- Patch [1]: [PATCH 1/3] drm/xe/hwmon: Detect unavailable temperature sensors ---
commit 0fa4d58443299c1d66e99a6115caeb31a5bd8c21
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Detect unavailable temperature sensors

Add is_temp_valid() to validate sensor presence.
A temperature reading of 0xFF on CRI platforms indicates that the
corresponding sensor is not present and should be treated as unavailable.
Use this check from xe_hwmon_temp_is_visible() callback so that attributes
for unavailable sensors are not exposed during hwmon device registration.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index de3f2aeffc3fb..c15312b0858ca 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c

[ ... ]

> @@ -932,11 +961,21 @@ static inline bool is_vram_ch_available(struct xe_hwmon *hwmon, int channel)
>  	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>  	int vram_id = channel - CHANNEL_VRAM_N;
>  	struct xe_reg vram_reg;
> +	u32 reg_val;
> +	u8 temp;
>  
>  	vram_reg = xe_hwmon_get_reg(hwmon, REG_TEMP, channel);
> -	if (!xe_reg_is_valid(vram_reg) || !xe_mmio_read32(mmio, vram_reg))
> +	if (!xe_reg_is_valid(vram_reg))
>  		return false;
>  
> +	reg_val = xe_mmio_read32(mmio, vram_reg);
> +	temp = REG_FIELD_GET(TEMP_MASK, reg_val);
> +	if (!is_temp_valid(hwmon, temp)) {

[Severity: High]
Does this code extract the correct bits for CHANNEL_VRAM_N?

For CHANNEL_VRAM_N, the hardware register stores the temperature in bits 30:8
(TEMP_MASK_VRAM_N). However, is_vram_ch_available() extracts the value using
TEMP_MASK (bits 7:0).

By evaluating unrelated bits against the sentinel value (0 or 0xFF), can this
cause valid VRAM sensors to be improperly hidden if the lower bits happen to
match the sentinel?

> +		drm_dbg(&hwmon->xe->drm, "vram channel %d unavailable, val 0x%x\n", vram_id,
> +			reg_val);
> +		return false;
> +	}
> +
>  	/* Create label only for available vram channel */
>  	sprintf(hwmon->temp.vram_label[vram_id], "vram_ch_%d", vram_id);
>  	return true;

[ ... ]

> @@ -953,8 +992,9 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  		case CHANNEL_VRAM:
>  			return hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN] ? 0444 : 0;
>  		case CHANNEL_MCTRL:
> +			return !get_mc_temp(hwmon, NULL) && hwmon->temp.count ? 0444 : 0;
>  		case CHANNEL_PCIE:
> -			return hwmon->temp.count ? 0444 : 0;
> +			return !get_pcie_temp(hwmon, NULL) && hwmon->temp.count ? 0444 : 0;

[Severity: Medium]
Does this reversed short-circuit logic cause unconditional hardware polling and
log spam?

Since !get_mc_temp() and !get_pcie_temp() are evaluated before checking
hwmon->temp.count, they will trigger PCODE reads even when the count is 0.

Furthermore, xe_hwmon_temp_is_visible() executes up to 5 times per channel
during hwmon registration. If a sensor is legitimately missing, the new
drm_warn() statements in get_mc_temp() and get_pcie_temp() will repeatedly
spam the kernel log with warnings for expected missing hardware.

Could this early polling also cause valid attributes to be permanently hidden
if a transient PCODE timeout occurs during registration?

>  		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>  			return (is_vram_ch_available(hwmon, channel) &&
>  				hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN]) ? 0444 : 0;

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