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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 13, 2016 at 05:21:43PM +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@xxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/omapdrm/omap_connector.c |   2 -
>  drivers/gpu/drm/omapdrm/omap_drv.c       | 229 ++++++++++++++++---------------
>  drivers/gpu/drm/omapdrm/omap_drv.h       |   1 +
>  drivers/gpu/drm/omapdrm/omap_gem.c       |   3 +-
>  4 files changed, 118 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
> index 691cffebb76e..f90e2d22c5ec 100644
> --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> @@ -240,8 +240,6 @@ struct drm_connector *omap_connector_init(struct drm_device *dev,
>  	connector->interlace_allowed = 1;
>  	connector->doublescan_allowed = 0;
>  
> -	drm_connector_register(connector);
> -
>  	return connector;
>  
>  fail:
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 0a2d461d62cf..dc282e023118 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -493,11 +493,6 @@ static int omap_modeset_init(struct drm_device *dev)
>  	return 0;
>  }
>  
> -static void omap_modeset_free(struct drm_device *dev)
> -{
> -	drm_mode_config_cleanup(dev);
> -}
> -
>  /*
>   * drm ioctl funcs
>   */
> @@ -633,95 +628,6 @@ static const struct drm_ioctl_desc ioctls[DRM_COMMAND_END - DRM_COMMAND_BASE] =
>   * drm driver funcs
>   */
>  
> -/**
> - * load - setup chip and create an initial config
> - * @dev: DRM device
> - * @flags: startup flags
> - *
> - * The driver load routine has to do several things:
> - *   - initialize the memory manager
> - *   - allocate initial config memory
> - *   - setup the DRM framebuffer with the allocated memory
> - */
> -static int dev_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct omap_drm_platform_data *pdata = dev->dev->platform_data;
> -	struct omap_drm_private *priv;
> -	unsigned int i;
> -	int ret;
> -
> -	DBG("load: dev=%p", dev);
> -
> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> -
> -	priv->omaprev = pdata->omaprev;
> -
> -	dev->dev_private = priv;
> -
> -	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
> -	init_waitqueue_head(&priv->commit.wait);
> -	spin_lock_init(&priv->commit.lock);
> -
> -	spin_lock_init(&priv->list_lock);
> -	INIT_LIST_HEAD(&priv->obj_list);
> -
> -	omap_gem_init(dev);
> -
> -	ret = omap_modeset_init(dev);
> -	if (ret) {
> -		dev_err(dev->dev, "omap_modeset_init failed: ret=%d\n", ret);
> -		dev->dev_private = NULL;
> -		kfree(priv);
> -		return ret;
> -	}
> -
> -	/* Initialize vblank handling, start with all CRTCs disabled. */
> -	ret = drm_vblank_init(dev, priv->num_crtcs);
> -	if (ret)
> -		dev_warn(dev->dev, "could not init vblank\n");
> -
> -	for (i = 0; i < priv->num_crtcs; i++)
> -		drm_crtc_vblank_off(priv->crtcs[i]);
> -
> -	priv->fbdev = omap_fbdev_init(dev);
> -
> -	/* store off drm_device for use in pm ops */
> -	dev_set_drvdata(dev->dev, dev);
> -
> -	drm_kms_helper_poll_init(dev);
> -
> -	return 0;
> -}
> -
> -static int dev_unload(struct drm_device *dev)
> -{
> -	struct omap_drm_private *priv = dev->dev_private;
> -
> -	DBG("unload: dev=%p", dev);
> -
> -	drm_kms_helper_poll_fini(dev);
> -
> -	if (priv->fbdev)
> -		omap_fbdev_free(dev);
> -
> -	omap_modeset_free(dev);
> -	omap_gem_deinit(dev);
> -
> -	destroy_workqueue(priv->wq);
> -
> -	drm_vblank_cleanup(dev);
> -	omap_drm_irq_uninstall(dev);
> -
> -	kfree(dev->dev_private);
> -	dev->dev_private = NULL;
> -
> -	dev_set_drvdata(dev->dev, NULL);
> -
> -	return 0;
> -}
> -
>  static int dev_open(struct drm_device *dev, struct drm_file *file)
>  {
>  	file->driver_priv = NULL;
> @@ -806,8 +712,6 @@ static const struct file_operations omapdriver_fops = {
>  static struct drm_driver omap_drm_driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM  | DRIVER_PRIME |
>  		DRIVER_ATOMIC,
> -	.load = dev_load,
> -	.unload = dev_unload,
>  	.open = dev_open,
>  	.lastclose = dev_lastclose,
>  	.get_vblank_counter = drm_vblank_no_hw_counter,
> @@ -837,30 +741,129 @@ static struct drm_driver omap_drm_driver = {
>  	.patchlevel = DRIVER_PATCHLEVEL,
>  };
>  
> -static int pdev_probe(struct platform_device *device)
> +static int pdev_probe(struct platform_device *pdev)
>  {
> -	int r;
> +	struct omap_drm_platform_data *pdata = pdev->dev.platform_data;
> +	struct omap_drm_private *priv;
> +	struct drm_device *ddev;
> +	unsigned int i;
> +	int ret;
> +
> +	DBG("%s", pdev->name);
>  
>  	if (omapdss_is_initialized() == false)
>  		return -EPROBE_DEFER;
>  
>  	omap_crtc_pre_init();
>  
> -	r = omap_connect_dssdevs();
> -	if (r) {
> +	ret = omap_connect_dssdevs();
> +	if (ret) {
>  		omap_crtc_pre_uninit();
> -		return r;
> +		goto err_crtc_uninit;
> +	}
> +
> +	/* Allocate and initialize the driver private structure. */
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err_disconnect_dssdevs;
> +	}
> +
> +	priv->omaprev = pdata->omaprev;
> +	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
> +
> +	init_waitqueue_head(&priv->commit.wait);
> +	spin_lock_init(&priv->commit.lock);
> +	spin_lock_init(&priv->list_lock);
> +	INIT_LIST_HEAD(&priv->obj_list);
> +
> +	platform_set_drvdata(pdev, priv);

So either I'm blind, or the old code stored the drm_device in here, but
the new one the omap private stuff. Too lazy to audit the full driver, and
either way this should be a separate patch if you want to change it.

> +
> +	/* Allocate and initialize the DRM device. */
> +	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);

Since this is a driver you work on, embedding is recommended, see
drm_dev_init(). Looks soo much better.

> +	if (IS_ERR(ddev)) {
> +		ret = PTR_ERR(ddev);
> +		goto err_free_priv;
> +	}
> +
> +	priv->drm_dev = ddev;
> +	ddev->dev_private = priv;
> +
> +	omap_gem_init(ddev);
> +
> +	ret = omap_modeset_init(ddev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", ret);
> +		goto err_free_drm_dev;
>  	}
>  
> -	DBG("%s", device->name);
> -	return drm_platform_init(&omap_drm_driver, device);
> +	/* Initialize vblank handling, start with all CRTCs disabled. */
> +	ret = drm_vblank_init(ddev, priv->num_crtcs);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not init vblank\n");
> +		goto err_cleanup_modeset;
> +	}
> +
> +	for (i = 0; i < priv->num_crtcs; i++)
> +		drm_crtc_vblank_off(priv->crtcs[i]);
> +
> +	priv->fbdev = omap_fbdev_init(ddev);
> +
> +	drm_kms_helper_poll_init(ddev);
> +
> +	/*
> +	 * Register the DRM device with the core and the connectors with
> +	 * sysfs.
> +	 */
> +	ret = drm_dev_register(ddev, 0);
> +	if (ret)
> +		goto err_cleanup_helpers;
> +
> +	return 0;
> +
> +err_cleanup_helpers:
> +	drm_kms_helper_poll_fini(ddev);
> +	if (priv->fbdev)
> +		omap_fbdev_free(ddev);
> +	drm_vblank_cleanup(ddev);
> +err_cleanup_modeset:
> +	drm_mode_config_cleanup(ddev);
> +	omap_drm_irq_uninstall(ddev);
> +err_free_drm_dev:
> +	omap_gem_deinit(ddev);
> +	drm_dev_unref(ddev);
> +err_free_priv:
> +	destroy_workqueue(priv->wq);
> +	kfree(priv);
> +err_disconnect_dssdevs:
> +	omap_disconnect_dssdevs();
> +err_crtc_uninit:
> +	omap_crtc_pre_uninit();
> +	return ret;
>  }
>  
> -static int pdev_remove(struct platform_device *device)
> +static int pdev_remove(struct platform_device *pdev)
>  {
> +	struct omap_drm_private *priv = platform_get_drvdata(pdev);
> +	struct drm_device *ddev = priv->drm_dev;
> +
>  	DBG("");
>  
> -	drm_put_dev(platform_get_drvdata(device));
> +	drm_dev_unregister(ddev);
> +
> +	if (priv->fbdev)
> +		omap_fbdev_free(ddev);
> +
> +	drm_kms_helper_poll_fini(ddev);

Probably want to keep poll_fini before you tear up the fbdev, because the
poll worker can call into the fbdev stuff. Not sure why you flipped those.

> +	drm_mode_config_cleanup(ddev);
> +


	drm_vblank_cleanup(dev);
^^ This one seems lost.

And I assume the driver core reset the drvdata to NULL, but too lazy to
double-check ;-)

With the above addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

But the semi-random slight re-ordering of the init/teardown sequence on
this made it a bit tricky to review ...
-Daniel

> +	omap_drm_irq_uninstall(ddev);
> +	omap_gem_deinit(ddev);
> +
> +	drm_dev_unref(ddev);
> +
> +	destroy_workqueue(priv->wq);
> +	kfree(priv);
>  
>  	omap_disconnect_dssdevs();
>  	omap_crtc_pre_uninit();
> @@ -907,26 +910,26 @@ static int omap_drm_resume_all_displays(void)
>  
>  static int omap_drm_suspend(struct device *dev)
>  {
> -	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +	struct omap_drm_private *priv = dev_get_drvdata(dev);
>  
> -	drm_kms_helper_poll_disable(drm_dev);
> +	drm_kms_helper_poll_disable(priv->drm_dev);
>  
> -	drm_modeset_lock_all(drm_dev);
> +	drm_modeset_lock_all(priv->drm_dev);
>  	omap_drm_suspend_all_displays();
> -	drm_modeset_unlock_all(drm_dev);
> +	drm_modeset_unlock_all(priv->drm_dev);
>  
>  	return 0;
>  }
>  
>  static int omap_drm_resume(struct device *dev)
>  {
> -	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +	struct omap_drm_private *priv = dev_get_drvdata(dev);
>  
> -	drm_modeset_lock_all(drm_dev);
> +	drm_modeset_lock_all(priv->drm_dev);
>  	omap_drm_resume_all_displays();
> -	drm_modeset_unlock_all(drm_dev);
> +	drm_modeset_unlock_all(priv->drm_dev);
>  
> -	drm_kms_helper_poll_enable(drm_dev);
> +	drm_kms_helper_poll_enable(priv->drm_dev);
>  
>  	return omap_gem_resume(dev);
>  }
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index b20377efd01b..c3dcd311f82b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -58,6 +58,7 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
>  
>  struct omap_drm_private {
>  	uint32_t omaprev;
> +	struct drm_device *drm_dev;
>  
>  	unsigned int num_crtcs;
>  	struct drm_crtc *crtcs[8];
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d4e1e11466f8..09a161736a48 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -1000,8 +1000,7 @@ void *omap_gem_vaddr(struct drm_gem_object *obj)
>  /* re-pin objects in DMM in resume path: */
>  int omap_gem_resume(struct device *dev)
>  {
> -	struct drm_device *drm_dev = dev_get_drvdata(dev);
> -	struct omap_drm_private *priv = drm_dev->dev_private;
> +	struct omap_drm_private *priv = dev_get_drvdata(dev);
>  	struct omap_gem_object *omap_obj;
>  	int ret = 0;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux