[Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths
Ilia Mirkin
imirkin at alum.mit.edu
Sun Dec 6 19:53:18 PST 2015
On Sun, Dec 6, 2015 at 10:48 PM, Ben Skeggs <skeggsb at gmail.com> wrote:
> On 12/07/2015 01:40 PM, Ilia Mirkin wrote:
>> 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?
> That would be lovely, but not possible. It has to be cleaned up by the
> pipe screen destroy() function, as that's the normal exit path. If the
> pipe driver creation path fails before it's got a struct, then the
> winsys will have to clean up after itself instead.
Couldn't you make the pipe_screen_create() function destroy the device
in all failure paths?
>
>>
>> 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