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

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@lechnology.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

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