On 13/03/14 00:45, Ilia Mirkin wrote:
> On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov at gmail.com>
> wrote:
>> In theory it's possible for any of the nouveau_getparam calls to
>> fail whist the last one being successful.
>>
>> Thus at least one of the following (hard requirements) drmVersion,
>> chipset and vram/gart memory size will be filled with garbage and
>> sent to the userspace drivers.
>
> What was wrong with the old logic again? Except annoying indentation?
>
> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
> if (ret == 0)
> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
> if (ret == 0)
> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
> if (ret) {
> nouveau_device_del(&dev);
> return ret;
> }
>
> It will only run each successive getparam if the previous one succeeded...
>
Good point, the lovely indentation got me. So it seems only !ver
handling is needed.
>
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>> nouveau/nouveau.c | 30 +++++++++++++++++++-----------
>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>> index ee7893b..d6013db 100644
>> --- a/nouveau/nouveau.c
>> +++ b/nouveau/nouveau.c
>> @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct
>> nouveau_device **pdev)
>> nvdev->base.fd = fd;
>>
>> ver = drmGetVersion(fd);
>> - if (ver) dev->drm_version = (ver->version_major << 24) |
>> - (ver->version_minor << 8) |
>> - ver->version_patchlevel;
>> + if (!ver) {
>> + ret = -errno;
>> + goto error;
>> + }
>> +
>> + dev->drm_version = (ver->version_major << 24) |
>> + (ver->version_minor << 8) |
>> + ver->version_patchlevel;
>> drmFreeVersion(ver);
>>
>> if ( dev->drm_version != 0x00000010 &&
>> (dev->drm_version < 0x01000000 ||
>> dev->drm_version >= 0x02000000)) {
>> - nouveau_device_del(&dev);
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + goto error;
>> }
>>
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
>> - if (ret == 0)
>> + if (ret)
>> + goto error;
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
>> - if (ret == 0)
>> + if (ret)
>> + goto error;
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
>> - if (ret) {
>> - nouveau_device_del(&dev);
>> - return ret;
>> - }
>> + if (ret)
>> + goto error;
>>
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE,
>> &bousage);
>> if (ret == 0)
>> @@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct
>> nouveau_device **pdev)
>>
>> *pdev = &nvdev->base;
>> return 0;
>> +error:
>> + nouveau_device_del(&dev);
>> + return -ret;
>
> you mean 'ret' of course.
>
Yikes, thanks.
-Emil
>> }
>>
>> int
>> --
>> 1.9.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
