From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/connector: Add HDMI 2.0 scrambler infrastructure Date: Mon, 25 May 2026 21:15:02 +1000 Message-ID: In-Reply-To: <20260520-dw-hdmi-qp-scramb-v6-2-24b74603b782@collabora.com> References: <20260520-dw-hdmi-qp-scramb-v6-0-24b74603b782@collabora.com> <20260520-dw-hdmi-qp-scramb-v6-2-24b74603b782@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review The infrastructure additions look reasonable. A few observations: 1. **`INIT_DELAYED_WORK` in `drm_connector_init_only()`**: This initializes `scdc_work` for *all* connectors, not just HDMI ones. While harmless (the work is never scheduled for non-HDMI connectors), it's slightly wasteful. However, this is consistent with how `mutex_init(&connector->hdmi.infoframes.lock)` is already done unconditionally on the adjacent line, so this follows existing pattern. 2. **`scdc_cb` function pointer without locking documentation**: The `scdc_cb` callback pointer is assigned and cleared from `drm_scdc_start_scrambling()`/`drm_scdc_stop_scrambling()`, and read from the work item `drm_connector_hdmi_scdc_work()`. The `scrambler_enabled` flag is protected by `READ_ONCE`/`WRITE_ONCE`, but `scdc_cb` itself has no such annotation. There's a potential race between `drm_scdc_stop_scrambling()` setting `scdc_cb = NULL` after `cancel_delayed_work_sync()`, and the work item checking `scdc_cb`. Since `cancel_delayed_work_sync()` ensures the work item completes before returning, and subsequent scheduling is prevented by `scrambler_enabled` being false, this is actually safe in practice. But a comment explaining the ordering guarantee would be helpful. --- Generated by Claude Code Patch Reviewer