[bug report] drm/xe/hwmon: Add support to manage power limits though mailbox
Dan Carpenter
dan.carpenter at linaro.org
Thu Jun 5 06:25:21 UTC 2025
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 '®_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
More information about the Intel-xe
mailing list