Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Hardware modification inside `atomic_create_state` violates the DRM API contract. - [Medium] The function returns `-EINVAL` instead of `-ENOMEM` when memory allocation for DMA descriptors fails. --
--- Patch [43]: [PATCH 43/60] drm/atmel-hlcdc: Convert to atomic_create_state --- commit 28ffcf15c4fc4a6c19751342c0e8a7f7e0d7a988 Author: Maxime Ripard <[email protected]> drm/atmel-hlcdc: Convert to atomic_create_state This commit converts the atmel-hlcdc plane reset implementation to use atomic_create_state. The conversion was primarily done using a Coccinelle semantic patch. > The plane reset implementation creates a custom state > subclass, but only initializes a pristine state without resetting any > hardware. This is equivalent to what atomic_create_state expects. Is this description of the old reset function accurate? Looking at atmel_hlcdc_csc_init(), which is called via the lcdc_csc_init() hook, it appears to perform direct register writes: atmel_hlcdc_csc_init() { ... atmel_hlcdc_layer_write_cfg(&plane->layer, desc->layout.csc + i, hlcdc_csc_coeffs[i]); } This seems to indicate the old reset function did actually touch hardware, contrary to the commit message. > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > index f17a832351e9d..ff35251a01846 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c [ ... ] > @@ -1183,36 +1183,33 @@ static void > atmel_hlcdc_plane_atomic_destroy_state(struct drm_plane *p, [ ... ] > - if (state) { > - if (atmel_hlcdc_plane_alloc_dscrs(p, state)) { > - kfree(state); > - drm_err(p->dev, > - "Failed to allocate initial plane state\n"); > - return; > - } > - __drm_atomic_helper_plane_reset(p, &state->base); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + if (atmel_hlcdc_plane_alloc_dscrs(p, state)) { > + kfree(state); > + drm_err(p->dev, "Failed to allocate initial plane state\n"); > + return ERR_PTR(-EINVAL); [Severity: Medium] Since atmel_hlcdc_plane_alloc_dscrs() internally returns -ENOMEM when dma_pool_alloc() fails, should this propagate that error or return ERR_PTR(-ENOMEM) instead of hardcoding -EINVAL? > } > + __drm_atomic_helper_plane_state_init(&state->base, p); > > if (plane->layer.desc->layout.csc) > dc->desc->ops->lcdc_csc_init(plane, plane->layer.desc); [Severity: Medium] Does this break the DRM API contract for atomic_create_state? The atomic_create_state hook is documented to have no side effects, but as noted above, lcdc_csc_init() writes directly to hardware registers. If the driver is later initialized using drm_mode_config_create_initial_state() before power domains are fully enabled, could these hardware accesses cause a crash or hang? > + > + return &state->base; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=43
