[Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths

Ilia Mirkin imirkin at alum.mit.edu
Sun Dec 6 19:40:17 PST 2015


This all seems very roundabout... Can't we do this in a somewhat
consistent way with the device being cleaned up in one place or
another but not both?

On Thu, Nov 26, 2015 at 8:04 PM, Ben Skeggs <skeggsb at gmail.com> wrote:
> From: Ben Skeggs <bskeggs at redhat.com>
>
> The winsys layer would attempt to cleanup the nouveau_device if screen
> init failed, however, in most paths the pipe driver would have already
> destroyed it, resulting in accesses to freed memory etc.
>
> This commit fixes the problem by allowing the winsys to detect whether
> the pipe driver's destroy function needs to be called or not.
>
> Signed-off-by: Ben Skeggs <bskeggs at redhat.com>
> ---
>  src/gallium/drivers/nouveau/nouveau_screen.c        |  8 ++++++--
>  src/gallium/drivers/nouveau/nv30/nv30_screen.c      | 19 ++++++++++---------
>  src/gallium/drivers/nouveau/nv50/nv50_screen.c      |  6 +++---
>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c      |  9 ++++-----
>  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 16 ++++++++++------
>  5 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c b/src/gallium/drivers/nouveau/nouveau_screen.c
> index a012579..3cdcc20 100644
> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
> @@ -147,6 +147,12 @@ nouveau_screen_init(struct nouveau_screen *screen, struct nouveau_device *dev)
>     if (nv_dbg)
>        nouveau_mesa_debug = atoi(nv_dbg);
>
> +   /* These must be set before any failure is possible, as the cleanup
> +    * paths assume they're responsible for deleting them.
> +    */
> +   screen->drm = nouveau_drm(&dev->object);
> +   screen->device = dev;
> +
>     /*
>      * this is initialized to 1 in nouveau_drm_screen_create after screen
>      * is fully constructed and added to the global screen list.
> @@ -175,8 +181,6 @@ nouveau_screen_init(struct nouveau_screen *screen, struct nouveau_device *dev)
>                              data, size, &screen->channel);
>     if (ret)
>        return ret;
> -   screen->drm = nouveau_drm(&dev->object);
> -   screen->device = dev;
>
>     ret = nouveau_client_new(screen->device, &screen->client);
>     if (ret)
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> index ea29811..854f70c 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> @@ -413,23 +413,20 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>  #define FAIL_SCREEN_INIT(str, err)                    \
>     do {                                               \
>        NOUVEAU_ERR(str, err);                          \
> -      nv30_screen_destroy(pscreen);                   \
> -      return NULL;                                    \
> +      screen->base.base.context_create = NULL;        \
> +      return &screen->base;                           \
>     } while(0)
>
>  struct nouveau_screen *
>  nv30_screen_create(struct nouveau_device *dev)
>  {
> -   struct nv30_screen *screen = CALLOC_STRUCT(nv30_screen);
> +   struct nv30_screen *screen;
>     struct pipe_screen *pscreen;
>     struct nouveau_pushbuf *push;
>     struct nv04_fifo *fifo;
>     unsigned oclass = 0;
>     int ret, i;
>
> -   if (!screen)
> -      return NULL;
> -
>     switch (dev->chipset & 0xf0) {
>     case 0x30:
>        if (RANKINE_0397_CHIPSET & (1 << (dev->chipset & 0x0f)))
> @@ -458,10 +455,16 @@ nv30_screen_create(struct nouveau_device *dev)
>
>     if (!oclass) {
>        NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
> -      FREE(screen);
>        return NULL;
>     }
>
> +   screen = CALLOC_STRUCT(nv30_screen);
> +   if (!screen)
> +      return NULL;
> +
> +   pscreen = &screen->base.base;
> +   pscreen->destroy = nv30_screen_destroy;
> +
>     /*
>      * Some modern apps try to use msaa without keeping in mind the
>      * restrictions on videomem of older cards. Resulting in dmesg saying:
> @@ -479,8 +482,6 @@ nv30_screen_create(struct nouveau_device *dev)
>     if (screen->max_sample_count > 4)
>        screen->max_sample_count = 4;
>
> -   pscreen = &screen->base.base;
> -   pscreen->destroy = nv30_screen_destroy;
>     pscreen->get_param = nv30_screen_get_param;
>     pscreen->get_paramf = nv30_screen_get_paramf;
>     pscreen->get_shader_param = nv30_screen_get_shader_param;
> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> index 82b9e93..46c812b 100644
> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> @@ -762,6 +762,7 @@ nv50_screen_create(struct nouveau_device *dev)
>     if (!screen)
>        return NULL;
>     pscreen = &screen->base.base;
> +   pscreen->destroy = nv50_screen_destroy;
>
>     ret = nouveau_screen_init(&screen->base, dev);
>     if (ret) {
> @@ -782,7 +783,6 @@ nv50_screen_create(struct nouveau_device *dev)
>
>     chan = screen->base.channel;
>
> -   pscreen->destroy = nv50_screen_destroy;
>     pscreen->context_create = nv50_create;
>     pscreen->is_format_supported = nv50_screen_is_format_supported;
>     pscreen->get_param = nv50_screen_get_param;
> @@ -964,8 +964,8 @@ nv50_screen_create(struct nouveau_device *dev)
>     return &screen->base;
>
>  fail:
> -   nv50_screen_destroy(pscreen);
> -   return NULL;
> +   screen->base.base.context_create = NULL;
> +   return &screen->base;
>  }
>
>  int
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> index e45031a..4897ebe 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> @@ -617,8 +617,7 @@ nvc0_screen_resize_tls_area(struct nvc0_screen *screen,
>  #define FAIL_SCREEN_INIT(str, err)                    \
>     do {                                               \
>        NOUVEAU_ERR(str, err);                          \
> -      nvc0_screen_destroy(pscreen);                   \
> -      return NULL;                                    \
> +      goto fail;                                      \
>     } while(0)
>
>  struct nouveau_screen *
> @@ -650,6 +649,7 @@ nvc0_screen_create(struct nouveau_device *dev)
>     if (!screen)
>        return NULL;
>     pscreen = &screen->base.base;
> +   pscreen->destroy = nvc0_screen_destroy;
>
>     ret = nouveau_screen_init(&screen->base, dev);
>     if (ret) {
> @@ -672,7 +672,6 @@ nvc0_screen_create(struct nouveau_device *dev)
>        screen->base.vidmem_bindings = 0;
>     }
>
> -   pscreen->destroy = nvc0_screen_destroy;
>     pscreen->context_create = nvc0_create;
>     pscreen->is_format_supported = nvc0_screen_is_format_supported;
>     pscreen->get_param = nvc0_screen_get_param;
> @@ -1065,8 +1064,8 @@ nvc0_screen_create(struct nouveau_device *dev)
>     return &screen->base;
>
>  fail:
> -   nvc0_screen_destroy(pscreen);
> -   return NULL;
> +   screen->base.base.context_create = NULL;
> +   return &screen->base;
>  }
>
>  int
> diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
> index e117dfc..456530d 100644
> --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
> +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
> @@ -59,7 +59,7 @@ nouveau_drm_screen_create(int fd)
>  {
>         struct nouveau_device *dev = NULL;
>         struct nouveau_screen *(*init)(struct nouveau_device *);
> -       struct nouveau_screen *screen;
> +       struct nouveau_screen *screen = NULL;
>         int ret, dupfd = -1;
>
>         pipe_mutex_lock(nouveau_screen_mutex);
> @@ -117,7 +117,7 @@ nouveau_drm_screen_create(int fd)
>         }
>
>         screen = init(dev);
> -       if (!screen)
> +       if (!screen || !screen->base.context_create)
>                 goto err;
>
>         /* Use dupfd in hash table, to avoid errors if the original fd gets
> @@ -130,10 +130,14 @@ nouveau_drm_screen_create(int fd)
>         return &screen->base;
>
>  err:
> -       if (dev)
> -               nouveau_device_del(&dev);
> -       else if (dupfd >= 0)
> -               close(dupfd);
> +       if (screen) {
> +               screen->base.destroy(&screen->base);
> +       } else {
> +               if (dev)
> +                       nouveau_device_del(&dev);
> +               else if (dupfd >= 0)
> +                       close(dupfd);
> +       }
>         pipe_mutex_unlock(nouveau_screen_mutex);
>         return NULL;
>  }
> --
> 2.6.3
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau


More information about the Nouveau mailing list