intel / intel/xpumanager

[BUG] Assorted defects: API contract violation in fabric throughput; uninitialised variable decides a user-facing message; sysfs writes report success without checking; a help string describes a different flag

Open
#170 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
194
Forks
34
PR merge metrics
No merged PRs in 30d

Description

**Repo:** intel/xpumanager · **Affected:** v2.1.0 **and current `main`** — every file cited
below is unchanged between the tag and `origin/main` as of 06.09.2026, except
`cmd_config.cpp`, whose changes did not touch the cited lines.
Package as installed: `xpu-smi 2.1.0+26.33.6468cec-1~26.04`.
**Hardware:** 8 × Intel Arc Pro B60 (`8086:e211`), Linux 7.0.0-31, `xe`, Level Zero 1.32.0.

Grouped here because each is small; two of them can corrupt memory or silently drop a write,
so they are not merely cosmetic.

## 1. 🔴 `zesFabricPortGetMultiPortThroughput` is called with the wrong indirection

The API expects an **array of `numPorts` pointers** — from `zes_api.h`, verbatim:

```c
zesFabricPortGetMultiPortThroughput(
zes_device_handle_t hDevice,
uint32_t numPorts,
zes_fabric_port_handle_t* phPort,
zes_fabric_port_throughput_t** pThroughput ///< [out][range(0, numPorts)] array of fabric
///< port throughput counters
);
```

xpumanager takes a single pointer and passes **its address**:

```cpp
// hal/core/fabric.cpp:417-425
ze_result_t fabric::portGetMultiPortThroughput(zes_device_handle_t device, uint32_t count,
zes_fabric_port_throughput_t *throughputs)
{
...
ze_result_t result = zesFabricPortGetMultiPortThroughput(device, count, ports, &throughputs);
```

`&throughputs` is the address of a local parameter — a pointer to **one** pointer — while the
driver is contracted to write `pThroughput[0] … pThroughput[numPorts-1]`.

⇒ With `numPorts > 1` the driver writes past that variable: **stack corruption in the
caller**. The call site in `zesRun` passes all ports at once, i.e. exactly when the count
exceeds one.

✅ Not reachable on our hardware — B60 has no fabric, `portCount = 0`. This matters on
platforms with Xe Link, where port counts are high.

## 2. 🔴 An uninitialised variable decides whether the user is told "a reload is needed"

```cpp
// hal/core/scheduler.cpp:255 (and again at :293, :324)
ze_bool_t pNeedReload; // never initialised
ze_result_t result = ZE_RESULT_SUCCESS;

for (uint32_t i = 0; i < schedulerCount; ++i) {
result = zesSchedulerSetTimeoutMode(schedulerHandles[i], &timeoutProperties, &pNeedReload);
...
if (pNeedReload) { INFO("... reload is needed ..."); } else { INFO("... No ... reload needed ..."); }
```

If the driver does not write the out-parameter — possible on partial-success paths — the
branch reads stack garbage. The harmful direction is confidently telling the user **"no
reload needed"** when one is: the setting then silently fails to take effect, which is the
classic "I changed it and nothing happened".

⚠️ This is the code path that configures the scheduler watchdog
(`timeoutProperties.watchdogTimeout`), which is exactly the setting one reaches for after an
engine hang.

**Fix:** `ze_bool_t pNeedReload = 0;` — three characters.

## 3. 🔴 `writeFile` reports success without verifying the write — and it is the sysfs path

```cpp
// hal/core/file_io.cpp:48-62
int writeFile(const std::string &path, const std::string &content)
{
std::ofstream ofs;
ofs.open(path, std::ios::out | std::ios::trunc);
if (!ofs) {
ERR("write: {} open failed\n", path.c_str());
ofs.close();
return -1;
}
ofs << content;
ofs.flush();
ofs.close();
return 0; // always success
}
```

Only the **open** is checked — never `good()`, `fail()`, or the state after `flush`/`close`.

For a regular file that is a minor omission. But this function writes to **sysfs**, where a
write is routinely rejected *after* a successful open: insufficient privilege, value out of
range, driver refusal, device busy. In every one of those cases the function returns `0` and
the caller believes the setting was applied.

Affected settings include `sriov_numvfs`, `sriov_drivers_autoprobe`, VF memory quotas and
`sched_priority` (`oal/lin/linvf.cpp:423-438`, `:624`).

**Fix:** check `ofs.good()` after `flush()`/`close()` and return non-zero on failure.

## 4. `--force-reset-gpus`: the short help describes a different flag

```cpp
// ial/cmn/cmd_config.cpp:2370-2371
sub.add_flag("--force-reset-gpus", configCmds[configCmdType::FORCE_RESET_GPUS].enabled,
"Force GPU reset even if user processes are present");
```

What the flag actually gates:

```cpp
// ial/cmn/cmd_config.cpp:2247-2250
if (!configCmds[configCmdType::FORCE_RESET_GPUS].enabled) {
std::vector peers = getDevicesSharingSlotWith(bdfStr);
if (!peers.empty()) {
ERR("Cold reset aborted: {} other PCI device(s) share the PCIe slot with GPU {} ...
```

It concerns **PCIe slot peers**, not processes. Processes are handled by a different flag,
whose help is correct:

```cpp
// :2368-2369
sub.add_flag("--ignore-gpu-user-processes", ..., "Skip check for running GPU user processes before reset");
```

⇒ A user reading the short help will pass `--force-reset-gpus` expecting to override the
process check, and will instead override the slot-sharing safety check — on a cold reset,
which power-cycles the slot and therefore every device on it.

## 5. `-y, --assumeyes` is declared, documented, and never read

```
hal/fwupd/fwupd.h:61 bool assumeYes;
ial/cmn/cmd_updatefw.cpp:92 "-y,--assumeyes Assume that the answer to any question ..."
ial/cmn/cmd_updatefw.cpp:134 sub.add_flag("-y,--assumeyes", fwInfo.assumeYes, "Assume yes to all questions");
```

Those three are the only occurrences: the value is stored and never tested. There is no
confirmation prompt in `updatefw` at all, so passing `-y` or omitting it produces identical
behaviour — an immediate flash. Either the prompt is missing or the flag is.

## 6. `hasAmcFirmware()` always returns false, and nothing calls it

```cpp
// hal/core/firmware.cpp:375-379
bool firmware::hasAmcFirmware()
{
if (!propertiesList || firmwareCount == 0) {
return false;
}
```

`propertiesList` is initialised to `nullptr` in the constructor and assigned nowhere else in
`hal/` — so the first check is always true. The docstring promises "a reliable way to detect
AMC capability". A repository-wide search also finds no callers: the function is dead both
inside and out.

## 7. `xpu-smi log` reports success regardless of what it managed to collect

`ial/cmn/cmd_log.cpp:79` prints `Logs collected successfully in file: {}` and returns 0 even
when parts of the collection were denied. Incomplete collection affects neither the message
nor the exit code, so a script cannot tell a full archive from a partial one.

## 8. `config --fancurve` leaves the fan table in a non-monotonic state

Verified on hardware. `--fancurve 40:25,60:50,80:90` (three points) replaces only the first
three of the ten table entries; the remaining seven keep their previous values:

```
40:64 60:128 80:230 55:61 60:130 65:191 70:230 75:237 80:255 90:255
└─ new three ─┘ └────────── stale tail from the previous curve ──────────┘
```

The temperature sequence becomes 40, 60, 80, 55, 60, … — no longer monotonic — and the
command reports `Succeeded ... with 3 points`.

⚠️ Aggravating: a subsequent write of a full curve is rejected by the firmware
(`PCODE Mailbox failed: -22 Illegal Data`), so the table cannot be restored without a power
cycle.

**Fix:** require a complete set of points, or fill/clear the remainder — but do not leave a
partially updated table and call it success.

Related: [#118](https://github.com/intel/xpumanager/issues/118) ("Support for setting a fan
curve on Linux for Arc Pro B-Series cards") — the feature now exists on Linux; this is about
its behaviour when given a partial curve.

Contributor guide

Open the contributing guide

Research direction

Start with hal/core/fabric.cpp, scheduler.cpp, file_io.cpp, ial/cmn/cmd_config.cpp, cmd_updatefw.cpp, firmware.cpp, and cmd_log.cpp, tracing callers such as zesRun and the cited linvf.cpp paths. Reproduce the fan-curve and reset/help behavior and inspect the updatefw and log entry points. Done means each listed defect has an agreed fix and its success or error reporting matches the observed result, with the grouped scope split or explicitly tracked.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, linux
Domain
cli, operating-systems, tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.