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/microchip: add a driver for the Microchip GFX2D GPU Date: Thu, 12 Feb 2026 20:22:54 +1000 Message-ID: In-Reply-To: <20260209-cpitchen-mainline_gfx2d-v7-2-0c12e64a0950@microchip.com> References: <20260209-cpitchen-mainline_gfx2d-v7-0-0c12e64a0950@microchip.com> <20260209-cpitchen-mainline_gfx2d-v7-2-0c12e64a0950@microchip.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Mailer: Claude Code Patch Reviewer Patch Review This is the main patch and contains a number of issues that need attention. **UAPI: `__kernel_size_t` in ioctl struct** > +struct drm_mchp_gfx2d_submit { > + __u64 rectangles; > + __kernel_size_t num_rectangles; > + __u32 target_handle; `__kernel_size_t` is architecture-dependent (4 bytes on 32-bit, 8 bytes on 64-bit). While this driver is ARM-only today, UAPI headers are shared and must have consistent layout. This should be `__u32` or `__u64`. This also means the struct layout changes between architectures, which would matter for any compat ioctl path. **UAPI: bare enum types in structs** > +struct drm_mchp_gfx2d_blend { > + __u32 src_color; > + __u32 dst_color; > + __u32 flags; > + enum drm_mchp_gfx2d_blend_function function; > + enum drm_mchp_gfx2d_blend_factor safactor; UAPI structures should not use `enum` types directly because the size of an enum is compiler-defined (could be 1, 2, or 4 bytes depending on compiler flags). All the ioctl structs use bare enums: `drm_mchp_gfx2d_submit` uses `enum drm_mchp_gfx2d_operation`, `drm_mchp_gfx2d_blend` uses several enum types, `drm_mchp_gfx2d_rop` uses `enum drm_mchp_gfx2d_rop_mode`, `drm_mchp_gfx2d_alloc_buffer` uses two enum types, etc. These should all be `__u32`. **UAPI: implicit padding holes** > +struct drm_mchp_gfx2d_alloc_buffer { > + __u32 size; > + __u16 width; > + __u16 height; > + __u16 stride; > + enum drm_mchp_gfx2d_pixel_format format; There is a 2-byte implicit padding hole between `stride` (__u16) and `format` (enum/4-byte aligned). Similarly in `drm_mchp_gfx2d_import_buffer`. UAPI structs should make padding explicit or avoid it. The padding bytes are not validated to be zero, which means userspace could pass garbage in them without detection, foreclosing future use of those bytes. **UAPI: no reserved fields** None of the UAPI structures have reserved fields. This is a general concern for future extensibility - there is no way to add new parameters without defining new ioctls. **UAPI: `drm_mchp_gfx2d_source` has 12-byte size** > +struct drm_mchp_gfx2d_source { > + __u32 handle; > + __s32 x; > + __s32 y; > +}; This is 12 bytes with no padding to a power-of-2 size. An array of these in `drm_mchp_gfx2d_submit` will work fine, but the lack of a padding field means it cannot be extended later. **No upper bound on `num_rectangles`** > + if (!args->num_rectangles) > + return -EINVAL; > + > + cmd = mchp_gfx2d_alloc_command(priv); > ... > + ret = mchp_gfx2d_alloc_rectangles(cmd, args->num_rectangles); `num_rectangles` is checked for zero but has no upper bound. A malicious userspace can request allocation of an arbitrarily large rectangle array via `kvmalloc_array()`. While `kvmalloc_array` handles overflow internally, it will still attempt very large allocations. Should there be a reasonable upper limit? **`copy_from_user` multiplication could overflow** > + ret = copy_from_user(cmd->rects, u64_to_user_ptr(args->rectangles), > + args->num_rectangles * sizeof(*cmd->rects)); The multiplication `args->num_rectangles * sizeof(*cmd->rects)` could overflow on 32-bit if `num_rectangles` is very large. `kvmalloc_array` protects against overflow in the allocation, so a smaller buffer would be allocated but `copy_from_user` would be called with the overflowed (smaller) size -- this would not cause a buffer overrun since less data is copied, but would silently truncate the input. However combined with the lack of bounds on `num_rectangles`, this is fragile. **`static uint32_t next_id` without locking** > +static struct mchp_gfx2d_command * > +mchp_gfx2d_alloc_command(struct mchp_gfx2d_device *priv) > +{ > + static uint32_t next_id; > ... > + cmd->id = next_id++; This is a non-atomic increment of a static variable with no locking. Multiple concurrent userspace processes calling submit could race here. The same pattern appears in `__mchp_gfx2d_gem_create`: > +static struct mchp_gfx2d_gem_object * > +__mchp_gfx2d_gem_create(struct drm_device *dev, size_t size) > +{ > + static u32 next_id; > ... > + gfx2d_obj->id = next_id++; Two separate `next_id` counters without synchronization. These are used for debug logging so duplicate IDs may not cause functional bugs, but the data race itself is undefined behavior. **Storing a copy of `struct vm_area_struct`** > + if (ret) > + drm_gem_vm_close(vma); > + else > + memcpy(&gfx2d_obj->vma, vma, sizeof(gfx2d_obj->vma)); Copying an entire `struct vm_area_struct` into a driver structure is problematic. The VMA can be modified or freed after mmap returns (e.g., by munmap, mremap, or the kernel's VMA management). The stored copy becomes stale. It is later used in `mchp_gfx2d_ioctl_sync_for_gpu`: > + dir = gfx2d_obj->direction; > + if (valid_dma_direction(dir)) { > + struct vm_area_struct *vma = &gfx2d_obj->vma; > + flush_cache_range(vma, vma->vm_start, vma->vm_end); > + } Using a stale VMA copy with `flush_cache_range` is unsafe. The `vma->vm_mm` pointer in the stale copy could reference a freed `mm_struct` if the original process has exited or the mapping has been removed. **`(void)cmd` cast in `mchp_gfx2d_copy`** > +static void mchp_gfx2d_copy(struct mchp_gfx2d_device *priv, > + const struct mchp_gfx2d_command *cmd, > + const struct mchp_gfx2d_rectangle *r) > +{ > + (void)cmd; The `(void)cmd` cast to suppress unused parameter warnings is not idiomatic kernel style. If the parameter is unused, consider using `__always_unused` or restructuring the function pointer interface. **Commented-out `break` statements** > + default: > + return -EINVAL; > + //break; Two instances of `//break;` comments after return statements in `to_dma_data_direction` and `mchp_gfx2d_valid_buffer_params`. These should be removed. **`__maybe_unused` on PM functions** > +static int __maybe_unused mchp_gfx2d_runtime_suspend(struct device *dev) > ... > +static const struct dev_pm_ops __maybe_unused mchp_gfx2d_pm_ops = { The `__maybe_unused` annotations are used to avoid warnings when PM is disabled, but the modern pattern is to use `DEFINE_RUNTIME_DEV_PM_OPS` or `pm_ptr()` (which is already used in the `platform_driver` definition). Having both `pm_ptr()` and `__maybe_unused` is redundant but not harmful. **`platform_get_resource` + `devm_ioremap_resource` pattern** > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->regs = devm_ioremap_resource(&pdev->dev, res); The modern pattern is `devm_platform_ioremap_resource(pdev, 0)` which combines both calls. **Missing MODULE_DEVICE_TABLE** The `of_device_id` table has no `MODULE_DEVICE_TABLE(of, ...)` annotation. Without this, the module will not auto-load when the device is present in the DT. --- Generated by Claude Code Patch Reviewer