diff mbox

[PATCHv3,3/4] drm/tilcdc: Use unload to handle initialization failures

Message ID b790d0ee5e473e245d5a523027531947c0c485e7.1478102115.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha Nov. 2, 2016, 3:57 p.m. UTC 
Use unload to handle initialization failures instead of complex goto
label mess. To do this the initialization sequence needed slight
reordering and some unload functions needed to become conditional.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c |  10 ++--
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 101 ++++++++++++-----------------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |   3 +-
 3 files changed, 43 insertions(+), 71 deletions(-)

Comments

Bartosz Golaszewski Nov. 18, 2016, 4:57 p.m. UTC | #1 
2016-11-02 16:57 GMT+01:00 Jyri Sarha <jsarha@ti.com>:
> Use unload to handle initialization failures instead of complex goto
> label mess. To do this the initialization sequence needed slight
> reordering and some unload functions needed to become conditional.
>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---

I'm not sure yet of the exact error path, but with this patch
tilcdc_crtc_destroy() fails with a NULL-pointer dereference at
dmam_free_coherent() due to crtc->dev being NULL if there are no
panels registered.

Thanks,
Bartosz Golaszewski
Jyri Sarha Nov. 21, 2016, 10:24 a.m. UTC | #2 
On 11/18/16 18:57, Bartosz Golaszewski wrote:
> 2016-11-02 16:57 GMT+01:00 Jyri Sarha <jsarha@ti.com>:
>> Use unload to handle initialization failures instead of complex goto
>> label mess. To do this the initialization sequence needed slight
>> reordering and some unload functions needed to become conditional.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
> 
> I'm not sure yet of the exact error path, but with this patch
> tilcdc_crtc_destroy() fails with a NULL-pointer dereference at
> dmam_free_coherent() due to crtc->dev being NULL if there are no
> panels registered.
> 

Argh, should have read the dmam_alloc_coherent() function documentation.
I just wondered what the extra m in function prefix was for and did not
realize that it was a devres version of the function (I would have
expected such a function to be called devm_dma_alloc_coherent()).

Anyway, I'll drop the "drm/tilcdc: Free palette dma memory in
tilcdc_crtc_destroy()" patch.

Thanks,
Jyri
Bartosz Golaszewski Nov. 21, 2016, 10:44 a.m. UTC | #3 
2016-11-21 11:24 GMT+01:00 Jyri Sarha <jsarha@ti.com>:
> On 11/18/16 18:57, Bartosz Golaszewski wrote:
>> 2016-11-02 16:57 GMT+01:00 Jyri Sarha <jsarha@ti.com>:
>>> Use unload to handle initialization failures instead of complex goto
>>> label mess. To do this the initialization sequence needed slight
>>> reordering and some unload functions needed to become conditional.
>>>
>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>> ---
>>
>> I'm not sure yet of the exact error path, but with this patch
>> tilcdc_crtc_destroy() fails with a NULL-pointer dereference at
>> dmam_free_coherent() due to crtc->dev being NULL if there are no
>> panels registered.
>>
>
> Argh, should have read the dmam_alloc_coherent() function documentation.
> I just wondered what the extra m in function prefix was for and did not
> realize that it was a devres version of the function (I would have
> expected such a function to be called devm_dma_alloc_coherent()).
>

I don't get it either - the original commit introducing devres
(9ac7849e35f7: "devres: device resource management") already had
different prefixes for different managed interfaces.

Maybe we should propose renaming them unless there's a good reason to
keep the dmam prefix?

Thanks,
Bartosz
diff mbox

Patch

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index ea79e09..6277363 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -177,14 +177,12 @@  static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	tilcdc_crtc->enabled = true;
 }
 
-void tilcdc_crtc_disable(struct drm_crtc *crtc)
+void tilcdc_crtc_off(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
-
 	if (!tilcdc_crtc->enabled)
 		return;
 
@@ -228,6 +226,12 @@  void tilcdc_crtc_disable(struct drm_crtc *crtc)
 	tilcdc_crtc->enabled = false;
 }
 
+static void tilcdc_crtc_disable(struct drm_crtc *crtc)
+{
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+	tilcdc_crtc_off(crtc);
+}
+
 static bool tilcdc_crtc_is_on(struct drm_crtc *crtc)
 {
 	return crtc->state && crtc->state->enable && crtc->state->active;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 11f3262..14896b6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -158,8 +158,6 @@  static int modeset_init(struct drm_device *dev)
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_module *mod;
 
-	drm_mode_config_init(dev);
-
 	priv->crtc = tilcdc_crtc_create(dev);
 
 	list_for_each_entry(mod, &module_list, list) {
@@ -198,22 +196,25 @@  static void tilcdc_fini(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	drm_modeset_lock_crtc(priv->crtc, NULL);
-	tilcdc_crtc_disable(priv->crtc);
-	drm_modeset_unlock_crtc(priv->crtc);
+	if (priv->crtc)
+		tilcdc_crtc_off(priv->crtc);
 
-	drm_dev_unregister(dev);
+	if (priv->is_registered)
+		drm_dev_unregister(dev);
 
 	drm_kms_helper_poll_fini(dev);
-	drm_fbdev_cma_fini(priv->fbdev);
+
+	if (priv->fbdev)
+		drm_fbdev_cma_fini(priv->fbdev);
+
 	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
-
 	tilcdc_remove_external_encoders(dev);
 
 #ifdef CONFIG_CPU_FREQ
-	cpufreq_unregister_notifier(&priv->freq_transition,
-			CPUFREQ_TRANSITION_NOTIFIER);
+	if (priv->freq_transition.notifier_call)
+		cpufreq_unregister_notifier(&priv->freq_transition,
+					    CPUFREQ_TRANSITION_NOTIFIER);
 #endif
 
 	if (priv->clk)
@@ -222,8 +223,10 @@  static void tilcdc_fini(struct drm_device *dev)
 	if (priv->mmio)
 		iounmap(priv->mmio);
 
-	flush_workqueue(priv->wq);
-	destroy_workqueue(priv->wq);
+	if (priv->wq) {
+		flush_workqueue(priv->wq);
+		destroy_workqueue(priv->wq);
+	}
 
 	dev->dev_private = NULL;
 
@@ -254,6 +257,8 @@  static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 
 	ddev->platformdev = pdev;
 	ddev->dev_private = priv;
+	platform_set_drvdata(pdev, ddev);
+	drm_mode_config_init(ddev);
 
 	priv->is_componentized =
 		tilcdc_get_external_components(dev, NULL) > 0;
@@ -261,28 +266,28 @@  static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 	priv->wq = alloc_ordered_workqueue("tilcdc", 0);
 	if (!priv->wq) {
 		ret = -ENOMEM;
-		goto fail_unset_priv;
+		goto init_failed;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "failed to get memory resource\n");
 		ret = -EINVAL;
-		goto fail_free_wq;
+		goto init_failed;
 	}
 
 	priv->mmio = ioremap_nocache(res->start, resource_size(res));
 	if (!priv->mmio) {
 		dev_err(dev, "failed to ioremap\n");
 		ret = -ENOMEM;
-		goto fail_free_wq;
+		goto init_failed;
 	}
 
 	priv->clk = clk_get(dev, "fck");
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "failed to get functional clock\n");
 		ret = -ENODEV;
-		goto fail_iounmap;
+		goto init_failed;
 	}
 
 #ifdef CONFIG_CPU_FREQ
@@ -291,7 +296,8 @@  static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 			CPUFREQ_TRANSITION_NOTIFIER);
 	if (ret) {
 		dev_err(dev, "failed to register cpufreq notifier\n");
-		goto fail_put_clk;
+		priv->freq_transition.notifier_call = NULL;
+		goto init_failed;
 	}
 #endif
 
@@ -367,37 +373,35 @@  static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 	ret = modeset_init(ddev);
 	if (ret < 0) {
 		dev_err(dev, "failed to initialize mode setting\n");
-		goto fail_cpufreq_unregister;
+		goto init_failed;
 	}
 
-	platform_set_drvdata(pdev, ddev);
-
 	if (priv->is_componentized) {
 		ret = component_bind_all(dev, ddev);
 		if (ret < 0)
-			goto fail_mode_config_cleanup;
+			goto init_failed;
 
 		ret = tilcdc_add_external_encoders(ddev);
 		if (ret < 0)
-			goto fail_component_cleanup;
+			goto init_failed;
 	}
 
 	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
 		dev_err(dev, "no encoders/connectors found\n");
 		ret = -ENXIO;
-		goto fail_external_cleanup;
+		goto init_failed;
 	}
 
 	ret = drm_vblank_init(ddev, 1);
 	if (ret < 0) {
 		dev_err(dev, "failed to initialize vblank\n");
-		goto fail_external_cleanup;
+		goto init_failed;
 	}
 
 	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
 	if (ret < 0) {
 		dev_err(dev, "failed to install IRQ handler\n");
-		goto fail_vblank_cleanup;
+		goto init_failed;
 	}
 
 	drm_mode_config_reset(ddev);
@@ -407,57 +411,20 @@  static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 			ddev->mode_config.num_connector);
 	if (IS_ERR(priv->fbdev)) {
 		ret = PTR_ERR(priv->fbdev);
-		goto fail_irq_uninstall;
+		goto init_failed;
 	}
 
 	drm_kms_helper_poll_init(ddev);
 
 	ret = drm_dev_register(ddev, 0);
 	if (ret)
-		goto fail_platform_init;
+		goto init_failed;
 
+	priv->is_registered = true;
 	return 0;
 
-fail_platform_init:
-	drm_kms_helper_poll_fini(ddev);
-	drm_fbdev_cma_fini(priv->fbdev);
-
-fail_irq_uninstall:
-	drm_irq_uninstall(ddev);
-
-fail_vblank_cleanup:
-	drm_vblank_cleanup(ddev);
-
-fail_component_cleanup:
-	if (priv->is_componentized)
-		component_unbind_all(dev, dev);
-
-fail_mode_config_cleanup:
-	drm_mode_config_cleanup(ddev);
-
-fail_external_cleanup:
-	tilcdc_remove_external_encoders(ddev);
-
-fail_cpufreq_unregister:
-	pm_runtime_disable(dev);
-#ifdef CONFIG_CPU_FREQ
-	cpufreq_unregister_notifier(&priv->freq_transition,
-			CPUFREQ_TRANSITION_NOTIFIER);
-
-fail_put_clk:
-#endif
-	clk_put(priv->clk);
-
-fail_iounmap:
-	iounmap(priv->mmio);
-
-fail_free_wq:
-	flush_workqueue(priv->wq);
-	destroy_workqueue(priv->wq);
-
-fail_unset_priv:
-	ddev->dev_private = NULL;
-	drm_dev_unref(ddev);
+init_failed:
+	tilcdc_fini(ddev);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 9780c37..7db23f2 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -89,6 +89,7 @@  struct tilcdc_drm_private {
 	struct drm_connector *connectors[8];
 	const struct drm_connector_helper_funcs *connector_funcs[8];
 
+	bool is_registered;
 	bool is_componentized;
 };
 
@@ -172,7 +173,7 @@  void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
 					bool simulate_vesa_sync);
 int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode);
 int tilcdc_crtc_max_width(struct drm_crtc *crtc);
-void tilcdc_crtc_disable(struct drm_crtc *crtc);
+void tilcdc_crtc_off(struct drm_crtc *crtc);
 int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 		struct drm_framebuffer *fb,
 		struct drm_pending_vblank_event *event);