sensors: make getsensoridentity/getsensorshort thread-safe (fix garbage names under concurrent callers)

[widgetii]

The sensor-detection entry points are not thread-safe, and a concurrent caller makes them return a binary-garbage sensor name (e.g. getsensoridentity() yielding ^P\xbf\xbf... instead of sc2332_i2c). Three defects on the path, none of them locked:

1. getsensorid() drives probing through the process-global i2c_adapter_nr and writes it in its retry loop (i2c_adapter_nr = i;). Two threads probing at once clobber each other's bus number mid-scan, so one probes the wrong adapter and detection fails.
2. getsensorid() falls off the end with no return when every bus fails — the retry for loop closes and so does the function, with no return false. The bool is then indeterminate (often true), so the caller treats an all-failed probe as success and reads an uninitialised sensor_ctx_t.
3. getsensoridentity() and getsensorshort() format into one static char sensor_indentity[16] via lsnprintf (a vsnprintf plus a second in-place tolower pass — not atomic). Concurrent calls tear the buffer.

Chained: a concurrent caller → wrong-bus probe (1) → all fail → fall through returning true with a garbage ctx (2) → that garbage is formatted into the shared buffer, torn by the other thread (3) → a binary-garbage identity string.

Standalone ipcinfo never hits this (single process, single thread). It shows up when a program links libipchw and calls these from more than one thread — e.g. one thread resolving the sensor at startup while another renders the sensor name on a timer.

Fix

Serialise detect-and-format under one mutex in the two public entry points (they call getsensorid() internally, which does not take the lock, so there's no self-deadlock and direct getsensorid() callers are unaffected), and give getsensorid() the missing return false on the exhausted-buses path. With the writes serialised, every returned value is one complete, valid string.

Verified

Reproduced on an Ingenic T31 (SC2332) by running a second thread calling getsensoridentity() in a loop while the main thread resolves the sensor, no SENSOR env, 20 starts:

build	garbage / 20
unpatched	3
this patch	0

pthread.h is already included in src/sensors.c.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Make sensor identity helpers thread-safe and fix getsensorid() fall-through UB

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Serialize getsensoridentity()/getsensorshort() to prevent concurrent I2C probing clobbering global
 adapter state.
• Protect shared static formatting buffer from torn writes under concurrent callers.
• Fix undefined behavior by returning false when all I2C buses are exhausted.

Diagram

graph TD
  A{{"Concurrent callers"}} --> B["getsensoridentity() / getsensorshort()"] --> C["sensor_indentity_mtx"] --> D["getsensorid()"] --> E["i2c_adapter_nr (global)"] --> F["I2C probe loop"]
  D --> G["sensor_indentity[16]"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Thread-local or caller-provided output buffer

• ➕ Eliminates shared static buffer without a global lock
• ➕ Allows truly parallel detection if underlying probe becomes thread-safe
• ➖ ABI/API change if caller-provided buffer is required
• ➖ TLS availability/portability concerns on some targets/toolchains

2. Remove global i2c_adapter_nr from probe path

• ➕ Fixes the root cause of cross-thread bus clobbering
• ➕ Allows concurrent probes without serialization
• ➖ Potentially larger refactor touching multiple call sites
• ➖ Higher regression risk if other code assumes the global side effects

3. Lock only around formatting (keep probe concurrent)

• ➕ Smaller critical section; avoids output tearing
• ➖ Does not address i2c_adapter_nr race; still fails detection under concurrency

Recommendation: The chosen approach (single mutex around detect+format in the two public entry points) is the best minimal-risk fix given the current design: it prevents both the global adapter clobbering and shared-buffer tearing without changing the public API. If higher concurrency becomes important later, consider a follow-up to remove i2c_adapter_nr global mutation and return results into caller-provided or thread-local buffers.

Files changed (1) +30 / -11

Bug fix (1) +30 / -11

sensors.cSerialize getsensor* formatting and fix getsensorid() exhausted-bus return +30/-11

Serialize getsensor* formatting and fix getsensorid() exhausted-bus return

• Adds an explicit 'return false' when all I2C buses fail to respond, avoiding undefined behavior and uninitialized context use. Introduces a process-wide mutex and wraps 'getsensoridentity()' / 'getsensorshort()' detect-and-format logic to prevent races on 'i2c_adapter_nr' and the shared static output buffer under concurrent callers.

src/sensors.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Missing pthread link ✓ Resolved 🐞 Bug ≡ Correctness

Description

src/sensors.c now calls pthread_mutex_lock/unlock, but the build system does not link the
library/executables against pthread/Threads, which can cause undefined-reference link failures or
missing -pthread flags on some toolchains.

Code

src/sensors.c[R1287-1290]

+static pthread_mutex_t sensor_indentity_mtx = PTHREAD_MUTEX_INITIALIZER;
static char sensor_indentity[16];
const char *getsensoridentity() {
+    pthread_mutex_lock(&sensor_indentity_mtx);

Evidence

The PR adds new pthread mutex calls in src/sensors.c, but the CMake targets that compile/link that
code only link against libm and do not add Threads/pthread, so link-time resolution can fail on many
environments.

src/sensors.c[1281-1312]
CMakeLists.txt[182-196]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/sensors.c` now uses `pthread_mutex_*`, but `CMakeLists.txt` only links targets with `m`. On platforms/configs where pthread is not folded into libc (and for correct `-pthread` compile/link flags), this can break the build at link time.
## Issue Context
- `ipchw` is a static library and is linked into executables (e.g., `ipcinfo`). If the pthread dependency isn’t on the link line of the final executable, you can get unresolved symbols.
## Fix Focus Areas
- CMakeLists.txt[182-196]
### Suggested change
- Add `find_package(Threads REQUIRED)` once.
- Link `Threads::Threads`:
- `target_link_libraries(ipchw PUBLIC m Threads::Threads)` (PUBLIC so dependents also link pthread as needed)
- `target_link_libraries(ipctool PRIVATE m Threads::Threads)` (since `ipctool` compiles `src/sensors.c` directly)
- Optionally ensure any other executable that directly compiles `src/sensors.c` also links `Threads::Threads`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[widgetii]

Addressed — 14d3744.

Missing pthread link — fixed. sensors.c's mutex is the first real pthread use in the tree (the other #include <pthread.h> are unused, and fake_symbols.c even had commented-out pthread stubs), and the targets only linked m. Added find_package(Threads REQUIRED) and linked Threads::Threads on ipchw (on the interface, so ipcinfo and external consumers of libipchw pull it in) and on ipctool.

Verified both link paths:

• Standalone host build: Found Threads: TRUE, ipcinfo links and nm shows pthread_mutex_lock resolved (T).
• As a consumed static lib (a downstream project linking libipchw, musl): configures and links clean.

On the approach question your summary raised: agreed the single mutex around detect+format is the minimal-risk fix for the current design. The cleaner long-term shape — drop the global i2c_adapter_nr mutation and format into a caller-provided buffer so probes can run truly concurrently — is a larger refactor and worth a separate change; I kept this one small and API-compatible.