[PATCH v3 0/9] drm: rockchip: Fix rockchip drm unbind crash error

[PATCH v3 0/9] drm: rockchip: Fix rockchip drm unbind crash error

[Jeffy Chen]

Verified on rk3399 chromebook kevin:
1/ stop ui && pkill -9 frecon
2/ unbind/bind drm

Changes in v3:
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.
Address Daniel Vetter <daniel@ffwll.ch>'s comments.
Update commit message.
Update commit message.

Changes in v2:
Fix some commit messages.

Jeffy Chen (9):
  drm: bridge: analogix: Detach panel when unbinding analogix dp
  drm: bridge: analogix: Unregister dp aux when unbinding
  drm: bridge: analogix: Destroy connector when unbinding
  drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
  drm/rockchip: vop: Enable pm domain before vop_initial
  drm/rockchip: Reoder drm bind/unbind sequence
  drm/rockchip: Shutdown all crtcs when unbinding drm
  drm/rockchip: gem: Don't alloc/free gem buf when dev_private is
    invalid
  drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 ++
 drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c        |  8 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 29 +++++++++----
 5 files changed, 66 insertions(+), 35 deletions(-)

-- 
2.1.4

[PATCH v3 4/9] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded

[Jeffy Chen]

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

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

[PATCH v3 5/9] drm/rockchip: vop: Enable pm domain before vop_initial

[Jeffy Chen]

We're trying to access vop registers here, so need to make sure
the pm domain is on.

Normally it should be enabled by the bootloader, but there's no
guarantee of it. And if we wanna do unbind/bind, it would also
cause the device to hang.

And this patch also does these:
1/ move vop_initial to the end of vop_bind for eaiser error handling.
2/ correct the err_put_pm_runtime of vop_enable.

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

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

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 2151e1c..b65b296 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -506,7 +506,7 @@ static int vop_enable(struct drm_crtc *crtc)
 	ret = pm_runtime_get_sync(vop->dev);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
-		goto err_put_pm_runtime;
+		return ret;
 	}
 
 	ret = clk_enable(vop->hclk);
@@ -1405,10 +1405,16 @@ static int vop_initial(struct vop *vop)
 		return PTR_ERR(vop->dclk);
 	}
 
+	ret = pm_runtime_get_sync(vop->dev);
+	if (ret < 0) {
+		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
+		return ret;
+	}
+
 	ret = clk_prepare(vop->dclk);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to prepare dclk\n");
-		return ret;
+		goto err_put_pm_runtime;
 	}
 
 	/* Enable both the hclk and aclk to setup the vop */
@@ -1468,6 +1474,8 @@ static int vop_initial(struct vop *vop)
 
 	vop->is_enabled = false;
 
+	pm_runtime_put_sync(vop->dev);
+
 	return 0;
 
 err_disable_aclk:
@@ -1476,6 +1484,8 @@ static int vop_initial(struct vop *vop)
 	clk_disable_unprepare(vop->hclk);
 err_unprepare_dclk:
 	clk_unprepare(vop->dclk);
+err_put_pm_runtime:
+	pm_runtime_put_sync(vop->dev);
 	return ret;
 }
 
@@ -1576,12 +1586,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	if (!vop->regsbak)
 		return -ENOMEM;
 
-	ret = vop_initial(vop);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret);
-		return ret;
-	}
-
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(dev, "cannot find irq for vop\n");
@@ -1608,8 +1612,17 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 
 	pm_runtime_enable(&pdev->dev);
 
+	ret = vop_initial(vop);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret);
+		goto err_disable_pm_runtime;
+	}
+
 	return 0;
 
+err_disable_pm_runtime:
+	pm_runtime_disable(&pdev->dev);
+	vop_destroy_crtc(vop);
 err_enable_irq:
 	enable_irq(vop->irq); /* To balance out the disable_irq above */
 	return ret;
-- 
2.1.4

[PATCH v3 6/9] drm/rockchip: Reoder drm bind/unbind sequence

[Jeffy Chen]

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

[PATCH v3 7/9] drm/rockchip: Shutdown all crtcs when unbinding drm

[Jeffy Chen]

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

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

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f24968f..c6b1b7f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -199,6 +199,7 @@ static void rockchip_drm_unbind(struct device *dev)
 	rockchip_drm_fbdev_fini(drm_dev);
 	drm_kms_helper_poll_fini(drm_dev);
 
+	drm_atomic_helper_shutdown(drm_dev);
 	drm_vblank_cleanup(drm_dev);
 	component_unbind_all(dev, drm_dev);
 	drm_mode_config_cleanup(drm_dev);
-- 
2.1.4

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[Jeffy Chen]

After unbinding drm, the userspace may still has a chance to access
gem buf.

Add a sanity check for a NULL dev_private to prevent that from
happening.

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

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

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index df9e570..205a3dc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -184,6 +184,9 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
 	struct drm_device *drm = obj->dev;
 	struct rockchip_drm_private *private = drm->dev_private;
 
+	if (!private)
+		return -ENODEV;
+
 	if (private->domain)
 		return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
 	else
@@ -208,6 +211,11 @@ static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
 
 static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
 {
+	struct drm_device *drm = rk_obj->base.dev;
+
+	if (!drm->dev_private)
+		return;
+
 	if (rk_obj->pages)
 		rockchip_gem_free_iommu(rk_obj);
 	else
-- 
2.1.4

[PATCH v3 9/9] drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding

[Jeffy Chen]

After snd_soc_unregister_codec, the dai link would remain bound to
the invalid codec. That would cause crashes after unbind dp driver.

Let's unregister audio codec when removing dp driver to prevent that.

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

Changes in v3:
Update commit message.

Changes in v2: None

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

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index ee4195d..a2169dd 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1092,8 +1092,6 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 		goto err_free_connector;
 	}
 
-	cdn_dp_audio_codec_init(dp, dev);
-
 	for (i = 0; i < dp->ports; i++) {
 		port = dp->port[i];
 
@@ -1128,7 +1126,6 @@ static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 	struct drm_connector *connector = &dp->connector;
 
 	cancel_work_sync(&dp->event_work);
-	platform_device_unregister(dp->audio_pdev);
 	cdn_dp_encoder_disable(encoder);
 	encoder->funcs->destroy(encoder);
 	connector->funcs->destroy(connector);
@@ -1221,6 +1218,8 @@ static int cdn_dp_probe(struct platform_device *pdev)
 	mutex_init(&dp->lock);
 	dev_set_drvdata(dev, dp);
 
+	cdn_dp_audio_codec_init(dp, dev);
+
 	return component_add(dev, &cdn_dp_component_ops);
 }
 
@@ -1228,6 +1227,7 @@ static int cdn_dp_remove(struct platform_device *pdev)
 {
 	struct cdn_dp_device *dp = platform_get_drvdata(pdev);
 
+	platform_device_unregister(dp->audio_pdev);
 	cdn_dp_suspend(dp->dev);
 	component_del(&pdev->dev, &cdn_dp_component_ops);
 
-- 
2.1.4

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[Sean Paul]

On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
> After unbinding drm, the userspace may still has a chance to access
> gem buf.
> 
> Add a sanity check for a NULL dev_private to prevent that from
> happening.

I still don't understand how this is happening. You're saying that these hooks
can be called after rockchip_drm_unbind() has finished? 

Sean

> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v3:
> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
> Update commit message.
> 
> Changes in v2: None
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index df9e570..205a3dc 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -184,6 +184,9 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
>  	struct drm_device *drm = obj->dev;
>  	struct rockchip_drm_private *private = drm->dev_private;
>  
> +	if (!private)
> +		return -ENODEV;
> +
>  	if (private->domain)
>  		return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
>  	else
> @@ -208,6 +211,11 @@ static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
>  
>  static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
>  {
> +	struct drm_device *drm = rk_obj->base.dev;
> +
> +	if (!drm->dev_private)
> +		return;
> +
>  	if (rk_obj->pages)
>  		rockchip_gem_free_iommu(rk_obj);
>  	else
> -- 
> 2.1.4
> 

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

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[jeffy]

Hi Sean,

On 04/06/2017 12:28 AM, Sean Paul wrote:
> On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
>> After unbinding drm, the userspace may still has a chance to access
>> gem buf.
>>
>> Add a sanity check for a NULL dev_private to prevent that from
>> happening.
>
> I still don't understand how this is happening. You're saying that these hooks
> can be called after rockchip_drm_unbind() has finished?
>
yes, tested on chromebook rk3399 kevin with kernel 4.4, if trigger 
unbind without killing display service(ui or frecon):

[   31.276889] [<ffffffc0002089e4>] dump_backtrace+0x0/0x164
[   31.282288] [<ffffffc000208b6c>] show_stack+0x24/0x30
[   31.287338] [<ffffffc0004be028>] dump_stack+0x98/0xb8
[   31.292389] [<ffffffc0005c0b8c>] rockchip_gem_create_object+0x6c/0x2ec
[   31.298910] [<ffffffc0005c0fa0>] 
rockchip_gem_create_with_handle+0x38/0x10c
[   31.305868] [<ffffffc0005c12b8>] rockchip_gem_create_ioctl+0x38/0x50
[   31.312221] [<ffffffc000598844>] drm_ioctl+0x2bc/0x438
[   31.317359] [<ffffffc0005b5ee0>] drm_compat_ioctl+0x3c/0x70
[   31.322935] [<ffffffc0003a6960>] compat_SyS_ioctl+0x134/0x1048
[   31.328766] [<ffffffc000203e90>] __sys_trace_return+0x0/0x4

> Sean
>
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> Changes in v3:
>> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
>> Update commit message.
>>
>> Changes in v2: None
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> index df9e570..205a3dc 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> @@ -184,6 +184,9 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
>>   	struct drm_device *drm = obj->dev;
>>   	struct rockchip_drm_private *private = drm->dev_private;
>>
>> +	if (!private)
>> +		return -ENODEV;
>> +
>>   	if (private->domain)
>>   		return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
>>   	else
>> @@ -208,6 +211,11 @@ static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
>>
>>   static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
>>   {
>> +	struct drm_device *drm = rk_obj->base.dev;
>> +
>> +	if (!drm->dev_private)
>> +		return;
>> +
>>   	if (rk_obj->pages)
>>   		rockchip_gem_free_iommu(rk_obj);
>>   	else
>> --
>> 2.1.4
>>
>

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[Daniel Vetter]

On Wed, Apr 05, 2017 at 12:28:40PM -0400, Sean Paul wrote:
> On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
> > After unbinding drm, the userspace may still has a chance to access
> > gem buf.
> > 
> > Add a sanity check for a NULL dev_private to prevent that from
> > happening.
> 
> I still don't understand how this is happening. You're saying that these hooks
> can be called after rockchip_drm_unbind() has finished? 

Yeah this is supposed to be impossible. If it isn't, we need to debug and
fix this properly. This smells like pretty bad duct-tape ...
-Daniel

> 
> Sean
> 
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> > 
> > Changes in v3:
> > Address Daniel Vetter <daniel@ffwll.ch>'s comments.
> > Update commit message.
> > 
> > Changes in v2: None
> > 
> >  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > index df9e570..205a3dc 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > @@ -184,6 +184,9 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
> >  	struct drm_device *drm = obj->dev;
> >  	struct rockchip_drm_private *private = drm->dev_private;
> >  
> > +	if (!private)
> > +		return -ENODEV;
> > +
> >  	if (private->domain)
> >  		return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
> >  	else
> > @@ -208,6 +211,11 @@ static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
> >  
> >  static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
> >  {
> > +	struct drm_device *drm = rk_obj->base.dev;
> > +
> > +	if (!drm->dev_private)
> > +		return;
> > +
> >  	if (rk_obj->pages)
> >  		rockchip_gem_free_iommu(rk_obj);
> >  	else
> > -- 
> > 2.1.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[jeffy]

Hi Daniel,

On 04/06/2017 04:26 PM, Daniel Vetter wrote:
> On Wed, Apr 05, 2017 at 12:28:40PM -0400, Sean Paul wrote:
>> On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
>>> After unbinding drm, the userspace may still has a chance to access
>>> gem buf.
>>>
>>> Add a sanity check for a NULL dev_private to prevent that from
>>> happening.
>>
>> I still don't understand how this is happening. You're saying that these hooks
>> can be called after rockchip_drm_unbind() has finished?
>
> Yeah this is supposed to be impossible. If it isn't, we need to debug and
> fix this properly. This smells like pretty bad duct-tape ...

it looks like after unbind, the user space may still own drm dev fd, and 
could be able to call ioctl:
lrwx------. 1 chronos chronos 64 Mar 15 12:53 28 -> /dev/dri/card1 (deleted)

and the drm_unplug_dev may help it, maybe we should call it in unbind? 
or just break drm_ioctl when drm_dev not registered?

> -Daniel
>
>>
>> Sean
>>
>>>
>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>> ---
>>>
>>> Changes in v3:
>>> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
>>> Update commit message.
>>>
>>> Changes in v2: None
>>>
>>>   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>> index df9e570..205a3dc 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>> @@ -184,6 +184,9 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
>>>   	struct drm_device *drm = obj->dev;
>>>   	struct rockchip_drm_private *private = drm->dev_private;
>>>
>>> +	if (!private)
>>> +		return -ENODEV;
>>> +
>>>   	if (private->domain)
>>>   		return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
>>>   	else
>>> @@ -208,6 +211,11 @@ static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
>>>
>>>   static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
>>>   {
>>> +	struct drm_device *drm = rk_obj->base.dev;
>>> +
>>> +	if (!drm->dev_private)
>>> +		return;
>>> +
>>>   	if (rk_obj->pages)
>>>   		rockchip_gem_free_iommu(rk_obj);
>>>   	else
>>> --
>>> 2.1.4
>>>
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[Sean Paul]

On Thu, Apr 06, 2017 at 10:47:59AM +0800, jeffy wrote:
> Hi Sean,
> 
> On 04/06/2017 12:28 AM, Sean Paul wrote:
> > On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
> > > After unbinding drm, the userspace may still has a chance to access
> > > gem buf.
> > > 
> > > Add a sanity check for a NULL dev_private to prevent that from
> > > happening.
> > 
> > I still don't understand how this is happening. You're saying that these hooks
> > can be called after rockchip_drm_unbind() has finished?
> > 
> yes, tested on chromebook rk3399 kevin with kernel 4.4, if trigger unbind
> without killing display service(ui or frecon):
> 
> [   31.276889] [<ffffffc0002089e4>] dump_backtrace+0x0/0x164
> [   31.282288] [<ffffffc000208b6c>] show_stack+0x24/0x30
> [   31.287338] [<ffffffc0004be028>] dump_stack+0x98/0xb8
> [   31.292389] [<ffffffc0005c0b8c>] rockchip_gem_create_object+0x6c/0x2ec
> [   31.298910] [<ffffffc0005c0fa0>]
> rockchip_gem_create_with_handle+0x38/0x10c
> [   31.305868] [<ffffffc0005c12b8>] rockchip_gem_create_ioctl+0x38/0x50
> [   31.312221] [<ffffffc000598844>] drm_ioctl+0x2bc/0x438
> [   31.317359] [<ffffffc0005b5ee0>] drm_compat_ioctl+0x3c/0x70
> [   31.322935] [<ffffffc0003a6960>] compat_SyS_ioctl+0x134/0x1048
> [   31.328766] [<ffffffc000203e90>] __sys_trace_return+0x0/0x4

Hi Jeffy,
I'm not suggesting this doesn't happen, I believe you :-). I'd really like to
know *why* it happens.

Are you sure that unbind has completely finished before this trace occurs?
Perhaps this is results from a race between unbind and gem allocate?

Sean

> 
> > Sean
> > 
> > > 
> > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > > ---
> > > 
> > > Changes in v3:
> > > Address Daniel Vetter <daniel@ffwll.ch>'s comments.
> > > Update commit message.
> > > 
> > > Changes in v2: None
> > > 
> > >   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > > index df9e570..205a3dc 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > > @@ -184,6 +184,9 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
> > >   	struct drm_device *drm = obj->dev;
> > >   	struct rockchip_drm_private *private = drm->dev_private;
> > > 
> > > +	if (!private)
> > > +		return -ENODEV;
> > > +
> > >   	if (private->domain)
> > >   		return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
> > >   	else
> > > @@ -208,6 +211,11 @@ static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
> > > 
> > >   static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
> > >   {
> > > +	struct drm_device *drm = rk_obj->base.dev;
> > > +
> > > +	if (!drm->dev_private)
> > > +		return;
> > > +
> > >   	if (rk_obj->pages)
> > >   		rockchip_gem_free_iommu(rk_obj);
> > >   	else
> > > --
> > > 2.1.4
> > > 
> > 
> 

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

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[jeffy]

Hi Sean,

On 04/06/2017 08:26 PM, Sean Paul wrote:
> On Thu, Apr 06, 2017 at 10:47:59AM +0800, jeffy wrote:
>> Hi Sean,
>>
>> On 04/06/2017 12:28 AM, Sean Paul wrote:
>>> On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
>>>> After unbinding drm, the userspace may still has a chance to access
>>>> gem buf.
>>>>
>>>> Add a sanity check for a NULL dev_private to prevent that from
>>>> happening.
>>>
>>> I still don't understand how this is happening. You're saying that these hooks
>>> can be called after rockchip_drm_unbind() has finished?
>>>
>> yes, tested on chromebook rk3399 kevin with kernel 4.4, if trigger unbind
>> without killing display service(ui or frecon):
>>
>> [   31.276889] [<ffffffc0002089e4>] dump_backtrace+0x0/0x164
>> [   31.282288] [<ffffffc000208b6c>] show_stack+0x24/0x30
>> [   31.287338] [<ffffffc0004be028>] dump_stack+0x98/0xb8
>> [   31.292389] [<ffffffc0005c0b8c>] rockchip_gem_create_object+0x6c/0x2ec
>> [   31.298910] [<ffffffc0005c0fa0>]
>> rockchip_gem_create_with_handle+0x38/0x10c
>> [   31.305868] [<ffffffc0005c12b8>] rockchip_gem_create_ioctl+0x38/0x50
>> [   31.312221] [<ffffffc000598844>] drm_ioctl+0x2bc/0x438
>> [   31.317359] [<ffffffc0005b5ee0>] drm_compat_ioctl+0x3c/0x70
>> [   31.322935] [<ffffffc0003a6960>] compat_SyS_ioctl+0x134/0x1048
>> [   31.328766] [<ffffffc000203e90>] __sys_trace_return+0x0/0x4
>
> Hi Jeffy,
> I'm not suggesting this doesn't happen, I believe you :-). I'd really like to
> know *why* it happens.
>
> Are you sure that unbind has completely finished before this trace occurs?
> Perhaps this is results from a race between unbind and gem allocate?
yes, that unbind is finished. and it looks like the display server still 
holds drm dev fd(even it been deleted after unbind).

sometimes i can see it trigger ioctl even seconds later.

i'll send a patch to break drm_ioctl after unbind.
>
> Sean
>
>>
>>> Sean
>>>
>>>>
>>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
>>>> Update commit message.
>>>>
>>>> Changes in v2: None
>>>>
>>>>    drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>>> index df9e570..205a3dc 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>>>> @@ -184,6 +184,9 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
>>>>    	struct drm_device *drm = obj->dev;
>>>>    	struct rockchip_drm_private *private = drm->dev_private;
>>>>
>>>> +	if (!private)
>>>> +		return -ENODEV;
>>>> +
>>>>    	if (private->domain)
>>>>    		return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
>>>>    	else
>>>> @@ -208,6 +211,11 @@ static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
>>>>
>>>>    static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
>>>>    {
>>>> +	struct drm_device *drm = rk_obj->base.dev;
>>>> +
>>>> +	if (!drm->dev_private)
>>>> +		return;
>>>> +
>>>>    	if (rk_obj->pages)
>>>>    		rockchip_gem_free_iommu(rk_obj);
>>>>    	else
>>>> --
>>>> 2.1.4
>>>>
>>>
>>
>

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[Daniel Vetter]

On Thu, Apr 6, 2017 at 1:09 PM, jeffy <jeffy.chen@rock-chips.com> wrote:
>
> On 04/06/2017 04:26 PM, Daniel Vetter wrote:
>>
>> On Wed, Apr 05, 2017 at 12:28:40PM -0400, Sean Paul wrote:
>>>
>>> On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
>>>>
>>>> After unbinding drm, the userspace may still has a chance to access
>>>> gem buf.
>>>>
>>>> Add a sanity check for a NULL dev_private to prevent that from
>>>> happening.
>>>
>>>
>>> I still don't understand how this is happening. You're saying that these
>>> hooks
>>> can be called after rockchip_drm_unbind() has finished?
>>
>>
>> Yeah this is supposed to be impossible. If it isn't, we need to debug and
>> fix this properly. This smells like pretty bad duct-tape ...
>
>
> it looks like after unbind, the user space may still own drm dev fd, and
> could be able to call ioctl:
> lrwx------. 1 chronos chronos 64 Mar 15 12:53 28 -> /dev/dri/card1 (deleted)
>
> and the drm_unplug_dev may help it, maybe we should call it in unbind? or
> just break drm_ioctl when drm_dev not registered?

Yes, by default unbind while userspace is running is totally broken in
drm. drm_unplug_dev would be the fix, but it's only used by udl and
not many use that. You might need to fix infrastructure up a bit.

For normal module unload the module reference will prevent unloading.
So why exactly do you care about the unbind use-case?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[jeffy]

Hi Daniel,

On 04/07/2017 02:30 PM, Daniel Vetter wrote:
> On Thu, Apr 6, 2017 at 1:09 PM, jeffy <jeffy.chen@rock-chips.com> wrote:
>>
>> On 04/06/2017 04:26 PM, Daniel Vetter wrote:
>>>
>>> On Wed, Apr 05, 2017 at 12:28:40PM -0400, Sean Paul wrote:
>>>>
>>>> On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
>>>>>
>>>>> After unbinding drm, the userspace may still has a chance to access
>>>>> gem buf.
>>>>>
>>>>> Add a sanity check for a NULL dev_private to prevent that from
>>>>> happening.
>>>>
>>>>
>>>> I still don't understand how this is happening. You're saying that these
>>>> hooks
>>>> can be called after rockchip_drm_unbind() has finished?
>>>
>>>
>>> Yeah this is supposed to be impossible. If it isn't, we need to debug and
>>> fix this properly. This smells like pretty bad duct-tape ...
>>
>>
>> it looks like after unbind, the user space may still own drm dev fd, and
>> could be able to call ioctl:
>> lrwx------. 1 chronos chronos 64 Mar 15 12:53 28 -> /dev/dri/card1 (deleted)
>>
>> and the drm_unplug_dev may help it, maybe we should call it in unbind? or
>> just break drm_ioctl when drm_dev not registered?
>
> Yes, by default unbind while userspace is running is totally broken in
> drm. drm_unplug_dev would be the fix, but it's only used by udl and
> not many use that. You might need to fix infrastructure up a bit.
please check this patch:
9667071 New          [v5,12/12] drm/drm_ioctl.c: Break ioctl when drm 
device not registered
>
> For normal module unload the module reference will prevent unloading.
> So why exactly do you care about the unbind use-case?
sometimes we use unbind/bind for testing ;)
> -Daniel
>

[PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid

[Daniel Vetter]

On Fri, Apr 07, 2017 at 02:44:05PM +0800, jeffy wrote:
> Hi Daniel,
> 
> On 04/07/2017 02:30 PM, Daniel Vetter wrote:
> > On Thu, Apr 6, 2017 at 1:09 PM, jeffy <jeffy.chen@rock-chips.com> wrote:
> > > 
> > > On 04/06/2017 04:26 PM, Daniel Vetter wrote:
> > > > 
> > > > On Wed, Apr 05, 2017 at 12:28:40PM -0400, Sean Paul wrote:
> > > > > 
> > > > > On Wed, Apr 05, 2017 at 04:29:26PM +0800, Jeffy Chen wrote:
> > > > > > 
> > > > > > After unbinding drm, the userspace may still has a chance to access
> > > > > > gem buf.
> > > > > > 
> > > > > > Add a sanity check for a NULL dev_private to prevent that from
> > > > > > happening.
> > > > > 
> > > > > 
> > > > > I still don't understand how this is happening. You're saying that these
> > > > > hooks
> > > > > can be called after rockchip_drm_unbind() has finished?
> > > > 
> > > > 
> > > > Yeah this is supposed to be impossible. If it isn't, we need to debug and
> > > > fix this properly. This smells like pretty bad duct-tape ...
> > > 
> > > 
> > > it looks like after unbind, the user space may still own drm dev fd, and
> > > could be able to call ioctl:
> > > lrwx------. 1 chronos chronos 64 Mar 15 12:53 28 -> /dev/dri/card1 (deleted)
> > > 
> > > and the drm_unplug_dev may help it, maybe we should call it in unbind? or
> > > just break drm_ioctl when drm_dev not registered?
> > 
> > Yes, by default unbind while userspace is running is totally broken in
> > drm. drm_unplug_dev would be the fix, but it's only used by udl and
> > not many use that. You might need to fix infrastructure up a bit.
> please check this patch:
> 9667071 New          [v5,12/12] drm/drm_ioctl.c: Break ioctl when drm device
> not registered
> > 
> > For normal module unload the module reference will prevent unloading.
> > So why exactly do you care about the unbind use-case?
> sometimes we use unbind/bind for testing ;)

Then make sure you stop your userspace first. Fixing unbind to be
completely race-free requires a bit of work in the drm core.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch