Den 28.08.2017 23.56, skrev Daniel Vetter:
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 module is still pinned isn't it? I can add sysfs unbind as a third way of triggering unbind:* - The sysfs driver _unbind_ file can be used to unbind the driver form the
* device. This can happen any time. *
+ * + * - 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 ...
Yeah, it's better to take the second ref in devm_tinydrm_register().
The reason I need 2 refs is because tinydrm is set up with 2 devm_
functions. This way I can leave all error path cleanup in the hands of
devres.
static int driver_probe(...)
{
ret = devm_tinydrm_init(...);
if (ret)
return ret;
ret = tinydrm_display_pipe_init(...);
if (ret)
return ret;
drm_mode_config_reset(...);
return devm_tinydrm_register(...);
}
+ + 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.
Flushing can happen while drm_dev_unplug() is called, and when we leave this function the device facing resources controlled by devres will be removed. Thus I have to make sure any such flushing is done before leaving so the next flush is stopped by the drm_dev_is_unplugged() check. I don't see any other way of ensuring that. I see now that I should move the call to drm_atomic_helper_shutdown() after drm_dev_unplug() to properly protect the pipe .enable/.disable callbacks. Noralf.
}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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
