Hi Tomi,
On Friday 16 Dec 2016 14:44:26 Tomi Valkeinen wrote:
> On 14/12/16 02:27, 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>
> > ---
> > Changes since v1:
> >
> > - Call drm_kms_helper_poll_fini() before omap_fbdev_free() in the remove
> >
> > handler.
> >
> > - Keep storing drm_device in the platform device private data.
> > ---
> >
> > drivers/gpu/drm/omapdrm/omap_connector.c | 2 -
> > drivers/gpu/drm/omapdrm/omap_drv.c | 212 +++++++++++++-------------
> > 2 files changed, 107 insertions(+), 107 deletions(-)
[snip]
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> > b/drivers/gpu/drm/omapdrm/omap_drv.c index 0a2d461d62cf..a3b37823b271
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
[snip]
> > @@ -837,30 +741,128 @@ 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;
>
> This calls omap_crtc_pre_uninit here and in the err handler.
Oops, will fix.
> > + }
> > +
> > + /* 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);
> > +
> > + /* Allocate and initialize the DRM device. */
> > + ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
> > + if (IS_ERR(ddev)) {
> > + ret = PTR_ERR(ddev);
> > + goto err_free_priv;
> > + }
> > +
> > + ddev->dev_private = priv;
> > + platform_set_drvdata(pdev, ddev);
> > +
> > + 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;
> > + }
> > +
> > + /* 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;
> > }
> >
> > - DBG("%s", device->name);
> > - return drm_platform_init(&omap_drm_driver, device);
> > + 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 drm_device *ddev = platform_get_drvdata(pdev);
> > + struct omap_drm_private *priv = ddev->dev_private;
> > +
> > DBG("");
> >
> > - drm_put_dev(platform_get_drvdata(device));
> > + drm_dev_unregister(ddev);
> > +
> > + drm_kms_helper_poll_fini(ddev);
> > +
> > + if (priv->fbdev)
> > + omap_fbdev_free(ddev);
> > +
> > + drm_mode_config_cleanup(ddev);
> > +
> > + 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();
>
> The old code calls drm_vblank_cleanup(), and the probe's error handling
> calls that, but not remove. Is that correct?
Now that drm_vblank_cleanup() is called by drm_dev_release() we can drop the
call from the probe error path. It used to be needed as the function was
called from drm_dev_unregister().
--
Regards,
Laurent Pinchart
