Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm/dpu: prepare_count underflow on writeback prepare failure --
commit 32d0ff950383106de0523f200d999646b3523a0a Author: Dmitry Baryshkov <[email protected]> drm/msm: fix framebuffer pin refcount leak on prepare failure This commit unwinds framebuffer preparation state on failure in msm_framebuffer_prepare(), unpinning any already-pinned planes, dropping VMA references, zeroing iova[], and decrementing prepare_count and dirtyfb. > diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c > index a38e5b3a1d1e9..792d47b4acc43 100644 > --- a/drivers/gpu/drm/msm/msm_fb.c > +++ b/drivers/gpu/drm/msm/msm_fb.c > @@ -80,6 +80,18 @@ void msm_framebuffer_describe(struct drm_framebuffer *fb, > struct seq_file *m) > } > #endif > > +static void msm_framebuffer_unpin_planes(struct drm_framebuffer *fb, int n) > +{ > + struct msm_drm_private *priv = fb->dev->dev_private; > + struct drm_gpuvm *vm = priv->kms->vm; > + int i; > + > + for (i = 0; i < n; i++) { > + msm_gem_unpin_iova(fb->obj[i], vm); > + msm_gem_vma_put(fb->obj[i]); > + } > +} > + > /* prepare/pin all the fb's bo's for scanout. > */ > int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb) > @@ -102,8 +114,17 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, > bool needs_dirtyfb) > ret = msm_gem_get_and_pin_iova(fb->obj[i], vm, > &msm_fb->iova[i]); > drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)\n", > fb->base.id, i, msm_fb->iova[i], ret); > - if (ret) > + if (ret) { > + msm_gem_vma_put(fb->obj[i]); > + msm_framebuffer_unpin_planes(fb, i); > + memset(msm_fb->iova, 0, sizeof(msm_fb->iova)); > + msm_fb->prepare_count--; > + > + if (needs_dirtyfb) > + refcount_dec(&msm_fb->dirtyfb); > + > return ret; > + } [Severity: High] Can this underflow msm_fb->prepare_count when msm_framebuffer_prepare() is called from the DPU writeback path? During atomic commit preparation, drm_atomic_helper_prepare_planes() calls: drm_writeback_prepare_job() dpu_wb_conn_prepare_job() dpu_encoder_prepare_wb_job() dpu_encoder_phys_wb_prepare_wb_job() In drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c, dpu_encoder_phys_wb_prepare_wb_job() drops the error returned by msm_framebuffer_prepare(): wb_enc->wb_job = job; wb_enc->wb_conn = job->connector; wb_cfg = &wb_enc->wb_cfg; memset(wb_cfg, 0, sizeof(struct dpu_hw_wb_cfg)); ret = msm_framebuffer_prepare(job->fb, false); if (ret) { DPU_ERROR("prep fb failed, %d\n", ret); return; } And in drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c, dpu_wb_conn_prepare_job() unconditionally returns 0: dpu_encoder_prepare_wb_job(dpu_wb_conn->wb_enc, job); return 0; Because dpu_wb_conn_prepare_job() returns 0, drm_writeback_prepare_job() sets job->prepared = true, and the commit proceeds with wb_cfg->dest.plane_addr[] left at 0. When drm_writeback_cleanup_job() later runs (either on commit completion or if a subsequent plane's prepare_fb() fails), job->prepared is true, so it calls: dpu_wb_conn_cleanup_job() dpu_encoder_cleanup_wb_job() dpu_encoder_phys_wb_cleanup_wb_job() msm_framebuffer_cleanup(job->fb, false) Since msm_framebuffer_prepare() now decrements msm_fb->prepare_count back to 0 on error, won't the subsequent call to msm_framebuffer_cleanup() decrement msm_fb->prepare_count a second time, underflowing it to UINT_MAX (or prematurely unpinning the framebuffer if another plane prepared it in the meantime)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
