Verified on rk3399 chromebook kevin:
1/ stop ui && pkill -9 frecon
2/ unbind/bind drm
Changes in v3:
Address Sean Paul <[email protected]>'s comments.
Update commit message.
Address Sean Paul <[email protected]>'s comments.
Update commit message.
Address Daniel Vetter <[email protected]>'s comments.
Update commit message.
Address Daniel Vetter <[email protected]>'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
The panel is attached when binding analogix dp.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v3: None
Changes in v2:
Fix some commit messages.
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index c26997a..28144a1 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1443,6 +1443,8 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
if (dp->plat_data->panel) {
if (drm_panel_unprepare(dp->plat_data->panel))
DRM_ERROR("failed to turnoff the panel\n");
+ if (drm_panel_detach(dp->plat_data->panel))
+ DRM_ERROR("failed to detach the panel\n");
}
pm_runtime_disable(dev);
--
2.1.4
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 <[email protected]>
---
Changes in v3:
Address Sean Paul <[email protected]>'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
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 <[email protected]>
---
Changes in v3:
Address Sean Paul <[email protected]>'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
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v3:
Address Daniel Vetter <[email protected]>'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
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 <[email protected]>
---
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
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 <[email protected]>
---
Changes in v3:
Address Daniel Vetter <[email protected]>'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
Signed-off-by: Jeffy Chen <[email protected]>
---
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
The dp aux is registered when binding analogix dp.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v3: None
Changes in v2: None
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 28144a1..7b75f82 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1447,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
DRM_ERROR("failed to detach the panel\n");
}
+ drm_dp_aux_unregister(&dp->aux);
pm_runtime_disable(dev);
}
EXPORT_SYMBOL_GPL(analogix_dp_unbind);
--
2.1.4
Normally we do this in drm_mode_config_cleanup. But analogix dp's
connector is allocated in bind, and freed after unbind. So we need
to destroy it in unbind to avoid further access.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v3: None
Changes in v2: None
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7b75f82..a96fd55 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1439,6 +1439,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
struct analogix_dp_device *dp = dev_get_drvdata(dev);
analogix_dp_bridge_disable(dp->bridge);
+ dp->connector.funcs->destroy(&dp->connector);
if (dp->plat_data->panel) {
if (drm_panel_unprepare(dp->plat_data->panel))
--
2.1.4
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 <[email protected]>
> ---
>
> Changes in v3:
> Address Daniel Vetter <[email protected]>'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
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 <[email protected]>
>> ---
>>
>> Changes in v3:
>> Address Daniel Vetter <[email protected]>'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
>>
>
On 05.04.2017 10:29, Jeffy Chen wrote:
> The panel is attached when binding analogix dp.
>
> Signed-off-by: Jeffy Chen <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
--
Regards
Andrzej
> ---
>
> Changes in v3: None
> Changes in v2:
> Fix some commit messages.
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index c26997a..28144a1 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1443,6 +1443,8 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
> if (dp->plat_data->panel) {
> if (drm_panel_unprepare(dp->plat_data->panel))
> DRM_ERROR("failed to turnoff the panel\n");
> + if (drm_panel_detach(dp->plat_data->panel))
> + DRM_ERROR("failed to detach the panel\n");
> }
>
> pm_runtime_disable(dev);
On 05.04.2017 10:29, Jeffy Chen wrote:
> The dp aux is registered when binding analogix dp.
>
> Signed-off-by: Jeffy Chen <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Btw, if you are working already in this area you can check also to
analogix_dp bind and unbind routines they looks suspiciously asymmetric.
I guess component framework deals correctly with devm_* stuff but I see
for example clk_prepare_enable not paired with clk_unprepare*.
--
Regards
Andrzej
> ---
>
> Changes in v3: None
> Changes in v2: None
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 28144a1..7b75f82 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1447,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
> DRM_ERROR("failed to detach the panel\n");
> }
>
> + drm_dp_aux_unregister(&dp->aux);
> pm_runtime_disable(dev);
> }
> EXPORT_SYMBOL_GPL(analogix_dp_unbind);
On 05.04.2017 10:29, Jeffy Chen wrote:
> Normally we do this in drm_mode_config_cleanup. But analogix dp's
> connector is allocated in bind, and freed after unbind. So we need
> to destroy it in unbind to avoid further access.
>
> Signed-off-by: Jeffy Chen <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
One comment below
> ---
>
> Changes in v3: None
> Changes in v2: None
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 7b75f82..a96fd55 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1439,6 +1439,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
> struct analogix_dp_device *dp = dev_get_drvdata(dev);
>
> analogix_dp_bridge_disable(dp->bridge);
Not related to the patch, but what about bridge, above it is only
disabled, but not detached, encoder cleanup code should do it but it is
also missing.
Regards
Andrzej
> + dp->connector.funcs->destroy(&dp->connector);
>
> if (dp->plat_data->panel) {
> if (drm_panel_unprepare(dp->plat_data->panel))
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 <[email protected]>
> > ---
> >
> > Changes in v3:
> > Address Daniel Vetter <[email protected]>'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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
>>> ---
>>>
>>> Changes in v3:
>>> Address Daniel Vetter <[email protected]>'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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Hi Andrzej,
On 04/06/2017 03:11 PM, Andrzej Hajda wrote:
> On 05.04.2017 10:29, Jeffy Chen wrote:
>> The dp aux is registered when binding analogix dp.
>>
>> Signed-off-by: Jeffy Chen <[email protected]>
> Reviewed-by: Andrzej Hajda <[email protected]>
>
> Btw, if you are working already in this area you can check also to
> analogix_dp bind and unbind routines they looks suspiciously asymmetric.
> I guess component framework deals correctly with devm_* stuff but I see
> for example clk_prepare_enable not paired with clk_unprepare*.
ok, well do...
>
> --
> Regards
> Andrzej
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index 28144a1..7b75f82 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1447,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
>> DRM_ERROR("failed to detach the panel\n");
>> }
>>
>> + drm_dp_aux_unregister(&dp->aux);
>> pm_runtime_disable(dev);
>> }
>> EXPORT_SYMBOL_GPL(analogix_dp_unbind);
>
>
>
>
>
Hi Andrzej,
On 04/06/2017 03:19 PM, Andrzej Hajda wrote:
> On 05.04.2017 10:29, Jeffy Chen wrote:
>> Normally we do this in drm_mode_config_cleanup. But analogix dp's
>> connector is allocated in bind, and freed after unbind. So we need
>> to destroy it in unbind to avoid further access.
>>
>> Signed-off-by: Jeffy Chen <[email protected]>
>
> Reviewed-by: Andrzej Hajda <[email protected]>
>
> One comment below
>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index 7b75f82..a96fd55 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1439,6 +1439,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
>> struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>
>> analogix_dp_bridge_disable(dp->bridge);
>
> Not related to the patch, but what about bridge, above it is only
> disabled, but not detached, encoder cleanup code should do it but it is
> also missing.
right, i should notice there's a drm_encoder_cleanup(dp->encoder) when
failed to create bridge...thanx for pointing that out :)
>
> Regards
> Andrzej
>
>> + dp->connector.funcs->destroy(&dp->connector);
>>
>> if (dp->plat_data->panel) {
>> if (drm_panel_unprepare(dp->plat_data->panel))
>
>
>
>
>
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 <[email protected]>
> > > ---
> > >
> > > Changes in v3:
> > > Address Daniel Vetter <[email protected]>'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
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 <[email protected]>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> Address Daniel Vetter <[email protected]>'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
>>>>
>>>
>>
>
On Thu, Apr 6, 2017 at 1:09 PM, jeffy <[email protected]> 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
Hi Daniel,
On 04/07/2017 02:30 PM, Daniel Vetter wrote:
> On Thu, Apr 6, 2017 at 1:09 PM, jeffy <[email protected]> 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
>
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 <[email protected]> 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