Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing NULL check for `dma_alloc_coherent` and potential integer overflow in `new_size` calculation. - [High] Calling `dma_alloc_coherent()` with `GFP_KERNEL` during `atomic_flush` violates DRM atomic context constraints. -- --- Patch [1]: [PATCH 01/70] drm/adp: Switch to drm_atomic_helper_crtc_create_state --- commit 3f2217690a441eaa2502ebbe2a7e5840385ac1e3 Author: Maxime Ripard <[email protected]> drm/adp: Switch to drm_atomic_helper_crtc_create_state The adp crtc implementation uses the deprecated drm_atomic_helper_crtc_reset() as its reset hook. Switch to drm_atomic_helper_crtc_create_state() instead. > diff --git a/drivers/gpu/drm/adp/adp_drv.c b/drivers/gpu/drm/adp/adp_drv.c > index ce9dd6222634d..fdf0e63161756 100644 > --- a/drivers/gpu/drm/adp/adp_drv.c > +++ b/drivers/gpu/drm/adp/adp_drv.c > @@ -351,7 +351,7 @@ static const struct drm_crtc_funcs adp_crtc_funcs = { > .destroy = drm_crtc_cleanup, > .set_config = drm_atomic_helper_set_config, > .page_flip = drm_atomic_helper_page_flip, > - .reset = drm_atomic_helper_crtc_reset, > + .atomic_create_state = drm_atomic_helper_crtc_create_state, > .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > .enable_vblank = adp_crtc_enable_vblank, [Severity: High] This is a pre-existing issue, but does calling dma_alloc_coherent() with GFP_KERNEL in adp_crtc_atomic_flush() violate DRM atomic context constraints? drivers/gpu/drm/adp/adp_drv.c:adp_crtc_atomic_flush() { ... if (new_size != 0) { adp->mask_buf = dma_alloc_coherent(crtc->dev->dev, new_size, &adp->mask_iova, GFP_KERNEL); adp_crtc_atomic_flush() executes during the hardware programming phase of drm_atomic_helper_commit_tail(), which is an atomic context where sleeping is forbidden. dma_alloc_coherent() with GFP_KERNEL can sleep, potentially causing kernel warnings and deadlocks. [Severity: High] This isn't a bug introduced by this patch, but what happens if dma_alloc_coherent() returns NULL? drivers/gpu/drm/adp/adp_drv.c:adp_crtc_atomic_flush() { ... if (new_size != 0) { adp->mask_buf = dma_alloc_coherent(crtc->dev->dev, new_size, &adp->mask_iova, GFP_KERNEL); memset(adp->mask_buf, 0xFF, new_size); The immediate memset() would dereference the NULL pointer. Additionally, does the calculation of new_size in adp_crtc_atomic_flush() run the risk of a 32-bit integer overflow for maliciously large resolutions? u64 new_size = ALIGN(new_state->mode.hdisplay * new_state->mode.vdisplay * 4, PAGE_SIZE); Is there a chance hdisplay * vdisplay * 4 could overflow before being cast to u64? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
