[bug report] drm/xe/hwmon: Add support to manage power limits though mailbox

[bug report] drm/xe/hwmon: Add support to manage power limits though mailbox

[Dan Carpenter]

Hello Karthik Poosa,

Commit 7596d839f622 ("drm/xe/hwmon: Add support to manage power
limits though mailbox") from May 29, 2025 (linux-next), leads to the
following Smatch static checker warning:

drivers/gpu/drm/xe/xe_hwmon.c:297 xe_hwmon_power_max_read() warn: passing casted pointer '&reg_val' to 'xe_hwmon_pcode_read_power_limit()' 64 vs 32.
drivers/gpu/drm/xe/xe_hwmon.c:494 xe_hwmon_power_max_interval_show() warn: passing casted pointer '&r' to 'xe_hwmon_pcode_read_power_limit()' 64 vs 32.
drivers/gpu/drm/xe/xe_hwmon.c:595 xe_hwmon_power_max_interval_store() warn: passing casted pointer '&r' to 'xe_hwmon_pcode_read_power_limit()' 64 vs 32.

drivers/gpu/drm/xe/xe_hwmon.c
    476 static ssize_t
    477 xe_hwmon_power_max_interval_show(struct device *dev, struct device_attribute *attr,
    478                                  char *buf)
    479 {
    480         struct xe_hwmon *hwmon = dev_get_drvdata(dev);
    481         struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
    482         u32 x, y, x_w = 2; /* 2 bits */
    483         u64 r, tau4, out;
                ^^^^^

    484         int channel = (to_sensor_dev_attr(attr)->index % 2) ? CHANNEL_PKG : CHANNEL_CARD;
    485         u32 power_attr = (to_sensor_dev_attr(attr)->index > 1) ? PL2_HWMON_ATTR : PL1_HWMON_ATTR;
    486 
    487         int ret = 0;
    488 
    489         xe_pm_runtime_get(hwmon->xe);
    490 
    491         mutex_lock(&hwmon->hwmon_lock);
    492 
    493         if (hwmon->xe->info.has_mbx_power_limits) {
--> 494                 ret = xe_hwmon_pcode_read_power_limit(hwmon, power_attr, channel, (u32 *)&r);

r is a u64 but this only sets 32 bits of the variable.  The other 32 bits
are uninitialized.

    495                 if (ret) {
    496                         drm_err(&hwmon->xe->drm,
    497                                 "power interval read fail, ch %d, attr %d, r 0%llx, ret %d\n",
    498                                 channel, power_attr, r, ret);
    499                         r = 0;
    500                 }
    501         } else {
    502                 r = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel));
    503         }
    504 
    505         mutex_unlock(&hwmon->hwmon_lock);
    506 
    507         xe_pm_runtime_put(hwmon->xe);
    508 
    509         x = REG_FIELD_GET(PWR_LIM_TIME_X, r);
    510         y = REG_FIELD_GET(PWR_LIM_TIME_Y, r);

It turns out it's fine because Intel is little endian and we eventually
mask away the uninitialized bits.  I haven't looked at the C standard on
this but I wouldn't be surprised if it were undefined behavior and I bet
that UBSan will detect this as a read of uninitialized data at runtime.

    511 
    512         /*
    513          * tau = (1 + (x / 4)) * power(2,y), x = bits(23:22), y = bits(21:17)
    514          *     = (4 | x) << (y - 2)
    515          *
    516          * Here (y - 2) ensures a 1.x fixed point representation of 1.x
    517          * As x is 2 bits so 1.x can be 1.0, 1.25, 1.50, 1.75
    518          *
    519          * As y can be < 2, we compute tau4 = (4 | x) << y
    520          * and then add 2 when doing the final right shift to account for units
    521          */
    522         tau4 = (u64)((1 << x_w) | x) << y;
    523 
    524         /* val in hwmon interface units (millisec) */
    525         out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
    526 
    527         return sysfs_emit(buf, "%llu\n", out);
    528 }

regards,
dan carpenter