Re: [PATCH v13 04/16] drm/xe/hwmon: Add helpers to validate thermal sensor readings

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Oct 14, 2026 at 06:47:01PM +0530, Poosa, Karthik wrote:
> 
> On 14-10-2026 17:41, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Low] drm/xe/hwmon: mbx_temp_valid() fails to reject 0x80 (-0°C) as invalid
> > --
> > 
> > commit cc835292455ece8a38adb02fe6e4c345fc3aa89a
> > Author: Karthik Poosa <karthik.poosa@intel.com>
> > 
> > drm/xe/hwmon: Add helpers to validate thermal sensor readings
> > 
> > This commit adds mmio_temp_valid() and mbx_temp_valid() to validate
> > temperature sensor readings. It uses the has_temp_sentinel flag to gate
> > these checks and rejects 0xFF for mailbox and 0xFFFFFFFF for MMIO.
> > 
> > > diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> > > index 1b4efbf4ac999..883a0ae1d90e3 100644
> > > --- a/drivers/gpu/drm/xe/xe_hwmon.c
> > > +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> > [ ... ]
> > > @@ -834,6 +837,30 @@ static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
> > >   	return ret;
> > >   }
> > [ ... ]
> > > +/* Mailbox temperature is sign-magnitude (bit 7 sign, bits 6:0 magnitude). */
> > > +static inline bool mbx_temp_valid(const struct xe_hwmon *hwmon, u8 value)
> > > +{
> > > +	if (value == TEMP_MBX_INVALID)
> > > +		return false;
> > > +
> > > +	return hwmon->xe->info.has_temp_sentinel || value != 0;
> > [Severity: Low]
> > Does this raw byte check fail to reject 0x80?
> > 
> > Since 0x80 represents -0 degrees Celsius in the sign-magnitude format (sign
> > bit 7 is set, magnitude bits 6:0 are zero), it evaluates to true here
> > because 0x80 is not equal to 0.
> hardware will not send 0x80, so this case is not expected in practice.
> > 
> > > +}
> > > +
> > > +/* Decode an 8-bit sign-magnitude mailbox temperature to degrees Celsius. */
> > > +static int mbx_temp_decode(u8 value)
> > > +{
> > > +	int temp = value & TEMP_MBX_MAG;
> > > +
> > > +	return (value & TEMP_MBX_SIGN) ? -temp : temp;
> > If 0x80 is passed here, it decodes to 0 degrees Celsius.
> > 
> > This means the fallback safety logic in mbx_temp_valid() intended to reject
> > uninitialized 0 degrees Celsius readings can be bypassed if the hardware
> > returns 0x80 instead of 0x00. This could fold invalid readings into the
> > temperature average.
> > 
> > > +}
> same as above


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


>