[Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths
Ben Skeggs
bskeggs at redhat.com
Sun Dec 6 19:57:02 PST 2015
On 12/07/2015 01:53 PM, Ilia Mirkin wrote:
> 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?
Probably. I guess that's slightly nicer, I'll re-spin them shortly.
>
>>
>>>
>>> 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
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nouveau/attachments/20151207/8b845470/attachment.sig>
More information about the Nouveau
mailing list