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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel