Re: [PATCH v2 5/5] drm/xe/hwmon: Disable memory controller and PCIe temperatures on CRI

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

On 02-09-2026 23:37, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The patch uses an enum ordering check that inadvertently disables memory controller and PCIe temperatures on Nova Lake P, splitting the Novalake platform family.
> --
>
> commit 91ec5832293666790affd1f3a6202739f54f4e49
> Author: Karthik Poosa <karthik.poosa@intel.com>
>
>      drm/xe/hwmon: Disable memory controller and PCIe temperatures on CRI
>
>      Temporarily disable memory controller and PCIe temperature reporting on
>      CRI platforms. The definitions and interpretation of these temperature
>      sources are not yet sufficiently clear, which may result in inaccurate
>      reporting.
>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index c2b8d4c79783a..5b2cef292ffcc 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -1061,8 +1061,14 @@ static bool is_temp_available(struct xe_hwmon *hwmon, int channel)
>>   
>>   		return true;
>>   	case CHANNEL_MCTRL:
>> +		if (hwmon->xe->info.platform >= XE_CRESCENTISLAND)
>> +			return false;
>> +
>>   		return hwmon->temp.count && !get_mc_temp(hwmon, NULL);
>>   	case CHANNEL_PCIE:
>> +		if (hwmon->xe->info.platform >= XE_CRESCENTISLAND)
>> +			return false;
>> +
> [Severity: Medium]
> Does this check inadvertently affect Nova Lake P?
>
> Looking at the enum definition in xe_platform_types.h, XE_NOVALAKE_P is
> defined after XE_CRESCENTISLAND, while XE_NOVALAKE_S is defined before it:
>
> 	XE_NOVALAKE_S,
> 	XE_CRESCENTISLAND,
> 	XE_NOVALAKE_P,
>
> Using the >= operator will disable memory controller and PCIe temperatures on
> Nova Lake P, while they remain enabled for Nova Lake S. This seems to
> contradict the commit message which states this disables temperatures on CRI
> platforms.
>
> Should these checks use == XE_CRESCENTISLAND instead to avoid splitting the
> Novalake family?

HWMON is only supported on discrete GPUs. You can see here,


               int xe_hwmon_register(struct xe_device *xe)
               {
                         struct device *dev = xe->drm.dev;
                         struct xe_hwmon *hwmon;
                         int ret;

                         /* hwmon is available only for dGfx */
                         if (!IS_DGFX(xe))
                         return 0;

                         ....

                 }

So for both Novalake platforms the function itself wont be called.

>
>>   		return hwmon->temp.count && !get_pcie_temp(hwmon, NULL);
>>   	default:
>>   		return false;