On 8/28/23 13:55, Boris Brezillon wrote: > On Sun, 27 Aug 2023 20:54:28 +0300 > Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> wrote: > >> Use separate flag for tracking page count bumped by shmem->sgt to avoid >> imbalanced page counter during of drm_gem_shmem_free() time. It's fragile >> to assume that populated shmem->pages at a freeing time means that the >> count was bumped by drm_gem_shmem_get_pages_sgt(), using a flag removes >> the ambiguity. >> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- >> drivers/gpu/drm/lima/lima_gem.c | 1 + >> include/drm/drm_gem_shmem_helper.h | 7 +++++++ >> 3 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index 78d9cf2355a5..db20b9123891 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -152,7 +152,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >> sg_free_table(shmem->sgt); >> kfree(shmem->sgt); >> } >> - if (shmem->pages) >> + if (shmem->got_sgt) >> drm_gem_shmem_put_pages(shmem); > > Can't we just move this drm_gem_shmem_put_pages() call in the > if (shmem->sgt) block? As you've seen in patch #1, the shmem->sgt may belong to imported dmabuf and pages aren't referenced in this case. I agree that the freeing code is confusing. The flags make it a better, not ideal. Though, the flags+comments solution is good enough to me. Please let me know if you have more suggestions, otherwise I'll add comment to the code and keep this patch for v16. BTW, I realized that the new flag wasn't placed properly in the Lima driver, causing unbalanced page count in the error path. Will correct it in v16. -- Best regards, Dmitry
