The tilcdc driver could be compiled as a module, but was severely broken and could not be used as such. This patchset attempts to fix the issues preventing a proper load/unload of the module.
Issues included dangling sysfs nodes, dangling devices, memory leaks and a double kfree.
It now seems to be working ok. We have tested this by loading and unloading the driver repeteadly, with both panel and slave connectors and found no flaws.
There is still one warning left on tilcdc_crtc_destroy, caused by destroying the connector while still in an ON status. We don't know why this happens or why it's an issue, so we did not fix it.
The first 7 patches are bug fixes which and should probably be applied in the stable trees. The last two are clean-ups.
The series applies cleanly over -next and 3.15-rc8, and will be resent for v3.16-rc1, once it's out.
Guido Martínez (9): drm/i2c: tda998x: move drm_i2c_encoder_destroy call drm/tilcdc: panel: fix dangling sysfs connector node drm/tilcdc: slave: fix dangling sysfs connector node drm/tilcdc: tfp410: fix dangling sysfs connector node drm/tilcdc: panel: fix leak when unloading the module drm/tilcdc: fix release order on exit drm/tilcdc: fix double kfree drm/tilcdc: remove submodule destroy calls drm/tilcdc: replace late_initcall with module_init
drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/tilcdc/Module.symvers | 0 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 15 +++++-------- drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 - drivers/gpu/drm/tilcdc/tilcdc_panel.c | 39 +++++++++++++++++----------------- drivers/gpu/drm/tilcdc/tilcdc_slave.c | 27 +++++++++++++---------- drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++--------------- 7 files changed, 59 insertions(+), 60 deletions(-) create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(), as part of the release procedure. Such calls need to access the I2C bus and therefore, we need to call them before drm_i2c_encoder_destroy() which unregisters the I2C device.
This commit moves the latter so it's done afterwards.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Signed-off-by: Ezequiel García ezequiel@vanguardiasur.com.ar Cc: stable@vger.kernel.org --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 219c7e8..d06eff6 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1183,7 +1183,6 @@ static void tda998x_encoder_destroy(struct drm_encoder *encoder) { struct tda998x_priv *priv = to_tda998x_priv(encoder); - drm_i2c_encoder_destroy(encoder);
/* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); @@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
if (priv->cec) i2c_unregister_device(priv->cec); + drm_i2c_encoder_destroy(encoder); kfree(priv); }
Add a drm_sysfs_connector_remove call when we destroy the panel to make sure the connector node in sysfs gets deleted.
This is required for proper unload and re-load of this driver as a module. Without this, we would get a warning at re-load time like so:
------------[ cut here ]------------ WARNING: CPU: 0 PID: 824 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74() sysfs: cannot create duplicate filename '/class/drm/card0-LVDS-1' Modules linked in: [...] CPU: 0 PID: 824 Comm: modprobe Not tainted 3.15.0-rc4-00027-g6484f96-dirty #81 [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14) [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88) [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40) [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74) [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8) [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520) [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4) [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c) [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204) [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1fec>] (panel_modeset_init+0xb8/0x134 [tilcdc]) [<bf0b1fec>] (panel_modeset_init [tilcdc]) from [<bf0b2bf0>] (tilcdc_load+0x214/0x4c0 [tilcdc]) [<bf0b2bf0>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104) [ .. snip .. ] ---[ end trace b2d09cd9578b0497 ]--- [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org --- drivers/gpu/drm/tilcdc/tilcdc_panel.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index 86c6732..1943b2f 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -151,6 +151,7 @@ struct panel_connector { static void panel_connector_destroy(struct drm_connector *connector) { struct panel_connector *panel_connector = to_panel_connector(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); kfree(panel_connector); }
Add a drm_sysfs_connector_remove call when we destroy the panel to make sure the connector node in sysfs gets deleted.
This is required for proper unload and re-load of this driver as a module. Without this, we would get a warning at re-load time like so:
tda998x 0-0070: found TDA19988 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 825 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74() sysfs: cannot create duplicate filename '/class/drm/card0-HDMI-A-1' Modules linked in: [..] CPU: 0 PID: 825 Comm: modprobe Not tainted 3.15.0-rc4-00027-g9dcdef4 #82 [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14) [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88) [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40) [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74) [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8) [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520) [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4) [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c) [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204) [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1b40>] (slave_modeset_init+0x120/0x1bc [tilcdc]) [<bf0b1b40>] (slave_modeset_init [tilcdc]) from [<bf0b2be8>] (tilcdc_load+0x214/0x4c0 [tilcdc]) [<bf0b2be8>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104) [..snip..] ---[ end trace 4df8d614936ebdee ]--- [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org --- drivers/gpu/drm/tilcdc/tilcdc_slave.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c index a37924e..28c8478 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c @@ -176,6 +176,7 @@ struct slave_connector { static void slave_connector_destroy(struct drm_connector *connector) { struct slave_connector *slave_connector = to_slave_connector(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); kfree(slave_connector); }
Add a drm_sysfs_connector_remove call when we destroy the panel to make sure the connector node in sysfs gets deleted.
This is required for proper unload and re-load of this driver, otherwise we will get a warning about a duplicate filename in sysfs.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org --- drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c index c38b56b..ce75ac8 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c @@ -167,6 +167,7 @@ struct tfp410_connector { static void tfp410_connector_destroy(struct drm_connector *connector) { struct tfp410_connector *tfp410_connector = to_tfp410_connector(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); kfree(tfp410_connector); }
The driver did not unregister the allocated framebuffer, which caused memory leaks (and memory manager WARNs) when unloading. Also, the framebuffer device under /dev still existed after unloading.
Add a call to drm_fbdev_cma_fini when unloading the module to prevent both issues.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 171a820..de34657 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -122,6 +122,7 @@ static int tilcdc_unload(struct drm_device *dev) struct tilcdc_drm_private *priv = dev->dev_private; struct tilcdc_module *mod, *cur;
+ drm_fbdev_cma_fini(priv->fbdev); drm_kms_helper_poll_fini(dev); drm_mode_config_cleanup(dev); drm_vblank_cleanup(dev);
Unregister resources in the correct order on tilcdc_drm_fini, which is the reverse order they were registered during tilcdc_drm_init.
This also means unregistering the driver before releasing its resources.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index de34657..0644429 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -629,10 +629,10 @@ static int __init tilcdc_drm_init(void) static void __exit tilcdc_drm_fini(void) { DBG("fini"); - tilcdc_tfp410_fini(); - tilcdc_slave_fini(); - tilcdc_panel_fini(); platform_driver_unregister(&tilcdc_platform_driver); + tilcdc_panel_fini(); + tilcdc_slave_fini(); + tilcdc_tfp410_fini(); }
late_initcall(tilcdc_drm_init);
display_timings_release calls kfree on the display_timings object passed to it. Calling kfree after it is wrong. SLUB debug showed the following warning:
============================================================================= BUG kmalloc-64 (Tainted: G W ): Object already free -----------------------------------------------------------------------------
Disabling lock debugging due to kernel taint INFO: Allocated in of_get_display_timings+0x2c/0x214 age=601 cpu=0 pid=884 __slab_alloc.constprop.79+0x2e0/0x33c kmem_cache_alloc+0xac/0xdc of_get_display_timings+0x2c/0x214 panel_probe+0x7c/0x314 [tilcdc] platform_drv_probe+0x18/0x48 [..snip..] INFO: Freed in panel_destroy+0x18/0x3c [tilcdc] age=0 cpu=0 pid=907 __slab_free+0x34/0x330 panel_destroy+0x18/0x3c [tilcdc] tilcdc_unload+0xd0/0x118 [tilcdc] drm_dev_unregister+0x24/0x98 [..snip..]
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org --- drivers/gpu/drm/tilcdc/tilcdc_panel.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index 1943b2f..b085dcc 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -286,10 +286,8 @@ static void panel_destroy(struct tilcdc_module *mod) { struct panel_module *panel_mod = to_panel_module(mod);
- if (panel_mod->timings) { + if (panel_mod->timings) display_timings_release(panel_mod->timings); - kfree(panel_mod->timings); - }
tilcdc_module_cleanup(mod); kfree(panel_mod->info);
The TI tilcdc driver is designed with a notion of submodules. Currently, at unload time, these submodules are iterated and destroyed.
Now that the tilcdc remove order is fixed, this can be handled perfectly by the kernel using the device infrastructure, since each submodule is a kernel driver itself, and they are only destroy()'ed at unload time. Therefore we move the destroy() functionality to each submodule's remove().
Also, remove some checks in the unloading process since the new code guarantees the resources are allocated and need a release.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar --- drivers/gpu/drm/tilcdc/Module.symvers | 0 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 ------ drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 - drivers/gpu/drm/tilcdc/tilcdc_panel.c | 36 +++++++++++++++++----------------- drivers/gpu/drm/tilcdc/tilcdc_slave.c | 26 +++++++++++++----------- drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++---------------- 6 files changed, 50 insertions(+), 53 deletions(-) create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers new file mode 100644 index 0000000..e69de29 diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 0644429..702315f 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb, static int tilcdc_unload(struct drm_device *dev) { struct tilcdc_drm_private *priv = dev->dev_private; - struct tilcdc_module *mod, *cur;
drm_fbdev_cma_fini(priv->fbdev); drm_kms_helper_poll_fini(dev); @@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
pm_runtime_disable(dev->dev);
- list_for_each_entry_safe(mod, cur, &module_list, list) { - DBG("destroying module: %s", mod->name); - mod->funcs->destroy(mod); - } - kfree(priv);
return 0; diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index 0938036..7596c14 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h @@ -98,7 +98,6 @@ struct tilcdc_module; struct tilcdc_module_ops { /* create appropriate encoders/connectors: */ int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev); - void (*destroy)(struct tilcdc_module *mod); #ifdef CONFIG_DEBUG_FS /* create debugfs nodes (can be NULL): */ int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor); diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index b085dcc..2f6efae 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev) return 0; }
-static void panel_destroy(struct tilcdc_module *mod) -{ - struct panel_module *panel_mod = to_panel_module(mod); - - if (panel_mod->timings) - display_timings_release(panel_mod->timings); - - tilcdc_module_cleanup(mod); - kfree(panel_mod->info); - kfree(panel_mod); -} - static const struct tilcdc_module_ops panel_module_ops = { .modeset_init = panel_modeset_init, - .destroy = panel_destroy, };
/* @@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev) return -ENOMEM;
mod = &panel_mod->base; + pdev->dev.platform_data = mod;
tilcdc_module_init(mod, "panel", &panel_module_ops);
@@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev) if (IS_ERR(pinctrl)) dev_warn(&pdev->dev, "pins are not configured\n");
- panel_mod->timings = of_get_display_timings(node); if (!panel_mod->timings) { dev_err(&pdev->dev, "could not get panel timings\n"); - goto fail; + goto fail_free; }
panel_mod->info = of_get_panel_info(node); if (!panel_mod->info) { dev_err(&pdev->dev, "could not get panel info\n"); - goto fail; + goto fail_timings; }
mod->preferred_bpp = panel_mod->info->bpp; @@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
return 0;
-fail: - panel_destroy(mod); +fail_timings: + display_timings_release(panel_mod->timings); + +fail_free: + kfree(panel_mod); + tilcdc_module_cleanup(mod); return ret; }
static int panel_remove(struct platform_device *pdev) { + struct tilcdc_module *mod = dev_get_platdata(&pdev->dev); + struct panel_module *panel_mod = to_panel_module(mod); + + display_timings_release(panel_mod->timings); + + tilcdc_module_cleanup(mod); + kfree(panel_mod->info); + kfree(panel_mod); + return 0; }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c index 28c8478..9e4b0b2 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c @@ -306,17 +306,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev) return 0; }
-static void slave_destroy(struct tilcdc_module *mod) -{ - struct slave_module *slave_mod = to_slave_module(mod); - - tilcdc_module_cleanup(mod); - kfree(slave_mod); -} - static const struct tilcdc_module_ops slave_module_ops = { .modeset_init = slave_modeset_init, - .destroy = slave_destroy, };
/* @@ -366,10 +357,13 @@ static int slave_probe(struct platform_device *pdev) }
slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL); - if (!slave_mod) - return -ENOMEM; + if (!slave_mod) { + ret = -ENOMEM; + goto fail_adapter; + }
mod = &slave_mod->base; + pdev->dev.platform_data = mod;
mod->preferred_bpp = slave_info.bpp;
@@ -384,10 +378,20 @@ static int slave_probe(struct platform_device *pdev) tilcdc_slave_probedefer(false);
return 0; + +fail_adapter: + i2c_put_adapter(slavei2c); + return ret; }
static int slave_remove(struct platform_device *pdev) { + struct tilcdc_module *mod = dev_get_platdata(&pdev->dev); + struct slave_module *slave_mod = to_slave_module(mod); + + tilcdc_module_cleanup(mod); + kfree(slave_mod); + return 0; }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c index ce75ac8..32a0d2d 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c @@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev return 0; }
-static void tfp410_destroy(struct tilcdc_module *mod) -{ - struct tfp410_module *tfp410_mod = to_tfp410_module(mod); - - if (tfp410_mod->i2c) - i2c_put_adapter(tfp410_mod->i2c); - - if (!IS_ERR_VALUE(tfp410_mod->gpio)) - gpio_free(tfp410_mod->gpio); - - tilcdc_module_cleanup(mod); - kfree(tfp410_mod); -} - static const struct tilcdc_module_ops tfp410_module_ops = { .modeset_init = tfp410_modeset_init, - .destroy = tfp410_destroy, };
/* @@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev) return -ENOMEM;
mod = &tfp410_mod->base; + pdev->dev.platform_data = mod;
tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
@@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev) tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node); if (!tfp410_mod->i2c) { dev_err(&pdev->dev, "could not get i2c\n"); + of_node_put(i2c_node); goto fail; }
@@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev) ret = gpio_request(tfp410_mod->gpio, "DVI_PDn"); if (ret) { dev_err(&pdev->dev, "could not get DVI_PDn gpio\n"); - goto fail; + goto fail_adapter; } }
return 0;
+fail_adapter: + i2c_put_adapter(tfp410_mod->i2c); + fail: - tfp410_destroy(mod); + kfree(tfp410_mod); + tilcdc_module_cleanup(mod); return ret; }
static int tfp410_remove(struct platform_device *pdev) { + struct tilcdc_module *mod = dev_get_platdata(&pdev->dev); + struct tfp410_module *tfp410_mod = to_tfp410_module(mod); + + i2c_put_adapter(tfp410_mod->i2c); + gpio_free(tfp410_mod->gpio); + + tilcdc_module_cleanup(mod); + kfree(tfp410_mod); + return 0; }
Use module_init instead of late_initcall, as is the norm for modular drivers.
module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall, but it does not explain why. Tests show it's working properly with module_init.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 702315f..80e6697 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void) tilcdc_tfp410_fini(); }
-late_initcall(tilcdc_drm_init); +module_init(tilcdc_drm_init); module_exit(tilcdc_drm_fini);
MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
The tilcdc driver could be compiled as a module, but was severely broken and could not be used as such. This patchset attempts to fix the issues preventing a proper load/unload of the module.
Issues included dangling sysfs nodes, dangling devices, memory leaks and a double kfree.
It now seems to be working ok. We have tested this by loading and unloading the driver repeteadly, with both panel and slave connectors and found no flaws.
There is still one warning left on tilcdc_crtc_destroy, caused by destroying the connector while still in an ON status. We don't know why this happens or why it's an issue, so we did not fix it.
The first 7 patches are bug fixes which and should probably be applied in the stable trees. The last two are clean-ups.
Resending this since I got no replies.
Guido Martínez (9): drm/i2c: tda998x: move drm_i2c_encoder_destroy call drm/tilcdc: panel: fix dangling sysfs connector node drm/tilcdc: slave: fix dangling sysfs connector node drm/tilcdc: tfp410: fix dangling sysfs connector node drm/tilcdc: panel: fix leak when unloading the module drm/tilcdc: fix release order on exit drm/tilcdc: fix double kfree drm/tilcdc: remove submodule destroy calls drm/tilcdc: replace late_initcall with module_init
drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/tilcdc/Module.symvers | 0 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 15 +++++-------- drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 - drivers/gpu/drm/tilcdc/tilcdc_panel.c | 39 +++++++++++++++++----------------- drivers/gpu/drm/tilcdc/tilcdc_slave.c | 27 +++++++++++++---------- drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 35 +++++++++++++++--------------- 7 files changed, 59 insertions(+), 60 deletions(-) create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
Currently tda998x_encoder_destroy() calls cec_write() and reg_clear(), as part of the release procedure. Such calls need to access the I2C bus and therefore, we need to call them before drm_i2c_encoder_destroy() which unregisters the I2C device.
This commit moves the latter so it's done afterwards.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Signed-off-by: Ezequiel García ezequiel@vanguardiasur.com.ar Cc: stable@vger.kernel.org #v3.9+ --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 240c331..db9515f 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1183,7 +1183,6 @@ static void tda998x_encoder_destroy(struct drm_encoder *encoder) { struct tda998x_priv *priv = to_tda998x_priv(encoder); - drm_i2c_encoder_destroy(encoder);
/* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); @@ -1193,6 +1192,7 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
if (priv->cec) i2c_unregister_device(priv->cec); + drm_i2c_encoder_destroy(encoder); kfree(priv); }
On Tue, Jun 17, 2014 at 11:17:03AM -0300, Guido Martínez wrote:
You really should have sent this with me in the To: header as I'm now the maintainer of this driver. Yes, this is a valid fix, and I'll apply it shortly. Thanks.
Hi Russell,
On Tue, Jun 24, 2014 at 05:38:13PM +0100, Russell King - ARM Linux wrote:
Sorry about that, I'm still kind of new to this whole deal. I'll keep it mind for future patches.
Thanks, Guido
Add a drm_sysfs_connector_remove call when we destroy the panel to make sure the connector node in sysfs gets deleted.
This is required for proper unload and re-load of this driver as a module. Without this, we would get a warning at re-load time like so:
------------[ cut here ]------------ WARNING: CPU: 0 PID: 824 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74() sysfs: cannot create duplicate filename '/class/drm/card0-LVDS-1' Modules linked in: [...] CPU: 0 PID: 824 Comm: modprobe Not tainted 3.15.0-rc4-00027-g6484f96-dirty #81 [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14) [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88) [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40) [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74) [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8) [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520) [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4) [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c) [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204) [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1fec>] (panel_modeset_init+0xb8/0x134 [tilcdc]) [<bf0b1fec>] (panel_modeset_init [tilcdc]) from [<bf0b2bf0>] (tilcdc_load+0x214/0x4c0 [tilcdc]) [<bf0b2bf0>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104) [ .. snip .. ] ---[ end trace b2d09cd9578b0497 ]--- [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org #v3.9+ --- drivers/gpu/drm/tilcdc/tilcdc_panel.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index 86c6732..1943b2f 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -151,6 +151,7 @@ struct panel_connector { static void panel_connector_destroy(struct drm_connector *connector) { struct panel_connector *panel_connector = to_panel_connector(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); kfree(panel_connector); }
Add a drm_sysfs_connector_remove call when we destroy the panel to make sure the connector node in sysfs gets deleted.
This is required for proper unload and re-load of this driver as a module. Without this, we would get a warning at re-load time like so:
tda998x 0-0070: found TDA19988 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 825 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x54/0x74() sysfs: cannot create duplicate filename '/class/drm/card0-HDMI-A-1' Modules linked in: [..] CPU: 0 PID: 825 Comm: modprobe Not tainted 3.15.0-rc4-00027-g9dcdef4 #82 [<c0013bb8>] (unwind_backtrace) from [<c0011824>] (show_stack+0x10/0x14) [<c0011824>] (show_stack) from [<c0034e8c>] (warn_slowpath_common+0x68/0x88) [<c0034e8c>] (warn_slowpath_common) from [<c0034edc>] (warn_slowpath_fmt+0x30/0x40) [<c0034edc>] (warn_slowpath_fmt) from [<c01243f4>] (sysfs_warn_dup+0x54/0x74) [<c01243f4>] (sysfs_warn_dup) from [<c0124708>] (sysfs_do_create_link_sd.isra.2+0xb0/0xb8) [<c0124708>] (sysfs_do_create_link_sd.isra.2) from [<c02ae37c>] (device_add+0x338/0x520) [<c02ae37c>] (device_add) from [<c02ae6e8>] (device_create_groups_vargs+0xa0/0xc4) [<c02ae6e8>] (device_create_groups_vargs) from [<c02ae758>] (device_create+0x24/0x2c) [<c02ae758>] (device_create) from [<c029b4ec>] (drm_sysfs_connector_add+0x64/0x204) [<c029b4ec>] (drm_sysfs_connector_add) from [<bf0b1b40>] (slave_modeset_init+0x120/0x1bc [tilcdc]) [<bf0b1b40>] (slave_modeset_init [tilcdc]) from [<bf0b2be8>] (tilcdc_load+0x214/0x4c0 [tilcdc]) [<bf0b2be8>] (tilcdc_load [tilcdc]) from [<c029955c>] (drm_dev_register+0xa4/0x104) [..snip..] ---[ end trace 4df8d614936ebdee ]--- [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -17
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org #v3.9+ --- drivers/gpu/drm/tilcdc/tilcdc_slave.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c index 595068b..2f83ffb 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c @@ -166,6 +166,7 @@ struct slave_connector { static void slave_connector_destroy(struct drm_connector *connector) { struct slave_connector *slave_connector = to_slave_connector(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); kfree(slave_connector); }
Add a drm_sysfs_connector_remove call when we destroy the panel to make sure the connector node in sysfs gets deleted.
This is required for proper unload and re-load of this driver, otherwise we will get a warning about a duplicate filename in sysfs.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org #v3.9+ --- drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c index c38b56b..ce75ac8 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c @@ -167,6 +167,7 @@ struct tfp410_connector { static void tfp410_connector_destroy(struct drm_connector *connector) { struct tfp410_connector *tfp410_connector = to_tfp410_connector(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); kfree(tfp410_connector); }
The driver did not unregister the allocated framebuffer, which caused memory leaks (and memory manager WARNs) when unloading. Also, the framebuffer device under /dev still existed after unloading.
Add a call to drm_fbdev_cma_fini when unloading the module to prevent both issues.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org #v3.9+ --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index b20b694..490aee7 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -122,6 +122,7 @@ static int tilcdc_unload(struct drm_device *dev) struct tilcdc_drm_private *priv = dev->dev_private; struct tilcdc_module *mod, *cur;
+ drm_fbdev_cma_fini(priv->fbdev); drm_kms_helper_poll_fini(dev); drm_mode_config_cleanup(dev); drm_vblank_cleanup(dev);
Unregister resources in the correct order on tilcdc_drm_fini, which is the reverse order they were registered during tilcdc_drm_init.
This also means unregistering the driver before releasing its resources.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org #v3.9+ --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 490aee7..006a30e 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -629,10 +629,10 @@ static int __init tilcdc_drm_init(void) static void __exit tilcdc_drm_fini(void) { DBG("fini"); - tilcdc_tfp410_fini(); - tilcdc_slave_fini(); - tilcdc_panel_fini(); platform_driver_unregister(&tilcdc_platform_driver); + tilcdc_panel_fini(); + tilcdc_slave_fini(); + tilcdc_tfp410_fini(); }
late_initcall(tilcdc_drm_init);
display_timings_release calls kfree on the display_timings object passed to it. Calling kfree after it is wrong. SLUB debug showed the following warning:
============================================================================= BUG kmalloc-64 (Tainted: G W ): Object already free -----------------------------------------------------------------------------
Disabling lock debugging due to kernel taint INFO: Allocated in of_get_display_timings+0x2c/0x214 age=601 cpu=0 pid=884 __slab_alloc.constprop.79+0x2e0/0x33c kmem_cache_alloc+0xac/0xdc of_get_display_timings+0x2c/0x214 panel_probe+0x7c/0x314 [tilcdc] platform_drv_probe+0x18/0x48 [..snip..] INFO: Freed in panel_destroy+0x18/0x3c [tilcdc] age=0 cpu=0 pid=907 __slab_free+0x34/0x330 panel_destroy+0x18/0x3c [tilcdc] tilcdc_unload+0xd0/0x118 [tilcdc] drm_dev_unregister+0x24/0x98 [..snip..]
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar Cc: stable@vger.kernel.org #v3.9+ --- drivers/gpu/drm/tilcdc/tilcdc_panel.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index 1943b2f..b085dcc 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -286,10 +286,8 @@ static void panel_destroy(struct tilcdc_module *mod) { struct panel_module *panel_mod = to_panel_module(mod);
- if (panel_mod->timings) { + if (panel_mod->timings) display_timings_release(panel_mod->timings); - kfree(panel_mod->timings); - }
tilcdc_module_cleanup(mod); kfree(panel_mod->info);
The TI tilcdc driver is designed with a notion of submodules. Currently, at unload time, these submodules are iterated and destroyed.
Now that the tilcdc remove order is fixed, this can be handled perfectly by the kernel using the device infrastructure, since each submodule is a kernel driver itself, and they are only destroy()'ed at unload time. Therefore we move the destroy() functionality to each submodule's remove().
Also, remove some checks in the unloading process since the new code guarantees the resources are allocated and need a release.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar --- drivers/gpu/drm/tilcdc/Module.symvers | 0 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 ------ drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 - drivers/gpu/drm/tilcdc/tilcdc_panel.c | 36 +++++++++++++++++----------------- drivers/gpu/drm/tilcdc/tilcdc_slave.c | 26 +++++++++++++----------- drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 34 ++++++++++++++++---------------- 6 files changed, 50 insertions(+), 53 deletions(-) create mode 100644 drivers/gpu/drm/tilcdc/Module.symvers
diff --git a/drivers/gpu/drm/tilcdc/Module.symvers b/drivers/gpu/drm/tilcdc/Module.symvers new file mode 100644 index 0000000..e69de29 diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 006a30e..2c860c4 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -120,7 +120,6 @@ static int cpufreq_transition(struct notifier_block *nb, static int tilcdc_unload(struct drm_device *dev) { struct tilcdc_drm_private *priv = dev->dev_private; - struct tilcdc_module *mod, *cur;
drm_fbdev_cma_fini(priv->fbdev); drm_kms_helper_poll_fini(dev); @@ -149,11 +148,6 @@ static int tilcdc_unload(struct drm_device *dev)
pm_runtime_disable(dev->dev);
- list_for_each_entry_safe(mod, cur, &module_list, list) { - DBG("destroying module: %s", mod->name); - mod->funcs->destroy(mod); - } - kfree(priv);
return 0; diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index 0938036..7596c14 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h @@ -98,7 +98,6 @@ struct tilcdc_module; struct tilcdc_module_ops { /* create appropriate encoders/connectors: */ int (*modeset_init)(struct tilcdc_module *mod, struct drm_device *dev); - void (*destroy)(struct tilcdc_module *mod); #ifdef CONFIG_DEBUG_FS /* create debugfs nodes (can be NULL): */ int (*debugfs_init)(struct tilcdc_module *mod, struct drm_minor *minor); diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index b085dcc..2f6efae 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -282,21 +282,8 @@ static int panel_modeset_init(struct tilcdc_module *mod, struct drm_device *dev) return 0; }
-static void panel_destroy(struct tilcdc_module *mod) -{ - struct panel_module *panel_mod = to_panel_module(mod); - - if (panel_mod->timings) - display_timings_release(panel_mod->timings); - - tilcdc_module_cleanup(mod); - kfree(panel_mod->info); - kfree(panel_mod); -} - static const struct tilcdc_module_ops panel_module_ops = { .modeset_init = panel_modeset_init, - .destroy = panel_destroy, };
/* @@ -372,6 +359,7 @@ static int panel_probe(struct platform_device *pdev) return -ENOMEM;
mod = &panel_mod->base; + pdev->dev.platform_data = mod;
tilcdc_module_init(mod, "panel", &panel_module_ops);
@@ -379,17 +367,16 @@ static int panel_probe(struct platform_device *pdev) if (IS_ERR(pinctrl)) dev_warn(&pdev->dev, "pins are not configured\n");
- panel_mod->timings = of_get_display_timings(node); if (!panel_mod->timings) { dev_err(&pdev->dev, "could not get panel timings\n"); - goto fail; + goto fail_free; }
panel_mod->info = of_get_panel_info(node); if (!panel_mod->info) { dev_err(&pdev->dev, "could not get panel info\n"); - goto fail; + goto fail_timings; }
mod->preferred_bpp = panel_mod->info->bpp; @@ -400,13 +387,26 @@ static int panel_probe(struct platform_device *pdev)
return 0;
-fail: - panel_destroy(mod); +fail_timings: + display_timings_release(panel_mod->timings); + +fail_free: + kfree(panel_mod); + tilcdc_module_cleanup(mod); return ret; }
static int panel_remove(struct platform_device *pdev) { + struct tilcdc_module *mod = dev_get_platdata(&pdev->dev); + struct panel_module *panel_mod = to_panel_module(mod); + + display_timings_release(panel_mod->timings); + + tilcdc_module_cleanup(mod); + kfree(panel_mod->info); + kfree(panel_mod); + return 0; }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c index 2f83ffb..1e568ca 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c @@ -296,17 +296,8 @@ static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev) return 0; }
-static void slave_destroy(struct tilcdc_module *mod) -{ - struct slave_module *slave_mod = to_slave_module(mod); - - tilcdc_module_cleanup(mod); - kfree(slave_mod); -} - static const struct tilcdc_module_ops slave_module_ops = { .modeset_init = slave_modeset_init, - .destroy = slave_destroy, };
/* @@ -356,10 +347,13 @@ static int slave_probe(struct platform_device *pdev) }
slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL); - if (!slave_mod) - return -ENOMEM; + if (!slave_mod) { + ret = -ENOMEM; + goto fail_adapter; + }
mod = &slave_mod->base; + pdev->dev.platform_data = mod;
mod->preferred_bpp = slave_info.bpp;
@@ -374,10 +368,20 @@ static int slave_probe(struct platform_device *pdev) tilcdc_slave_probedefer(false);
return 0; + +fail_adapter: + i2c_put_adapter(slavei2c); + return ret; }
static int slave_remove(struct platform_device *pdev) { + struct tilcdc_module *mod = dev_get_platdata(&pdev->dev); + struct slave_module *slave_mod = to_slave_module(mod); + + tilcdc_module_cleanup(mod); + kfree(slave_mod); + return 0; }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c index ce75ac8..32a0d2d 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c @@ -296,23 +296,8 @@ static int tfp410_modeset_init(struct tilcdc_module *mod, struct drm_device *dev return 0; }
-static void tfp410_destroy(struct tilcdc_module *mod) -{ - struct tfp410_module *tfp410_mod = to_tfp410_module(mod); - - if (tfp410_mod->i2c) - i2c_put_adapter(tfp410_mod->i2c); - - if (!IS_ERR_VALUE(tfp410_mod->gpio)) - gpio_free(tfp410_mod->gpio); - - tilcdc_module_cleanup(mod); - kfree(tfp410_mod); -} - static const struct tilcdc_module_ops tfp410_module_ops = { .modeset_init = tfp410_modeset_init, - .destroy = tfp410_destroy, };
/* @@ -342,6 +327,7 @@ static int tfp410_probe(struct platform_device *pdev) return -ENOMEM;
mod = &tfp410_mod->base; + pdev->dev.platform_data = mod;
tilcdc_module_init(mod, "tfp410", &tfp410_module_ops);
@@ -365,6 +351,7 @@ static int tfp410_probe(struct platform_device *pdev) tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node); if (!tfp410_mod->i2c) { dev_err(&pdev->dev, "could not get i2c\n"); + of_node_put(i2c_node); goto fail; }
@@ -378,19 +365,32 @@ static int tfp410_probe(struct platform_device *pdev) ret = gpio_request(tfp410_mod->gpio, "DVI_PDn"); if (ret) { dev_err(&pdev->dev, "could not get DVI_PDn gpio\n"); - goto fail; + goto fail_adapter; } }
return 0;
+fail_adapter: + i2c_put_adapter(tfp410_mod->i2c); + fail: - tfp410_destroy(mod); + kfree(tfp410_mod); + tilcdc_module_cleanup(mod); return ret; }
static int tfp410_remove(struct platform_device *pdev) { + struct tilcdc_module *mod = dev_get_platdata(&pdev->dev); + struct tfp410_module *tfp410_mod = to_tfp410_module(mod); + + i2c_put_adapter(tfp410_mod->i2c); + gpio_free(tfp410_mod->gpio); + + tilcdc_module_cleanup(mod); + kfree(tfp410_mod); + return 0; }
Guido,
On 06/17/2014 09:17 AM, Guido Martínez wrote:
I am not sure I understand the first part of the above sentence - did something change with tilcdc ordering? I think you a referring to previous patches in your series which really mean tilcdc can actually unload now. So really the method this patch uses could always have been used, it just wasn't for some reason?
I have tested all of the other patches in your series and all looks good on BeagleBone Black and AM335xEVM, I tested as both built-ins and modules and can load/unload on BeagleBone Black with HDMI enabled correctly.
I want to play around a bit more with this particular patch, to try and understand how it differs from Rob's original intent with his module registration/deregistration scheme. I prefer your method, but do we loose anything that Rob's originally had in mind?
Darren
On Tue, Jun 24, 2014 at 05:06:24PM -0500, Darren Etheridge wrote:
Right, I guess I got a bit dizzy while writing that commit log :). If we find the patch reasonable I'll send a better explanation.
So really the method this patch uses could always have been used, it just wasn't for some reason?
Yes, I think so.
Nice to hear that :)
Nothing really comes to mind, but I may be wrong on this...
Thanks a lot for your review!
On 24 Jun 05:06 PM, Darren Etheridge wrote:
Yes, patch [PATCH/RESEND 6/9] drm/tilcdc: fix release order on exit changes the tilcdc remove ordering.
Currently, the tilcdc DRM is removed with this:
tilcdc_tfp410_fini(); tilcdc_slave_fini(); tilcdc_panel_fini(); platform_driver_unregister(&tilcdc_platform_driver);
Which is wrong as you shouldn't remove the tilcdc "modules" (panel, slave, tfp410) before the DRM driver itself. So the above patch fixed it to be:
platform_driver_unregister(&tilcdc_platform_driver); tilcdc_panel_fini(); tilcdc_slave_fini(); tilcdc_tfp410_fini();
No, I believe this patch which removes the tilcdc sub-module destroy infrastructure can only be applied *after* the above remove order is fixed (iow, the 6/9 patch mentioned above).
In other words, only once you have a proper remove() that releases things in the right order you can rely in the kernel and avoid any custom implementation.
Use module_init instead of late_initcall, as is the norm for modular drivers.
module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall, but it does not explain why. Tests show it's working properly with module_init.
Signed-off-by: Guido Martínez guido@vanguardiasur.com.ar --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 2c860c4..6be623b 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void) tilcdc_tfp410_fini(); }
-late_initcall(tilcdc_drm_init); +module_init(tilcdc_drm_init); module_exit(tilcdc_drm_fini);
MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
On 06/17/2014 09:17 AM, Guido Martínez wrote:
If I recall, the late_initcall stuff was done to try and make sure the tda998x/i2c subsystem came up before tilcdc. However it didn't always work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try and ensure the ordering. This might be why changing back to module_init is fine (and I agree with your assessment from my testing).
On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote:
There's a solution to that... I have patches which convert the tda998x driver to also register into the component helpers as well as remaining as a drm slave device. This makes it possible to transition tilcdc to use the component helper to solve the initialisation ordering.
I'll (re-)post them later today.
On Wed, Jun 25, 2014 at 02:00:42PM +0100, Russell King - ARM Linux wrote:
Except Daniel Mack will not be receiving any copies of them:
zonque@gmail.com SMTP error from remote mail server after end of data: host gmail-smtp-in.l.google.com [2a00:1450:400c:c03::1a]: 550-5.7.1 [2001:4d48:ad52:3201:214:fdff:fe10:1be6 12] Our system has 550-5.7.1 detected that this message is likely unsolicited mail. To reduce the 550-5.7.1 amount of spam sent to Gmail, this message has been blocked. Please 550-5.7.1 visit 550-5.7.1 http://support.google.com/mail/bin/answer.py?hl=en&answer=188131 for 550 5.7.1 more information. fs8si6627678wib.99 - gsmtp
Google's anti-ham filter seems to be working perfectly, allowing spam through and blocking real messages... as usual. And as usual, google provides no support for this crap. Gmail has a history of increasingly blocking legitimate email in their alleged anti-spam fight. Don't use gmail.
(Ccing Guido back)
Hello Russell, Darren,
On 25 Jun 02:00 PM, Russell King - ARM Linux wrote:
That doesn't make any sense. Using late_initcall for the tilcdc DRM driver would make the tilcdc DRM get probed before any other regular module_init driver, including the tda998x encoder.
That commit is adds a proper probe deferal mechanism in case the i2c is not available. OMAP is special because it has its own nasty initcall to probe the i2c busses. However, that should be fixed and then i2c would be always probed *after* the DRM, as per the ordering in drivers/Makefile.
There's a solution to that...
A solution to *what* ?
AFAIK, there's no issue with the initialisation ordering, except that the DRM driver is currently probed before the TDA998x encoder and so the probe is defered. Unless I'm missing something that's very easy to fix, you just need to change the link order to guarantee that i2c encoders are probed *before* the DRM drivers that require them.
I have just posted a very simple patch to fix it, but it seems Thierry wasn't happy with it, not sure why yet [1].
[1] http://www.spinics.net/lists/dri-devel/msg61987.html
On Wed, Jun 25, 2014 at 11:32:46AM -0300, Ezequiel García wrote:
A module_init() is a device_initcall(), which is at level 6. A late_initcall() is at level 7. Level 6 initcalls are run before level 7 initcalls. The tda998x is a module_init(), so the tda998x gets initialised *before* tilcdc's late_initcall().
Now, if you build everything as a module, then you have no initialisation ordering, and you can't rely on any kind of order. Initialisation functions can even run in parallel on different CPUs due to modules being loaded from userspace in a multi-threaded way. So anything which relies on a certain initcall ordering is fundamentally broken if it can be modular.
There's a solution to that...
A solution to *what* ?
Maybe if you left the context above that line in place, you might understand that my "that" refers to what was discussed in that context. That being the initialisation ordering.
Convention and proper Internet etiquette is to trim the quoted text and place relies below the paragraph to which they refer, the quoted paragraph giving the context to the reply.
Hi Russell,
On 25 Jun 03:46 PM, Russell King - ARM Linux wrote:
Oh, thanks a lot for correcting me.
OK, but is there any outstanding issue with the tilcdc initialization, either when compiled as built-in or as modules, *after* this patchset?
Of course, this driver only worked as built-in (see the horrible bugs we are fixing here, that prevented unloading and re-loading it). This series fixes all such bugs, and also adds the required changes to allow to build everything as a module.
Again, I can't see any issues that remain to be fixed after this patchset. When compiled as built-in, the tilcdc DRM will be defered and once the tda998x is ready, the tilcdc DRM will load properly.
When compiled as module you can load any of them in any order. If the tda998x module is not loaded by the time you load the tilcdc module, the former will get loaded.
Hm... now that I think about this. We still get into trouble if the tda998x is built as module, but the tilcdc is built-in. Shouldn't we just forbid that? Either both built-ins or both as modules?
Yeah, sorry about that.
Guido,
Thanks and sorry I missed this the first time around. I'll give it a try on a few of my AM335x based boards when I have access to them again on Monday.
Darren
On 06/17/2014 09:17 AM, Guido Martínez wrote:
Guido,
On 06/17/2014 09:17 AM, Guido Martínez wrote:
Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy because DRM_MODE_DPMS_ON is still set. This WARN_ON does make some sense because DPMS_OFF would have the effect of turning off clocks and putting the monitor to sleep which seems logical considering we have torn down the display. Adding a tilcdc_crtc_dpms(DPMS_OFF) right before the WARN_ON confirms this, but it seems strange that this hasn't happened automatically (+ Russell doesn't need to do it in his Armada driver) - so I suspect there is a better way.
Otherwise I think this is a good and useful patch series.
Darren
On Fri, Jun 27, 2014 at 6:08 PM, Darren Etheridge detheridge@ti.com wrote:
tbh, I'm not entirely sure offhand why drm_mode_config_cleanup() doesn't remove the fb's first (which should have the effect of shutting down any lit crtc/encoder/connector).. that would seem like the sensible way to shut down..
BR, -R
On Sat, Jun 28, 2014 at 06:51:15AM -0400, Rob Clark wrote:
All userspace fbs should be cleared already before going into the driver unload. Which only leaves you with driver internal fbs (usually just one for fbdev emulation). It's the driver's job to clean that up explicitly. Then you can call mode_config_cleanup and the WARN_ON in there is a really nice space leak check.
If we'd unconditionally clean up all fbs we'd have trouble with driver-private embedded fbs and their refcounting and would loose the space leak check. -Daniel
On Fri, Jun 27, 2014 at 05:08:51PM -0500, Darren Etheridge wrote:
[snip]
Otherwise I think this is a good and useful patch series.
It that a Tested-by tag? :)
Thanks! Guido
On 07/01/2014 06:39 PM, Guido Martínez wrote:
Sure - I would prefer that the WARN_ON was silenced when the tilcdc module is removed, but the series adds improvements over what is there now. I have tested it across a few variants of AM335x boards and attached display hardware in both module form and built-in to the kernel, therefore:
Tested-by: Darren Etheridge detheridge@ti.com
On 2 July 2014 12:31, Darren Etheridge detheridge@ti.com wrote:
Has someone gathered that tags and put these in a git tree by any chance?
Dave.
On 02 Jul 02:08 PM, Dave Airlie wrote:
I don't think anyone has. Is that a problem?
If you need a pull request with these, I can prepare something in bitbucket. Would that work you?
On 3 July 2014 06:38, Ezequiel García ezequiel@vanguardiasur.com.ar wrote:
I've picked them up manually now, so should be all fine.
Thanks, Dave.
Hi Dave,
did you take a look at this patchset? I foolishly missed adding you on the To: header, so apologies for that in advance.
Thanks,
On Tue, Jun 17, 2014 at 11:17:02AM -0300, Guido Martínez wrote:
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Darren Etheridge -
Darren Etheridge
-
Dave Airlie -
Ezequiel García -
Guido Martínez -
Rob Clark -
Russell King - ARM Linux