Hi Daniel,
On Tuesday 13 Dec 2016 22:21:10 Daniel Vetter wrote:
> 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(-)
[snip]
> > 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
[snip]
> > @@ -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.
OK, I'll split it to a separate patch.
> > +
> > + /* 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.
How does that work with drm_dev_unref() then ? When the last reference goes
away drm_dev_release() will kfree() the drm_device instance, which is only
valid if I allocate it dynamically.
> > + 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.
Good question. I'll fix that.
> > + drm_mode_config_cleanup(ddev);
> > +
>
> drm_vblank_cleanup(dev);
> ^^ This one seems lost.
drm_dev_unregister() calls drm_vblank_cleanup().
> And I assume the driver core reset the drvdata to NULL, but too lazy to
> double-check ;-)
Yes it does.
> 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();
[snip]
--
Regards,
Laurent Pinchart
