From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Qualcomm Sahara protocol enhancements. Date: Mon, 09 Mar 2026 08:08:59 +1000 Message-ID: In-Reply-To: <20260307-sahara_protocol_new_v2-v2-0-29dc748b5e9c@oss.qualcomm.com> References: <20260307-sahara_protocol_new_v2-v2-0-29dc748b5e9c@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Overall Series Review Subject: Qualcomm Sahara protocol enhancements. Author: Kishore Batta Patches: 13 Reviewed: 2026-03-09T08:08:59.981979 --- This 9-patch series relocates the Sahara protocol driver from the QAIC accelerator subsystem (`drivers/accel/qaic/`) to `drivers/bus/mhi/sahara/`, generalizes it for multiple MHI devices, adds QDU100 device support, implements Sahara command mode for DDR training data capture, and exposes the training data via sysfs. The overall goal is sound -- making a reusable MHI protocol driver rather than embedding it in the QAIC-specific tree. **Key concerns:** 1. **Patch ordering is wrong** -- Patch 7 (command mode) arrives in the mbox after patch 8 (sysfs exposure) and patch 9 (ABI docs), even though patches 8 and 9 depend on structures and functions introduced in patch 7. The Message-Id numbering is correct (v2-7, v2-8, v2-9) but the mbox delivery order is 8, 9, 7. This will break `git am` unless reordered. 2. **Copyright header regression** -- Patch 2 changes the SPDX license from `GPL-2.0-only` to `GPL-2.0` and replaces the copyright with a different (older) one. This looks like an accidental revert to an old internal header rather than a deliberate change. 3. **Concurrency issues in the DL callback** -- The DL transfer callback (patch 7) accesses `context->is_cmd_mode`, `context->receiving_trng_data`, and other shared state without locking while they can be modified concurrently from workqueues. The MHI DL callback runs in interrupt/softirq context, making mutexes unsuitable there. 4. **The `sahara_ctrl_trng_get()` function doubles as allocator and finder** -- it silently creates the devres entry if missing, which is called both from workqueue context and from the DL callback (interrupt context). The `devres_alloc` with `GFP_KERNEL` in the DL callback path is a sleeping allocation in atomic context. 5. **Documentation placement** -- `Documentation/sahara/` is a new top-level doc directory for a single driver, which is unusual. Typically this would be under `Documentation/bus/` or `Documentation/driver-api/`. --- --- Generated by Claude Code Patch Reviewer