* [PATCH v9 0/1] drm: rockchip: Fix rockchip drm unbind crash error
@ 2017-04-12 8:56 Jeffy Chen
2017-04-12 8:56 ` [PATCH v9] drm: Unplug drm device when unregistering it Jeffy Chen
0 siblings, 1 reply; 4+ messages in thread
From: Jeffy Chen @ 2017-04-12 8:56 UTC (permalink / raw)
To: linux-kernel
Cc: briannorris, dianders, tfiga, seanpaul, zyw, marcheu, mark.yao,
hshi, Jeffy Chen, Daniel Vetter, Jani Nikula, dri-devel,
Chris Wilson, David Airlie, Tom Gundersen, Dave Airlie
Verified on rk3399 chromebook kevin(with cros 4.4 kernel), no more crashes during unbind/bind drm with/out ui service running.
Changes in v9:
Move drm_device_set_plug_state into drm_drv.c .
Changes in v8:
Fix hang when unregistering drm dev with open_count 0.
Changes in v7:
Add drm_device_set_plug_state helper.
Changes in v6:
Reuse unplug status.
Changes in v5:
Fix wrong git account.
Changes in v2:
Fix some commit messages.
Jeffy Chen (1):
drm: Unplug drm device when unregistering it
drivers/gpu/drm/drm_drv.c | 26 ++++++++++----------------
drivers/gpu/drm/udl/udl_drv.c | 2 +-
include/drm/drmP.h | 6 ------
include/drm/drm_drv.h | 1 -
4 files changed, 11 insertions(+), 24 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v9] drm: Unplug drm device when unregistering it
2017-04-12 8:56 [PATCH v9 0/1] drm: rockchip: Fix rockchip drm unbind crash error Jeffy Chen
@ 2017-04-12 8:56 ` Jeffy Chen
2017-04-12 15:03 ` Sean Paul
0 siblings, 1 reply; 4+ messages in thread
From: Jeffy Chen @ 2017-04-12 8:56 UTC (permalink / raw)
To: linux-kernel
Cc: briannorris, dianders, tfiga, seanpaul, zyw, marcheu, mark.yao,
hshi, Jeffy Chen, Daniel Vetter, Jani Nikula, dri-devel,
Chris Wilson, David Airlie, Tom Gundersen, Dave Airlie
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.
Also drop drm_unplug_dev, because it would be unused after other changes.
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Changes in v9:
Move drm_device_set_plug_state into drm_drv.c .
Changes in v8:
Fix hang when unregistering drm dev with open_count 0.
Changes in v7:
Add drm_device_set_plug_state helper.
Changes in v6:
Reuse unplug status.
Changes in v5:
Fix wrong git account.
Changes in v2:
Fix some commit messages.
---
drivers/gpu/drm/drm_drv.c | 26 ++++++++++----------------
drivers/gpu/drm/udl/udl_drv.c | 2 +-
include/drm/drmP.h | 6 ------
include/drm/drm_drv.h | 1 -
4 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb4..e1da4d1 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -355,22 +355,6 @@ void drm_put_dev(struct drm_device *dev)
}
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);
-
- if (dev->open_count == 0) {
- drm_put_dev(dev);
- }
- mutex_unlock(&drm_global_mutex);
-}
-EXPORT_SYMBOL(drm_unplug_dev);
-
/*
* DRM internal mount
* We want to be able to allocate our own "struct address_space" to control
@@ -733,6 +717,13 @@ static void remove_compat_control_link(struct drm_device *dev)
kfree(name);
}
+static inline void drm_device_set_plug_state(struct drm_device *dev,
+ bool plugged)
+{
+ smp_wmb();
+ atomic_set(&dev->unplugged, !plugged);
+}
+
/**
* drm_dev_register - Register DRM device
* @dev: Device to register
@@ -787,6 +778,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 +819,7 @@ void drm_dev_unregister(struct drm_device *dev)
drm_lastclose(dev);
dev->registered = false;
+ drm_device_set_plug_state(dev, false);
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..980a204 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -488,12 +488,6 @@ 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)
-{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
-}
-
static inline int drm_device_is_unplugged(struct drm_device *dev)
{
int ret = atomic_read(&dev->unplugged);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 0fefc3f..eb63078 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -544,7 +544,6 @@ void drm_dev_unregister(struct drm_device *dev);
void drm_dev_ref(struct drm_device *dev);
void drm_dev_unref(struct drm_device *dev);
void drm_put_dev(struct drm_device *dev);
-void drm_unplug_dev(struct drm_device *dev);
int drm_dev_set_unique(struct drm_device *dev, const char *name);
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v9] drm: Unplug drm device when unregistering it
2017-04-12 8:56 ` [PATCH v9] drm: Unplug drm device when unregistering it Jeffy Chen
@ 2017-04-12 15:03 ` Sean Paul
2017-04-13 1:28 ` jeffy
0 siblings, 1 reply; 4+ messages in thread
From: Sean Paul @ 2017-04-12 15:03 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, briannorris, dianders, tfiga, seanpaul, zyw,
marcheu, mark.yao, hshi, Daniel Vetter, Jani Nikula, dri-devel,
Chris Wilson, David Airlie, Tom Gundersen, Dave Airlie
On Wed, Apr 12, 2017 at 04:56:21PM +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.
>
> Also drop drm_unplug_dev, because it would be unused after other changes.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>
> Changes in v9:
> Move drm_device_set_plug_state into drm_drv.c .
>
> Changes in v8:
> Fix hang when unregistering drm dev with open_count 0.
>
> Changes in v7:
> Add drm_device_set_plug_state helper.
>
> Changes in v6:
> Reuse unplug status.
>
> Changes in v5:
> Fix wrong git account.
>
> Changes in v2:
> Fix some commit messages.
>
Hi Jeffy,
A couple bookkeeping notes:
- Please end the commit message with your Signed-off-by tag
- No need to send cover letters for one patch. All info should be in the commit
message. Include superfluous information (such as tested on 4.4 kernel) after
---
> ---
> drivers/gpu/drm/drm_drv.c | 26 ++++++++++----------------
> drivers/gpu/drm/udl/udl_drv.c | 2 +-
> include/drm/drmP.h | 6 ------
> include/drm/drm_drv.h | 1 -
> 4 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b5c6bb4..e1da4d1 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -355,22 +355,6 @@ void drm_put_dev(struct drm_device *dev)
> }
> 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);
> -
> - if (dev->open_count == 0) {
> - drm_put_dev(dev);
> - }
> - mutex_unlock(&drm_global_mutex);
> -}
> -EXPORT_SYMBOL(drm_unplug_dev);
> -
> /*
> * DRM internal mount
> * We want to be able to allocate our own "struct address_space" to control
> @@ -733,6 +717,13 @@ static void remove_compat_control_link(struct drm_device *dev)
> kfree(name);
> }
>
> +static inline void drm_device_set_plug_state(struct drm_device *dev,
> + bool plugged)
> +{
> + smp_wmb();
> + atomic_set(&dev->unplugged, !plugged);
> +}
> +
> /**
> * drm_dev_register - Register DRM device
> * @dev: Device to register
> @@ -787,6 +778,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 +819,7 @@ void drm_dev_unregister(struct drm_device *dev)
> drm_lastclose(dev);
>
> dev->registered = false;
> + drm_device_set_plug_state(dev, false);
>
> 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);
I think you need drm_dev_unref here as well.
Sean
> }
>
> /*
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3bfafcd..980a204 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -488,12 +488,6 @@ 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)
> -{
> - smp_wmb();
> - atomic_set(&dev->unplugged, 1);
> -}
> -
> static inline int drm_device_is_unplugged(struct drm_device *dev)
> {
> int ret = atomic_read(&dev->unplugged);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 0fefc3f..eb63078 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -544,7 +544,6 @@ void drm_dev_unregister(struct drm_device *dev);
> void drm_dev_ref(struct drm_device *dev);
> void drm_dev_unref(struct drm_device *dev);
> void drm_put_dev(struct drm_device *dev);
> -void drm_unplug_dev(struct drm_device *dev);
>
> int drm_dev_set_unique(struct drm_device *dev, const char *name);
>
> --
> 2.1.4
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v9] drm: Unplug drm device when unregistering it
2017-04-12 15:03 ` Sean Paul
@ 2017-04-13 1:28 ` jeffy
0 siblings, 0 replies; 4+ messages in thread
From: jeffy @ 2017-04-13 1:28 UTC (permalink / raw)
To: Sean Paul
Cc: linux-kernel, briannorris, dianders, tfiga, zyw, marcheu,
mark.yao, hshi, Daniel Vetter, Jani Nikula, dri-devel,
Chris Wilson, David Airlie, Tom Gundersen, Dave Airlie
Hi Sean,
On 04/12/2017 11:03 PM, Sean Paul wrote:
> On Wed, Apr 12, 2017 at 04:56:21PM +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.
>>
>> Also drop drm_unplug_dev, because it would be unused after other changes.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>
>> Changes in v9:
>> Move drm_device_set_plug_state into drm_drv.c .
>>
>> Changes in v8:
>> Fix hang when unregistering drm dev with open_count 0.
>>
>> Changes in v7:
>> Add drm_device_set_plug_state helper.
>>
>> Changes in v6:
>> Reuse unplug status.
>>
>> Changes in v5:
>> Fix wrong git account.
>>
>> Changes in v2:
>> Fix some commit messages.
>>
>
> Hi Jeffy,
> A couple bookkeeping notes:
>
> - Please end the commit message with your Signed-off-by tag
> - No need to send cover letters for one patch. All info should be in the commit
> message. Include superfluous information (such as tested on 4.4 kernel) after
> ---
hmm...i'm using u-boot's patman tool to send patches. and the changelogs
would be formated by the tool.
i'll switch changelogs to drm style.
>
>> ---
>> drivers/gpu/drm/drm_drv.c | 26 ++++++++++----------------
>> drivers/gpu/drm/udl/udl_drv.c | 2 +-
>> include/drm/drmP.h | 6 ------
>> include/drm/drm_drv.h | 1 -
>> 4 files changed, 11 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index b5c6bb4..e1da4d1 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -355,22 +355,6 @@ void drm_put_dev(struct drm_device *dev)
>> }
>> 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);
>> -
>> - if (dev->open_count == 0) {
>> - drm_put_dev(dev);
>> - }
>> - mutex_unlock(&drm_global_mutex);
>> -}
>> -EXPORT_SYMBOL(drm_unplug_dev);
>> -
>> /*
>> * DRM internal mount
>> * We want to be able to allocate our own "struct address_space" to control
>> @@ -733,6 +717,13 @@ static void remove_compat_control_link(struct drm_device *dev)
>> kfree(name);
>> }
>>
>> +static inline void drm_device_set_plug_state(struct drm_device *dev,
>> + bool plugged)
>> +{
>> + smp_wmb();
>> + atomic_set(&dev->unplugged, !plugged);
>> +}
>> +
>> /**
>> * drm_dev_register - Register DRM device
>> * @dev: Device to register
>> @@ -787,6 +778,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 +819,7 @@ void drm_dev_unregister(struct drm_device *dev)
>> drm_lastclose(dev);
>>
>> dev->registered = false;
>> + drm_device_set_plug_state(dev, false);
>>
>> 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);
>
> I think you need drm_dev_unref here as well.
right, will do.
>
> Sean
>
>> }
>>
>> /*
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index 3bfafcd..980a204 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -488,12 +488,6 @@ 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)
>> -{
>> - smp_wmb();
>> - atomic_set(&dev->unplugged, 1);
>> -}
>> -
>> static inline int drm_device_is_unplugged(struct drm_device *dev)
>> {
>> int ret = atomic_read(&dev->unplugged);
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 0fefc3f..eb63078 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -544,7 +544,6 @@ void drm_dev_unregister(struct drm_device *dev);
>> void drm_dev_ref(struct drm_device *dev);
>> void drm_dev_unref(struct drm_device *dev);
>> void drm_put_dev(struct drm_device *dev);
>> -void drm_unplug_dev(struct drm_device *dev);
>>
>> int drm_dev_set_unique(struct drm_device *dev, const char *name);
>>
>> --
>> 2.1.4
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-13 1:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-12 8:56 [PATCH v9 0/1] drm: rockchip: Fix rockchip drm unbind crash error Jeffy Chen
2017-04-12 8:56 ` [PATCH v9] drm: Unplug drm device when unregistering it Jeffy Chen
2017-04-12 15:03 ` Sean Paul
2017-04-13 1:28 ` jeffy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).