From: Heiko Stuebner heiko@sntech.de
[ Upstream commit bf2de43060d528e52e372c63182a94b95c80d305 ]
qnap_mcu_write can return errors and those were not checked before. So do that now.
Signed-off-by: Heiko Stuebner heiko@sntech.de Link: https://lore.kernel.org/r/20250804130726.3180806-3-heiko@sntech.de Signed-off-by: Lee Jones lee@kernel.org Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
YES
- What it fixes - Prior to this change, `qnap_mcu_exec()` ignored failures from the low-level write routine, always proceeding to wait for a reply. That loses the original error and forces a needless timeout path. The commit makes `qnap_mcu_exec()` bail out immediately on write errors, propagating the true cause to callers. - Evidence: `qnap_mcu_write()` can and does return negative errors from `serdev_device_write()` (drivers/mfd/qnap-mcu.c:81), but previously the call site discarded the return. The commit changes the call site to capture and check the return value.
- Specific code changes - Before: `qnap_mcu_exec()` called `qnap_mcu_write(mcu, cmd_data, cmd_data_size);` without checking its return. - After: `ret = qnap_mcu_write(mcu, cmd_data, cmd_data_size); if (ret < 0) { … return ret; }` so failures are handled early. - Current code location for the effect: drivers/mfd/qnap-mcu.c:167 (assign return), drivers/mfd/qnap-mcu.c:168 (early return on `< 0`). - The rest of the flow is unchanged: it still waits for transmit completion (drivers/mfd/qnap-mcu.c:171), waits for the reply with a timeout (drivers/mfd/qnap-mcu.c:173), and validates checksum (drivers/mfd/qnap-mcu.c:178). - In trees without `guard(mutex)`, the patch explicitly unlocks the bus mutex before returning on error, preserving the original locking discipline in the error path. In newer trees (like current HEAD), `guard(mutex)` covers this automatically.
- Why it matters to users - If the UART write fails (e.g., device disconnected, runtime PM, transient serdev error), the old code would block up to `QNAP_MCU_TIMEOUT_MS` and convert the condition into a misleading `-ETIMEDOUT`. This affects all clients using `qnap_mcu_exec()`: - LEDs: LED state updates via `qnap_mcu_exec_with_ack()` return later and with the wrong error. - hwmon: sensor reads wait unnecessarily and mask the real I/O error. - input: command/ack round-trips behave similarly. - Power-off path: system shutdown delays by a timeout and logs an unrelated error code. - The fix returns the precise failure from the actual write, reducing latency and aiding diagnostics.
- Scope and risk - Small, contained change: 1 file, 5 insertions/1 deletion in the original patch; no API/ABI changes, no architectural changes, success path unchanged. - Touches only the QNAP MCU MFD core; no critical kernel subsystems. - Regression risk is minimal: it only alters behavior when a low-level write already failed, in which case proceeding never had a chance to succeed. Callers already propagate non-zero `ret` values.
- History and applicability - The bug was introduced when the base driver landed (mfd: Add base driver for qnap-mcu devices, likely 998f70d1806bb, 2024-11-07). Backporting should target all stable series that include that driver. - Follow-up refactors (convert to `guard(mutex)`, structure cleanups) are not required to realize this fix; the original patch includes the explicit `mutex_unlock()` to keep locking correct on older branches.
- Stable criteria - Fixes a real bug with user-visible impact (spurious timeouts, loss of original error). - Small, localized, and low-risk patch. - No new features; adheres to stable rules. - No explicit “Cc: stable” in the message, but still a textbook stable-worthy bug fix.
Conclusion: Backporting will improve reliability and diagnostics for all users of the QNAP MCU driver with negligible risk.
drivers/mfd/qnap-mcu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c index 89a8a1913d42d..9d3edc3e7d93b 100644 --- a/drivers/mfd/qnap-mcu.c +++ b/drivers/mfd/qnap-mcu.c @@ -163,7 +163,11 @@ int qnap_mcu_exec(struct qnap_mcu *mcu, reply->received = 0; reinit_completion(&reply->done);
- qnap_mcu_write(mcu, cmd_data, cmd_data_size); + ret = qnap_mcu_write(mcu, cmd_data, cmd_data_size); + if (ret < 0) { + mutex_unlock(&mcu->bus_lock); + return ret; + }
serdev_device_wait_until_sent(mcu->serdev, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS));