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

[Daniel Vetter]

On Wed, Feb 10, 2016 at 02:02:46PM -0800, Stéphane Marchesin wrote:
> On Wed, Feb 10, 2016 at 1:54 PM, David Herrmann  
> wrote:
> > Hi
> >
> > On Wed, Feb 10, 2016 at 10:46 PM, Stéphane Marchesin
> >  wrote:
> >> On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann  
> >> wrote:
> >>> Hi
> >>>
> >>> On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi  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 :)

Yeah, dma-buf and fence lifetime is entirely unsolved. I agree that they
/should/ keep being alive. Of course the actual data in it might be toast,
but that's not any different from a gpu hang. At least i915 only gives you
asynchronous signalling for "bad stuff happened" in that case, buffer
access to corrupted data continues to work. And must do so, because
X/compositors/clients would just die if we don't do that, and that's Not
Good(tm).

> > If you rip out hardware resources, then you better be able to deal
> > with it. Sure, UDL is an exception as it doesn't have memory resources
> > on the chip. But it sounds fishy to me, if you base your API on it. On
> > a lot of other devices, the memory will simply not be there. So you
> > cannot keep it around.
> 
> The thing is, you are not unplugging a device here; you are unplugging
> a USB monitor. As a proof that this is just a monitor, I can plug
> another USB monitor with the same driver and pick up where I left off.
> I guess I am saying that the concept of unplugging a device is not
> applicable here (or to any driver that I know, for that matter).
> 
> Other drivers already handle all this by, for example, failing page
> flips if the monitor is gone. We basically want to do the same for
> UDL; I don't think we need to invent a new level of unplug here.

Just an aside: Imo failing pageflips is really bad behaviour of some
kernel drivers (yes mst does it by force-disabling sinks). Imo userspace
should ask for things to get disabled explicitly, much less potential for
races. For mst I think the right solution is to send out the uevent, stope
enumerating the port, but keep it internally alive until it's all gone.

Something similar could make sense for uld.

> > There are many ways to invalidate memory mappings. You either unmap
> > the entire range (and user-space must deal with SIGBUS, which is
> > completely feasible and a lot of code already does it), or you replace
> > all with a zero page, or you duplicate all pages, ... IMO, user-space
> > has to start dealing with hardware unplug properly and stop pretending
> > it cannot happen.
> 
> What you are suggesting is much more complicated than you claim, for
> example if you destroy the dmabuf which is shared with another driver,
> what happens? User space definitely can't deal with that.
> 
> I think we should wait until we have unpluggable display hardware
> before inventing really complex support for it.

I agree that for now we probably should just have hacks for udl (and yeah
fixing up mst to no longer just go poof is on my todo list somewhere), and
leave the larger issue of drivers disappearing unfixed. Atm module unload
is the only real user for that (except udl ofc), and that's a developer
feature.

Fixing all the dma-buf/fence/drm device lifetime issues properly is super
hard I think. And tons of work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel 

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

[David Herrmann]

Hi

On Wed, Feb 10, 2016 at 11:02 PM, Stéphane Marchesin
 wrote:
> On Wed, Feb 10, 2016 at 1:54 PM, David Herrmann  
> wrote:
 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 :)
>>
>> If you rip out hardware resources, then you better be able to deal
>> with it. Sure, UDL is an exception as it doesn't have memory resources
>> on the chip. But it sounds fishy to me, if you base your API on it. On
>> a lot of other devices, the memory will simply not be there. So you
>> cannot keep it around.
>
> The thing is, you are not unplugging a device here; you are unplugging
> a USB monitor. As a proof that this is just a monitor, I can plug
> another USB monitor with the same driver and pick up where I left off.
> I guess I am saying that the concept of unplugging a device is not
> applicable here (or to any driver that I know, for that matter).
>
> Other drivers already handle all this by, for example, failing page
> flips if the monitor is gone. We basically want to do the same for
> UDL; I don't think we need to invent a new level of unplug here.

No, you rip out a display controller. You don't just unplug a monitor,
you remove the whole hardware. If UDL were treated on the same level
as connector-hotplugging, then you should implement it that way. But
currently it is not.

Btw., even if you do this, you still have to deal with devices going
away, and new devices being plugged. DRM does not support
CRTC/connector hotplugging, yet. Hence, making UDL a singleton driver
will not work either.

>>
>> There are many ways to invalidate memory mappings. You either unmap
>> the entire range (and user-space must deal with SIGBUS, which is
>> completely feasible and a lot of code already does it), or you replace
>> all with a zero page, or you duplicate all pages, ... IMO, user-space
>> has to start dealing with hardware unplug properly and stop pretending
>> it cannot happen.
>
> What you are suggesting is much more complicated than you claim, for
> example if you destroy the dmabuf which is shared with another driver,
> what happens? User space definitely can't deal with that.
>
> I think we should wait until we have unpluggable display hardware
> before inventing really complex support for it.

PCI Hotplugging. Has been around since a decade. Intel just got lucky
that they cannot be ripped out. The other drivers don't care (and
break horribly if you do).

Feel free to come up with a better way to deal with UDL hotplugging.
However, I for myself recommend doing it properly, on the bus level.
Like all other subsystems do.

Thanks
David

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

[David Herrmann]

Hi

On Wed, Feb 10, 2016 at 10:46 PM, Stéphane Marchesin
 wrote:
> On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann  
> wrote:
>> Hi
>>
>> On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi  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 :)

If you rip out hardware resources, then you better be able to deal
with it. Sure, UDL is an exception as it doesn't have memory resources
on the chip. But it sounds fishy to me, if you base your API on it. On
a lot of other devices, the memory will simply not be there. So you
cannot keep it around.

There are many ways to invalidate memory mappings. You either unmap
the entire range (and user-space must deal with SIGBUS, which is
completely feasible and a lot of code already does it), or you replace
all with a zero page, or you duplicate all pages, ... IMO, user-space
has to start dealing with hardware unplug properly and stop pretending
it cannot happen.

If you mmap() your filesystem, and you rip out your block device, then
you also will get SIGBUS if you access pages that are not in
pagecache. Why are graphics buffers different?

Thanks
David

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

[David Herrmann]

Hi

On Wed, Feb 10, 2016 at 10:38 PM, Haixia Shi  wrote:
> David
>
> I am having trouble getting the reference to "drm_global_mutex" to link
> correctly in drm/udl. The error is
>
> ERROR: "drm_global_mutex" [drivers/gpu/drm/udl/udl.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
>
> This seems to be only accessed in the common drm code. Do you have a
> suggestion how to get around it?

You need to add:
EXPORT_SYMBOL(drm_global_mutex);

Thanks
David

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

[David Herrmann]

Hi

On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi  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.

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

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

[David Herrmann]

Hi

On Wed, Feb 10, 2016 at 9:51 PM, Haixia Shi  wrote:
>> This should rather be:
>>
>> drm_release(inode, filp);
>> mutex_lock(_global_mutex);
>> if (!dev->open_count && udl_device_is_unplugged(dev))
>> drm_put_dev(dev);
>> mutex_unlock(_global_mutex);
>>
>> return 0;
>>
>> There is no reason to look at the return code of drm_release(), ever.
>
> But drm_release() does return a retcode. It would still make sense to return
> that as-is in case any existing code relies on it.

Nobody should ever return error codes from fops.release(). It is
completely bogus. You rather confuse generic user-space that calls
close(), than getting any benefit out of it.

But TBH, I don't care. Feel free to forward the return value. But
still, please change the order of the calls as I did.

Thanks
David

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

[David Herrmann]

Hi

On Wed, Feb 10, 2016 at 10:00 PM, Haixia Shi  wrote:
> Also note that it is not currently feasible for UDL code to access
> drm_global_mutex.

Then change this.

> Please also refer to the similar comments ("FIXME: open_count is protected
> by drm_global_mutex but that would lead to locking inversion with the driver
> load path.") in nouveau, i915 and amdgpu.

This is unrelated to your change. I don't see any locking inversion in
your patch.

Thanks
David

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

[Stéphane Marchesin]

On Wed, Feb 10, 2016 at 1:54 PM, David Herrmann  
wrote:
> Hi
>
> On Wed, Feb 10, 2016 at 10:46 PM, Stéphane Marchesin
>  wrote:
>> On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann  
>> wrote:
>>> Hi
>>>
>>> On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi  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 :)
>
> If you rip out hardware resources, then you better be able to deal
> with it. Sure, UDL is an exception as it doesn't have memory resources
> on the chip. But it sounds fishy to me, if you base your API on it. On
> a lot of other devices, the memory will simply not be there. So you
> cannot keep it around.

The thing is, you are not unplugging a device here; you are unplugging
a USB monitor. As a proof that this is just a monitor, I can plug
another USB monitor with the same driver and pick up where I left off.
I guess I am saying that the concept of unplugging a device is not
applicable here (or to any driver that I know, for that matter).

Other drivers already handle all this by, for example, failing page
flips if the monitor is gone. We basically want to do the same for
UDL; I don't think we need to invent a new level of unplug here.

>
> There are many ways to invalidate memory mappings. You either unmap
> the entire range (and user-space must deal with SIGBUS, which is
> completely feasible and a lot of code already does it), or you replace
> all with a zero page, or you duplicate all pages, ... IMO, user-space
> has to start dealing with hardware unplug properly and stop pretending
> it cannot happen.

What you are suggesting is much more complicated than you claim, for
example if you destroy the dmabuf which is shared with another driver,
what happens? User space definitely can't deal with that.

I think we should wait until we have unpluggable display hardware
before inventing really complex support for it.

Stéphane

>
> If you mmap() your filesystem, and you rip out your block device, then
> you also will get SIGBUS if you access pages that are not in
> pagecache. Why are graphics buffers different?
>
> Thanks
> David

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

[Stéphane Marchesin]

On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann  
wrote:
> Hi
>
> On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi  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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Haixia Shi]

David

I am having trouble getting the reference to "drm_global_mutex" to link
correctly in drm/udl. The error is

ERROR: "drm_global_mutex" [drivers/gpu/drm/udl/udl.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

This seems to be only accessed in the common drm code. Do you have a
suggestion how to get around it?

On Wed, Feb 10, 2016 at 1:35 PM, David Herrmann 
wrote:

> Hi
>
> On Wed, Feb 10, 2016 at 9:51 PM, Haixia Shi  wrote:
> >> This should rather be:
> >>
> >> drm_release(inode, filp);
> >> mutex_lock(_global_mutex);
> >> if (!dev->open_count && udl_device_is_unplugged(dev))
> >> drm_put_dev(dev);
> >> mutex_unlock(_global_mutex);
> >>
> >> return 0;
> >>
> >> There is no reason to look at the return code of drm_release(), ever.
> >
> > But drm_release() does return a retcode. It would still make sense to
> return
> > that as-is in case any existing code relies on it.
>
> Nobody should ever return error codes from fops.release(). It is
> completely bogus. You rather confuse generic user-space that calls
> close(), than getting any benefit out of it.
>
> But TBH, I don't care. Feel free to forward the return value. But
> still, please change the order of the calls as I did.
>
> Thanks
> David
>
-- next part --
An HTML attachment was scrubbed...
URL: 

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

[David Herrmann]

Hi

On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi  wrote:
> Remove the general drm_device_is_unplugged() checks, and move the unplugged
> flag handling logic into drm/udl. In general we want to keep driver-specific
> logic out of common drm code.
>
> Signed-off-by: Haixia Shi 
> Reviewed-by: Stéphane Marchesin 
> ---
>  drivers/gpu/drm/drm_drv.c   |  6 
>  drivers/gpu/drm/drm_fops.c  |  2 --
>  drivers/gpu/drm/drm_gem.c   |  3 --
>  drivers/gpu/drm/drm_ioctl.c |  3 --
>  drivers/gpu/drm/drm_vm.c|  3 --
>  drivers/gpu/drm/udl/udl_connector.c |  2 +-
>  drivers/gpu/drm/udl/udl_drv.c   |  7 +++--
>  drivers/gpu/drm/udl/udl_drv.h   |  6 
>  drivers/gpu/drm/udl/udl_fb.c|  2 +-
>  drivers/gpu/drm/udl/udl_gem.c   |  5 +++
>  drivers/gpu/drm/udl/udl_main.c  | 61 
> +
>  include/drm/drmP.h  | 14 -
>  12 files changed, 78 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..f93ee12 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
>
> if (!minor) {
> return ERR_PTR(-ENODEV);
> -   } else if (drm_device_is_unplugged(minor->dev)) {
> -   drm_dev_unref(minor->dev);
> -   return ERR_PTR(-ENODEV);
> }
>
> return minor;
> @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
> drm_minor_unregister(dev, DRM_MINOR_CONTROL);
>
> mutex_lock(_global_mutex);
> -
> -   drm_device_set_unplugged(dev);
> -
> if (dev->open_count == 0) {
> drm_put_dev(dev);
> }
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 1ea8790..b4332d4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
>
> if (!--dev->open_count) {
> retcode = drm_lastclose(dev);
> -   if (drm_device_is_unplugged(dev))
> -   drm_put_dev(dev);
> }
> mutex_unlock(_global_mutex);
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e8c77e..c622e32 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct 
> *vma)
> struct drm_vma_offset_node *node;
> int ret;
>
> -   if (drm_device_is_unplugged(dev))
> -   return -ENODEV;
> -
> drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>   vma->vm_pgoff,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 8ce2a0c..f959074 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
>
> dev = file_priv->minor->dev;
>
> -   if (drm_device_is_unplugged(dev))
> -   return -ENODEV;
> -
> is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
>
> if (is_driver_ioctl) {
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index f90bd5f..3a68be4 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct 
> vm_area_struct *vma)
> struct drm_device *dev = priv->minor->dev;
> int ret;
>
> -   if (drm_device_is_unplugged(dev))
> -   return -ENODEV;
> -
> mutex_lock(>struct_mutex);
> ret = drm_mmap_locked(filp, vma);
> mutex_unlock(>struct_mutex);
> diff --git a/drivers/gpu/drm/udl/udl_connector.c 
> b/drivers/gpu/drm/udl/udl_connector.c
> index 4709b54..b168f2c 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector,
>  static enum drm_connector_status
>  udl_detect(struct drm_connector *connector, bool force)
>  {
> -   if (drm_device_is_unplugged(connector->dev))
> +   if (udl_device_is_unplugged(connector->dev))
> return connector_status_disconnected;
> return connector_status_connected;
>  }
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index d5728ec..f5c2a97 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -24,12 +24,12 @@ static const struct vm_operations_struct udl_gem_vm_ops = 
> {
>
>  static const struct file_operations udl_driver_fops = {
> .owner = THIS_MODULE,
> -   .open = drm_open,
> +   .open = udl_drm_open,
> .mmap = udl_drm_gem_mmap,
> .poll = drm_poll,
> 

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

[Haixia Shi]

Also note that it is not currently feasible for UDL code to access
drm_global_mutex.

Please also refer to the similar comments ("FIXME: open_count is protected
by drm_global_mutex but that would lead to locking inversion with the
driver load path.") in nouveau, i915 and amdgpu.

On Wed, Feb 10, 2016 at 12:51 PM, Haixia Shi  wrote:

> > This should rather be:
> >
> > drm_release(inode, filp);
> > mutex_lock(_global_mutex);
> > if (!dev->open_count && udl_device_is_unplugged(dev))
> > drm_put_dev(dev);
> > mutex_unlock(_global_mutex);
> >
> > return 0;
> >
> > There is no reason to look at the return code of drm_release(), ever.
>
> But drm_release() does return a retcode. It would still make sense to
> return that as-is in case any existing code relies on it.
>
> On Wed, Feb 10, 2016 at 4:24 AM, David Herrmann 
> wrote:
>
>> Hi
>>
>> On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi  wrote:
>> > Remove the general drm_device_is_unplugged() checks, and move the
>> unplugged
>> > flag handling logic into drm/udl. In general we want to keep
>> driver-specific
>> > logic out of common drm code.
>> >
>> > Signed-off-by: Haixia Shi 
>> > Reviewed-by: Stéphane Marchesin 
>> > ---
>> >  drivers/gpu/drm/drm_drv.c   |  6 
>> >  drivers/gpu/drm/drm_fops.c  |  2 --
>> >  drivers/gpu/drm/drm_gem.c   |  3 --
>> >  drivers/gpu/drm/drm_ioctl.c |  3 --
>> >  drivers/gpu/drm/drm_vm.c|  3 --
>> >  drivers/gpu/drm/udl/udl_connector.c |  2 +-
>> >  drivers/gpu/drm/udl/udl_drv.c   |  7 +++--
>> >  drivers/gpu/drm/udl/udl_drv.h   |  6 
>> >  drivers/gpu/drm/udl/udl_fb.c|  2 +-
>> >  drivers/gpu/drm/udl/udl_gem.c   |  5 +++
>> >  drivers/gpu/drm/udl/udl_main.c  | 61
>> +
>> >  include/drm/drmP.h  | 14 -
>> >  12 files changed, 78 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> > index 167c8d3..f93ee12 100644
>> > --- a/drivers/gpu/drm/drm_drv.c
>> > +++ b/drivers/gpu/drm/drm_drv.c
>> > @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
>> minor_id)
>> >
>> > if (!minor) {
>> > return ERR_PTR(-ENODEV);
>> > -   } else if (drm_device_is_unplugged(minor->dev)) {
>> > -   drm_dev_unref(minor->dev);
>> > -   return ERR_PTR(-ENODEV);
>> > }
>> >
>> > return minor;
>> > @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
>> > drm_minor_unregister(dev, DRM_MINOR_CONTROL);
>> >
>> > mutex_lock(_global_mutex);
>> > -
>> > -   drm_device_set_unplugged(dev);
>> > -
>> > if (dev->open_count == 0) {
>> > drm_put_dev(dev);
>> > }
>> > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> > index 1ea8790..b4332d4 100644
>> > --- a/drivers/gpu/drm/drm_fops.c
>> > +++ b/drivers/gpu/drm/drm_fops.c
>> > @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
>> *filp)
>> >
>> > if (!--dev->open_count) {
>> > retcode = drm_lastclose(dev);
>> > -   if (drm_device_is_unplugged(dev))
>> > -   drm_put_dev(dev);
>> > }
>> > mutex_unlock(_global_mutex);
>> >
>> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> > index 2e8c77e..c622e32 100644
>> > --- a/drivers/gpu/drm/drm_gem.c
>> > +++ b/drivers/gpu/drm/drm_gem.c
>> > @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>> > struct drm_vma_offset_node *node;
>> > int ret;
>> >
>> > -   if (drm_device_is_unplugged(dev))
>> > -   return -ENODEV;
>> > -
>> > drm_vma_offset_lock_lookup(dev->vma_offset_manager);
>> > node =
>> drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>> >   vma->vm_pgoff,
>> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> > index 8ce2a0c..f959074 100644
>> > --- a/drivers/gpu/drm/drm_ioctl.c
>> > +++ b/drivers/gpu/drm/drm_ioctl.c
>> > @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
>> >
>> > dev = file_priv->minor->dev;
>> >
>> > -   if (drm_device_is_unplugged(dev))
>> > -   return -ENODEV;
>> > -
>> > is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr <
>> DRM_COMMAND_END;
>> >
>> > if (is_driver_ioctl) {
>> > diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
>> > index f90bd5f..3a68be4 100644
>> > --- a/drivers/gpu/drm/drm_vm.c
>> > +++ b/drivers/gpu/drm/drm_vm.c
>> > @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>> > struct drm_device *dev = priv->minor->dev;
>> > int ret;
>> >
>> > -   if (drm_device_is_unplugged(dev))
>> > -   return -ENODEV;
>> > -
>> > 

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

[Haixia Shi]

> This should rather be:
>
> drm_release(inode, filp);
> mutex_lock(_global_mutex);
> if (!dev->open_count && udl_device_is_unplugged(dev))
> drm_put_dev(dev);
> mutex_unlock(_global_mutex);
>
> return 0;
>
> There is no reason to look at the return code of drm_release(), ever.

But drm_release() does return a retcode. It would still make sense to
return that as-is in case any existing code relies on it.

On Wed, Feb 10, 2016 at 4:24 AM, David Herrmann 
wrote:

> Hi
>
> On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi  wrote:
> > Remove the general drm_device_is_unplugged() checks, and move the
> unplugged
> > flag handling logic into drm/udl. In general we want to keep
> driver-specific
> > logic out of common drm code.
> >
> > Signed-off-by: Haixia Shi 
> > Reviewed-by: Stéphane Marchesin 
> > ---
> >  drivers/gpu/drm/drm_drv.c   |  6 
> >  drivers/gpu/drm/drm_fops.c  |  2 --
> >  drivers/gpu/drm/drm_gem.c   |  3 --
> >  drivers/gpu/drm/drm_ioctl.c |  3 --
> >  drivers/gpu/drm/drm_vm.c|  3 --
> >  drivers/gpu/drm/udl/udl_connector.c |  2 +-
> >  drivers/gpu/drm/udl/udl_drv.c   |  7 +++--
> >  drivers/gpu/drm/udl/udl_drv.h   |  6 
> >  drivers/gpu/drm/udl/udl_fb.c|  2 +-
> >  drivers/gpu/drm/udl/udl_gem.c   |  5 +++
> >  drivers/gpu/drm/udl/udl_main.c  | 61
> +
> >  include/drm/drmP.h  | 14 -
> >  12 files changed, 78 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 167c8d3..f93ee12 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
> minor_id)
> >
> > if (!minor) {
> > return ERR_PTR(-ENODEV);
> > -   } else if (drm_device_is_unplugged(minor->dev)) {
> > -   drm_dev_unref(minor->dev);
> > -   return ERR_PTR(-ENODEV);
> > }
> >
> > return minor;
> > @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
> > drm_minor_unregister(dev, DRM_MINOR_CONTROL);
> >
> > mutex_lock(_global_mutex);
> > -
> > -   drm_device_set_unplugged(dev);
> > -
> > if (dev->open_count == 0) {
> > drm_put_dev(dev);
> > }
> > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > index 1ea8790..b4332d4 100644
> > --- a/drivers/gpu/drm/drm_fops.c
> > +++ b/drivers/gpu/drm/drm_fops.c
> > @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
> *filp)
> >
> > if (!--dev->open_count) {
> > retcode = drm_lastclose(dev);
> > -   if (drm_device_is_unplugged(dev))
> > -   drm_put_dev(dev);
> > }
> > mutex_unlock(_global_mutex);
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 2e8c77e..c622e32 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
> vm_area_struct *vma)
> > struct drm_vma_offset_node *node;
> > int ret;
> >
> > -   if (drm_device_is_unplugged(dev))
> > -   return -ENODEV;
> > -
> > drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> > node =
> drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> >   vma->vm_pgoff,
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 8ce2a0c..f959074 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
> >
> > dev = file_priv->minor->dev;
> >
> > -   if (drm_device_is_unplugged(dev))
> > -   return -ENODEV;
> > -
> > is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
> >
> > if (is_driver_ioctl) {
> > diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> > index f90bd5f..3a68be4 100644
> > --- a/drivers/gpu/drm/drm_vm.c
> > +++ b/drivers/gpu/drm/drm_vm.c
> > @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct
> vm_area_struct *vma)
> > struct drm_device *dev = priv->minor->dev;
> > int ret;
> >
> > -   if (drm_device_is_unplugged(dev))
> > -   return -ENODEV;
> > -
> > mutex_lock(>struct_mutex);
> > ret = drm_mmap_locked(filp, vma);
> > mutex_unlock(>struct_mutex);
> > diff --git a/drivers/gpu/drm/udl/udl_connector.c
> b/drivers/gpu/drm/udl/udl_connector.c
> > index 4709b54..b168f2c 100644
> > --- a/drivers/gpu/drm/udl/udl_connector.c
> > +++ b/drivers/gpu/drm/udl/udl_connector.c
> > @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector
> *connector,
> >  static enum drm_connector_status
> >  udl_detect(struct 

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

[Haixia Shi]

> +   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/


On Wed, Feb 10, 2016 at 4:24 AM, David Herrmann 
wrote:

> Hi
>
> On Tue, Feb 9, 2016 at 10:05 PM, Haixia Shi  wrote:
> > Remove the general drm_device_is_unplugged() checks, and move the
> unplugged
> > flag handling logic into drm/udl. In general we want to keep
> driver-specific
> > logic out of common drm code.
> >
> > Signed-off-by: Haixia Shi 
> > Reviewed-by: Stéphane Marchesin 
> > ---
> >  drivers/gpu/drm/drm_drv.c   |  6 
> >  drivers/gpu/drm/drm_fops.c  |  2 --
> >  drivers/gpu/drm/drm_gem.c   |  3 --
> >  drivers/gpu/drm/drm_ioctl.c |  3 --
> >  drivers/gpu/drm/drm_vm.c|  3 --
> >  drivers/gpu/drm/udl/udl_connector.c |  2 +-
> >  drivers/gpu/drm/udl/udl_drv.c   |  7 +++--
> >  drivers/gpu/drm/udl/udl_drv.h   |  6 
> >  drivers/gpu/drm/udl/udl_fb.c|  2 +-
> >  drivers/gpu/drm/udl/udl_gem.c   |  5 +++
> >  drivers/gpu/drm/udl/udl_main.c  | 61
> +
> >  include/drm/drmP.h  | 14 -
> >  12 files changed, 78 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 167c8d3..f93ee12 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
> minor_id)
> >
> > if (!minor) {
> > return ERR_PTR(-ENODEV);
> > -   } else if (drm_device_is_unplugged(minor->dev)) {
> > -   drm_dev_unref(minor->dev);
> > -   return ERR_PTR(-ENODEV);
> > }
> >
> > return minor;
> > @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
> > drm_minor_unregister(dev, DRM_MINOR_CONTROL);
> >
> > mutex_lock(_global_mutex);
> > -
> > -   drm_device_set_unplugged(dev);
> > -
> > if (dev->open_count == 0) {
> > drm_put_dev(dev);
> > }
> > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > index 1ea8790..b4332d4 100644
> > --- a/drivers/gpu/drm/drm_fops.c
> > +++ b/drivers/gpu/drm/drm_fops.c
> > @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
> *filp)
> >
> > if (!--dev->open_count) {
> > retcode = drm_lastclose(dev);
> > -   if (drm_device_is_unplugged(dev))
> > -   drm_put_dev(dev);
> > }
> > mutex_unlock(_global_mutex);
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 2e8c77e..c622e32 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
> vm_area_struct *vma)
> > struct drm_vma_offset_node *node;
> > int ret;
> >
> > -   if (drm_device_is_unplugged(dev))
> > -   return -ENODEV;
> > -
> > drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> > node =
> drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> >   vma->vm_pgoff,
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 8ce2a0c..f959074 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
> >
> > dev = file_priv->minor->dev;
> >
> > -   if (drm_device_is_unplugged(dev))
> > -   return -ENODEV;
> > -
> > is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
> >
> > if (is_driver_ioctl) {
> > diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> > index f90bd5f..3a68be4 100644
> > --- a/drivers/gpu/drm/drm_vm.c
> > +++ b/drivers/gpu/drm/drm_vm.c
> > @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct
> vm_area_struct *vma)
> > struct drm_device *dev = 

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

[Daniel Vetter]

On Tue, Feb 09, 2016 at 01:05:43PM -0800, Haixia Shi wrote:
> Remove the general drm_device_is_unplugged() checks, and move the unplugged
> flag handling logic into drm/udl. In general we want to keep driver-specific
> logic out of common drm code.
> 
> Signed-off-by: Haixia Shi 
> Reviewed-by: Stéphane Marchesin 

Looks like this is v2 to address David's feedback, but doesn't mention
that anywhere. Please resend with:
- v2 added, plus a small in-patch changelog of what you've changed in the
  commit message (see other patches on dri-devel).
- add David to the Cc: section of the patch to make sure he doesn't miss
  it.

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_drv.c   |  6 
>  drivers/gpu/drm/drm_fops.c  |  2 --
>  drivers/gpu/drm/drm_gem.c   |  3 --
>  drivers/gpu/drm/drm_ioctl.c |  3 --
>  drivers/gpu/drm/drm_vm.c|  3 --
>  drivers/gpu/drm/udl/udl_connector.c |  2 +-
>  drivers/gpu/drm/udl/udl_drv.c   |  7 +++--
>  drivers/gpu/drm/udl/udl_drv.h   |  6 
>  drivers/gpu/drm/udl/udl_fb.c|  2 +-
>  drivers/gpu/drm/udl/udl_gem.c   |  5 +++
>  drivers/gpu/drm/udl/udl_main.c  | 61 
> +
>  include/drm/drmP.h  | 14 -
>  12 files changed, 78 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..f93ee12 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
>  
>   if (!minor) {
>   return ERR_PTR(-ENODEV);
> - } else if (drm_device_is_unplugged(minor->dev)) {
> - drm_dev_unref(minor->dev);
> - return ERR_PTR(-ENODEV);
>   }
>  
>   return minor;
> @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
>   drm_minor_unregister(dev, DRM_MINOR_CONTROL);
>  
>   mutex_lock(_global_mutex);
> -
> - drm_device_set_unplugged(dev);
> -
>   if (dev->open_count == 0) {
>   drm_put_dev(dev);
>   }
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 1ea8790..b4332d4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
>  
>   if (!--dev->open_count) {
>   retcode = drm_lastclose(dev);
> - if (drm_device_is_unplugged(dev))
> - drm_put_dev(dev);
>   }
>   mutex_unlock(_global_mutex);
>  
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e8c77e..c622e32 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct 
> *vma)
>   struct drm_vma_offset_node *node;
>   int ret;
>  
> - if (drm_device_is_unplugged(dev))
> - return -ENODEV;
> -
>   drm_vma_offset_lock_lookup(dev->vma_offset_manager);
>   node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> vma->vm_pgoff,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 8ce2a0c..f959074 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
>  
>   dev = file_priv->minor->dev;
>  
> - if (drm_device_is_unplugged(dev))
> - return -ENODEV;
> -
>   is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
>  
>   if (is_driver_ioctl) {
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index f90bd5f..3a68be4 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct 
> vm_area_struct *vma)
>   struct drm_device *dev = priv->minor->dev;
>   int ret;
>  
> - if (drm_device_is_unplugged(dev))
> - return -ENODEV;
> -
>   mutex_lock(>struct_mutex);
>   ret = drm_mmap_locked(filp, vma);
>   mutex_unlock(>struct_mutex);
> diff --git a/drivers/gpu/drm/udl/udl_connector.c 
> b/drivers/gpu/drm/udl/udl_connector.c
> index 4709b54..b168f2c 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector,
>  static enum drm_connector_status
>  udl_detect(struct drm_connector *connector, bool force)
>  {
> - if (drm_device_is_unplugged(connector->dev))
> + if (udl_device_is_unplugged(connector->dev))
>   return connector_status_disconnected;
>   return connector_status_connected;
>  }
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index d5728ec..f5c2a97 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -24,12 +24,12 @@ static const struct 

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

[David Herrmann]

Hi

On Tue, Feb 9, 2016 at 9:45 PM, Haixia Shi  wrote:
> Regarding the following comment:
>
>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> index 1ea8790..b4332d4 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
>> *filp)
>>
>> if (!--dev->open_count) {
>> retcode = drm_lastclose(dev);
>> -   if (drm_device_is_unplugged(dev))
>> -   drm_put_dev(dev);
>
>>Again, you cannot drop this without replacement! In this case, you
>>really should wrap fops.release() from UDL and call into drm_release()
>>before copying this unplug-logic.
>
> Does this really work? drm_release() will release minor at the end,
> regardless of dev->open_count.

"open_count" is about "drm_device". "drm_minor" is completely unrelated to this.

> If minor is released, can I still safely get dev and call drm_put_dev(dev)
> afterwards?

Of course!

David

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

[David Herrmann]

Hi

On Fri, Feb 5, 2016 at 10:57 PM, Haixia Shi  wrote:
> Remove the general drm_device_is_unplugged() checks, and move the unplugged
> flag handling logic into drm/udl. In general we want to keep driver-specific
> logic out of common drm code.

I like the idea of moving this hack into udl. However, I don't think
this patch is sufficient, see below for comments.

Btw., hotplug might happen on other buses as well. It just happens
that no-one tried pci/platform unplugging so far.. We used to have
some patches flying around to make it work properly (with enter/leave
markers), but no-one picked those up. But I like the idea of moving
this unplugged marker into UDL, to make clear if someone else needs
it, they better do it properly.

> Signed-off-by: Haixia Shi 
> Reviewed-by: Stéphane Marchesin 
> ---
>  drivers/gpu/drm/drm_drv.c   |  6 --
>  drivers/gpu/drm/drm_fops.c  |  2 --
>  drivers/gpu/drm/drm_gem.c   |  3 ---
>  drivers/gpu/drm/drm_ioctl.c |  3 ---
>  drivers/gpu/drm/drm_vm.c|  3 ---
>  drivers/gpu/drm/udl/udl_connector.c |  2 +-
>  drivers/gpu/drm/udl/udl_drv.c   |  1 +
>  drivers/gpu/drm/udl/udl_drv.h   |  3 +++
>  drivers/gpu/drm/udl/udl_fb.c|  2 +-
>  drivers/gpu/drm/udl/udl_main.c  | 15 +++
>  include/drm/drmP.h  | 14 --
>  11 files changed, 21 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..f93ee12 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
>
> if (!minor) {
> return ERR_PTR(-ENODEV);
> -   } else if (drm_device_is_unplugged(minor->dev)) {
> -   drm_dev_unref(minor->dev);
> -   return ERR_PTR(-ENODEV);
> }

You cannot just drop this without a replacement. You should add a
drm_driver.open callback to UDL which checks for this.
drm_minor_acquire() is only ever called from drm_stub_open(), and as
such only from drm_open().

Alternatively, you can provide fops.open from UDL and wrap drm_open().

>
> return minor;
> @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
> drm_minor_unregister(dev, DRM_MINOR_CONTROL);
>
> mutex_lock(_global_mutex);
> -
> -   drm_device_set_unplugged(dev);
> -

You replace this by moving it into the caller of drm_unplug_dev().
Sounds good to me. There has never been a real reason to put it
underneath the global mutex, anyway.

> if (dev->open_count == 0) {
> drm_put_dev(dev);
> }
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 1ea8790..b4332d4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)
>
> if (!--dev->open_count) {
> retcode = drm_lastclose(dev);
> -   if (drm_device_is_unplugged(dev))
> -   drm_put_dev(dev);

Again, you cannot drop this without replacement! In this case, you
really should wrap fops.release() from UDL and call into drm_release()
before copying this unplug-logic.

> }
> mutex_unlock(_global_mutex);
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e8c77e..c622e32 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct 
> *vma)
> struct drm_vma_offset_node *node;
> int ret;
>
> -   if (drm_device_is_unplugged(dev))
> -   return -ENODEV;
> -

Rather than just dropping it, you better move it into udl_drm_gem_mmap().

> drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>   vma->vm_pgoff,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 8ce2a0c..f959074 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,
>
> dev = file_priv->minor->dev;
>
> -   if (drm_device_is_unplugged(dev))
> -   return -ENODEV;
> -

You *must* provide a wrapper for fops.ioctl() in udl which does this check.

> is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
>
> if (is_driver_ioctl) {
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index f90bd5f..3a68be4 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct 
> vm_area_struct *vma)
> struct drm_device *dev = priv->minor->dev;
> int ret;
>
> -   if (drm_device_is_unplugged(dev))
> -   return -ENODEV;
> -

This looks 

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

[Haixia Shi]

Remove the general drm_device_is_unplugged() checks, and move the unplugged
flag handling logic into drm/udl. In general we want to keep driver-specific
logic out of common drm code.

Signed-off-by: Haixia Shi 
Reviewed-by: Stéphane Marchesin 
---
 drivers/gpu/drm/drm_drv.c   |  6 
 drivers/gpu/drm/drm_fops.c  |  2 --
 drivers/gpu/drm/drm_gem.c   |  3 --
 drivers/gpu/drm/drm_ioctl.c |  3 --
 drivers/gpu/drm/drm_vm.c|  3 --
 drivers/gpu/drm/udl/udl_connector.c |  2 +-
 drivers/gpu/drm/udl/udl_drv.c   |  7 +++--
 drivers/gpu/drm/udl/udl_drv.h   |  6 
 drivers/gpu/drm/udl/udl_fb.c|  2 +-
 drivers/gpu/drm/udl/udl_gem.c   |  5 +++
 drivers/gpu/drm/udl/udl_main.c  | 61 +
 include/drm/drmP.h  | 14 -
 12 files changed, 78 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 167c8d3..f93ee12 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)

if (!minor) {
return ERR_PTR(-ENODEV);
-   } else if (drm_device_is_unplugged(minor->dev)) {
-   drm_dev_unref(minor->dev);
-   return ERR_PTR(-ENODEV);
}

return minor;
@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
drm_minor_unregister(dev, DRM_MINOR_CONTROL);

mutex_lock(_global_mutex);
-
-   drm_device_set_unplugged(dev);
-
if (dev->open_count == 0) {
drm_put_dev(dev);
}
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 1ea8790..b4332d4 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)

if (!--dev->open_count) {
retcode = drm_lastclose(dev);
-   if (drm_device_is_unplugged(dev))
-   drm_put_dev(dev);
}
mutex_unlock(_global_mutex);

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2e8c77e..c622e32 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct 
*vma)
struct drm_vma_offset_node *node;
int ret;

-   if (drm_device_is_unplugged(dev))
-   return -ENODEV;
-
drm_vma_offset_lock_lookup(dev->vma_offset_manager);
node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
  vma->vm_pgoff,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8ce2a0c..f959074 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,

dev = file_priv->minor->dev;

-   if (drm_device_is_unplugged(dev))
-   return -ENODEV;
-
is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;

if (is_driver_ioctl) {
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index f90bd5f..3a68be4 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct 
vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev;
int ret;

-   if (drm_device_is_unplugged(dev))
-   return -ENODEV;
-
mutex_lock(>struct_mutex);
ret = drm_mmap_locked(filp, vma);
mutex_unlock(>struct_mutex);
diff --git a/drivers/gpu/drm/udl/udl_connector.c 
b/drivers/gpu/drm/udl/udl_connector.c
index 4709b54..b168f2c 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector,
 static enum drm_connector_status
 udl_detect(struct drm_connector *connector, bool force)
 {
-   if (drm_device_is_unplugged(connector->dev))
+   if (udl_device_is_unplugged(connector->dev))
return connector_status_disconnected;
return connector_status_connected;
 }
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index d5728ec..f5c2a97 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -24,12 +24,12 @@ static const struct vm_operations_struct udl_gem_vm_ops = {

 static const struct file_operations udl_driver_fops = {
.owner = THIS_MODULE,
-   .open = drm_open,
+   .open = udl_drm_open,
.mmap = udl_drm_gem_mmap,
.poll = drm_poll,
.read = drm_read,
-   .unlocked_ioctl = drm_ioctl,
-   .release = drm_release,
+   .unlocked_ioctl = udl_drm_ioctl,
+   .release = udl_drm_release,
 #ifdef CONFIG_COMPAT
.compat_ioctl = drm_compat_ioctl,
 #endif
@@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface 

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

[Haixia Shi]

Regarding the following comment:

> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 1ea8790..b4332d4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
*filp)
>
> if (!--dev->open_count) {
> retcode = drm_lastclose(dev);
> -   if (drm_device_is_unplugged(dev))
> -   drm_put_dev(dev);

>Again, you cannot drop this without replacement! In this case, you
>really should wrap fops.release() from UDL and call into drm_release()
>before copying this unplug-logic.

Does this really work? drm_release() will release minor at the end,
regardless of dev->open_count.

If minor is released, can I still safely get dev and call drm_put_dev(dev)
afterwards?

On Tue, Feb 9, 2016 at 4:44 AM, David Herrmann 
wrote:

> Hi
>
> On Fri, Feb 5, 2016 at 10:57 PM, Haixia Shi  wrote:
> > Remove the general drm_device_is_unplugged() checks, and move the
> unplugged
> > flag handling logic into drm/udl. In general we want to keep
> driver-specific
> > logic out of common drm code.
>
> I like the idea of moving this hack into udl. However, I don't think
> this patch is sufficient, see below for comments.
>
> Btw., hotplug might happen on other buses as well. It just happens
> that no-one tried pci/platform unplugging so far.. We used to have
> some patches flying around to make it work properly (with enter/leave
> markers), but no-one picked those up. But I like the idea of moving
> this unplugged marker into UDL, to make clear if someone else needs
> it, they better do it properly.
>
> > Signed-off-by: Haixia Shi 
> > Reviewed-by: Stéphane Marchesin 
> > ---
> >  drivers/gpu/drm/drm_drv.c   |  6 --
> >  drivers/gpu/drm/drm_fops.c  |  2 --
> >  drivers/gpu/drm/drm_gem.c   |  3 ---
> >  drivers/gpu/drm/drm_ioctl.c |  3 ---
> >  drivers/gpu/drm/drm_vm.c|  3 ---
> >  drivers/gpu/drm/udl/udl_connector.c |  2 +-
> >  drivers/gpu/drm/udl/udl_drv.c   |  1 +
> >  drivers/gpu/drm/udl/udl_drv.h   |  3 +++
> >  drivers/gpu/drm/udl/udl_fb.c|  2 +-
> >  drivers/gpu/drm/udl/udl_main.c  | 15 +++
> >  include/drm/drmP.h  | 14 --
> >  11 files changed, 21 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 167c8d3..f93ee12 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int
> minor_id)
> >
> > if (!minor) {
> > return ERR_PTR(-ENODEV);
> > -   } else if (drm_device_is_unplugged(minor->dev)) {
> > -   drm_dev_unref(minor->dev);
> > -   return ERR_PTR(-ENODEV);
> > }
>
> You cannot just drop this without a replacement. You should add a
> drm_driver.open callback to UDL which checks for this.
> drm_minor_acquire() is only ever called from drm_stub_open(), and as
> such only from drm_open().
>
> Alternatively, you can provide fops.open from UDL and wrap drm_open().
>
> >
> > return minor;
> > @@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
> > drm_minor_unregister(dev, DRM_MINOR_CONTROL);
> >
> > mutex_lock(_global_mutex);
> > -
> > -   drm_device_set_unplugged(dev);
> > -
>
> You replace this by moving it into the caller of drm_unplug_dev().
> Sounds good to me. There has never been a real reason to put it
> underneath the global mutex, anyway.
>
> > if (dev->open_count == 0) {
> > drm_put_dev(dev);
> > }
> > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > index 1ea8790..b4332d4 100644
> > --- a/drivers/gpu/drm/drm_fops.c
> > +++ b/drivers/gpu/drm/drm_fops.c
> > @@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file
> *filp)
> >
> > if (!--dev->open_count) {
> > retcode = drm_lastclose(dev);
> > -   if (drm_device_is_unplugged(dev))
> > -   drm_put_dev(dev);
>
> Again, you cannot drop this without replacement! In this case, you
> really should wrap fops.release() from UDL and call into drm_release()
> before copying this unplug-logic.
>
> > }
> > mutex_unlock(_global_mutex);
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 2e8c77e..c622e32 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct
> vm_area_struct *vma)
> > struct drm_vma_offset_node *node;
> > int ret;
> >
> > -   if (drm_device_is_unplugged(dev))
> > -   return -ENODEV;
> > -
>
> Rather than just dropping it, you better move it into udl_drm_gem_mmap().
>
> > drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> > node =
> 

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

[Haixia Shi]

Remove the general drm_device_is_unplugged() checks, and move the unplugged
flag handling logic into drm/udl. In general we want to keep driver-specific
logic out of common drm code.

Signed-off-by: Haixia Shi 
Reviewed-by: Stéphane Marchesin 
---
 drivers/gpu/drm/drm_drv.c   |  6 --
 drivers/gpu/drm/drm_fops.c  |  2 --
 drivers/gpu/drm/drm_gem.c   |  3 ---
 drivers/gpu/drm/drm_ioctl.c |  3 ---
 drivers/gpu/drm/drm_vm.c|  3 ---
 drivers/gpu/drm/udl/udl_connector.c |  2 +-
 drivers/gpu/drm/udl/udl_drv.c   |  1 +
 drivers/gpu/drm/udl/udl_drv.h   |  3 +++
 drivers/gpu/drm/udl/udl_fb.c|  2 +-
 drivers/gpu/drm/udl/udl_main.c  | 15 +++
 include/drm/drmP.h  | 14 --
 11 files changed, 21 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 167c8d3..f93ee12 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -376,9 +376,6 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)

if (!minor) {
return ERR_PTR(-ENODEV);
-   } else if (drm_device_is_unplugged(minor->dev)) {
-   drm_dev_unref(minor->dev);
-   return ERR_PTR(-ENODEV);
}

return minor;
@@ -464,9 +461,6 @@ void drm_unplug_dev(struct drm_device *dev)
drm_minor_unregister(dev, DRM_MINOR_CONTROL);

mutex_lock(_global_mutex);
-
-   drm_device_set_unplugged(dev);
-
if (dev->open_count == 0) {
drm_put_dev(dev);
}
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 1ea8790..b4332d4 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -497,8 +497,6 @@ int drm_release(struct inode *inode, struct file *filp)

if (!--dev->open_count) {
retcode = drm_lastclose(dev);
-   if (drm_device_is_unplugged(dev))
-   drm_put_dev(dev);
}
mutex_unlock(_global_mutex);

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2e8c77e..c622e32 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -900,9 +900,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct 
*vma)
struct drm_vma_offset_node *node;
int ret;

-   if (drm_device_is_unplugged(dev))
-   return -ENODEV;
-
drm_vma_offset_lock_lookup(dev->vma_offset_manager);
node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
  vma->vm_pgoff,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8ce2a0c..f959074 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -695,9 +695,6 @@ long drm_ioctl(struct file *filp,

dev = file_priv->minor->dev;

-   if (drm_device_is_unplugged(dev))
-   return -ENODEV;
-
is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;

if (is_driver_ioctl) {
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index f90bd5f..3a68be4 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -657,9 +657,6 @@ int drm_legacy_mmap(struct file *filp, struct 
vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev;
int ret;

-   if (drm_device_is_unplugged(dev))
-   return -ENODEV;
-
mutex_lock(>struct_mutex);
ret = drm_mmap_locked(filp, vma);
mutex_unlock(>struct_mutex);
diff --git a/drivers/gpu/drm/udl/udl_connector.c 
b/drivers/gpu/drm/udl/udl_connector.c
index 4709b54..b168f2c 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector,
 static enum drm_connector_status
 udl_detect(struct drm_connector *connector, bool force)
 {
-   if (drm_device_is_unplugged(connector->dev))
+   if (udl_device_is_unplugged(connector->dev))
return connector_status_disconnected;
return connector_status_connected;
 }
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index d5728ec..3cbafe7 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -97,6 +97,7 @@ static void udl_usb_disconnect(struct usb_interface 
*interface)
drm_connector_unplug_all(dev);
udl_fbdev_unplug(dev);
udl_drop_usb(dev);
+   udl_device_set_unplugged(dev);
drm_unplug_dev(dev);
 }

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 4a064ef..61e117a 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -65,6 +65,7 @@ struct udl_device {
atomic_t bytes_identical; /* saved effort with backbuffer comparison */
atomic_t bytes_sent; /* to usb, after compression including overhead */
atomic_t cpu_kcycles_used;