Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1617961 > unrolled thread

[PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

Started byJeffy Chen <jeffy.chen@rock-chips.com>
First post2017-04-06 14:40 +0200
Last post2017-04-07 19:40 +0200
Articles 8 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-06 14:40 +0200
    [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-06 14:40 +0200
      Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device  not registered Daniel Vetter <daniel@ffwll.ch> - 2017-04-07 09:20 +0200
        Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device  not registered jeffy <jeffy.chen@rock-chips.com> - 2017-04-07 11:30 +0200
          Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device  not registered Daniel Vetter <daniel@ffwll.ch> - 2017-04-07 20:40 +0200
    [PATCH v5 05/12] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-06 14:40 +0200
    [PATCH v5 10/12] drm/rockchip: Reoder drm bind/unbind sequence Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-06 14:40 +0200
    Re: [PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash  error Sean Paul <seanpaul@chromium.org> - 2017-04-07 19:40 +0200

#1617961 — [PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

FromJeffy Chen <jeffy.chen@rock-chips.com>
Date2017-04-06 14:40 +0200
Subject[PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error
Message-ID<tteRH-5K-3@gated-at.bofh.it>
Verified on rk3399 chromebook kevin:
1/ stop ui && pkill -9 frecon
2/ unbind/bind drm

Changes in v5:
Fix wrong git account.

Changes in v4:
Address Andrzej Hajda <a.hajda@samsung.com>'s comments.

Changes in v3:
Update commit message.
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.
Address Daniel Vetter <daniel@ffwll.ch>'s comments.
Update commit message.

Changes in v2:
Fix some commit messages.

Jeffy Chen (12):
  drm: bridge: analogix: Detach panel when unbinding analogix dp
  drm: bridge: analogix: Unregister dp aux when unbinding
  drm: bridge: analogix: Disable clock when unbinding
  drm: bridge: analogix: Destroy connector & encoder when unbinding
  drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
  drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding
  drm/rockchip: vop: Enable pm domain before vop_initial
  drm/rockchip: vop: Unprepare clocks when unbinding
  drm/rockchip: analogix_dp: Disable clock when unbinding
  drm/rockchip: Reoder drm bind/unbind sequence
  drm/rockchip: Shutdown all crtcs when unbinding drm
  drm/drm_ioctl.c: Break ioctl when drm device not registered

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  6 +++
 drivers/gpu/drm/drm_ioctl.c                        |  2 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  3 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 33 ++++++++++----
 6 files changed, 67 insertions(+), 37 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1617962 — [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

FromJeffy Chen <jeffy.chen@rock-chips.com>
Date2017-04-06 14:40 +0200
Subject[PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered
Message-ID<tteRI-5K-33@gated-at.bofh.it>
In reply to#1617961
After unbinding drm, the user space may still owns the drm dev fd,
and may still be able to call drm ioctl.

Add a sanity check here to prevent that from happening.

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

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/drm_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 7d6deaa..15beb11 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
 
 	dev = file_priv->minor->dev;
 
-	if (drm_device_is_unplugged(dev))
+	if (drm_device_is_unplugged(dev) || !dev->registered)
 		return -ENODEV;
 
 	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1618514 — Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-04-07 09:20 +0200
SubjectRe: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered
Message-ID<ttwlz-40I-5@gated-at.bofh.it>
In reply to#1617962
On Thu, Apr 06, 2017 at 08:31:25PM +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.
> 
> Add a sanity check here to prevent that from happening.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/drm_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 7d6deaa..15beb11 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
>  
>  	dev = file_priv->minor->dev;
>  
> -	if (drm_device_is_unplugged(dev))
> +	if (drm_device_is_unplugged(dev) || !dev->registered)

Shouldn't we instead automatically unplug the device in
drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
|| !registered all over the place?

That should catch a few more issues where userspace might creep into the
driver after unregistering ...
-Daniel

>  		return -ENODEV;
>  
>  	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
> -- 
> 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

[toc] | [prev] | [next] | [standalone]

#1618620 — Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-04-07 11:30 +0200
SubjectRe: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered
Message-ID<ttyno-5g3-5@gated-at.bofh.it>
In reply to#1618514
Hi Daniel,

On 04/07/2017 03:16 PM, Daniel Vetter wrote:
> On Thu, Apr 06, 2017 at 08:31:25PM +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.
>>
>> Add a sanity check here to prevent that from happening.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/gpu/drm/drm_ioctl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 7d6deaa..15beb11 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
>>
>>   	dev = file_priv->minor->dev;
>>
>> -	if (drm_device_is_unplugged(dev))
>> +	if (drm_device_is_unplugged(dev) || !dev->registered)
>
> Shouldn't we instead automatically unplug the device in
> drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
> || !registered all over the place?
>
it looks like the drm_unplug_dev would call drm_dev_unregister...
maybe we can:
1/ replace the dev_unplug_dev in udl_drv.c to drm_dev_unregister
2/ call dev_unplug_dev in drm_dev_unregister, and remove 
drm_dev_unregister in dev_unplug_dev
3/ add a drm_plug_dev or drm_device_set_plugged, and call it in 
drm_dev_register

> That should catch a few more issues where userspace might creep into the
> driver after unregistering ...
> -Daniel
>
>>   		return -ENODEV;
>>
>>   	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
>> --
>> 2.1.4
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[toc] | [prev] | [next] | [standalone]

#1619040 — Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-04-07 20:40 +0200
SubjectRe: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered
Message-ID<ttGXF-2Kw-49@gated-at.bofh.it>
In reply to#1618620
On Fri, Apr 07, 2017 at 05:24:59PM +0800, jeffy wrote:
> Hi Daniel,
> 
> On 04/07/2017 03:16 PM, Daniel Vetter wrote:
> > On Thu, Apr 06, 2017 at 08:31:25PM +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.
> > > 
> > > Add a sanity check here to prevent that from happening.
> > > 
> > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > > ---
> > > 
> > > Changes in v5: None
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > > 
> > >   drivers/gpu/drm/drm_ioctl.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index 7d6deaa..15beb11 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
> > > 
> > >   	dev = file_priv->minor->dev;
> > > 
> > > -	if (drm_device_is_unplugged(dev))
> > > +	if (drm_device_is_unplugged(dev) || !dev->registered)
> > 
> > Shouldn't we instead automatically unplug the device in
> > drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
> > || !registered all over the place?
> > 
> it looks like the drm_unplug_dev would call drm_dev_unregister...
> maybe we can:
> 1/ replace the dev_unplug_dev in udl_drv.c to drm_dev_unregister
> 2/ call dev_unplug_dev in drm_dev_unregister, and remove drm_dev_unregister
> in dev_unplug_dev
> 3/ add a drm_plug_dev or drm_device_set_plugged, and call it in
> drm_dev_register

Yeah, sounds like a reasonable plan. I didn't review the full implications
of this because Fri evening :-) So pls double-check before you rewrite the
world ...

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

[toc] | [prev] | [next] | [standalone]

#1617963 — [PATCH v5 05/12] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded

FromJeffy Chen <jeffy.chen@rock-chips.com>
Date2017-04-06 14:40 +0200
Subject[PATCH v5 05/12] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
Message-ID<tteRI-5K-39@gated-at.bofh.it>
In reply to#1617961
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 4e55d63..ee4195d 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1053,6 +1053,7 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 	dp->connected = false;
 	dp->active = false;
 	dp->active_port = -1;
+	dp->fw_loaded = false;
 
 	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
 
@@ -1133,7 +1134,8 @@ static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 	connector->funcs->destroy(connector);
 
 	pm_runtime_disable(dev);
-	release_firmware(dp->fw);
+	if (dp->fw_loaded)
+		release_firmware(dp->fw);
 	kfree(dp->edid);
 	dp->edid = NULL;
 }
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1617964 — [PATCH v5 10/12] drm/rockchip: Reoder drm bind/unbind sequence

FromJeffy Chen <jeffy.chen@rock-chips.com>
Date2017-04-06 14:40 +0200
Subject[PATCH v5 10/12] drm/rockchip: Reoder drm bind/unbind sequence
Message-ID<tteRI-5K-37@gated-at.bofh.it>
In reply to#1617961
Current drm bind/unbind sequence would cause some memory issues.
For example we should not cleanup iommu before cleanup mode config.

Reorder bind/unbind sequence, follow exynos drm.

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

Changes in v5: None
Changes in v4: None
Changes in v3:
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 49 +++++++++++++++--------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index cd7d02e1..f24968f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -136,21 +136,24 @@ static int rockchip_drm_bind(struct device *dev)
 	INIT_LIST_HEAD(&private->psr_list);
 	spin_lock_init(&private->psr_list_lock);
 
+	ret = rockchip_drm_init_iommu(drm_dev);
+	if (ret)
+		goto err_free;
+
 	drm_mode_config_init(drm_dev);
 
 	rockchip_drm_mode_config_init(drm_dev);
 
-	ret = rockchip_drm_init_iommu(drm_dev);
-	if (ret)
-		goto err_config_cleanup;
-
 	/* Try to bind all sub drivers. */
 	ret = component_bind_all(dev, drm_dev);
 	if (ret)
-		goto err_iommu_cleanup;
+		goto err_mode_config_cleanup;
 
-	/* init kms poll for handling hpd */
-	drm_kms_helper_poll_init(drm_dev);
+	ret = drm_vblank_init(drm_dev, drm_dev->mode_config.num_crtc);
+	if (ret)
+		goto err_unbind_all;
+
+	drm_mode_config_reset(drm_dev);
 
 	/*
 	 * enable drm irq mode.
@@ -158,15 +161,12 @@ static int rockchip_drm_bind(struct device *dev)
 	 */
 	drm_dev->irq_enabled = true;
 
-	ret = drm_vblank_init(drm_dev, ROCKCHIP_MAX_CRTC);
-	if (ret)
-		goto err_kms_helper_poll_fini;
-
-	drm_mode_config_reset(drm_dev);
+	/* init kms poll for handling hpd */
+	drm_kms_helper_poll_init(drm_dev);
 
 	ret = rockchip_drm_fbdev_init(drm_dev);
 	if (ret)
-		goto err_vblank_cleanup;
+		goto err_kms_helper_poll_fini;
 
 	ret = drm_dev_register(drm_dev, 0);
 	if (ret)
@@ -175,17 +175,17 @@ static int rockchip_drm_bind(struct device *dev)
 	return 0;
 err_fbdev_fini:
 	rockchip_drm_fbdev_fini(drm_dev);
-err_vblank_cleanup:
-	drm_vblank_cleanup(drm_dev);
 err_kms_helper_poll_fini:
 	drm_kms_helper_poll_fini(drm_dev);
+	drm_vblank_cleanup(drm_dev);
+err_unbind_all:
 	component_unbind_all(dev, drm_dev);
-err_iommu_cleanup:
-	rockchip_iommu_cleanup(drm_dev);
-err_config_cleanup:
+err_mode_config_cleanup:
 	drm_mode_config_cleanup(drm_dev);
-	drm_dev->dev_private = NULL;
+	rockchip_iommu_cleanup(drm_dev);
 err_free:
+	drm_dev->dev_private = NULL;
+	dev_set_drvdata(dev, NULL);
 	drm_dev_unref(drm_dev);
 	return ret;
 }
@@ -194,16 +194,19 @@ static void rockchip_drm_unbind(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 
+	drm_dev_unregister(drm_dev);
+
 	rockchip_drm_fbdev_fini(drm_dev);
-	drm_vblank_cleanup(drm_dev);
 	drm_kms_helper_poll_fini(drm_dev);
+
+	drm_vblank_cleanup(drm_dev);
 	component_unbind_all(dev, drm_dev);
-	rockchip_iommu_cleanup(drm_dev);
 	drm_mode_config_cleanup(drm_dev);
+	rockchip_iommu_cleanup(drm_dev);
+
 	drm_dev->dev_private = NULL;
-	drm_dev_unregister(drm_dev);
-	drm_dev_unref(drm_dev);
 	dev_set_drvdata(dev, NULL);
+	drm_dev_unref(drm_dev);
 }
 
 static void rockchip_drm_lastclose(struct drm_device *dev)
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1618997 — Re: [PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

FromSean Paul <seanpaul@chromium.org>
Date2017-04-07 19:40 +0200
SubjectRe: [PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error
Message-ID<ttG1z-1WS-5@gated-at.bofh.it>
In reply to#1617961
On Thu, Apr 06, 2017 at 08:31:13PM +0800, Jeffy Chen wrote:
> 
> Verified on rk3399 chromebook kevin:
> 1/ stop ui && pkill -9 frecon
> 2/ unbind/bind drm
> 
> Changes in v5:
> Fix wrong git account.
> 
> Changes in v4:
> Address Andrzej Hajda <a.hajda@samsung.com>'s comments.
> 
> Changes in v3:
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
> Update commit message.
> 
> Changes in v2:
> Fix some commit messages.
> 
> Jeffy Chen (12):
>   drm: bridge: analogix: Detach panel when unbinding analogix dp
>   drm: bridge: analogix: Unregister dp aux when unbinding
>   drm: bridge: analogix: Disable clock when unbinding
>   drm: bridge: analogix: Destroy connector & encoder when unbinding
>   drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
>   drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding
>   drm/rockchip: vop: Enable pm domain before vop_initial
>   drm/rockchip: vop: Unprepare clocks when unbinding
>   drm/rockchip: analogix_dp: Disable clock when unbinding
>   drm/rockchip: Reoder drm bind/unbind sequence
>   drm/rockchip: Shutdown all crtcs when unbinding drm

Hi Jeffy,
I've applied the first 11 patches from this set to -misc. The last patch
is still a work in progress.

Thanks,

Sean

>   drm/drm_ioctl.c: Break ioctl when drm device not registered
> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  6 +++
>  drivers/gpu/drm/drm_ioctl.c                        |  2 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  3 +-
>  drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 33 ++++++++++----
>  6 files changed, 67 insertions(+), 37 deletions(-)
> 
> -- 
> 2.1.4
> 

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web