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

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

[Jeffy Chen]

Verified on rk3399 chromebook kevin, no more crashes during unbind/bind drm.

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 mode config

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

-- 
2.1.4

[PATCH v6 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>
---

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     | 6 +++---
 drivers/gpu/drm/udl/udl_drv.c | 2 +-
 include/drm/drmP.h            | 6 ++++++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb4..f38de26 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -357,9 +357,6 @@ 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);
@@ -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_plugged(dev);
+
 	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..c930a77 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -488,6 +488,12 @@ 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_plugged(struct drm_device *dev)
+{
+	smp_wmb();
+	atomic_set(&dev->unplugged, 0);
+}
+
 static inline void drm_device_set_unplugged(struct drm_device *dev)
 {
 	smp_wmb();
-- 
2.1.4

[PATCH v6 2/2] drm: Prevent release fb after cleanup mode config

[Jeffy Chen]

After unbinding drm, the user space may still owns the drm dev fd,
and may trigger fb release after cleanup mode config.

Add a sanity check to prevent that.

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

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 v6 0/2] drm: rockchip: Fix rockchip drm unbind crash error

[Sean Paul]

On Mon, Apr 10, 2017 at 06:00:43PM +0800, Jeffy Chen wrote:

Hi Jeffy,
Thanks for sending this up again.

> 
> Verified on rk3399 chromebook kevin, no more crashes during unbind/bind drm.

I'm assuming this is on the chromeos-4.4 kernel? If so, you should probably
mention that when you're posting upstream.

Sean


> 
> 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 mode config
> 
>  drivers/gpu/drm/drm_drv.c         | 6 +++---
>  drivers/gpu/drm/drm_framebuffer.c | 5 +++++
>  drivers/gpu/drm/udl/udl_drv.c     | 2 +-
>  include/drm/drmP.h                | 6 ++++++
>  4 files changed, 15 insertions(+), 4 deletions(-)
> 
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Sean Paul]

On Mon, Apr 10, 2017 at 06:00:43PM +0800, Jeffy Chen wrote:

Hi Jeffy,
Thanks for sending this up again.

> 
> Verified on rk3399 chromebook kevin, no more crashes during unbind/bind drm.

I'm assuming this is on the chromeos-4.4 kernel? If so, you should probably
mention that when you're posting upstream.

Sean


> 
> 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 mode config
> 
>  drivers/gpu/drm/drm_drv.c         | 6 +++---
>  drivers/gpu/drm/drm_framebuffer.c | 5 +++++
>  drivers/gpu/drm/udl/udl_drv.c     | 2 +-
>  include/drm/drmP.h                | 6 ++++++
>  4 files changed, 15 insertions(+), 4 deletions(-)
> 
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

On Mon, Apr 10, 2017 at 06:00:44PM +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>
> ---
> 
> 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     | 6 +++---
>  drivers/gpu/drm/udl/udl_drv.c | 2 +-
>  include/drm/drmP.h            | 6 ++++++
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b5c6bb4..f38de26 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -357,9 +357,6 @@ 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);
> @@ -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_plugged(dev);
> +
>  	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..c930a77 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -488,6 +488,12 @@ 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_plugged(struct drm_device *dev)
> +{
> +	smp_wmb();
> +	atomic_set(&dev->unplugged, 0);
> +}
> +
>  static inline void drm_device_set_unplugged(struct drm_device *dev)

Instead of introducing a new function, just rename this to
drm_device_set_plug_state(struct drm_device *dev, bool plugged)

and call it from both plug and unplug

With that nit, this is

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>  {
>  	smp_wmb();
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Sean Paul]

On Mon, Apr 10, 2017 at 06:00:44PM +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>
> ---
> 
> 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     | 6 +++---
>  drivers/gpu/drm/udl/udl_drv.c | 2 +-
>  include/drm/drmP.h            | 6 ++++++
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b5c6bb4..f38de26 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -357,9 +357,6 @@ 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);
> @@ -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_plugged(dev);
> +
>  	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..c930a77 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -488,6 +488,12 @@ 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_plugged(struct drm_device *dev)
> +{
> +	smp_wmb();
> +	atomic_set(&dev->unplugged, 0);
> +}
> +
>  static inline void drm_device_set_unplugged(struct drm_device *dev)

Instead of introducing a new function, just rename this to
drm_device_set_plug_state(struct drm_device *dev, bool plugged)

and call it from both plug and unplug

With that nit, this is

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>  {
>  	smp_wmb();
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/2] drm: Prevent release fb after cleanup mode config

[Sean Paul]

On Mon, Apr 10, 2017 at 06:00:45PM +0800, Jeffy Chen wrote:
> After unbinding drm, the user space may still owns the drm dev fd,
> and may trigger fb release after cleanup mode config.
> 
> Add a sanity check to prevent that.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> 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)))

Have you actually seen this happen? num_fb should be tightly couple to
priv->fbs, so it seems like this could only result from a driver bug (or I'm not
reading the code correctly).

Sean

> +		return;
>  
>  	INIT_LIST_HEAD(&arg.fbs);
>  
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v6 2/2] drm: Prevent release fb after cleanup mode config

[Sean Paul]

On Mon, Apr 10, 2017 at 06:00:45PM +0800, Jeffy Chen wrote:
> After unbinding drm, the user space may still owns the drm dev fd,
> and may trigger fb release after cleanup mode config.
> 
> Add a sanity check to prevent that.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> 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)))

Have you actually seen this happen? num_fb should be tightly couple to
priv->fbs, so it seems like this could only result from a driver bug (or I'm not
reading the code correctly).

Sean

> +		return;
>  
>  	INIT_LIST_HEAD(&arg.fbs);
>  
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/2] drm: Prevent release fb after cleanup mode config

[jeffy]

Hi Sean,

On 04/11/2017 04:31 AM, Sean Paul wrote:
> On Mon, Apr 10, 2017 at 06:00:45PM +0800, Jeffy Chen wrote:
>> After unbinding drm, the user space may still owns the drm dev fd,
>> and may trigger fb release after cleanup mode config.
>>
>> Add a sanity check to prevent that.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> 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)))
>
> Have you actually seen this happen? num_fb should be tightly couple to
> priv->fbs, so it seems like this could only result from a driver bug (or I'm not
> reading the code correctly).
yes, 100% repro by:
1/ start display server
2/ unbind drm
3/ stop display server

the num_fb would be decreased(with a warning in 
drm_mode_config_cleanup's fb_list check) in 
drm_mode_config_cleanup->drm_framebuffer_free->rockchip_drm_fb_destroy->drm_framebuffer_cleanup

this flow would not modify the priv->fbs at the same time. so it would 
still remains the pointer of those freed fb.

>
> Sean
>
>> +		return;
>>
>>   	INIT_LIST_HEAD(&arg.fbs);
>>
>> --
>> 2.1.4
>>
>

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

[jeffy]

Hi Sean,

On 04/11/2017 03:26 AM, Sean Paul wrote:
> On Mon, Apr 10, 2017 at 06:00:43PM +0800, Jeffy Chen wrote:
>
> Hi Jeffy,
> Thanks for sending this up again.
>
>>
>> Verified on rk3399 chromebook kevin, no more crashes during unbind/bind drm.
>
> I'm assuming this is on the chromeos-4.4 kernel? If so, you should probably
> mention that when you're posting upstream.
>
right, will do in next version.
> Sean
>
>
>>
>> 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 mode config
>>
>>   drivers/gpu/drm/drm_drv.c         | 6 +++---
>>   drivers/gpu/drm/drm_framebuffer.c | 5 +++++
>>   drivers/gpu/drm/udl/udl_drv.c     | 2 +-
>>   include/drm/drmP.h                | 6 ++++++
>>   4 files changed, 15 insertions(+), 4 deletions(-)
>>
>> --
>> 2.1.4
>>
>