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

[Laurent Pinchart]

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