Re: [PATCH 2/2] drm: make unplugged flag specific to udl driver

["Stéphane Marchesin" <stephane.marchesin@gmail.com>]

On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi <hshi@chromium.org> wrote:
>>
>>> +       if (udl_device_is_unplugged(dev) &&
>>> +               nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&
>>> +               nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&
>>> +               nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))
>>> +               return -ENODEV;
>>>
>>>Why?
>>>
>>>Just do:
>>>
>>>        if (udl_device_is_unplugged(dev))
>>>                return -ENODEV;
>>>
>>>Why this complex logic here?
>>
>> Because there are legitimate ioctl calls after UDL is unplugged. See
>> crbug.com/583508 and crbug.com/583758 for some background.
>>
>> The userspace (Chrome in this case) has allocated FBs and Dumb buffers on
>> the drm device and needs to be given a chance to properly deallocate them
>> (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with
>> fb_id = 0 to properly release the last refcount on the primary fb.
>>
>> I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we
>> can whitelist them on a case-by-case basis but that proposal got shot down
>> as being unnecessary, but you can see my original patch at
>> https://chromium-review.googlesource.com/#/c/326160/
>
> If a device is unplugged, you should consider all your resources to be
> destroyed. There is no reason to release them manually. User-space
> *must* be able to deal with asynchronous unplugs.

So the problem if you do that is that things like a buffer's memory
pages can disappear from under you. How would you deal with that case?
User space certainly can't have a segfault handler catch that just in
case :)

Stéphane

>
> If UDL does not release your resources, and you actually hit bugs due
> to this, then fix UDL to do this. But extending the already broken
> UNPLUG-hacks we have sounds wrong to me.
>
> Anyway, this change is unrelated to your patch, so please drop it.
> Feel free to send a separate patch, if you want to continue the
> discussion.
>
> Thanks
> David
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel