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 at ideasonboard.com>
> ---
> 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 at ffwll.ch>
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
