[PATCH] drm: shmobile: Perform initialization/cleanup at probe/remove time

[PATCH] drm: shmobile: Perform initialization/cleanup at probe/remove time

[Laurent Pinchart]

The drm driver .load() operation is prone to race conditions as it
initializes the driver after registering the device nodes. Its usage is
deprecated, inline it in the probe function and call drm_dev_alloc() and
drm_dev_register() explicitly.

For consistency inline the .unload() handler in the remove function as
well.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/shmobile/shmob_drm_drv.c | 206 +++++++++++++++----------------
 1 file changed, 103 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
index 38dd55f4af81..ec7a5eb809a2 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
@@ -104,102 +104,6 @@ static int shmob_drm_setup_clocks(struct shmob_drm_device *sdev,
  * DRM operations
  */
 
-static int shmob_drm_unload(struct drm_device *dev)
-{
-	drm_kms_helper_poll_fini(dev);
-	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
-	drm_irq_uninstall(dev);
-
-	dev->dev_private = NULL;
-
-	return 0;
-}
-
-static int shmob_drm_load(struct drm_device *dev, unsigned long flags)
-{
-	struct shmob_drm_platform_data *pdata = dev->dev->platform_data;
-	struct platform_device *pdev = dev->platformdev;
-	struct shmob_drm_device *sdev;
-	struct resource *res;
-	unsigned int i;
-	int ret;
-
-	if (pdata == NULL) {
-		dev_err(dev->dev, "no platform data\n");
-		return -EINVAL;
-	}
-
-	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
-	if (sdev == NULL) {
-		dev_err(dev->dev, "failed to allocate private data\n");
-		return -ENOMEM;
-	}
-
-	sdev->dev = &pdev->dev;
-	sdev->pdata = pdata;
-	spin_lock_init(&sdev->irq_lock);
-
-	sdev->ddev = dev;
-	dev->dev_private = sdev;
-
-	/* I/O resources and clocks */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res == NULL) {
-		dev_err(&pdev->dev, "failed to get memory resource\n");
-		return -EINVAL;
-	}
-
-	sdev->mmio = devm_ioremap_nocache(&pdev->dev, res->start,
-					  resource_size(res));
-	if (sdev->mmio == NULL) {
-		dev_err(&pdev->dev, "failed to remap memory resource\n");
-		return -ENOMEM;
-	}
-
-	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
-	if (ret < 0)
-		return ret;
-
-	ret = shmob_drm_init_interface(sdev);
-	if (ret < 0)
-		return ret;
-
-	ret = shmob_drm_modeset_init(sdev);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to initialize mode setting\n");
-		return ret;
-	}
-
-	for (i = 0; i < 4; ++i) {
-		ret = shmob_drm_plane_create(sdev, i);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "failed to create plane %u\n", i);
-			goto done;
-		}
-	}
-
-	ret = drm_vblank_init(dev, 1);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to initialize vblank\n");
-		goto done;
-	}
-
-	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to install IRQ handler\n");
-		goto done;
-	}
-
-	platform_set_drvdata(pdev, sdev);
-
-done:
-	if (ret)
-		shmob_drm_unload(dev);
-
-	return ret;
-}
-
 static irqreturn_t shmob_drm_irq(int irq, void *arg)
 {
 	struct drm_device *dev = arg;
@@ -255,8 +159,6 @@ static const struct file_operations shmob_drm_fops = {
 static struct drm_driver shmob_drm_driver = {
 	.driver_features	= DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET
 				| DRIVER_PRIME,
-	.load			= shmob_drm_load,
-	.unload			= shmob_drm_unload,
 	.irq_handler		= shmob_drm_irq,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= shmob_drm_enable_vblank,
@@ -319,18 +221,116 @@ static const struct dev_pm_ops shmob_drm_pm_ops = {
  * Platform driver
  */
 
-static int shmob_drm_probe(struct platform_device *pdev)
+static int shmob_drm_remove(struct platform_device *pdev)
 {
-	return drm_platform_init(&shmob_drm_driver, pdev);
+	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
+	struct drm_device *ddev = sdev->ddev;
+
+	drm_dev_unregister(ddev);
+	drm_kms_helper_poll_fini(ddev);
+	drm_mode_config_cleanup(ddev);
+	drm_irq_uninstall(ddev);
+	drm_dev_unref(ddev);
+
+	return 0;
 }
 
-static int shmob_drm_remove(struct platform_device *pdev)
+static int shmob_drm_probe(struct platform_device *pdev)
 {
-	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
+	struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
+	struct shmob_drm_device *sdev;
+	struct drm_device *ddev;
+	struct resource *res;
+	unsigned int i;
+	int ret;
+
+	if (pdata == NULL) {
+		dev_err(&pdev->dev, "no platform data\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Allocate and initialize the driver private data, I/O resources and
+	 * clocks.
+	 */
+	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
+	if (sdev == NULL)
+		return -ENOMEM;
+
+	sdev->dev = &pdev->dev;
+	sdev->pdata = pdata;
+	spin_lock_init(&sdev->irq_lock);
+
+	platform_set_drvdata(pdev, sdev);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sdev->mmio = devm_ioremap_resource(&pdev->dev, res);
+	if (sdev->mmio == NULL)
+		return -ENOMEM;
+
+	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
+	if (ret < 0)
+		return ret;
 
-	drm_put_dev(sdev->ddev);
+	ret = shmob_drm_init_interface(sdev);
+	if (ret < 0)
+		return ret;
+
+	/* Allocate and initialize the DRM device. */
+	ddev = drm_dev_alloc(&shmob_drm_driver, &pdev->dev);
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	sdev->ddev = ddev;
+	ddev->dev_private = sdev;
+
+	ret = shmob_drm_modeset_init(sdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to initialize mode setting\n");
+		goto err_free_drm_dev;
+	}
+
+	for (i = 0; i < 4; ++i) {
+		ret = shmob_drm_plane_create(sdev, i);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to create plane %u\n", i);
+			goto err_modeset_cleanup;
+		}
+	}
+
+	ret = drm_vblank_init(ddev, 1);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to initialize vblank\n");
+		goto err_modeset_cleanup;
+	}
+
+	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to install IRQ handler\n");
+		goto err_vblank_cleanup;
+	}
+
+	/*
+	 * Register the DRM device with the core and the connectors with
+	 * sysfs.
+	 */
+	ret = drm_dev_register(ddev, 0);
+	if (ret < 0)
+		goto err_irq_uninstall;
 
 	return 0;
+
+err_irq_uninstall:
+	drm_irq_uninstall(ddev);
+err_vblank_cleanup:
+	drm_vblank_cleanup(ddev);
+err_modeset_cleanup:
+	drm_kms_helper_poll_fini(ddev);
+	drm_mode_config_cleanup(ddev);
+err_free_drm_dev:
+	drm_dev_unref(ddev);
+
+	return ret;
 }
 
 static struct platform_driver shmob_drm_platform_driver = {
-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: shmobile: Perform initialization/cleanup at probe/remove time

[Daniel Vetter]

On Mon, Dec 12, 2016 at 11:08:48PM +0200, Laurent Pinchart wrote:
> The drm driver .load() operation is prone to race conditions as it
> initializes the driver after registering the device nodes. Its usage is
> deprecated, inline it in the probe function and call drm_dev_alloc() and
> drm_dev_register() explicitly.
> 
> For consistency inline the .unload() handler in the remove function as
> well.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Yay, another put_dev gone.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c | 206 +++++++++++++++----------------
>  1 file changed, 103 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> index 38dd55f4af81..ec7a5eb809a2 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> @@ -104,102 +104,6 @@ static int shmob_drm_setup_clocks(struct shmob_drm_device *sdev,
>   * DRM operations
>   */
>  
> -static int shmob_drm_unload(struct drm_device *dev)
> -{
> -	drm_kms_helper_poll_fini(dev);
> -	drm_mode_config_cleanup(dev);
> -	drm_vblank_cleanup(dev);
> -	drm_irq_uninstall(dev);
> -
> -	dev->dev_private = NULL;
> -
> -	return 0;
> -}
> -
> -static int shmob_drm_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct shmob_drm_platform_data *pdata = dev->dev->platform_data;
> -	struct platform_device *pdev = dev->platformdev;
> -	struct shmob_drm_device *sdev;
> -	struct resource *res;
> -	unsigned int i;
> -	int ret;
> -
> -	if (pdata == NULL) {
> -		dev_err(dev->dev, "no platform data\n");
> -		return -EINVAL;
> -	}
> -
> -	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
> -	if (sdev == NULL) {
> -		dev_err(dev->dev, "failed to allocate private data\n");
> -		return -ENOMEM;
> -	}
> -
> -	sdev->dev = &pdev->dev;
> -	sdev->pdata = pdata;
> -	spin_lock_init(&sdev->irq_lock);
> -
> -	sdev->ddev = dev;
> -	dev->dev_private = sdev;
> -
> -	/* I/O resources and clocks */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (res == NULL) {
> -		dev_err(&pdev->dev, "failed to get memory resource\n");
> -		return -EINVAL;
> -	}
> -
> -	sdev->mmio = devm_ioremap_nocache(&pdev->dev, res->start,
> -					  resource_size(res));
> -	if (sdev->mmio == NULL) {
> -		dev_err(&pdev->dev, "failed to remap memory resource\n");
> -		return -ENOMEM;
> -	}
> -
> -	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = shmob_drm_init_interface(sdev);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = shmob_drm_modeset_init(sdev);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to initialize mode setting\n");
> -		return ret;
> -	}
> -
> -	for (i = 0; i < 4; ++i) {
> -		ret = shmob_drm_plane_create(sdev, i);
> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "failed to create plane %u\n", i);
> -			goto done;
> -		}
> -	}
> -
> -	ret = drm_vblank_init(dev, 1);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to initialize vblank\n");
> -		goto done;
> -	}
> -
> -	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to install IRQ handler\n");
> -		goto done;
> -	}
> -
> -	platform_set_drvdata(pdev, sdev);
> -
> -done:
> -	if (ret)
> -		shmob_drm_unload(dev);
> -
> -	return ret;
> -}
> -
>  static irqreturn_t shmob_drm_irq(int irq, void *arg)
>  {
>  	struct drm_device *dev = arg;
> @@ -255,8 +159,6 @@ static const struct file_operations shmob_drm_fops = {
>  static struct drm_driver shmob_drm_driver = {
>  	.driver_features	= DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET
>  				| DRIVER_PRIME,
> -	.load			= shmob_drm_load,
> -	.unload			= shmob_drm_unload,
>  	.irq_handler		= shmob_drm_irq,
>  	.get_vblank_counter	= drm_vblank_no_hw_counter,
>  	.enable_vblank		= shmob_drm_enable_vblank,
> @@ -319,18 +221,116 @@ static const struct dev_pm_ops shmob_drm_pm_ops = {
>   * Platform driver
>   */
>  
> -static int shmob_drm_probe(struct platform_device *pdev)
> +static int shmob_drm_remove(struct platform_device *pdev)
>  {
> -	return drm_platform_init(&shmob_drm_driver, pdev);
> +	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
> +	struct drm_device *ddev = sdev->ddev;
> +
> +	drm_dev_unregister(ddev);
> +	drm_kms_helper_poll_fini(ddev);
> +	drm_mode_config_cleanup(ddev);
> +	drm_irq_uninstall(ddev);
> +	drm_dev_unref(ddev);
> +
> +	return 0;
>  }
>  
> -static int shmob_drm_remove(struct platform_device *pdev)
> +static int shmob_drm_probe(struct platform_device *pdev)
>  {
> -	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
> +	struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
> +	struct shmob_drm_device *sdev;
> +	struct drm_device *ddev;
> +	struct resource *res;
> +	unsigned int i;
> +	int ret;
> +
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "no platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Allocate and initialize the driver private data, I/O resources and
> +	 * clocks.
> +	 */
> +	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
> +	if (sdev == NULL)
> +		return -ENOMEM;
> +
> +	sdev->dev = &pdev->dev;
> +	sdev->pdata = pdata;
> +	spin_lock_init(&sdev->irq_lock);
> +
> +	platform_set_drvdata(pdev, sdev);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> +	if (sdev->mmio == NULL)
> +		return -ENOMEM;
> +
> +	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
> +	if (ret < 0)
> +		return ret;
>  
> -	drm_put_dev(sdev->ddev);
> +	ret = shmob_drm_init_interface(sdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Allocate and initialize the DRM device. */
> +	ddev = drm_dev_alloc(&shmob_drm_driver, &pdev->dev);
> +	if (IS_ERR(ddev))
> +		return PTR_ERR(ddev);
> +
> +	sdev->ddev = ddev;
> +	ddev->dev_private = sdev;
> +
> +	ret = shmob_drm_modeset_init(sdev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to initialize mode setting\n");
> +		goto err_free_drm_dev;
> +	}
> +
> +	for (i = 0; i < 4; ++i) {
> +		ret = shmob_drm_plane_create(sdev, i);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to create plane %u\n", i);
> +			goto err_modeset_cleanup;
> +		}
> +	}
> +
> +	ret = drm_vblank_init(ddev, 1);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to initialize vblank\n");
> +		goto err_modeset_cleanup;
> +	}
> +
> +	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to install IRQ handler\n");
> +		goto err_vblank_cleanup;
> +	}
> +
> +	/*
> +	 * Register the DRM device with the core and the connectors with
> +	 * sysfs.
> +	 */
> +	ret = drm_dev_register(ddev, 0);
> +	if (ret < 0)
> +		goto err_irq_uninstall;
>  
>  	return 0;
> +
> +err_irq_uninstall:
> +	drm_irq_uninstall(ddev);
> +err_vblank_cleanup:
> +	drm_vblank_cleanup(ddev);
> +err_modeset_cleanup:
> +	drm_kms_helper_poll_fini(ddev);
> +	drm_mode_config_cleanup(ddev);
> +err_free_drm_dev:
> +	drm_dev_unref(ddev);
> +
> +	return ret;
>  }
>  
>  static struct platform_driver shmob_drm_platform_driver = {
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> 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

[PATCH v2] drm: shmobile: Perform initialization/cleanup at probe/remove time

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The drm driver .load() operation is prone to race conditions as it
initializes the driver after registering the device nodes. Its usage is
deprecated, inline it in the probe function and call drm_dev_alloc() and
drm_dev_register() explicitly.

For consistency inline the .unload() handler in the remove function as
well.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
Changes since v1:

- Removed manual the drm_connector_register() that caused sysfs-related
  warnings

 drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   7 +-
 drivers/gpu/drm/shmobile/shmob_drm_drv.c  | 206 +++++++++++++++---------------
 2 files changed, 104 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
index dddbdd62bed0..20bd060bc5a8 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
@@ -692,13 +692,10 @@ int shmob_drm_connector_create(struct shmob_drm_device *sdev,
 		return ret;
 
 	drm_connector_helper_add(connector, &connector_helper_funcs);
-	ret = drm_connector_register(connector);
-	if (ret < 0)
-		goto err_cleanup;
 
 	ret = shmob_drm_backlight_init(&sdev->connector);
 	if (ret < 0)
-		goto err_sysfs;
+		goto err_cleanup;
 
 	ret = drm_mode_connector_attach_encoder(connector, encoder);
 	if (ret < 0)
@@ -712,8 +709,6 @@ int shmob_drm_connector_create(struct shmob_drm_device *sdev,
 
 err_backlight:
 	shmob_drm_backlight_exit(&sdev->connector);
-err_sysfs:
-	drm_connector_unregister(connector);
 err_cleanup:
 	drm_connector_cleanup(connector);
 	return ret;
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
index 38dd55f4af81..ec7a5eb809a2 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
@@ -104,102 +104,6 @@ static int shmob_drm_setup_clocks(struct shmob_drm_device *sdev,
  * DRM operations
  */
 
-static int shmob_drm_unload(struct drm_device *dev)
-{
-	drm_kms_helper_poll_fini(dev);
-	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
-	drm_irq_uninstall(dev);
-
-	dev->dev_private = NULL;
-
-	return 0;
-}
-
-static int shmob_drm_load(struct drm_device *dev, unsigned long flags)
-{
-	struct shmob_drm_platform_data *pdata = dev->dev->platform_data;
-	struct platform_device *pdev = dev->platformdev;
-	struct shmob_drm_device *sdev;
-	struct resource *res;
-	unsigned int i;
-	int ret;
-
-	if (pdata == NULL) {
-		dev_err(dev->dev, "no platform data\n");
-		return -EINVAL;
-	}
-
-	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
-	if (sdev == NULL) {
-		dev_err(dev->dev, "failed to allocate private data\n");
-		return -ENOMEM;
-	}
-
-	sdev->dev = &pdev->dev;
-	sdev->pdata = pdata;
-	spin_lock_init(&sdev->irq_lock);
-
-	sdev->ddev = dev;
-	dev->dev_private = sdev;
-
-	/* I/O resources and clocks */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res == NULL) {
-		dev_err(&pdev->dev, "failed to get memory resource\n");
-		return -EINVAL;
-	}
-
-	sdev->mmio = devm_ioremap_nocache(&pdev->dev, res->start,
-					  resource_size(res));
-	if (sdev->mmio == NULL) {
-		dev_err(&pdev->dev, "failed to remap memory resource\n");
-		return -ENOMEM;
-	}
-
-	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
-	if (ret < 0)
-		return ret;
-
-	ret = shmob_drm_init_interface(sdev);
-	if (ret < 0)
-		return ret;
-
-	ret = shmob_drm_modeset_init(sdev);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to initialize mode setting\n");
-		return ret;
-	}
-
-	for (i = 0; i < 4; ++i) {
-		ret = shmob_drm_plane_create(sdev, i);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "failed to create plane %u\n", i);
-			goto done;
-		}
-	}
-
-	ret = drm_vblank_init(dev, 1);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to initialize vblank\n");
-		goto done;
-	}
-
-	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to install IRQ handler\n");
-		goto done;
-	}
-
-	platform_set_drvdata(pdev, sdev);
-
-done:
-	if (ret)
-		shmob_drm_unload(dev);
-
-	return ret;
-}
-
 static irqreturn_t shmob_drm_irq(int irq, void *arg)
 {
 	struct drm_device *dev = arg;
@@ -255,8 +159,6 @@ static const struct file_operations shmob_drm_fops = {
 static struct drm_driver shmob_drm_driver = {
 	.driver_features	= DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET
 				| DRIVER_PRIME,
-	.load			= shmob_drm_load,
-	.unload			= shmob_drm_unload,
 	.irq_handler		= shmob_drm_irq,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= shmob_drm_enable_vblank,
@@ -319,18 +221,116 @@ static const struct dev_pm_ops shmob_drm_pm_ops = {
  * Platform driver
  */
 
-static int shmob_drm_probe(struct platform_device *pdev)
+static int shmob_drm_remove(struct platform_device *pdev)
 {
-	return drm_platform_init(&shmob_drm_driver, pdev);
+	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
+	struct drm_device *ddev = sdev->ddev;
+
+	drm_dev_unregister(ddev);
+	drm_kms_helper_poll_fini(ddev);
+	drm_mode_config_cleanup(ddev);
+	drm_irq_uninstall(ddev);
+	drm_dev_unref(ddev);
+
+	return 0;
 }
 
-static int shmob_drm_remove(struct platform_device *pdev)
+static int shmob_drm_probe(struct platform_device *pdev)
 {
-	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
+	struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
+	struct shmob_drm_device *sdev;
+	struct drm_device *ddev;
+	struct resource *res;
+	unsigned int i;
+	int ret;
+
+	if (pdata == NULL) {
+		dev_err(&pdev->dev, "no platform data\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Allocate and initialize the driver private data, I/O resources and
+	 * clocks.
+	 */
+	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
+	if (sdev == NULL)
+		return -ENOMEM;
+
+	sdev->dev = &pdev->dev;
+	sdev->pdata = pdata;
+	spin_lock_init(&sdev->irq_lock);
+
+	platform_set_drvdata(pdev, sdev);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sdev->mmio = devm_ioremap_resource(&pdev->dev, res);
+	if (sdev->mmio == NULL)
+		return -ENOMEM;
+
+	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
+	if (ret < 0)
+		return ret;
 
-	drm_put_dev(sdev->ddev);
+	ret = shmob_drm_init_interface(sdev);
+	if (ret < 0)
+		return ret;
+
+	/* Allocate and initialize the DRM device. */
+	ddev = drm_dev_alloc(&shmob_drm_driver, &pdev->dev);
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	sdev->ddev = ddev;
+	ddev->dev_private = sdev;
+
+	ret = shmob_drm_modeset_init(sdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to initialize mode setting\n");
+		goto err_free_drm_dev;
+	}
+
+	for (i = 0; i < 4; ++i) {
+		ret = shmob_drm_plane_create(sdev, i);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to create plane %u\n", i);
+			goto err_modeset_cleanup;
+		}
+	}
+
+	ret = drm_vblank_init(ddev, 1);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to initialize vblank\n");
+		goto err_modeset_cleanup;
+	}
+
+	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to install IRQ handler\n");
+		goto err_vblank_cleanup;
+	}
+
+	/*
+	 * Register the DRM device with the core and the connectors with
+	 * sysfs.
+	 */
+	ret = drm_dev_register(ddev, 0);
+	if (ret < 0)
+		goto err_irq_uninstall;
 
 	return 0;
+
+err_irq_uninstall:
+	drm_irq_uninstall(ddev);
+err_vblank_cleanup:
+	drm_vblank_cleanup(ddev);
+err_modeset_cleanup:
+	drm_kms_helper_poll_fini(ddev);
+	drm_mode_config_cleanup(ddev);
+err_free_drm_dev:
+	drm_dev_unref(ddev);
+
+	return ret;
 }
 
 static struct platform_driver shmob_drm_platform_driver = {
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drm: shmobile: Perform initialization/cleanup at probe/remove time

[Daniel Vetter]

On Tue, Dec 13, 2016 at 02:59:48PM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> The drm driver .load() operation is prone to race conditions as it
> initializes the driver after registering the device nodes. Its usage is
> deprecated, inline it in the probe function and call drm_dev_alloc() and
> drm_dev_register() explicitly.
> 
> For consistency inline the .unload() handler in the remove function as
> well.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> Changes since v1:
> 
> - Removed manual the drm_connector_register() that caused sysfs-related
>   warnings

Hm, what did go boom there? We should catch multiple calls to
drm_connector_register ...
-Daniel

> 
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   7 +-
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c  | 206 +++++++++++++++---------------
>  2 files changed, 104 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> index dddbdd62bed0..20bd060bc5a8 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> @@ -692,13 +692,10 @@ int shmob_drm_connector_create(struct shmob_drm_device *sdev,
>  		return ret;
>  
>  	drm_connector_helper_add(connector, &connector_helper_funcs);
> -	ret = drm_connector_register(connector);
> -	if (ret < 0)
> -		goto err_cleanup;
>  
>  	ret = shmob_drm_backlight_init(&sdev->connector);
>  	if (ret < 0)
> -		goto err_sysfs;
> +		goto err_cleanup;
>  
>  	ret = drm_mode_connector_attach_encoder(connector, encoder);
>  	if (ret < 0)
> @@ -712,8 +709,6 @@ int shmob_drm_connector_create(struct shmob_drm_device *sdev,
>  
>  err_backlight:
>  	shmob_drm_backlight_exit(&sdev->connector);
> -err_sysfs:
> -	drm_connector_unregister(connector);
>  err_cleanup:
>  	drm_connector_cleanup(connector);
>  	return ret;
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> index 38dd55f4af81..ec7a5eb809a2 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> @@ -104,102 +104,6 @@ static int shmob_drm_setup_clocks(struct shmob_drm_device *sdev,
>   * DRM operations
>   */
>  
> -static int shmob_drm_unload(struct drm_device *dev)
> -{
> -	drm_kms_helper_poll_fini(dev);
> -	drm_mode_config_cleanup(dev);
> -	drm_vblank_cleanup(dev);
> -	drm_irq_uninstall(dev);
> -
> -	dev->dev_private = NULL;
> -
> -	return 0;
> -}
> -
> -static int shmob_drm_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct shmob_drm_platform_data *pdata = dev->dev->platform_data;
> -	struct platform_device *pdev = dev->platformdev;
> -	struct shmob_drm_device *sdev;
> -	struct resource *res;
> -	unsigned int i;
> -	int ret;
> -
> -	if (pdata == NULL) {
> -		dev_err(dev->dev, "no platform data\n");
> -		return -EINVAL;
> -	}
> -
> -	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
> -	if (sdev == NULL) {
> -		dev_err(dev->dev, "failed to allocate private data\n");
> -		return -ENOMEM;
> -	}
> -
> -	sdev->dev = &pdev->dev;
> -	sdev->pdata = pdata;
> -	spin_lock_init(&sdev->irq_lock);
> -
> -	sdev->ddev = dev;
> -	dev->dev_private = sdev;
> -
> -	/* I/O resources and clocks */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (res == NULL) {
> -		dev_err(&pdev->dev, "failed to get memory resource\n");
> -		return -EINVAL;
> -	}
> -
> -	sdev->mmio = devm_ioremap_nocache(&pdev->dev, res->start,
> -					  resource_size(res));
> -	if (sdev->mmio == NULL) {
> -		dev_err(&pdev->dev, "failed to remap memory resource\n");
> -		return -ENOMEM;
> -	}
> -
> -	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = shmob_drm_init_interface(sdev);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = shmob_drm_modeset_init(sdev);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to initialize mode setting\n");
> -		return ret;
> -	}
> -
> -	for (i = 0; i < 4; ++i) {
> -		ret = shmob_drm_plane_create(sdev, i);
> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "failed to create plane %u\n", i);
> -			goto done;
> -		}
> -	}
> -
> -	ret = drm_vblank_init(dev, 1);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to initialize vblank\n");
> -		goto done;
> -	}
> -
> -	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to install IRQ handler\n");
> -		goto done;
> -	}
> -
> -	platform_set_drvdata(pdev, sdev);
> -
> -done:
> -	if (ret)
> -		shmob_drm_unload(dev);
> -
> -	return ret;
> -}
> -
>  static irqreturn_t shmob_drm_irq(int irq, void *arg)
>  {
>  	struct drm_device *dev = arg;
> @@ -255,8 +159,6 @@ static const struct file_operations shmob_drm_fops = {
>  static struct drm_driver shmob_drm_driver = {
>  	.driver_features	= DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET
>  				| DRIVER_PRIME,
> -	.load			= shmob_drm_load,
> -	.unload			= shmob_drm_unload,
>  	.irq_handler		= shmob_drm_irq,
>  	.get_vblank_counter	= drm_vblank_no_hw_counter,
>  	.enable_vblank		= shmob_drm_enable_vblank,
> @@ -319,18 +221,116 @@ static const struct dev_pm_ops shmob_drm_pm_ops = {
>   * Platform driver
>   */
>  
> -static int shmob_drm_probe(struct platform_device *pdev)
> +static int shmob_drm_remove(struct platform_device *pdev)
>  {
> -	return drm_platform_init(&shmob_drm_driver, pdev);
> +	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
> +	struct drm_device *ddev = sdev->ddev;
> +
> +	drm_dev_unregister(ddev);
> +	drm_kms_helper_poll_fini(ddev);
> +	drm_mode_config_cleanup(ddev);
> +	drm_irq_uninstall(ddev);
> +	drm_dev_unref(ddev);
> +
> +	return 0;
>  }
>  
> -static int shmob_drm_remove(struct platform_device *pdev)
> +static int shmob_drm_probe(struct platform_device *pdev)
>  {
> -	struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
> +	struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
> +	struct shmob_drm_device *sdev;
> +	struct drm_device *ddev;
> +	struct resource *res;
> +	unsigned int i;
> +	int ret;
> +
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "no platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Allocate and initialize the driver private data, I/O resources and
> +	 * clocks.
> +	 */
> +	sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
> +	if (sdev == NULL)
> +		return -ENOMEM;
> +
> +	sdev->dev = &pdev->dev;
> +	sdev->pdata = pdata;
> +	spin_lock_init(&sdev->irq_lock);
> +
> +	platform_set_drvdata(pdev, sdev);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> +	if (sdev->mmio == NULL)
> +		return -ENOMEM;
> +
> +	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
> +	if (ret < 0)
> +		return ret;
>  
> -	drm_put_dev(sdev->ddev);
> +	ret = shmob_drm_init_interface(sdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Allocate and initialize the DRM device. */
> +	ddev = drm_dev_alloc(&shmob_drm_driver, &pdev->dev);
> +	if (IS_ERR(ddev))
> +		return PTR_ERR(ddev);
> +
> +	sdev->ddev = ddev;
> +	ddev->dev_private = sdev;
> +
> +	ret = shmob_drm_modeset_init(sdev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to initialize mode setting\n");
> +		goto err_free_drm_dev;
> +	}
> +
> +	for (i = 0; i < 4; ++i) {
> +		ret = shmob_drm_plane_create(sdev, i);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to create plane %u\n", i);
> +			goto err_modeset_cleanup;
> +		}
> +	}
> +
> +	ret = drm_vblank_init(ddev, 1);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to initialize vblank\n");
> +		goto err_modeset_cleanup;
> +	}
> +
> +	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to install IRQ handler\n");
> +		goto err_vblank_cleanup;
> +	}
> +
> +	/*
> +	 * Register the DRM device with the core and the connectors with
> +	 * sysfs.
> +	 */
> +	ret = drm_dev_register(ddev, 0);
> +	if (ret < 0)
> +		goto err_irq_uninstall;
>  
>  	return 0;
> +
> +err_irq_uninstall:
> +	drm_irq_uninstall(ddev);
> +err_vblank_cleanup:
> +	drm_vblank_cleanup(ddev);
> +err_modeset_cleanup:
> +	drm_kms_helper_poll_fini(ddev);
> +	drm_mode_config_cleanup(ddev);
> +err_free_drm_dev:
> +	drm_dev_unref(ddev);
> +
> +	return ret;
>  }
>  
>  static struct platform_driver shmob_drm_platform_driver = {
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: shmobile: Perform initialization/cleanup at probe/remove time

[Laurent Pinchart]

Hi Daniel,

On Tuesday 13 Dec 2016 18:11:34 Daniel Vetter wrote:
> On Tue, Dec 13, 2016 at 02:59:48PM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > The drm driver .load() operation is prone to race conditions as it
> > initializes the driver after registering the device nodes. Its usage is
> > deprecated, inline it in the probe function and call drm_dev_alloc() and
> > drm_dev_register() explicitly.
> > 
> > For consistency inline the .unload() handler in the remove function as
> > well.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> > Changes since v1:
> > 
> > - Removed manual the drm_connector_register() that caused sysfs-related
> > 
> >   warnings
> 
> Hm, what did go boom there? We should catch multiple calls to
> drm_connector_register ...

Trying to register the connector before the DRM device is registered makes 
sysfs unhappy due to the lack of a parent.

> >  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   7 +-
> >  drivers/gpu/drm/shmobile/shmob_drm_drv.c  | 206 +++++++++++-------------
> >  2 files changed, 104 insertions(+), 109 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drm: shmobile: Perform initialization/cleanup at probe/remove time

[Daniel Vetter]

On Tue, Dec 13, 2016 at 07:19:15PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Tuesday 13 Dec 2016 18:11:34 Daniel Vetter wrote:
> > On Tue, Dec 13, 2016 at 02:59:48PM +0200, Laurent Pinchart wrote:
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > The drm driver .load() operation is prone to race conditions as it
> > > initializes the driver after registering the device nodes. Its usage is
> > > deprecated, inline it in the probe function and call drm_dev_alloc() and
> > > drm_dev_register() explicitly.
> > > 
> > > For consistency inline the .unload() handler in the remove function as
> > > well.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > > Changes since v1:
> > > 
> > > - Removed manual the drm_connector_register() that caused sysfs-related
> > > 
> > >   warnings
> > 
> > Hm, what did go boom there? We should catch multiple calls to
> > drm_connector_register ...
> 
> Trying to register the connector before the DRM device is registered makes 
> sysfs unhappy due to the lack of a parent.

Autch. I think for i915 we're safe though - our trouble is that mst
hotplug starts to happen before we call drm_dev_register (but it's still
safe there already), because atm the initial topology detection works out
to be async. But we already have the drm_device at least initialized,
which I guess with DT and probe-defer isn't guaranteed.

Or does this even go boom if you just register a child before the parent?
-Daniel

> 
> > >  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   7 +-
> > >  drivers/gpu/drm/shmobile/shmob_drm_drv.c  | 206 +++++++++++-------------
> > >  2 files changed, 104 insertions(+), 109 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: shmobile: Perform initialization/cleanup at probe/remove time

[Laurent Pinchart]

Hi Daniel,

On Tuesday 13 Dec 2016 22:01:36 Daniel Vetter wrote:
> On Tue, Dec 13, 2016 at 07:19:15PM +0200, Laurent Pinchart wrote:
> > On Tuesday 13 Dec 2016 18:11:34 Daniel Vetter wrote:
> >> On Tue, Dec 13, 2016 at 02:59:48PM +0200, Laurent Pinchart wrote:
> >>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> 
> >>> The drm driver .load() operation is prone to race conditions as it
> >>> initializes the driver after registering the device nodes. Its usage
> >>> is deprecated, inline it in the probe function and call
> >>> drm_dev_alloc() and drm_dev_register() explicitly.
> >>> 
> >>> For consistency inline the .unload() handler in the remove function as
> >>> well.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> ---
> >>> Changes since v1:
> >>> 
> >>> - Removed manual the drm_connector_register() that caused
> >>>   sysfs-related warnings
> >> 
> >> Hm, what did go boom there? We should catch multiple calls to
> >> drm_connector_register ...
> > 
> > Trying to register the connector before the DRM device is registered makes
> > sysfs unhappy due to the lack of a parent.
> 
> Autch. I think for i915 we're safe though - our trouble is that mst
> hotplug starts to happen before we call drm_dev_register (but it's still
> safe there already), because atm the initial topology detection works out
> to be async. But we already have the drm_device at least initialized,
> which I guess with DT and probe-defer isn't guaranteed.
> 
> Or does this even go boom if you just register a child before the parent?

I think it does, but I haven't investigated this any further. Given that we 
have support for auto-registration of connectors at drm_device registration 
time, and that I don't need to register connectors later, I just went the easy 
way. For connectors that can be registered at any time, something more 
elaborate is likely needed.

> >>>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   7 +-
> >>>  drivers/gpu/drm/shmobile/shmob_drm_drv.c  | 206 ++++++++++-------------
> >>>  2 files changed, 104 insertions(+), 109 deletions(-)

-- 
Regards,

Laurent Pinchart