[PATCH v7 0/2] drm: rockchip: Fix rockchip drm unbind crash error

[PATCH v7 0/2] drm: rockchip: Fix rockchip drm unbind crash error

[Jeffy Chen]

Verified on rk3399 chromebook kevin(with cros 4.4 kernel), no more crashes during unbind/bind drm.

Changes in v7:
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.

Changes in v6:
Address Daniel Vetter <daniel@ffwll.ch>'s comments.

Changes in v5:
Fix wrong git account.

Changes in v2:
Fix some commit messages.

Jeffy Chen (2):
  drm: Unplug drm device when unregistering it
  drm: Prevent release fb after cleanup drm_mode_config

 drivers/gpu/drm/drm_drv.c         | 8 ++++----
 drivers/gpu/drm/drm_framebuffer.c | 5 +++++
 drivers/gpu/drm/udl/udl_drv.c     | 2 +-
 include/drm/drmP.h                | 5 +++--
 4 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.1.4

[PATCH v7 1/2] drm: Unplug drm device when unregistering it

[Jeffy Chen]

After unbinding drm, the user space may still owns the drm dev fd,
and may still be able to call drm ioctl.

We're using an unplugged state to prevent something like that, so
let's reuse it here.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>

---

Changes in v7:
Address Sean Paul <seanpaul@chromium.org>'s comments.

Changes in v6:
Address Daniel Vetter <daniel@ffwll.ch>'s comments.

Changes in v5:
Fix wrong git account.

Changes in v2:
Fix some commit messages.

 drivers/gpu/drm/drm_drv.c     | 8 ++++----
 drivers/gpu/drm/udl/udl_drv.c | 2 +-
 include/drm/drmP.h            | 5 +++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb4..ad13e20 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -357,12 +357,9 @@ EXPORT_SYMBOL(drm_put_dev);
 
 void drm_unplug_dev(struct drm_device *dev)
 {
-	/* for a USB device */
-	drm_dev_unregister(dev);
-
 	mutex_lock(&drm_global_mutex);
 
-	drm_device_set_unplugged(dev);
+	drm_device_set_plug_state(dev, false);
 
 	if (dev->open_count == 0) {
 		drm_put_dev(dev);
@@ -787,6 +784,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_modeset_register_all(dev);
 
+	drm_device_set_plug_state(dev, true);
+
 	ret = 0;
 
 	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
@@ -826,6 +825,7 @@ void drm_dev_unregister(struct drm_device *dev)
 	drm_lastclose(dev);
 
 	dev->registered = false;
+	drm_unplug_dev(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_modeset_unregister_all(dev);
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index cd8b017..5dbd916 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
 	drm_kms_helper_poll_disable(dev);
 	udl_fbdev_unplug(dev);
 	udl_drop_usb(dev);
-	drm_unplug_dev(dev);
+	drm_dev_unregister(dev);
 }
 
 /*
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3bfafcd..a9a5a64 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev,
 	return ((dev->driver->driver_features & feature) ? 1 : 0);
 }
 
-static inline void drm_device_set_unplugged(struct drm_device *dev)
+static inline void drm_device_set_plug_state(struct drm_device *dev,
+					     bool plugged)
 {
 	smp_wmb();
-	atomic_set(&dev->unplugged, 1);
+	atomic_set(&dev->unplugged, !plugged);
 }
 
 static inline int drm_device_is_unplugged(struct drm_device *dev)
-- 
2.1.4

[PATCH v7 2/2] drm: Prevent release fb after cleanup drm_mode_config

[Jeffy Chen]

We are freeing all framebuffers in drm_mode_config_cleanup without
sync the drm_file's fbs list.

So if someone try to unbind drm before release drm dev fd, the fbs
list would remain some invalid fb references. And that would cause
crash later in drm_fb_release.

Add a sanity check to prevent that.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

---

Changes in v7:
Update commit message.

Changes in v6: None
Changes in v5: None
Changes in v2: None

 drivers/gpu/drm/drm_framebuffer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index e8f9c13..03c1632 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv)
 {
 	struct drm_framebuffer *fb, *tfb;
 	struct drm_mode_rmfb_work arg;
+	struct drm_minor *minor = priv->minor;
+	struct drm_device *dev = minor->dev;
+
+	if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs)))
+		return;
 
 	INIT_LIST_HEAD(&arg.fbs);
 
-- 
2.1.4

Re: [PATCH v7 1/2] drm: Unplug drm device when unregistering it

[Daniel Vetter]

On Tue, Apr 11, 2017 at 11:31:41AM +0800, Jeffy Chen wrote:
> After unbinding drm, the user space may still owns the drm dev fd,
> and may still be able to call drm ioctl.
> 
> We're using an unplugged state to prevent something like that, so
> let's reuse it here.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
> ---
> 
> Changes in v7:
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> 
> Changes in v6:
> Address Daniel Vetter <daniel@ffwll.ch>'s comments.

Please don't just list the names, but what you've changed. As-is this
isn't very informative ... Also, in drm we prefer the patch changelog
above the --- line, for more credit to the review process.

A few questions below.
-Daniel

> 
> Changes in v5:
> Fix wrong git account.
> 
> Changes in v2:
> Fix some commit messages.
> 
>  drivers/gpu/drm/drm_drv.c     | 8 ++++----
>  drivers/gpu/drm/udl/udl_drv.c | 2 +-
>  include/drm/drmP.h            | 5 +++--
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b5c6bb4..ad13e20 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -357,12 +357,9 @@ EXPORT_SYMBOL(drm_put_dev);
>  
>  void drm_unplug_dev(struct drm_device *dev)

I think this function can now be unexported and made static?

>  {
> -	/* for a USB device */
> -	drm_dev_unregister(dev);
> -
>  	mutex_lock(&drm_global_mutex);
>  
> -	drm_device_set_unplugged(dev);
> +	drm_device_set_plug_state(dev, false);
>  
>  	if (dev->open_count == 0) {
>  		drm_put_dev(dev);
> @@ -787,6 +784,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_modeset_register_all(dev);
>  
> +	drm_device_set_plug_state(dev, true);
> +
>  	ret = 0;
>  
>  	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
> @@ -826,6 +825,7 @@ void drm_dev_unregister(struct drm_device *dev)
>  	drm_lastclose(dev);
>  
>  	dev->registered = false;
> +	drm_unplug_dev(dev);
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_modeset_unregister_all(dev);
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index cd8b017..5dbd916 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
>  	drm_kms_helper_poll_disable(dev);
>  	udl_fbdev_unplug(dev);
>  	udl_drop_usb(dev);
> -	drm_unplug_dev(dev);
> +	drm_dev_unregister(dev);
>  }
>  
>  /*
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3bfafcd..a9a5a64 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev,
>  	return ((dev->driver->driver_features & feature) ? 1 : 0);
>  }
>  
> -static inline void drm_device_set_unplugged(struct drm_device *dev)
> +static inline void drm_device_set_plug_state(struct drm_device *dev,
> +					     bool plugged)

Why this change here? I thought the plan was to just unplug _all_ devices
at unregister time? Also, I think we want to make this into a static
function in drm_drv.c

>  {
>  	smp_wmb();
> -	atomic_set(&dev->unplugged, 1);
> +	atomic_set(&dev->unplugged, !plugged);
>  }
>  
>  static inline int drm_device_is_unplugged(struct drm_device *dev)
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 1/2] drm: Unplug drm device when unregistering it

[Daniel Vetter]

On Tue, Apr 11, 2017 at 11:31:41AM +0800, Jeffy Chen wrote:
> After unbinding drm, the user space may still owns the drm dev fd,
> and may still be able to call drm ioctl.
> 
> We're using an unplugged state to prevent something like that, so
> let's reuse it here.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
> ---
> 
> Changes in v7:
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> 
> Changes in v6:
> Address Daniel Vetter <daniel@ffwll.ch>'s comments.

Please don't just list the names, but what you've changed. As-is this
isn't very informative ... Also, in drm we prefer the patch changelog
above the --- line, for more credit to the review process.

A few questions below.
-Daniel

> 
> Changes in v5:
> Fix wrong git account.
> 
> Changes in v2:
> Fix some commit messages.
> 
>  drivers/gpu/drm/drm_drv.c     | 8 ++++----
>  drivers/gpu/drm/udl/udl_drv.c | 2 +-
>  include/drm/drmP.h            | 5 +++--
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b5c6bb4..ad13e20 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -357,12 +357,9 @@ EXPORT_SYMBOL(drm_put_dev);
>  
>  void drm_unplug_dev(struct drm_device *dev)

I think this function can now be unexported and made static?

>  {
> -	/* for a USB device */
> -	drm_dev_unregister(dev);
> -
>  	mutex_lock(&drm_global_mutex);
>  
> -	drm_device_set_unplugged(dev);
> +	drm_device_set_plug_state(dev, false);
>  
>  	if (dev->open_count == 0) {
>  		drm_put_dev(dev);
> @@ -787,6 +784,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_modeset_register_all(dev);
>  
> +	drm_device_set_plug_state(dev, true);
> +
>  	ret = 0;
>  
>  	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
> @@ -826,6 +825,7 @@ void drm_dev_unregister(struct drm_device *dev)
>  	drm_lastclose(dev);
>  
>  	dev->registered = false;
> +	drm_unplug_dev(dev);
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_modeset_unregister_all(dev);
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index cd8b017..5dbd916 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
>  	drm_kms_helper_poll_disable(dev);
>  	udl_fbdev_unplug(dev);
>  	udl_drop_usb(dev);
> -	drm_unplug_dev(dev);
> +	drm_dev_unregister(dev);
>  }
>  
>  /*
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3bfafcd..a9a5a64 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev,
>  	return ((dev->driver->driver_features & feature) ? 1 : 0);
>  }
>  
> -static inline void drm_device_set_unplugged(struct drm_device *dev)
> +static inline void drm_device_set_plug_state(struct drm_device *dev,
> +					     bool plugged)

Why this change here? I thought the plan was to just unplug _all_ devices
at unregister time? Also, I think we want to make this into a static
function in drm_drv.c

>  {
>  	smp_wmb();
> -	atomic_set(&dev->unplugged, 1);
> +	atomic_set(&dev->unplugged, !plugged);
>  }
>  
>  static inline int drm_device_is_unplugged(struct drm_device *dev)
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 2/2] drm: Prevent release fb after cleanup drm_mode_config

[Daniel Vetter]

On Tue, Apr 11, 2017 at 11:31:42AM +0800, Jeffy Chen wrote:
> We are freeing all framebuffers in drm_mode_config_cleanup without
> sync the drm_file's fbs list.
> 
> So if someone try to unbind drm before release drm dev fd, the fbs
> list would remain some invalid fb references. And that would cause
> crash later in drm_fb_release.
> 
> Add a sanity check to prevent that.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

This feels like duct-tape. The problem is that when we unplug a drm
device, we don't properly clean this up. I think we should first clean up
all the drm files (and make sure all ioctl and anything else completed),
before we proceed further in the driver cleanup.

Like I said, fixing unplug is going to be serious amounts of work, not
sure you really want to do this just for a  pure debug use-cases.
-Daniel

> 
> ---
> 
> Changes in v7:
> Update commit message.
> 
> Changes in v6: None
> Changes in v5: None
> Changes in v2: None
> 
>  drivers/gpu/drm/drm_framebuffer.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index e8f9c13..03c1632 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv)
>  {
>  	struct drm_framebuffer *fb, *tfb;
>  	struct drm_mode_rmfb_work arg;
> +	struct drm_minor *minor = priv->minor;
> +	struct drm_device *dev = minor->dev;
> +
> +	if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs)))
> +		return;
>  
>  	INIT_LIST_HEAD(&arg.fbs);
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 2/2] drm: Prevent release fb after cleanup drm_mode_config

[Daniel Vetter]

On Tue, Apr 11, 2017 at 11:31:42AM +0800, Jeffy Chen wrote:
> We are freeing all framebuffers in drm_mode_config_cleanup without
> sync the drm_file's fbs list.
> 
> So if someone try to unbind drm before release drm dev fd, the fbs
> list would remain some invalid fb references. And that would cause
> crash later in drm_fb_release.
> 
> Add a sanity check to prevent that.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

This feels like duct-tape. The problem is that when we unplug a drm
device, we don't properly clean this up. I think we should first clean up
all the drm files (and make sure all ioctl and anything else completed),
before we proceed further in the driver cleanup.

Like I said, fixing unplug is going to be serious amounts of work, not
sure you really want to do this just for a  pure debug use-cases.
-Daniel

> 
> ---
> 
> Changes in v7:
> Update commit message.
> 
> Changes in v6: None
> Changes in v5: None
> Changes in v2: None
> 
>  drivers/gpu/drm/drm_framebuffer.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index e8f9c13..03c1632 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv)
>  {
>  	struct drm_framebuffer *fb, *tfb;
>  	struct drm_mode_rmfb_work arg;
> +	struct drm_minor *minor = priv->minor;
> +	struct drm_device *dev = minor->dev;
> +
> +	if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs)))
> +		return;
>  
>  	INIT_LIST_HEAD(&arg.fbs);
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/2] drm: Unplug drm device when unregistering it

[jeffy]

Hi Daniel,

On 04/12/2017 02:33 PM, Daniel Vetter wrote:
> On Tue, Apr 11, 2017 at 11:31:41AM +0800, Jeffy Chen wrote:
>> After unbinding drm, the user space may still owns the drm dev fd,
>> and may still be able to call drm ioctl.
>>
>> We're using an unplugged state to prevent something like that, so
>> let's reuse it here.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>>
>> ---
>>
>> Changes in v7:
>> Address Sean Paul <seanpaul@chromium.org>'s comments.
>>
>> Changes in v6:
>> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
>
> Please don't just list the names, but what you've changed. As-is this
> isn't very informative ... Also, in drm we prefer the patch changelog
> above the --- line, for more credit to the review process.
>
> A few questions below.
> -Daniel
>
>>
>> Changes in v5:
>> Fix wrong git account.
>>
>> Changes in v2:
>> Fix some commit messages.
>>
>>   drivers/gpu/drm/drm_drv.c     | 8 ++++----
>>   drivers/gpu/drm/udl/udl_drv.c | 2 +-
>>   include/drm/drmP.h            | 5 +++--
>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index b5c6bb4..ad13e20 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -357,12 +357,9 @@ EXPORT_SYMBOL(drm_put_dev);
>>
>>   void drm_unplug_dev(struct drm_device *dev)
>
> I think this function can now be unexported and made static?
>
>>   {
>> -	/* for a USB device */
>> -	drm_dev_unregister(dev);
>> -
>>   	mutex_lock(&drm_global_mutex);
>>
>> -	drm_device_set_unplugged(dev);
>> +	drm_device_set_plug_state(dev, false);
>>
>>   	if (dev->open_count == 0) {
>>   		drm_put_dev(dev);
>> @@ -787,6 +784,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>>   	if (drm_core_check_feature(dev, DRIVER_MODESET))
>>   		drm_modeset_register_all(dev);
>>
>> +	drm_device_set_plug_state(dev, true);
>> +
>>   	ret = 0;
>>
>>   	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>> @@ -826,6 +825,7 @@ void drm_dev_unregister(struct drm_device *dev)
>>   	drm_lastclose(dev);
>>
>>   	dev->registered = false;
>> +	drm_unplug_dev(dev);
>>
>>   	if (drm_core_check_feature(dev, DRIVER_MODESET))
>>   		drm_modeset_unregister_all(dev);
>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>> index cd8b017..5dbd916 100644
>> --- a/drivers/gpu/drm/udl/udl_drv.c
>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>> @@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
>>   	drm_kms_helper_poll_disable(dev);
>>   	udl_fbdev_unplug(dev);
>>   	udl_drop_usb(dev);
>> -	drm_unplug_dev(dev);
>> +	drm_dev_unregister(dev);
>>   }
>>
>>   /*
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index 3bfafcd..a9a5a64 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev,
>>   	return ((dev->driver->driver_features & feature) ? 1 : 0);
>>   }
>>
>> -static inline void drm_device_set_unplugged(struct drm_device *dev)
>> +static inline void drm_device_set_plug_state(struct drm_device *dev,
>> +					     bool plugged)
>
> Why this change here? I thought the plan was to just unplug _all_ devices
> at unregister time? Also, I think we want to make this into a static
> function in drm_drv.c
calling drm_unplug_dev when unregistering drm device may cause 
hang(drm_unplug_dev will try to put drm dev when open_count is 0).

so i think we only need to update the unplug state at that time?

maybe put drm_device_set_plug_state into drm_drv.c(because no one else 
would use it)?
>
>>   {
>>   	smp_wmb();
>> -	atomic_set(&dev->unplugged, 1);
>> +	atomic_set(&dev->unplugged, !plugged);
>>   }
>>
>>   static inline int drm_device_is_unplugged(struct drm_device *dev)
>> --
>> 2.1.4
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH v7 2/2] drm: Prevent release fb after cleanup drm_mode_config

[jeffy]

Hi Daniel,

On 04/12/2017 02:36 PM, Daniel Vetter wrote:
> On Tue, Apr 11, 2017 at 11:31:42AM +0800, Jeffy Chen wrote:
>> We are freeing all framebuffers in drm_mode_config_cleanup without
>> sync the drm_file's fbs list.
>>
>> So if someone try to unbind drm before release drm dev fd, the fbs
>> list would remain some invalid fb references. And that would cause
>> crash later in drm_fb_release.
>>
>> Add a sanity check to prevent that.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>
> This feels like duct-tape. The problem is that when we unplug a drm
> device, we don't properly clean this up. I think we should first clean up
> all the drm files (and make sure all ioctl and anything else completed),
> before we proceed further in the driver cleanup.
>
> Like I said, fixing unplug is going to be serious amounts of work, not
> sure you really want to do this just for a  pure debug use-cases.
> -Daniel
right, and it's ok to drop this 2 patches, the rests are already enough 
for the testing :)
>
>>
>> ---
>>
>> Changes in v7:
>> Update commit message.
>>
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v2: None
>>
>>   drivers/gpu/drm/drm_framebuffer.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> index e8f9c13..03c1632 100644
>> --- a/drivers/gpu/drm/drm_framebuffer.c
>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>> @@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv)
>>   {
>>   	struct drm_framebuffer *fb, *tfb;
>>   	struct drm_mode_rmfb_work arg;
>> +	struct drm_minor *minor = priv->minor;
>> +	struct drm_device *dev = minor->dev;
>> +
>> +	if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs)))
>> +		return;
>>
>>   	INIT_LIST_HEAD(&arg.fbs);
>>
>> --
>> 2.1.4
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH v7 1/2] drm: Unplug drm device when unregistering it

[jeffy]

Hi Daniel,

missed some questions...

On 04/12/2017 04:17 PM, jeffy wrote:
> Hi Daniel,
>
> On 04/12/2017 02:33 PM, Daniel Vetter wrote:
>> On Tue, Apr 11, 2017 at 11:31:41AM +0800, Jeffy Chen wrote:
>>> After unbinding drm, the user space may still owns the drm dev fd,
>>> and may still be able to call drm ioctl.
>>>
>>> We're using an unplugged state to prevent something like that, so
>>> let's reuse it here.
>>>
>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>>>
>>> ---
>>>
>>> Changes in v7:
>>> Address Sean Paul <seanpaul@chromium.org>'s comments.
>>>
>>> Changes in v6:
>>> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
>>
>> Please don't just list the names, but what you've changed. As-is this
>> isn't very informative ... Also, in drm we prefer the patch changelog
>> above the --- line, for more credit to the review process.
>>
oops, sorry, the --- line and changelogs are ordered by patman tool, 
i'll try to check did i config that wrong.

>> A few questions below.
>> -Daniel
>>
>>>
>>> Changes in v5:
>>> Fix wrong git account.
>>>
>>> Changes in v2:
>>> Fix some commit messages.
>>>
>>>   drivers/gpu/drm/drm_drv.c     | 8 ++++----
>>>   drivers/gpu/drm/udl/udl_drv.c | 2 +-
>>>   include/drm/drmP.h            | 5 +++--
>>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index b5c6bb4..ad13e20 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -357,12 +357,9 @@ EXPORT_SYMBOL(drm_put_dev);
>>>
>>>   void drm_unplug_dev(struct drm_device *dev)
>>
>> I think this function can now be unexported and made static?
there's a patch v8, which removed this function :)
>>
>>>   {
>>> -    /* for a USB device */
>>> -    drm_dev_unregister(dev);
>>> -
>>>       mutex_lock(&drm_global_mutex);
>>>
>>> -    drm_device_set_unplugged(dev);
>>> +    drm_device_set_plug_state(dev, false);
>>>
>>>       if (dev->open_count == 0) {
>>>           drm_put_dev(dev);
>>> @@ -787,6 +784,8 @@ int drm_dev_register(struct drm_device *dev,
>>> unsigned long flags)
>>>       if (drm_core_check_feature(dev, DRIVER_MODESET))
>>>           drm_modeset_register_all(dev);
>>>
>>> +    drm_device_set_plug_state(dev, true);
>>> +
>>>       ret = 0;
>>>
>>>       DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>>> @@ -826,6 +825,7 @@ void drm_dev_unregister(struct drm_device *dev)
>>>       drm_lastclose(dev);
>>>
>>>       dev->registered = false;
>>> +    drm_unplug_dev(dev);
>>>
>>>       if (drm_core_check_feature(dev, DRIVER_MODESET))
>>>           drm_modeset_unregister_all(dev);
>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c
>>> b/drivers/gpu/drm/udl/udl_drv.c
>>> index cd8b017..5dbd916 100644
>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>> @@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct
>>> usb_interface *interface)
>>>       drm_kms_helper_poll_disable(dev);
>>>       udl_fbdev_unplug(dev);
>>>       udl_drop_usb(dev);
>>> -    drm_unplug_dev(dev);
>>> +    drm_dev_unregister(dev);
>>>   }
>>>
>>>   /*
>>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>>> index 3bfafcd..a9a5a64 100644
>>> --- a/include/drm/drmP.h
>>> +++ b/include/drm/drmP.h
>>> @@ -488,10 +488,11 @@ static __inline__ int
>>> drm_core_check_feature(struct drm_device *dev,
>>>       return ((dev->driver->driver_features & feature) ? 1 : 0);
>>>   }
>>>
>>> -static inline void drm_device_set_unplugged(struct drm_device *dev)
>>> +static inline void drm_device_set_plug_state(struct drm_device *dev,
>>> +                         bool plugged)
>>
>> Why this change here? I thought the plan was to just unplug _all_ devices
>> at unregister time? Also, I think we want to make this into a static
>> function in drm_drv.c
> calling drm_unplug_dev when unregistering drm device may cause
> hang(drm_unplug_dev will try to put drm dev when open_count is 0).
>
> so i think we only need to update the unplug state at that time?
>
> maybe put drm_device_set_plug_state into drm_drv.c(because no one else
> would use it)?
>>
>>>   {
>>>       smp_wmb();
>>> -    atomic_set(&dev->unplugged, 1);
>>> +    atomic_set(&dev->unplugged, !plugged);
>>>   }
>>>
>>>   static inline int drm_device_is_unplugged(struct drm_device *dev)
>>> --
>>> 2.1.4
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>