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

Groups > linux.kernel > #1616693 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/9] drm: rockchip: Fix rockchip drm unbind crash error Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
    [PATCH v3 2/9] drm: bridge: analogix: Unregister dp aux when unbinding Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
      Re: [PATCH v3 2/9] drm: bridge: analogix: Unregister dp aux when  unbinding Andrzej Hajda <a.hajda@samsung.com> - 2017-04-06 09:20 +0200
        Re: [PATCH v3 2/9] drm: bridge: analogix: Unregister dp aux when  unbinding jeffy <jeffy.chen@rock-chips.com> - 2017-04-06 14:20 +0200
    [PATCH v3 3/9] drm: bridge: analogix: Destroy connector when unbinding Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
      Re: [PATCH v3 3/9] drm: bridge: analogix: Destroy connector when  unbinding Andrzej Hajda <a.hajda@samsung.com> - 2017-04-06 09:30 +0200
        Re: [PATCH v3 3/9] drm: bridge: analogix: Destroy connector when  unbinding jeffy <jeffy.chen@rock-chips.com> - 2017-04-06 14:30 +0200
    [PATCH v3 4/9] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
    [PATCH v3 6/9] drm/rockchip: Reoder drm bind/unbind sequence Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
    [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when dev_private is invalid Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
      Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid Sean Paul <seanpaul@chromium.org> - 2017-04-05 18:40 +0200
        Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid jeffy <jeffy.chen@rock-chips.com> - 2017-04-06 05:00 +0200
          Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid Sean Paul <seanpaul@chromium.org> - 2017-04-06 14:30 +0200
            Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid jeffy <jeffy.chen@rock-chips.com> - 2017-04-06 15:00 +0200
        Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid Daniel Vetter <daniel@ffwll.ch> - 2017-04-06 10:30 +0200
          Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid jeffy <jeffy.chen@rock-chips.com> - 2017-04-06 13:20 +0200
            Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid Daniel Vetter <daniel@ffwll.ch> - 2017-04-07 08:40 +0200
              Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid jeffy <jeffy.chen@rock-chips.com> - 2017-04-07 08:50 +0200
                Re: [PATCH v3 8/9] drm/rockchip: gem: Don't alloc/free gem buf when  dev_private is invalid Daniel Vetter <daniel@ffwll.ch> - 2017-04-07 09:20 +0200
    [PATCH v3 7/9] drm/rockchip: Shutdown all crtcs when unbinding drm Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
    [PATCH v3 1/9] drm: bridge: analogix: Detach panel when unbinding analogix dp Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-04-05 10:40 +0200
      Re: [PATCH v3 1/9] drm: bridge: analogix: Detach panel when unbinding  analogix dp Andrzej Hajda <a.hajda@samsung.com> - 2017-04-06 09:00 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

#1616694 — [PATCH v3 2/9] drm: bridge: analogix: Unregister dp aux when unbinding

The dp aux is registered when binding analogix dp.

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

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

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

#1617559 — Re: [PATCH v3 2/9] drm: bridge: analogix: Unregister dp aux when unbinding

On 05.04.2017 10:29, Jeffy Chen wrote:
> The dp aux is registered when binding analogix dp.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

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

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

#1617919 — Re: [PATCH v3 2/9] drm: bridge: analogix: Unregister dp aux when unbinding

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 <jeffy.chen@rock-chips.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>
> 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);
>
>
>
>
>

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

#1616695 — [PATCH v3 3/9] drm: bridge: analogix: Destroy connector when unbinding

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 <jeffy.chen@rock-chips.com>
---

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

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

#1617565 — Re: [PATCH v3 3/9] drm: bridge: analogix: Destroy connector when unbinding

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 <jeffy.chen@rock-chips.com>

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

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

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

#1617945 — Re: [PATCH v3 3/9] drm: bridge: analogix: Destroy connector when unbinding

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 <jeffy.chen@rock-chips.com>
>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>
> 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))
>
>
>
>
>

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

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

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

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

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

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
>

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

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

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

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

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

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

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web