Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 30-05-17 14:02, Hans de Goede wrote:
Hi,

On 29-05-17 21:02, Daniel Vetter wrote:
On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
Since commit a39be606f99d ("drm: Do a full device unregister when
unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
drm_dev_unregister again. Since drm_dev_unregister is not protected
against being called multiple times this leads to havoc.

This commit fixes this by calling drm_dev_unref instead of drm_put_dev.

Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@xxxxxxxxx>
Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
Note I don't have any USB display devices at hand for testing atm so
this patch has only been compile tested.
---
Changes in v2:
-Remove unnecessary mutex changes

Offending patch is in 4.8 ... do we need cc: stable?

Yes I think a Cc: stable would be good.

Does this need a bugreport link?

There is no bug-report that I know of (Marco reported the issues
to me by email). This likely fixes some crashes when unplugging
USB display link devices.

Note I plan to actually test this patch with an USB display device
tomorrow, so you may want to hold on merging this till I've got
around to testing this.

So as promised I've been testing (and then debugging) this today,
this patch is no good.

Chris was right when he said maybe we should not call drm_dev_unregister()
at all, that indeed is bad and causes oopses and an unusable system on
unplug. Reverting Chris' original commit replaces those oopses with
different oopses and still results in an unusable system. I believe
that I know how to fix this now, but it is almost 10pm over here
and I think it is better if I continue this tomorrow.

In the mean time in case it was not clear: self-NACK for this patch.

Regards,

Hans




Regards,

Hans


---
  drivers/gpu/drm/drm_drv.c  | 6 +++---
  drivers/gpu/drm/drm_file.c | 2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb46a425..30b5382bf877 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -364,9 +364,9 @@ void drm_unplug_dev(struct drm_device *dev)
      drm_device_set_unplugged(dev);
-    if (dev->open_count == 0) {
-        drm_put_dev(dev);
-    }
+    if (dev->open_count == 0)
+        drm_dev_unref(dev);
+
      mutex_unlock(&drm_global_mutex);
  }
  EXPORT_SYMBOL(drm_unplug_dev);
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 3783b659cd38..edba71c8ccc3 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -424,7 +424,7 @@ int drm_release(struct inode *inode, struct file *filp)
      if (!--dev->open_count) {
          drm_lastclose(dev);
          if (drm_device_is_unplugged(dev))
-            drm_put_dev(dev);
+            drm_dev_unref(dev);
      }
      mutex_unlock(&drm_global_mutex);
--
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux