Re: [PATCH 6/6] drm/tinydrm: Support device unplug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:
> Support device unplugging to make tinydrm suitable for USB devices.
> 
> Cc: David Lechner <david@xxxxxxxxxxxxxx>
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 69 ++++++++++++++++++++++++-----
>  drivers/gpu/drm/tinydrm/mi0283qt.c          |  4 ++
>  drivers/gpu/drm/tinydrm/mipi-dbi.c          |  5 ++-
>  drivers/gpu/drm/tinydrm/repaper.c           |  9 +++-
>  drivers/gpu/drm/tinydrm/st7586.c            |  9 +++-
>  include/drm/tinydrm/tinydrm.h               |  5 +++
>  6 files changed, 87 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> index f11f4cd..3ccbcc5 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> @@ -32,6 +32,29 @@
>   * The driver allocates &tinydrm_device, initializes it using
>   * devm_tinydrm_init(), sets up the pipeline using tinydrm_display_pipe_init()
>   * and registers the DRM device using devm_tinydrm_register().
> + *
> + * Device unplug
> + * -------------
> + *
> + * tinydrm supports device unplugging when there's still open DRM or fbdev file
> + * handles.
> + *
> + * There are 2 ways for driver-device unbinding to happen:
> + *
> + * - The driver module is unloaded causing the driver to be unregistered.
> + *   This can't happen as long as there's open file handles because a reference
> + *   is taken on the module.

Aside: you can do that, but then it works like a hw unplug, through the
unbind property in sysfs.

> + *
> + * - The device is removed (USB, Device Tree overlay).
> + *   This can happen at any time.
> + *
> + * The driver needs to protect device resources from access after the device is
> + * gone. This is done checking drm_dev_is_unplugged(), typically in
> + * &drm_framebuffer_funcs.dirty, &drm_simple_display_pipe_funcs.enable and
> + * \.disable. Resources that doesn't face userspace and is only used with the
> + * device can be setup using devm\_ functions, but &tinydrm_device must be
> + * allocated using plain kzalloc() since it's lifetime can exceed that of the
> + * device. tinydrm_release() will free the structure.

So here's a bit a dragon: There's no prevention of is_unplugged racing
against a drm_dev_unplug(). There's been various attempts to fixing this,
but they're all somewhat ugly.

Either way, that's a bug in the drm core :-)

>   */
>  
>  /**
> @@ -138,6 +161,29 @@ static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = {
>  	.atomic_commit = drm_atomic_helper_commit,
>  };
>  
> +/**
> + * tinydrm_release - DRM driver release helper
> + * @drm: DRM device
> + *
> + * This function cleans up and finalizes &drm_device and frees &tinydrm_device.
> + *
> + * Drivers must use this as their &drm_driver->release callback.
> + */
> +void tinydrm_release(struct drm_device *drm)
> +{
> +	struct tinydrm_device *tdev = drm_to_tinydrm(drm);
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	drm_mode_config_cleanup(drm);
> +	drm_dev_fini(drm);
> +
> +	mutex_destroy(&tdev->dirty_lock);
> +	kfree(tdev->fbdev_cma);
> +	kfree(tdev);
> +}
> +EXPORT_SYMBOL(tinydrm_release);
> +
>  static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
>  			const struct drm_framebuffer_funcs *fb_funcs,
>  			struct drm_driver *driver)
> @@ -160,8 +206,6 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
>  
>  static void tinydrm_fini(struct tinydrm_device *tdev)
>  {
> -	drm_mode_config_cleanup(&tdev->drm);
> -	mutex_destroy(&tdev->dirty_lock);
>  	drm_dev_unref(&tdev->drm);
>  }
>  
> @@ -178,8 +222,8 @@ static void devm_tinydrm_release(void *data)
>   * @driver: DRM driver
>   *
>   * This function initializes @tdev, the underlying DRM device and it's
> - * mode_config. Resources will be automatically freed on driver detach (devres)
> - * using drm_mode_config_cleanup() and drm_dev_unref().
> + * mode_config. drm_dev_unref() is called on driver detach (devres) and when
> + * all refs are dropped, tinydrm_release() is called.
>   *
>   * Returns:
>   * Zero on success, negative error code on failure.
> @@ -226,14 +270,17 @@ static int tinydrm_register(struct tinydrm_device *tdev)
>  
>  static void tinydrm_unregister(struct tinydrm_device *tdev)
>  {
> -	struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
> -
>  	drm_atomic_helper_shutdown(&tdev->drm);
> -	/* don't restore fbdev in lastclose, keep pipeline disabled */
> -	tdev->fbdev_cma = NULL;
> -	drm_dev_unregister(&tdev->drm);
> -	if (fbdev_cma)
> -		drm_fbdev_cma_fini(fbdev_cma);
> +
> +	/* Get a ref that will be put in tinydrm_fini() */
> +	drm_dev_ref(&tdev->drm);

Why do we need that private ref? Grabbing references in unregister code
looks like a recipe for leaks ...

> +
> +	drm_fbdev_cma_dev_unplug(tdev->fbdev_cma);
> +	drm_dev_unplug(&tdev->drm);
> +
> +	/* Make sure framebuffer flushing is done */
> +	mutex_lock(&tdev->dirty_lock);
> +	mutex_unlock(&tdev->dirty_lock);

Is this really needed? Or, doesn't it just paper over a driver bug you
have already anyway, since native kms userspace can directly call
fb->funcs->dirty too, and you already protect against that.

This definitely looks like the fbdev helper is leaking implementation
details to callers where it shouldn't do that.

>  }
>  
>  static void devm_tinydrm_register_release(void *data)
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 2465489..84ab8d1 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -31,6 +31,9 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	if (drm_dev_is_unplugged(&tdev->drm))
> +		return;
> +
>  	ret = regulator_enable(mipi->regulator);
>  	if (ret) {
>  		dev_err(dev, "Failed to enable regulator (%d)\n", ret);
> @@ -133,6 +136,7 @@ static struct drm_driver mi0283qt_driver = {
>  				  DRIVER_ATOMIC,
>  	.fops			= &mi0283qt_fops,
>  	TINYDRM_GEM_DRIVER_OPS,
> +	.release		= tinydrm_release,
>  	.lastclose		= tinydrm_lastclose,
>  	.debugfs_init		= mipi_dbi_debugfs_init,
>  	.name			= "mi0283qt",
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index f5b9b772..49d03ab 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -209,7 +209,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
>  
>  	mutex_lock(&tdev->dirty_lock);
>  
> -	if (!mipi->enabled)
> +	if (!mipi->enabled || drm_dev_is_unplugged(fb->dev))
>  		goto out_unlock;
>  
>  	/* fbdev can flush even when we're not interested */
> @@ -314,6 +314,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	if (drm_dev_is_unplugged(&tdev->drm))
> +		return;
> +
>  	mipi->enabled = false;
>  
>  	if (mipi->backlight)
> diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
> index b8fc8eb..3869d362 100644
> --- a/drivers/gpu/drm/tinydrm/repaper.c
> +++ b/drivers/gpu/drm/tinydrm/repaper.c
> @@ -542,7 +542,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
>  
>  	mutex_lock(&tdev->dirty_lock);
>  
> -	if (!epd->enabled)
> +	if (!epd->enabled || drm_dev_is_unplugged(fb->dev))
>  		goto out_unlock;
>  
>  	/* fbdev can flush even when we're not interested */
> @@ -670,6 +670,9 @@ static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	DRM_DEBUG_DRIVER("\n");
>  
> +	if (drm_dev_is_unplugged(&tdev->drm))
> +		return;
> +
>  	/* Power up sequence */
>  	gpiod_set_value_cansleep(epd->reset, 0);
>  	gpiod_set_value_cansleep(epd->panel_on, 0);
> @@ -803,6 +806,9 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)
>  
>  	DRM_DEBUG_DRIVER("\n");
>  
> +	if (drm_dev_is_unplugged(&tdev->drm))
> +		return;
> +
>  	mutex_lock(&tdev->dirty_lock);
>  	epd->enabled = false;
>  	mutex_unlock(&tdev->dirty_lock);
> @@ -894,6 +900,7 @@ static struct drm_driver repaper_driver = {
>  				  DRIVER_ATOMIC,
>  	.fops			= &repaper_fops,
>  	TINYDRM_GEM_DRIVER_OPS,
> +	.release		= tinydrm_release,
>  	.name			= "repaper",
>  	.desc			= "Pervasive Displays RePaper e-ink panels",
>  	.date			= "20170405",
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> index bc2b905..b1bfc4e 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -120,7 +120,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
>  
>  	mutex_lock(&tdev->dirty_lock);
>  
> -	if (!mipi->enabled)
> +	if (!mipi->enabled || drm_dev_is_unplugged(fb->dev))
>  		goto out_unlock;
>  
>  	/* fbdev can flush even when we're not interested */
> @@ -184,6 +184,9 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	if (drm_dev_is_unplugged(&tdev->drm))
> +		return;
> +
>  	mipi_dbi_hw_reset(mipi);
>  	ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
>  	if (ret) {
> @@ -252,6 +255,9 @@ static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	if (drm_dev_is_unplugged(&tdev->drm))
> +		return;
> +
>  	if (!mipi->enabled)
>  		return;
>  
> @@ -319,6 +325,7 @@ static struct drm_driver st7586_driver = {
>  				  DRIVER_ATOMIC,
>  	.fops			= &st7586_fops,
>  	TINYDRM_GEM_DRIVER_OPS,
> +	.release		= tinydrm_release,
>  	.lastclose		= tinydrm_lastclose,
>  	.debugfs_init		= mipi_dbi_debugfs_init,
>  	.name			= "st7586",
> diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
> index ded9817..4265f51 100644
> --- a/include/drm/tinydrm/tinydrm.h
> +++ b/include/drm/tinydrm/tinydrm.h
> @@ -23,6 +23,10 @@
>   * @fbdev_cma: CMA fbdev structure
>   * @suspend_state: Atomic state when suspended
>   * @fb_funcs: Framebuffer functions used when creating framebuffers
> + *
> + * Drivers that embed &tinydrm_device must set it as the first member because
> + * it is freed in tinydrm_release(). This means that the structure must be
> + * allocated with kzalloc() and not the devm\_ version.
>   */
>  struct tinydrm_device {
>  	struct drm_device drm;
> @@ -94,6 +98,7 @@ struct drm_gem_object *
>  tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm,
>  				      struct dma_buf_attachment *attach,
>  				      struct sg_table *sgt);
> +void tinydrm_release(struct drm_device *drm);
>  int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
>  		      const struct drm_framebuffer_funcs *fb_funcs,
>  		      struct drm_driver *driver);
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux