[PATCH 0/2] drm/exynos: fix null pointer dereferenece issue

[PATCH 0/2] drm/exynos: fix null pointer dereferenece issue

[Inki Dae]

This patch set fixes null pointer dereference issue incurred when
Exynos drm driver is closed. And it rebases a existing cleanup patch
posted by Andrzej on top of exynos-drm-fixes after the infinite loop
and null pointer dereference issues are resolved.

Andrzej Hajda (1):
  drm/exynos: remove ifdeferry from initialization code

Inki Dae (1):
  drm/exynos: fix null pointer dereference issue

 drivers/gpu/drm/exynos/exynos_drm_drv.c |  214 ++++++++++---------------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h |   25 ++--
 2 files changed, 81 insertions(+), 158 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] drm/exynos: fix null pointer dereference issue

[Inki Dae]

This patch fixes null pointer dereference issue incurred
when ipp driver is enabled and Exynos drm driver is closed.

Non kms driver should register its own sub driver to setup necessary
resources, which is done by load(). So null pointer dereference
occurs when ipp driver is enabled and Exynos drm driver is closed
because ipp core device is registered after component_master_add_with_match
call.

This patch makes exynos_drm_device_subdrv_probe() to be called after all non
kms drivers are registered.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index e5c4c6c..f0bcdc6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -108,11 +108,6 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
 	if (ret)
 		goto err_unbind_all;
 
-	/* Probe non kms sub drivers and virtual display driver. */
-	ret = exynos_drm_device_subdrv_probe(dev);
-	if (ret)
-		goto err_cleanup_vblank;
-
 	/*
 	 * enable drm irq mode.
 	 * - with irq_enabled = true, we can use the vblank feature.
@@ -138,8 +133,6 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
 
 	return 0;
 
-err_cleanup_vblank:
-	drm_vblank_cleanup(dev);
 err_unbind_all:
 	component_unbind_all(dev->dev, dev);
 err_mode_config_cleanup:
@@ -153,8 +146,6 @@ err_free_private:
 
 static int exynos_drm_unload(struct drm_device *dev)
 {
-	exynos_drm_device_subdrv_remove(dev);
-
 	exynos_drm_fbdev_fini(dev);
 	drm_kms_helper_poll_fini(dev);
 
@@ -636,9 +627,16 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
 		goto err_unregister_ipp_drv;
 #endif
 
+	/* Probe non kms sub drivers and virtual display driver. */
+	ret = exynos_drm_device_subdrv_probe(platform_get_drvdata(pdev));
+	if (ret)
+		goto err_unregister_resources;
+
 	return ret;
 
+err_unregister_resources:
 #ifdef CONFIG_DRM_EXYNOS_IPP
+	exynos_platform_device_ipp_unregister();
 err_unregister_ipp_drv:
 	platform_driver_unregister(&ipp_driver);
 err_unregister_gsc_drv:
@@ -691,6 +689,8 @@ err_unregister_fimd_drv:
 
 static int exynos_drm_platform_remove(struct platform_device *pdev)
 {
+	exynos_drm_device_subdrv_remove(platform_get_drvdata(pdev));
+
 #ifdef CONFIG_DRM_EXYNOS_IPP
 	exynos_platform_device_ipp_unregister();
 	platform_driver_unregister(&ipp_driver);
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/exynos: remove ifdeferry from initialization code

[Inki Dae]

From: Andrzej Hajda <a.hajda@samsung.com>

The patch replaces separate calls to driver (de)registration by
loops over the array of drivers. As a result it significantly
decreases number of ifdefs. Additionally it moves device registration
related ifdefs to header file.

Changelog v2:
- Rebased.
- Consider non kms driver in respect to infinite loop issue.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c |  196 ++++++++++---------------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h |   25 ++--
 2 files changed, 72 insertions(+), 149 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index f0bcdc6..8aee629 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -547,85 +547,74 @@ static const struct component_master_ops exynos_drm_ops = {
 	.unbind		= exynos_drm_unbind,
 };
 
-static int exynos_drm_platform_probe(struct platform_device *pdev)
-{
-	struct component_match *match;
-	int ret;
-
-	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	exynos_drm_driver.num_ioctls = ARRAY_SIZE(exynos_ioctls);
-
+static struct platform_driver *const exynos_drm_kms_drivers[] = {
 #ifdef CONFIG_DRM_EXYNOS_FIMD
-	ret = platform_driver_register(&fimd_driver);
-	if (ret < 0)
-		return ret;
+	&fimd_driver,
 #endif
-
 #ifdef CONFIG_DRM_EXYNOS_DP
-	ret = platform_driver_register(&dp_driver);
-	if (ret < 0)
-		goto err_unregister_fimd_drv;
+	&dp_driver,
 #endif
-
 #ifdef CONFIG_DRM_EXYNOS_DSI
-	ret = platform_driver_register(&dsi_driver);
-	if (ret < 0)
-		goto err_unregister_dp_drv;
+	&dsi_driver,
 #endif
-
 #ifdef CONFIG_DRM_EXYNOS_HDMI
-	ret = platform_driver_register(&mixer_driver);
-	if (ret < 0)
-		goto err_unregister_dsi_drv;
-	ret = platform_driver_register(&hdmi_driver);
-	if (ret < 0)
-		goto err_unregister_mixer_drv;
+	&mixer_driver,
+	&hdmi_driver,
 #endif
+};
 
-	match = exynos_drm_match_add(&pdev->dev);
-	if (IS_ERR(match)) {
-		ret = PTR_ERR(match);
-		goto err_unregister_hdmi_drv;
-	}
-
-	ret = component_master_add_with_match(&pdev->dev, &exynos_drm_ops,
-						match);
-	if (ret < 0)
-		goto err_unregister_hdmi_drv;
-
+static struct platform_driver *const exynos_drm_non_kms_drivers[] = {
 #ifdef CONFIG_DRM_EXYNOS_G2D
-	ret = platform_driver_register(&g2d_driver);
-	if (ret < 0)
-		goto err_del_component_master;
+	&g2d_driver,
 #endif
-
 #ifdef CONFIG_DRM_EXYNOS_FIMC
-	ret = platform_driver_register(&fimc_driver);
-	if (ret < 0)
-		goto err_unregister_g2d_drv;
+	&fimc_driver,
 #endif
-
 #ifdef CONFIG_DRM_EXYNOS_ROTATOR
-	ret = platform_driver_register(&rotator_driver);
-	if (ret < 0)
-		goto err_unregister_fimc_drv;
+	&rotator_driver,
 #endif
-
 #ifdef CONFIG_DRM_EXYNOS_GSC
-	ret = platform_driver_register(&gsc_driver);
-	if (ret < 0)
-		goto err_unregister_rotator_drv;
+	&gsc_driver,
 #endif
-
 #ifdef CONFIG_DRM_EXYNOS_IPP
-	ret = platform_driver_register(&ipp_driver);
+	&ipp_driver,
+#endif
+};
+
+static int exynos_drm_platform_probe(struct platform_device *pdev)
+{
+	struct component_match *match;
+	int ret, i, j;
+
+	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	exynos_drm_driver.num_ioctls = ARRAY_SIZE(exynos_ioctls);
+
+	for (i = 0; i < ARRAY_SIZE(exynos_drm_kms_drivers); ++i) {
+		ret = platform_driver_register(exynos_drm_kms_drivers[i]);
+		if (ret < 0)
+			goto err_unregister_kms_drivers;
+	}
+
+	match = exynos_drm_match_add(&pdev->dev);
+	if (IS_ERR(match)) {
+		ret = PTR_ERR(match);
+		goto err_unregister_kms_drivers;
+	}
+
+	ret = component_master_add_with_match(&pdev->dev, &exynos_drm_ops,
+						match);
 	if (ret < 0)
-		goto err_unregister_gsc_drv;
+		goto err_unregister_kms_drivers;
+
+	for (j = 0; j < ARRAY_SIZE(exynos_drm_non_kms_drivers); ++j) {
+		ret = platform_driver_register(exynos_drm_non_kms_drivers[j]);
+		if (ret < 0)
+			goto err_del_component_master;
+	}
 
 	ret = exynos_platform_device_ipp_register();
 	if (ret < 0)
-		goto err_unregister_ipp_drv;
-#endif
+		goto err_unregister_non_kms_drivers;
 
 	/* Probe non kms sub drivers and virtual display driver. */
 	ret = exynos_drm_device_subdrv_probe(platform_get_drvdata(pdev));
@@ -637,98 +626,39 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
 err_unregister_resources:
 #ifdef CONFIG_DRM_EXYNOS_IPP
 	exynos_platform_device_ipp_unregister();
-err_unregister_ipp_drv:
-	platform_driver_unregister(&ipp_driver);
-err_unregister_gsc_drv:
 #endif
+err_unregister_non_kms_drivers:
+	while (--j >= 0)
+		platform_driver_unregister(exynos_drm_non_kms_drivers[j]);
 
-#ifdef CONFIG_DRM_EXYNOS_GSC
-	platform_driver_unregister(&gsc_driver);
-err_unregister_rotator_drv:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_ROTATOR
-	platform_driver_unregister(&rotator_driver);
-err_unregister_fimc_drv:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_FIMC
-	platform_driver_unregister(&fimc_driver);
-err_unregister_g2d_drv:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_G2D
-	platform_driver_unregister(&g2d_driver);
 err_del_component_master:
-#endif
 	component_master_del(&pdev->dev, &exynos_drm_ops);
 
-err_unregister_hdmi_drv:
-#ifdef CONFIG_DRM_EXYNOS_HDMI
-	platform_driver_unregister(&hdmi_driver);
-err_unregister_mixer_drv:
-	platform_driver_unregister(&mixer_driver);
-err_unregister_dsi_drv:
-#endif
+err_unregister_kms_drivers:
+	while (--i >= 0)
+		platform_driver_unregister(exynos_drm_kms_drivers[i]);
 
-#ifdef CONFIG_DRM_EXYNOS_DSI
-	platform_driver_unregister(&dsi_driver);
-err_unregister_dp_drv:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_DP
-	platform_driver_unregister(&dp_driver);
-err_unregister_fimd_drv:
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_FIMD
-	platform_driver_unregister(&fimd_driver);
-#endif
 	return ret;
 }
 
 static int exynos_drm_platform_remove(struct platform_device *pdev)
 {
+	int i;
+
 	exynos_drm_device_subdrv_remove(platform_get_drvdata(pdev));
 
 #ifdef CONFIG_DRM_EXYNOS_IPP
 	exynos_platform_device_ipp_unregister();
-	platform_driver_unregister(&ipp_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_GSC
-	platform_driver_unregister(&gsc_driver);
 #endif
 
-#ifdef CONFIG_DRM_EXYNOS_ROTATOR
-	platform_driver_unregister(&rotator_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_FIMC
-	platform_driver_unregister(&fimc_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_G2D
-	platform_driver_unregister(&g2d_driver);
-#endif
-
-#ifdef CONFIG_DRM_EXYNOS_HDMI
-	platform_driver_unregister(&mixer_driver);
-	platform_driver_unregister(&hdmi_driver);
-#endif
+	for (i = ARRAY_SIZE(exynos_drm_non_kms_drivers) - 1; i >= 0; --i)
+		platform_driver_unregister(exynos_drm_non_kms_drivers[i]);
 
-#ifdef CONFIG_DRM_EXYNOS_FIMD
-	platform_driver_unregister(&fimd_driver);
-#endif
+	component_master_del(&pdev->dev, &exynos_drm_ops);
 
-#ifdef CONFIG_DRM_EXYNOS_DSI
-	platform_driver_unregister(&dsi_driver);
-#endif
+	for (i = ARRAY_SIZE(exynos_drm_kms_drivers) - 1; i >= 0; --i)
+		platform_driver_unregister(exynos_drm_kms_drivers[i]);
 
-#ifdef CONFIG_DRM_EXYNOS_DP
-	platform_driver_unregister(&dp_driver);
-#endif
-	component_master_del(&pdev->dev, &exynos_drm_ops);
 	return 0;
 }
 
@@ -763,11 +693,9 @@ static int exynos_drm_init(void)
 	if (IS_ERR(exynos_drm_pdev))
 		return PTR_ERR(exynos_drm_pdev);
 
-#ifdef CONFIG_DRM_EXYNOS_VIDI
 	ret = exynos_drm_probe_vidi();
 	if (ret < 0)
 		goto err_unregister_pd;
-#endif
 
 	ret = platform_driver_register(&exynos_drm_platform_driver);
 	if (ret)
@@ -776,11 +704,9 @@ static int exynos_drm_init(void)
 	return 0;
 
 err_remove_vidi:
-#ifdef CONFIG_DRM_EXYNOS_VIDI
 	exynos_drm_remove_vidi();
 
 err_unregister_pd:
-#endif
 	platform_device_unregister(exynos_drm_pdev);
 
 	return ret;
@@ -789,9 +715,9 @@ err_unregister_pd:
 static void exynos_drm_exit(void)
 {
 	platform_driver_unregister(&exynos_drm_platform_driver);
-#ifdef CONFIG_DRM_EXYNOS_VIDI
+
 	exynos_drm_remove_vidi();
-#endif
+
 	platform_device_unregister(exynos_drm_pdev);
 }
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index d22e640..3c81c4b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -323,15 +323,14 @@ int exynos_platform_device_hdmi_register(void);
  */
 void exynos_platform_device_hdmi_unregister(void);
 
-/*
- * this function registers exynos drm ipp platform device.
- */
+#ifdef CONFIG_DRM_EXYNOS_IPP
 int exynos_platform_device_ipp_register(void);
-
-/*
- * this function unregisters exynos drm ipp platform device if it exists.
- */
 void exynos_platform_device_ipp_unregister(void);
+#else
+static inline int exynos_platform_device_ipp_register(void) { return 0; }
+static inline void exynos_platform_device_ipp_unregister(void) {}
+#endif
+
 
 #ifdef CONFIG_DRM_EXYNOS_DPI
 struct exynos_drm_display * exynos_dpi_probe(struct device *dev);
@@ -342,15 +341,13 @@ exynos_dpi_probe(struct device *dev) { return NULL; }
 static inline int exynos_dpi_remove(struct device *dev) { return 0; }
 #endif
 
-/*
- * this function registers exynos drm vidi platform device/driver.
- */
+#ifdef CONFIG_DRM_EXYNOS_VIDI
 int exynos_drm_probe_vidi(void);
-
-/*
- * this function unregister exynos drm vidi platform device/driver.
- */
 void exynos_drm_remove_vidi(void);
+#else
+static inline int exynos_drm_probe_vidi(void) { return 0; }
+static inline void exynos_drm_remove_vidi(void) {}
+#endif
 
 /* This function creates a encoder and a connector, and initializes them. */
 int exynos_drm_create_enc_conn(struct drm_device *dev,
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: fix null pointer dereference issue

[Gustavo Padovan]

2014-11-13 Inki Dae <inki.dae@samsung.com>:

> This patch fixes null pointer dereference issue incurred
> when ipp driver is enabled and Exynos drm driver is closed.
> 
> Non kms driver should register its own sub driver to setup necessary
> resources, which is done by load(). So null pointer dereference
> occurs when ipp driver is enabled and Exynos drm driver is closed
> because ipp core device is registered after component_master_add_with_match
> call.
> 
> This patch makes exynos_drm_device_subdrv_probe() to be called after all non
> kms drivers are registered.

This patch is breaking exynos initialization, exynos_drm_device_subdrv_probe()
needs the drvdata but it is still NULL at this point which make the whole
exynos init fails. The drvdata is only set in exynos_drm_load() so we need
call exynos_drm_device_subdrv_probe() after that.

Do you have the crash output for this? What is the issue you are fixing?
Usually you should add this kind of information to you commit message, it
helps us understand what you are fixing, specially in cases when a regression
is introduced, like this patch for example

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: fix null pointer dereference issue

[Inki Dae]

On 2014년 11월 21일 08:12, Gustavo Padovan wrote:
> 2014-11-13 Inki Dae <inki.dae@samsung.com>:
> 
>> This patch fixes null pointer dereference issue incurred
>> when ipp driver is enabled and Exynos drm driver is closed.
>>
>> Non kms driver should register its own sub driver to setup necessary
>> resources, which is done by load(). So null pointer dereference
>> occurs when ipp driver is enabled and Exynos drm driver is closed
>> because ipp core device is registered after component_master_add_with_match
>> call.
>>
>> This patch makes exynos_drm_device_subdrv_probe() to be called after all non
>> kms drivers are registered.
> 
> This patch is breaking exynos initialization, exynos_drm_device_subdrv_probe()
> needs the drvdata but it is still NULL at this point which make the whole
> exynos init fails. The drvdata is only set in exynos_drm_load() so we need
> call exynos_drm_device_subdrv_probe() after that.

There might be my missing point but with this patch,
exynos_drm_device_subdrv_probe() will be called after exynos_drm_load()
call because all kms drivers are probed before
component_master_add_with_match call so exynos_drm_load() must be called
by component_master_add_with_match function before
exynos_drm_device_subdrv_probe call.

So could you show me the error messages you faced with? There might be a
corner case I missed.

> 
> Do you have the crash output for this? What is the issue you are fixing?

Ok, below is the error messages,
# modetest
[    5.653291] ------------[ cut here ]------------
[    5.656469] WARNING: CPU: 2 PID: 1404 at kernel/locking/mutex.c:511
__mutex_lock_slowpath+0x3d4/0x3d8()
[    5.665816] DEBUG_LOCKS_WARN_ON(l->magic != l)
[    5.670069] Modules linked in:
[    5.673286] CPU: 2 PID: 1404 Comm: modetest Not tainted
3.18.0-rc3-146775-gbcfef97 #1149
[    5.681389] [<c0014400>] (unwind_backtrace) from [<c0011570>]
(show_stack+0x10/0x14)
[    5.689090] [<c0011570>] (show_stack) from [<c0474060>]
(dump_stack+0x84/0xc4)
[    5.696304] [<c0474060>] (dump_stack) from [<c0021918>]
(warn_slowpath_common+0x6c/0x88)
[    5.704364] [<c0021918>] (warn_slowpath_common) from [<c0021964>]
(warn_slowpath_fmt+0x30/0x40)
[    5.713047] [<c0021964>] (warn_slowpath_fmt) from [<c0477a4c>]
(__mutex_lock_slowpath+0x3d4/0x3d8)
[    5.721984] [<c0477a4c>] (__mutex_lock_slowpath) from [<c0477a5c>]
(mutex_lock+0xc/0x24)
[    5.730069] [<c0477a5c>] (mutex_lock) from [<c028e6fc>]
(ipp_subdrv_close+0x4c/0x13c)
[    5.737881] [<c028e6fc>] (ipp_subdrv_close) from [<c027a51c>]
(exynos_drm_subdrv_close+0x3c/0x4c)
[    5.746731] [<c027a51c>] (exynos_drm_subdrv_close) from [<c025eadc>]
(drm_release+0x94/0x4c8)
[    5.755228] [<c025eadc>] (drm_release) from [<c00cbdd4>]
(__fput+0x80/0x1c8)
[    5.762268] [<c00cbdd4>] (__fput) from [<c0037840>]
(task_work_run+0xac/0xe4)
[    5.769382] [<c0037840>] (task_work_run) from [<c00110f8>]
(do_work_pending+0x94/0xb4)
[    5.777275] [<c00110f8>] (do_work_pending) from [<c000e6e0>]
(work_pending+0xc/0x20)
[    5.784994] ---[ end trace bb48a41ae89d1f25 ]---
[    5.789598] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
[    5.797664] pgd = ee3b8000
[    5.800354] [00000000] *pgd=6e366831, *pte=00000000, *ppte=00000000
[    5.806610] Internal error: Oops: 817 [#1] PREEMPT SMP ARM
[    5.812074] Modules linked in:
[    5.815117] CPU: 2 PID: 1404 Comm: modetest Tainted: G        W
3.18.0-rc3-146775-gbcfef97 #1149
[    5.824314] task: eea90800 ti: ee33c000 task.ti: ee33c000
[    5.829704] PC is at __mutex_lock_slowpath+0xf4/0x3d8
[    5.834730] LR is at __mutex_lock_slowpath+0xdc/0x3d8
[    5.839765] pc : [<c047776c>]    lr : [<c0477754>]    psr: 80000093
[    5.839765] sp : ee33de88  ip : ee33de98  fp : c06cb814
[    5.851220] r10: ee0f5854  r9 : c0700784  r8 : eea90800
[    5.856429] r7 : ee33c008  r6 : 60000013  r5 : ee0f5844  r4 : ee0f5840
[    5.862938] r3 : 00000000  r2 : 00000000  r1 : ee33de88  r0 : ee0f5840
[    5.869451] Flags: Nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment user
[    5.876654] Control: 10c5387d  Table: 6e3b804a  DAC: 00000015
[    5.882383] Process modetest (pid: 1404, stack limit = 0xee33c240)
[    5.888544] Stack: (0xee33de88 to 0xee33e000)
[    5.892888] de80:                   ee0f5854 00000000 11111111
ee33de88 0001d030 ee0f5840
[    5.901048] dea0: c06b1d94 ee0f5810 c0705e4c ee0eeb00 00000000
ee0f5840 ee0f5810 c0477a5c
[    5.909207] dec0: eeb4a510 c028e6fc ee14a434 eeb4a510 c06b2fe4
ee14a400 ee33c000 eeb4a510
[    5.917366] dee0: c06b1d94 ee0eeb00 ee14a400 ee14a434 00000008
ee0eea08 00000000 c027a51c
[    5.925525] df00: c0705e4c ee0eeb00 ee0eea00 ee14a400 ee14a434
c025eadc ee0eea08 00000001
[    5.933684] df20: eebce000 00000000 00000000 00000000 00000021
ee0eea00 ee3354e0 00000000
[    5.941843] df40: ee32a250 ee711428 00000008 ee0eea08 00000000
c00cbdd4 00000000 00000000
[    5.950002] df60: eea90b4c 00000000 c06ca604 eea90800 c000e824
ee33c000 00000000 c0037840
[    5.958161] df80: ee33c018 c000e824 ee33dfb0 ee33c000 c000e824
c00110f8 00000003 00000001
[    5.966320] dfa0: beff0a4c 00000006 c000e824 c000e6e0 00000000
0001d000 00000003 0001d000
[    5.974479] dfc0: 00000003 00000001 beff0a4c 00000006 00000000
00000000 00000000 00000000
[    5.982639] dfe0: 00000000 beff09a4 b6f74257 b6eef626 40000030
00000003 00000000 00000000
[    5.990807] [<c047776c>] (__mutex_lock_slowpath) from [<c0477a5c>]
(mutex_lock+0xc/0x24)
[    5.998883] [<c0477a5c>] (mutex_lock) from [<c028e6fc>]
(ipp_subdrv_close+0x4c/0x13c)
[    6.006693] [<c028e6fc>] (ipp_subdrv_close) from [<c027a51c>]
(exynos_drm_subdrv_close+0x3c/0x4c)
[    6.015546] [<c027a51c>] (exynos_drm_subdrv_close) from [<c025eadc>]
(drm_release+0x94/0x4c8)
[    6.024047] [<c025eadc>] (drm_release) from [<c00cbdd4>]
(__fput+0x80/0x1c8)
[    6.031082] [<c00cbdd4>] (__fput) from [<c0037840>]
(task_work_run+0xac/0xe4)
[    6.038200] [<c0037840>] (task_work_run) from [<c00110f8>]
(do_work_pending+0x94/0xb4)
[    6.046094] [<c00110f8>] (do_work_pending) from [<c000e6e0>]
(work_pending+0xc/0x20)
[    6.053816] Code: e284a014 e584d018 e58da000 e58d3004 (e583d000)
[    6.059892] ---[ end trace bb48a41ae89d1f26 ]---

Thanks,
Inki Dae

> Usually you should add this kind of information to you commit message, it
> helps us understand what you are fixing, specially in cases when a regression
> is introduced, like this patch for example
> 
> 	Gustavo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: fix null pointer dereference issue

[Gustavo Padovan]

2014-11-21 Inki Dae <inki.dae@samsung.com>:

> On 2014년 11월 21일 08:12, Gustavo Padovan wrote:
> > 2014-11-13 Inki Dae <inki.dae@samsung.com>:
> > 
> >> This patch fixes null pointer dereference issue incurred
> >> when ipp driver is enabled and Exynos drm driver is closed.
> >>
> >> Non kms driver should register its own sub driver to setup necessary
> >> resources, which is done by load(). So null pointer dereference
> >> occurs when ipp driver is enabled and Exynos drm driver is closed
> >> because ipp core device is registered after component_master_add_with_match
> >> call.
> >>
> >> This patch makes exynos_drm_device_subdrv_probe() to be called after all non
> >> kms drivers are registered.
> > 
> > This patch is breaking exynos initialization, exynos_drm_device_subdrv_probe()
> > needs the drvdata but it is still NULL at this point which make the whole
> > exynos init fails. The drvdata is only set in exynos_drm_load() so we need
> > call exynos_drm_device_subdrv_probe() after that.
> 
> There might be my missing point but with this patch,
> exynos_drm_device_subdrv_probe() will be called after exynos_drm_load()
> call because all kms drivers are probed before
> component_master_add_with_match call so exynos_drm_load() must be called
> by component_master_add_with_match function before
> exynos_drm_device_subdrv_probe call.
> 
> So could you show me the error messages you faced with? There might be a
> corner case I missed.

I've added some debug output to it. It fails on
exynos_drm_device_subdrv_probe() because the drvdata is NULL. I've added debug
output to exynos_drm_load() and as you can see it doesn't get called before
subdrv_probe(). I'm testing this on snow.

[    1.767835] [drm] Initialized drm 1.1.0 20060810
[    1.771120] [drm:exynos_drm_init] 
[    1.774774] [drm:exynos_drm_platform_probe] 
[    1.778760] platform exynos-drm: Driver exynos-drm requests probe deferral
[    1.786178] platform 145b0000.dp-controller: Driver exynos-dp requests
probe deferral
[    1.794374] exynos-drm-ipp exynos-drm-ipp: drm ipp registered successfully.
[    1.800372] [drm:exynos_drm_device_subdrv_probe] dev   (null)


	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: fix null pointer dereference issue

[Inki Dae]

2014-11-21 23:41 GMT+09:00 Gustavo Padovan <gustavo@padovan.org>:
> 2014-11-21 Inki Dae <inki.dae@samsung.com>:
>
>> On 2014년 11월 21일 08:12, Gustavo Padovan wrote:
>> > 2014-11-13 Inki Dae <inki.dae@samsung.com>:
>> >
>> >> This patch fixes null pointer dereference issue incurred
>> >> when ipp driver is enabled and Exynos drm driver is closed.
>> >>
>> >> Non kms driver should register its own sub driver to setup necessary
>> >> resources, which is done by load(). So null pointer dereference
>> >> occurs when ipp driver is enabled and Exynos drm driver is closed
>> >> because ipp core device is registered after component_master_add_with_match
>> >> call.
>> >>
>> >> This patch makes exynos_drm_device_subdrv_probe() to be called after all non
>> >> kms drivers are registered.
>> >
>> > This patch is breaking exynos initialization, exynos_drm_device_subdrv_probe()
>> > needs the drvdata but it is still NULL at this point which make the whole
>> > exynos init fails. The drvdata is only set in exynos_drm_load() so we need
>> > call exynos_drm_device_subdrv_probe() after that.
>>
>> There might be my missing point but with this patch,
>> exynos_drm_device_subdrv_probe() will be called after exynos_drm_load()
>> call because all kms drivers are probed before
>> component_master_add_with_match call so exynos_drm_load() must be called
>> by component_master_add_with_match function before
>> exynos_drm_device_subdrv_probe call.
>>
>> So could you show me the error messages you faced with? There might be a
>> corner case I missed.
>
> I've added some debug output to it. It fails on
> exynos_drm_device_subdrv_probe() because the drvdata is NULL. I've added debug
> output to exynos_drm_load() and as you can see it doesn't get called before
> subdrv_probe(). I'm testing this on snow.
>
> [    1.767835] [drm] Initialized drm 1.1.0 20060810
> [    1.771120] [drm:exynos_drm_init]
> [    1.774774] [drm:exynos_drm_platform_probe]
> [    1.778760] platform exynos-drm: Driver exynos-drm requests probe deferral
> [    1.786178] platform 145b0000.dp-controller: Driver exynos-dp requests
> probe deferral
> [    1.794374] exynos-drm-ipp exynos-drm-ipp: drm ipp registered successfully.
> [    1.800372] [drm:exynos_drm_device_subdrv_probe] dev   (null)
>

Still strange to me. I'd like to reproduce this error because it's
impossible that exynos_drm_device_subdrv_probe is called without load
call.
even If error, this patch must be no problem so in this case, we would
need check if there might be something we missed about component
binding not this patch.
So can you let me know where the git repository is and which defconfig
file did you tested?

Thanks,
Inki Dae

>
>         Gustavo
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: fix null pointer dereference issue

[Inki Dae]

2014-11-23 0:28 GMT+09:00 Inki Dae <inki.dae@samsung.com>:
> 2014-11-21 23:41 GMT+09:00 Gustavo Padovan <gustavo@padovan.org>:
>> 2014-11-21 Inki Dae <inki.dae@samsung.com>:
>>
>>> On 2014년 11월 21일 08:12, Gustavo Padovan wrote:
>>> > 2014-11-13 Inki Dae <inki.dae@samsung.com>:
>>> >
>>> >> This patch fixes null pointer dereference issue incurred
>>> >> when ipp driver is enabled and Exynos drm driver is closed.
>>> >>
>>> >> Non kms driver should register its own sub driver to setup necessary
>>> >> resources, which is done by load(). So null pointer dereference
>>> >> occurs when ipp driver is enabled and Exynos drm driver is closed
>>> >> because ipp core device is registered after component_master_add_with_match
>>> >> call.
>>> >>
>>> >> This patch makes exynos_drm_device_subdrv_probe() to be called after all non
>>> >> kms drivers are registered.
>>> >
>>> > This patch is breaking exynos initialization, exynos_drm_device_subdrv_probe()
>>> > needs the drvdata but it is still NULL at this point which make the whole
>>> > exynos init fails. The drvdata is only set in exynos_drm_load() so we need
>>> > call exynos_drm_device_subdrv_probe() after that.
>>>
>>> There might be my missing point but with this patch,
>>> exynos_drm_device_subdrv_probe() will be called after exynos_drm_load()
>>> call because all kms drivers are probed before
>>> component_master_add_with_match call so exynos_drm_load() must be called
>>> by component_master_add_with_match function before
>>> exynos_drm_device_subdrv_probe call.
>>>
>>> So could you show me the error messages you faced with? There might be a
>>> corner case I missed.
>>
>> I've added some debug output to it. It fails on
>> exynos_drm_device_subdrv_probe() because the drvdata is NULL. I've added debug
>> output to exynos_drm_load() and as you can see it doesn't get called before
>> subdrv_probe(). I'm testing this on snow.
>>
>> [    1.767835] [drm] Initialized drm 1.1.0 20060810
>> [    1.771120] [drm:exynos_drm_init]
>> [    1.774774] [drm:exynos_drm_platform_probe]
>> [    1.778760] platform exynos-drm: Driver exynos-drm requests probe deferral
>> [    1.786178] platform 145b0000.dp-controller: Driver exynos-dp requests
>> probe deferral
>> [    1.794374] exynos-drm-ipp exynos-drm-ipp: drm ipp registered successfully.
>> [    1.800372] [drm:exynos_drm_device_subdrv_probe] dev   (null)
>>
>
> Still strange to me. I'd like to reproduce this error because it's
> impossible that exynos_drm_device_subdrv_probe is called without load
> call.
> even If error, this patch must be no problem so in this case, we would
> need check if there might be something we missed about component
> binding not this patch.

But it's better to call exynos_drm_device_subdrv_probe() in load()
like before because we should move all register codes of sub drivers
into init() for the meantime. So let's just revert it.

Thanks,
Inki Dae

> So can you let me know where the git repository is and which defconfig
> file did you tested?
>
> Thanks,
> Inki Dae
>
>>
>>         Gustavo
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel