Nouveau - Dec 2014 - [PATCH] drm: sgdma: free allocated memory if TT init fails

When ttm_dma_tt_init() fails in nouveau_sgdma_create_ttm(), we should
free the previously allocated memory before returning NULL.

Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
 drm/nouveau_sgdma.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drm/nouveau_sgdma.c b/drm/nouveau_sgdma.c
index 01707e7deaf5..e4fc494d688d 100644
--- a/drm/nouveau_sgdma.c
+++ b/drm/nouveau_sgdma.c
@@ -107,7 +107,10 @@ nouveau_sgdma_create_ttm(struct ttm_bo_device *bdev,
 	else
 		nvbe->ttm.ttm.func = &nv50_sgdma_backend;
 
-	if (ttm_dma_tt_init(&nvbe->ttm, bdev, size, page_flags,
dummy_read_page))
+	if (ttm_dma_tt_init(&nvbe->ttm, bdev, size, page_flags,
dummy_read_page)) {
+		kfree(nvbe);
 		return NULL;
+	}
+
 	return &nvbe->ttm.ttm;
 }
-- 
2.1.3

We get this patch every few months... How about one that instead adds
a comment which explains why this kfree isn't needed :) [I'm leaving
the reason to the reader, which will force you to glance at the ttm
code.]

On Wed, Dec 10, 2014 at 1:37 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
> When ttm_dma_tt_init() fails in nouveau_sgdma_create_ttm(), we should
> free the previously allocated memory before returning NULL.
>
> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
>  drm/nouveau_sgdma.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drm/nouveau_sgdma.c b/drm/nouveau_sgdma.c
> index 01707e7deaf5..e4fc494d688d 100644
> --- a/drm/nouveau_sgdma.c
> +++ b/drm/nouveau_sgdma.c
> @@ -107,7 +107,10 @@ nouveau_sgdma_create_ttm(struct ttm_bo_device *bdev,
>         else
>                 nvbe->ttm.ttm.func = &nv50_sgdma_backend;
>
> -       if (ttm_dma_tt_init(&nvbe->ttm, bdev, size, page_flags,
dummy_read_page))
> +       if (ttm_dma_tt_init(&nvbe->ttm, bdev, size, page_flags,
dummy_read_page)) {
> +               kfree(nvbe);
>                 return NULL;
> +       }
> +
>         return &nvbe->ttm.ttm;
>  }
> --
> 2.1.3
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

On 12/11/2014 03:47 AM, Ilia Mirkin wrote:
> We get this patch every few months... How about one that instead adds
> a comment which explains why this kfree isn't needed :) [I'm
leaving
> the reason to the reader, which will force you to glance at the ttm
> code.]
ttm_dma_tt_init -> ttm_tt_destroy -> nouveau_sgdma_destroy -> kfree

Urrk.

I'm definitely not a fan of things happening this way, but let it be.

Indeed a comment would be nice to have here, let me prep a patch for 
this instead...