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: client: add splash client Date: Sat, 16 May 2026 16:02:57 +1000 Message-ID: In-Reply-To: <20260510-drm_client_splash-v3-1-a9aee9f0b2fc@valla.it> References: <20260510-drm_client_splash-v3-0-a9aee9f0b2fc@valla.it> <20260510-drm_client_splash-v3-1-a9aee9f0b2fc@valla.it> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Critical: Crash on unregister if hotplug never fires** `drm_splash_client_unregister()` unconditionally calls `kthread_stop(splash= ->thread)`, but `splash->thread` is only assigned in `drm_splash_client_hot= plug()`. If the device is removed before any hotplug event, `splash->thread= ` is NULL (kzalloc zero-init), and `kthread_stop(NULL)` will crash. ```c +static void drm_splash_client_unregister(struct drm_client_dev *client) +{ + struct drm_splash *splash =3D client_to_drm_splash(client); + + kthread_stop(splash->thread); ``` Fix: guard with `if (splash->thread)`, or check `splash->initialized`. --- **Critical: Out-of-bounds read in BMP pixel blitting** All three blit functions (`drm_splash_blit_pix16/24/32`) read 4 bytes from = 3-byte-per-pixel BMP data: ```c + scolor =3D *(const u32 *)(&sbuf8[src_offset + 3 * x]); ``` This reads one byte beyond each pixel's data. On the last pixel of each row= , this can read past the source buffer entirely. Additionally, this is an u= naligned access cast through a pointer, which is undefined behavior and wil= l fault on some architectures (ARM, etc.). The patch cover letter mentions = testing on Beagleplay and i.MX93 (both ARM), so this is surprising. Use `get_unaligned_le32()` or better yet, manually compose the 3-byte pixel: ```c scolor =3D sbuf8[src_offset + 3*x] | (sbuf8[src_offset + 3*x + 1] << 8) | (sbuf8[src_offset + 3*x + 2] << 16); ``` --- **Bug: Render thread sleep/wake race** The sleep pattern at the bottom of the render loop has a classic race: ```c + if (atomic_xchg(&splash->pending, 0) =3D=3D 0) + set_current_state(TASK_UNINTERRUPTIBLE); + + schedule(); ``` Between `atomic_xchg` returning 0 and `set_current_state`, a wakeup from `d= rm_splash_fw_callback` can be lost because the task state is still `TASK_RU= NNING` when `wake_up_process` is called. The fix is the standard pattern: s= et the state *before* testing the condition: ```c set_current_state(TASK_UNINTERRUPTIBLE); if (atomic_xchg(&splash->pending, 0) !=3D 0) __set_current_state(TASK_RUNNING); schedule(); ``` --- **Bug: Tiled display dimension accumulation looks swapped** ```c + tiled =3D get_scanout_from_tile_group(splash, new_id); + if (tiled !=3D NULL) { + if (!modeset->x) + tiled->width +=3D modeset->mode->vdisplay; + if (!modeset->y) + tiled->height +=3D modeset->mode->hdisplay; ``` When `modeset->x =3D=3D 0` (left-edge tile), the code adds `vdisplay` to `w= idth`, but that should presumably be `hdisplay`. Similarly, when `modeset->= y =3D=3D 0` (top-edge tile), it adds `hdisplay` to `height`, but that shoul= d be `vdisplay`. These appear to be swapped. --- **Bug: `modeset_mask` / scanout index tracking is broken** In `drm_splash_init_client`, the variable `j` is used as both a modeset cou= nter and scanout index, but it increments for every modeset (even skipped t= iled ones via `continue`), while scanouts are added via `splash->n_scanout+= +`: ```c + j =3D 0; + drm_client_for_each_modeset(modeset, client) { + ... + if (modeset->connectors[0]->has_tile) { + ... + tiled =3D get_scanout_from_tile_group(splash, new_id); + if (tiled !=3D NULL) { + ... + continue; // j not incremented here + } + id =3D new_id; + } + ... + modeset_mask |=3D BIT(j); + j++; + } ``` Then in the second loop, `j` is used to index both `modeset_mask` bits and = `splash->scanout[]`: ```c + j =3D 0; + drm_client_for_each_modeset(modeset, client) { + if (!(modeset_mask & BIT(j))) + continue; + scanout =3D &splash->scanout[j]; + j++; ``` If any modeset was skipped (no mode, or continued due to tile dedup), `j` i= n the second loop won't match the scanout array index. The scanout array is= dense (indexed 0..n_scanout-1) but `modeset_mask` bits correspond to the f= irst-loop's `j` which skips some values. A separate scanout index counter i= s needed in the second loop. --- **Minor: `drm_splash_fill_solid_color` ignores error from `drm_splash_fill_= solid_color` in render thread** ```c + if (!scanout->bg_drawn) { + ... + drm_splash_fill_solid_color(scanout->buffer, color); + scanout->bg_drawn =3D true; ``` The return value is ignored. If the vmap fails, `bg_drawn` is still set to = true and the background will never be retried. --- **Minor: Variable declaration after statement (C90 style)** ```c + if (!scanout->bg_drawn) { + drm_dbg(client->dev, "draw background for scanout %d", i); + u32 color =3D drm_draw_color_from_xrgb8888(splash_color, + scanout->format); ``` While mixed declarations are allowed in kernel code (C11), this specific pa= ttern =E2=80=94 a declaration after a function call (`drm_dbg`) within the = same block =E2=80=94 may trigger warnings with some compiler configurations= . Consider moving the declaration to the top of the block. --- **Minor: `splash_color` module_param type mismatch** ```c +static unsigned int splash_color =3D CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_C= OLOR; +module_param(splash_color, uint, 0400); ``` `CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_COLOR` is a `hex` Kconfig type which p= roduces an integer literal. This works, but the module parameter uses `uint= ` (decimal input), while users might expect to pass hex values on the comma= nd line. Consider documenting this or using a custom param type. --- **Minor: `drm_splash_image_cleanup` always calls `memunmap`** ```c +static void drm_splash_image_cleanup(struct drm_splash *splash) +{ + memunmap(splash->map_data); ``` `memunmap(NULL)` is safe, so this is functionally fine, but it's called fro= m both the render thread (after image load) and `drm_splash_client_unregist= er`. If the render thread already cleaned up, `map_data` should be NULL. Th= is is fine but worth noting =E2=80=94 there's no double-free risk since `me= munmap` is idempotent on NULL, but `release_firmware` with NULL is also saf= e. This path is OK. --- **Nit: Comment style** Several multi-line comments use the `/* text` style rather than the preferr= ed kernel style: ```c + /* If commit returns EBUSY, another master showed up. + * This means that the splash is no more required. + */ ``` Kernel preferred style has `/*` on its own line for multi-line comments. --- --- Generated by Claude Code Patch Reviewer