Re: [PATCH v5 09/13] drm/xe/hwmon: Add platform-aware VRAM thermal channel support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hwmon: VRAM temperature array overlaps with CRI control registers
- [Medium] hwmon: VRAM temperature channels truncated for sparse MSU masks
--

commit e9469679c20c906facf743cdf68972d046f4524d
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Add platform-aware VRAM thermal channel support

On CRI, derive the number of available VRAM temperature channels from the
CRI_MSU_VRAM_ENABLE register, where each enabled MSU contributes
VRAM_CHANNELS_PER_MSU channels.
 
Increase the maximum supported VRAM channel count to 80 and use the
derived count to control VRAM sensor enumeration, channel access, and
VRAM label allocation. Add a has_fixed_vram_channels platform flag to
preserve the existing fixed-channel behavior on non-CRI platforms.

> diff --git a/drivers/gpu/drm/xe/regs/xe_pcode_regs.h b/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> index c63b409d7a821..11916b4f7cfc0 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> @@ -29,5 +29,6 @@
>  
>  #define CRI_PACKAGE_ENERGY_STATUS		XE_REG(0x138120)
>  #define CRI_PLATFORM_ENERGY_STATUS		XE_REG(0x138458)
> +#define CRI_MSU_VRAM_ENABLE             XE_REG(0x138340)
>  
>  #endif /* _XE_PCODE_REGS_H_ */
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index cf5d51c685360..1b587bd9487e1 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -39,7 +39,11 @@ enum xe_hwmon_reg_operation {
>  	REG_READ64,
>  };
>  
> -#define MAX_VRAM_CHANNELS      (16)
> +#define MAX_VRAM_CHANNELS	80

[Severity: High]
Does the VRAM temperature array overlap with the newly added CRI control
registers?

The BMG_VRAM_TEMPERATURE_N(n) macro calculates the register address as
0x138260 + (n * 4). By increasing MAX_VRAM_CHANNELS to 80, this expands the
temperature array significantly.

For channel 56, the address evaluates to 0x138260 + 0xE0 = 0x138340, which
overlaps with the newly added CRI_MSU_VRAM_ENABLE. Channel 57 overlaps with
CRI_PACKAGE_TEMPERATURE (0x138344), and channel 58 overlaps with
CRI_VRAM_TEMPERATURE (0x138348).

Reading VRAM temperature channels >= 56 will read unrelated control or
status registers. This regression can lead to bogus temperature reporting or
potential hardware side-effects on platforms that enable them.

> +#define FIXED_VRAM_CHANNELS	16
> +
> +/* Each MSU enable bit maps to one VRAM subsystem of 4 channels. */
> +#define VRAM_CHANNELS_PER_MSU	4
>  
>  enum xe_hwmon_channel {
[ ... ]
> @@ -805,15 +887,38 @@ static const struct hwmon_channel_info * const hwmon_info[] = {
>  static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
>  {
>  	struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
> +	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>  	u32 config = 0;
>  	int ret;
>  
> +	/*
> +	 * Only CRI reports dynamic VRAM channel state. Fixed-channel platforms
> +	 * set the count up front so those sensors stay visible even if the thermal
> +	 * mailbox reads below fail.
> +	 */
> +	if (hwmon->xe->info.has_fixed_vram_channels)
> +		hwmon->temp.vram_count = FIXED_VRAM_CHANNELS;
> +
> +	if (hwmon->xe->info.platform == XE_CRESCENTISLAND) {
> +		hwmon->temp.msu_mask = xe_mmio_read32(mmio, CRI_MSU_VRAM_ENABLE);
> +		/* Each enabled MSU bit maps to VRAM_CHANNELS_PER_MSU channels. */
> +		hwmon->temp.vram_count = hweight32(hwmon->temp.msu_mask) * VRAM_CHANNELS_PER_MSU;

[Severity: Medium]
Does this calculate the upper bound as the population count rather than the
maximum active index?

If the msu_mask is sparse (e.g., bit 0 and bit 4 enabled, yielding 8 total
channels), vram_count will evaluate to 8.

Later in xe_hwmon_get_reg(), this vram_count is used as an upper bound:
in_range(channel, CHANNEL_VRAM_N, hwmon->temp.vram_count)

When iterating, channels 16-19 (corresponding to bit 4) will fail the
in_range(..., 8) check because their relative index (16) is >= 8. This
regression causes valid hardware sensors for higher-indexed MSUs to be
silently skipped and not exported to userspace.

> +		drm_dbg(&hwmon->xe->drm, "MSU VRAM enable mask 0x%x, VRAM channel count %d\n",
> +			hwmon->temp.msu_mask, hwmon->temp.vram_count);

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