Re: [PATCH 4/4] drm/tilcdc: Use unload to handle initialization failures

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jyri,

Thank you for the patch.

On Wednesday 19 Oct 2016 00:33:56 Jyri Sarha wrote:
> 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.

While at it, you could replace the deprecated drm_put_dev() calls and use 
drm_dev_unregister() and drm_dev_unref() directly. Note that the former 
contains a call to drm_vblank_cleanup(), you can thus remove the direct call 
to that function. As far as I understand drm_dev_unregister() is safe to call 
after drm_mode_config_init() only but doesn't require a previous call to even 
drm_dev_register(). I'm not sure if that's guaranteed though, or if it just 
works by chance.

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 91 +++++++++++-----------------------
>  1 file changed, 28 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 2dca822..8820133 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -160,8 +160,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) {
> @@ -200,18 +198,19 @@ static int tilcdc_unload(struct drm_device *dev)
>  {
>  	struct tilcdc_drm_private *priv = dev->dev_private;
> 
> -	tilcdc_remove_external_encoders(dev);
> +	if (priv->fbdev)
> +		drm_fbdev_cma_fini(priv->fbdev);
> 
> -	drm_fbdev_cma_fini(priv->fbdev);
>  	drm_kms_helper_poll_fini(dev);
>  	drm_mode_config_cleanup(dev);
>  	drm_vblank_cleanup(dev);
> -
>  	drm_irq_uninstall(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)
> @@ -220,8 +219,10 @@ static int tilcdc_unload(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;
> 
> @@ -252,6 +253,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;
> @@ -259,28 +262,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
> @@ -289,7 +292,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
> 
> @@ -365,37 +369,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);
> @@ -405,56 +407,19 @@ 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;
> 
>  	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;
> +init_failed:
> +	tilcdc_unload(ddev);
>  	drm_dev_unref(ddev);
> 
>  	return ret;

-- 
Regards,

Laurent Pinchart

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