Hi Laurent, On Fri, Oct 13, 2017 at 05:59:17PM +0300, Laurent Pinchart wrote: > To simplify implementation of debugfs seq_file show handlers, the driver > passes the pointer to the show function through the debugfs_create_file > data pointer. This prevents using the pointer to pass driver private > data to the show handler, and requires all handlers to use global > variables to access private data. > > To prepare for the removal of global private data in the driver, rework > the debugfs infrastructure to allow passing a private data pointer to > show handlers. > > The price to pay is explicit removal of debugfs files to free the > internally allocated memory. This is desirable anyway as debugfs entries > should be removed when a component driver is unbound, otherwise crashes > will occur due to access to freed memory when the components will be > dynamically allocated instead of stored in global variables. > > Signed-off-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > ---
Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
(One nit-pick in dss.h, see below)
-- Sebastian
> drivers/gpu/drm/omapdrm/dss/dispc.c | 13 ++++--
> drivers/gpu/drm/omapdrm/dss/dsi.c | 40 +++++++++++-----
> drivers/gpu/drm/omapdrm/dss/dss.c | 92
> +++++++++++++++++++++++++------------
> drivers/gpu/drm/omapdrm/dss/dss.h | 27 +++++++----
> drivers/gpu/drm/omapdrm/dss/hdmi.h | 2 +
> drivers/gpu/drm/omapdrm/dss/hdmi4.c | 9 ++--
> drivers/gpu/drm/omapdrm/dss/hdmi5.c | 9 ++--
> drivers/gpu/drm/omapdrm/dss/venc.c | 11 +++--
> 8 files changed, 140 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index f0ae6be36a4e..1afd2802e807 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -168,6 +168,8 @@ static struct {
> struct platform_device *pdev;
> void __iomem *base;
>
> + struct dss_debugfs_entry *debugfs;
> +
> int irq;
> irq_handler_t user_handler;
> void *user_data;
> @@ -3269,7 +3271,7 @@ void dispc_dump_clocks(struct seq_file *s)
> dispc_runtime_put();
> }
>
> -static void dispc_dump_regs(struct seq_file *s)
> +static int dispc_dump_regs(struct seq_file *s, void *p)
> {
> int i, j;
> const char *mgr_names[] = {
> @@ -3290,7 +3292,7 @@ static void dispc_dump_regs(struct seq_file *s)
> #define DUMPREG(r) seq_printf(s, "%-50s %08x\n", #r, dispc_read_reg(r))
>
> if (dispc_runtime_get())
> - return;
> + return 0;
>
> /* DISPC common registers */
> DUMPREG(DISPC_REVISION);
> @@ -3462,6 +3464,8 @@ static void dispc_dump_regs(struct seq_file *s)
>
> #undef DISPC_REG
> #undef DUMPREG
> +
> + return 0;
> }
>
> /* calculate clock rates using dividers in cinfo */
> @@ -4606,7 +4610,8 @@ static int dispc_bind(struct device *dev, struct device
> *master, void *data)
>
> dispc_set_ops(&dispc_ops);
>
> - dss_debugfs_create_file("dispc", dispc_dump_regs);
> + dispc.debugfs = dss_debugfs_create_file("dispc", dispc_dump_regs,
> + &dispc);
>
> return 0;
>
> @@ -4618,6 +4623,8 @@ static int dispc_bind(struct device *dev, struct device
> *master, void *data)
> static void dispc_unbind(struct device *dev, struct device *master,
> void *data)
> {
> + dss_debugfs_remove_file(dispc.debugfs);
> +
> dispc_set_ops(NULL);
>
> pm_runtime_disable(dev);
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
> b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 6a1569149453..a64e6a39ebf1 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -402,6 +402,10 @@ struct dsi_data {
> #endif
> int debug_read;
> int debug_write;
> + struct {
> + struct dss_debugfs_entry *irqs;
> + struct dss_debugfs_entry *regs;
> + } debugfs;
>
> #ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
> spinlock_t irq_stats_lock;
> @@ -1659,18 +1663,20 @@ static void dsi_dump_dsidev_irqs(struct
> platform_device *dsidev,
> #undef PIS
> }
>
> -static void dsi1_dump_irqs(struct seq_file *s)
> +static int dsi1_dump_irqs(struct seq_file *s, void *p)
> {
> struct platform_device *dsidev = dsi_get_dsidev_from_id(0);
>
> dsi_dump_dsidev_irqs(dsidev, s);
> + return 0;
> }
>
> -static void dsi2_dump_irqs(struct seq_file *s)
> +static int dsi2_dump_irqs(struct seq_file *s, void *p)
> {
> struct platform_device *dsidev = dsi_get_dsidev_from_id(1);
>
> dsi_dump_dsidev_irqs(dsidev, s);
> + return 0;
> }
> #endif
>
> @@ -1758,18 +1764,20 @@ static void dsi_dump_dsidev_regs(struct
> platform_device *dsidev,
> #undef DUMPREG
> }
>
> -static void dsi1_dump_regs(struct seq_file *s)
> +static int dsi1_dump_regs(struct seq_file *s, void *p)
> {
> struct platform_device *dsidev = dsi_get_dsidev_from_id(0);
>
> dsi_dump_dsidev_regs(dsidev, s);
> + return 0;
> }
>
> -static void dsi2_dump_regs(struct seq_file *s)
> +static int dsi2_dump_regs(struct seq_file *s, void *p)
> {
> struct platform_device *dsidev = dsi_get_dsidev_from_id(1);
>
> dsi_dump_dsidev_regs(dsidev, s);
> + return 0;
> }
>
> enum dsi_cio_power_state {
> @@ -5567,15 +5575,22 @@ static int dsi_bind(struct device *dev, struct device
> *master, void *data)
> dsi_runtime_put(dsidev);
>
> if (dsi->module_id == 0)
> - dss_debugfs_create_file("dsi1_regs", dsi1_dump_regs);
> - else if (dsi->module_id == 1)
> - dss_debugfs_create_file("dsi2_regs", dsi2_dump_regs);
> -
> + dsi->debugfs.regs = dss_debugfs_create_file("dsi1_regs",
> + dsi1_dump_regs,
> + &dsi);
> + else
> + dsi->debugfs.regs = dss_debugfs_create_file("dsi2_regs",
> + dsi2_dump_regs,
> + &dsi);
> #ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
> if (dsi->module_id == 0)
> - dss_debugfs_create_file("dsi1_irqs", dsi1_dump_irqs);
> - else if (dsi->module_id == 1)
> - dss_debugfs_create_file("dsi2_irqs", dsi2_dump_irqs);
> + dsi->debugfs.irqs = dss_debugfs_create_file("dsi1_irqs",
> + dsi1_dump_irqs,
> + &dsi);
> + else
> + dsi->debugfs.irqs = dss_debugfs_create_file("dsi2_irqs",
> + dsi2_dump_irqs,
> + &dsi);
> #endif
>
> return 0;
> @@ -5594,6 +5609,9 @@ static void dsi_unbind(struct device *dev, struct
> device *master, void *data)
> struct platform_device *dsidev = to_platform_device(dev);
> struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
>
> + dss_debugfs_remove_file(dsi->debugfs.irqs);
> + dss_debugfs_remove_file(dsi->debugfs.regs);
> +
> of_platform_depopulate(&dsidev->dev);
>
> WARN_ON(dsi->scp_clk_refcount > 0);
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c
> b/drivers/gpu/drm/omapdrm/dss/dss.c
> index 5721f3d64bdd..b45641f6a844 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss.c
> @@ -118,6 +118,11 @@ static struct {
>
> const struct dss_features *feat;
>
> + struct {
> + struct dss_debugfs_entry *clk;
> + struct dss_debugfs_entry *dss;
> + } debugfs;
> +
> struct dss_pll *video1_pll;
> struct dss_pll *video2_pll;
> } dss;
> @@ -393,12 +398,12 @@ static void dss_dump_clocks(struct seq_file *s)
> }
> #endif
>
> -static void dss_dump_regs(struct seq_file *s)
> +static int dss_dump_regs(struct seq_file *s, void *p)
> {
> #define DUMPREG(r) seq_printf(s, "%-35s %08x\n", #r, dss_read_reg(r))
>
> if (dss_runtime_get())
> - return;
> + return 0;
>
> DUMPREG(DSS_REVISION);
> DUMPREG(DSS_SYSCONFIG);
> @@ -413,6 +418,7 @@ static void dss_dump_regs(struct seq_file *s)
>
> dss_runtime_put();
> #undef DUMPREG
> + return 0;
> }
>
> static int dss_get_channel_index(enum omap_channel channel)
> @@ -906,35 +912,16 @@ void dss_runtime_put(void)
>
> /* DEBUGFS */
> #if defined(CONFIG_OMAP2_DSS_DEBUGFS)
> -static void dss_debug_dump_clocks(struct seq_file *s)
> +static int dss_debug_dump_clocks(struct seq_file *s, void *p)
> {
> dss_dump_clocks(s);
> dispc_dump_clocks(s);
> #ifdef CONFIG_OMAP2_DSS_DSI
> dsi_dump_clocks(s);
> #endif
> -}
> -
> -static int dss_debug_show(struct seq_file *s, void *unused)
> -{
> - void (*func)(struct seq_file *) = s->private;
> -
> - func(s);
> return 0;
> }
>
> -static int dss_debug_open(struct inode *inode, struct file *file)
> -{
> - return single_open(file, dss_debug_show, inode->i_private);
> -}
> -
> -static const struct file_operations dss_debug_fops = {
> - .open = dss_debug_open,
> - .read = seq_read,
> - .llseek = seq_lseek,
> - .release = single_release,
> -};
> -
> static struct dentry *dss_debugfs_dir;
>
> static int dss_initialize_debugfs(void)
> @@ -947,9 +934,6 @@ static int dss_initialize_debugfs(void)
> return err;
> }
>
> - debugfs_create_file("clk", S_IRUGO, dss_debugfs_dir,
> - &dss_debug_dump_clocks, &dss_debug_fops);
> -
> return 0;
> }
>
> @@ -959,15 +943,59 @@ static void dss_uninitialize_debugfs(void)
> debugfs_remove_recursive(dss_debugfs_dir);
> }
>
> -int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file
> *))
> +struct dss_debugfs_entry {
> + struct dentry *dentry;
> + int (*show_fn)(struct seq_file *s, void *data);
> + void *data;
> +};
> +
> +static int dss_debug_open(struct inode *inode, struct file *file)
> {
> + struct dss_debugfs_entry *entry = inode->i_private;
> +
> + return single_open(file, entry->show_fn, entry->data);
> +}
> +
> +static const struct file_operations dss_debug_fops = {
> + .open = dss_debug_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +struct dss_debugfs_entry *dss_debugfs_create_file(const char *name,
> + int (*show_fn)(struct seq_file *s, void *data), void *data)
> +{
> + struct dss_debugfs_entry *entry;
> struct dentry *d;
>
> - d = debugfs_create_file(name, S_IRUGO, dss_debugfs_dir,
> - write, &dss_debug_fops);
> + entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> + if (!entry)
> + return ERR_PTR(-ENOMEM);
> +
> + entry->show_fn = show_fn;
> + entry->data = data;
>
> - return PTR_ERR_OR_ZERO(d);
> + d = debugfs_create_file(name, 0444, dss_debugfs_dir, entry,
> + &dss_debug_fops);
> + if (IS_ERR(d)) {
> + kfree(entry);
> + return ERR_PTR(PTR_ERR(d));
> + }
> +
> + entry->dentry = d;
> + return entry;
> +}
> +
> +void dss_debugfs_remove_file(struct dss_debugfs_entry *entry)
> +{
> + if (IS_ERR_OR_NULL(entry))
> + return;
> +
> + debugfs_remove(entry->dentry);
> + kfree(entry);
> }
> +
> #else /* CONFIG_OMAP2_DSS_DEBUGFS */
> static inline int dss_initialize_debugfs(void)
> {
> @@ -1364,7 +1392,9 @@ static int dss_bind(struct device *dev)
> if (r)
> goto err_component;
>
> - dss_debugfs_create_file("dss", dss_dump_regs);
> + dss.debugfs.clk = dss_debugfs_create_file("clk", dss_debug_dump_clocks,
> + &dss);
> + dss.debugfs.dss = dss_debugfs_create_file("dss", dss_dump_regs, &dss);
>
> pm_set_vt_switch(0);
>
> @@ -1377,6 +1407,8 @@ static int dss_bind(struct device *dev)
> return 0;
>
> err_drm_init:
> + dss_debugfs_remove_file(dss.debugfs.clk);
> + dss_debugfs_remove_file(dss.debugfs.dss);
> component_unbind_all(&pdev->dev, NULL);
> err_component:
> err_runtime_get:
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h
> b/drivers/gpu/drm/omapdrm/dss/dss.h
> index db529481b364..e688e937da28 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/dss.h
> @@ -27,6 +27,11 @@
>
> #include "omapdss.h"
>
> +struct dentry;
You can drop this one. It is not used in dss.h.
> +struct dss_debugfs_entry;
> +struct platform_device;
> +struct seq_file;
> +
> #define MAX_DSS_LCD_MANAGERS 3
> #define MAX_NUM_DSI 2
>
> @@ -234,9 +239,6 @@ struct dss_lcd_mgr_config {
> int lcden_sig_polarity;
> };
>
> -struct seq_file;
> -struct platform_device;
> -
> /* core */
> static inline int dss_set_min_bus_tput(struct device *dev, unsigned long
> tput)
> {
> @@ -255,12 +257,20 @@ static inline bool dss_mgr_is_lcd(enum omap_channel id)
>
> /* DSS */
> #if defined(CONFIG_OMAP2_DSS_DEBUGFS)
> -int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file
> *));
> +struct dss_debugfs_entry *dss_debugfs_create_file(const char *name,
> + int (*show_fn)(struct seq_file *s, void *data), void *data);
> +void dss_debugfs_remove_file(struct dss_debugfs_entry *entry);
> #else
> -static inline int dss_debugfs_create_file(const char *name,
> - void (*write)(struct seq_file *))
> +static inline struct dss_debugfs_entry *
> +dss_debugfs_create_file(const char *name,
> + int (*show_fn)(struct seq_file *s, void *data),
> + void *data)
> +{
> + return NULL;
> +}
> +
> +static inline void dss_debugfs_remove_file(struct dss_debugfs_entry *entry)
> {
> - return 0;
> }
> #endif /* CONFIG_OMAP2_DSS_DEBUGFS */
>
> @@ -325,9 +335,6 @@ static inline void sdi_uninit_port(struct device_node
> *port)
>
> #ifdef CONFIG_OMAP2_DSS_DSI
>
> -struct dentry;
> -struct file_operations;
> -
> int dsi_init_platform_driver(void) __init;
> void dsi_uninit_platform_driver(void);
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi.h
> b/drivers/gpu/drm/omapdrm/dss/hdmi.h
> index c2609c448ddc..a66f8ff06c24 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi.h
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi.h
> @@ -358,6 +358,8 @@ struct omap_hdmi {
> struct mutex lock;
> struct platform_device *pdev;
>
> + struct dss_debugfs_entry *debugfs;
> +
> struct hdmi_wp_data wp;
> struct hdmi_pll_data pll;
> struct hdmi_phy_data phy;
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> index 21ca7bd13fdc..ada4e3a9dba7 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> @@ -302,13 +302,13 @@ static void hdmi_display_get_timings(struct
> omap_dss_device *dssdev,
> *vm = hdmi.cfg.vm;
> }
>
> -static void hdmi_dump_regs(struct seq_file *s)
> +static int hdmi_dump_regs(struct seq_file *s, void *p)
> {
> mutex_lock(&hdmi.lock);
>
> if (hdmi_runtime_get()) {
> mutex_unlock(&hdmi.lock);
> - return;
> + return 0;
> }
>
> hdmi_wp_dump(&hdmi.wp, s);
> @@ -318,6 +318,7 @@ static void hdmi_dump_regs(struct seq_file *s)
>
> hdmi_runtime_put();
> mutex_unlock(&hdmi.lock);
> + return 0;
> }
>
> static int read_edid(u8 *buf, int len)
> @@ -776,7 +777,7 @@ static int hdmi4_bind(struct device *dev, struct device
> *master, void *data)
> return r;
> }
>
> - dss_debugfs_create_file("hdmi", hdmi_dump_regs);
> + hdmi.debugfs = dss_debugfs_create_file("hdmi", hdmi_dump_regs, &hdmi);
>
> return 0;
> err:
> @@ -788,6 +789,8 @@ static void hdmi4_unbind(struct device *dev, struct
> device *master, void *data)
> {
> struct platform_device *pdev = to_platform_device(dev);
>
> + dss_debugfs_remove_file(hdmi.debugfs);
> +
> if (hdmi.audio_pdev)
> platform_device_unregister(hdmi.audio_pdev);
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> index f6c60a6e54ae..00ea975b75f9 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> @@ -299,13 +299,13 @@ static void hdmi_display_get_timings(struct
> omap_dss_device *dssdev,
> *vm = hdmi.cfg.vm;
> }
>
> -static void hdmi_dump_regs(struct seq_file *s)
> +static int hdmi_dump_regs(struct seq_file *s, void *p)
> {
> mutex_lock(&hdmi.lock);
>
> if (hdmi_runtime_get()) {
> mutex_unlock(&hdmi.lock);
> - return;
> + return 0;
> }
>
> hdmi_wp_dump(&hdmi.wp, s);
> @@ -315,6 +315,7 @@ static void hdmi_dump_regs(struct seq_file *s)
>
> hdmi_runtime_put();
> mutex_unlock(&hdmi.lock);
> + return 0;
> }
>
> static int read_edid(u8 *buf, int len)
> @@ -774,7 +775,7 @@ static int hdmi5_bind(struct device *dev, struct device
> *master, void *data)
> return r;
> }
>
> - dss_debugfs_create_file("hdmi", hdmi_dump_regs);
> + hdmi.debugfs = dss_debugfs_create_file("hdmi", hdmi_dump_regs, &hdmi);
>
> return 0;
> err:
> @@ -786,6 +787,8 @@ static void hdmi5_unbind(struct device *dev, struct
> device *master, void *data)
> {
> struct platform_device *pdev = to_platform_device(dev);
>
> + dss_debugfs_remove_file(hdmi.debugfs);
> +
> if (hdmi.audio_pdev)
> platform_device_unregister(hdmi.audio_pdev);
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c
> b/drivers/gpu/drm/omapdrm/dss/venc.c
> index 1b0fa952b494..68035c1acf1f 100644
> --- a/drivers/gpu/drm/omapdrm/dss/venc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/venc.c
> @@ -328,6 +328,8 @@ static struct {
> u32 wss_data;
> struct regulator *vdda_dac_reg;
>
> + struct dss_debugfs_entry *debugfs;
> +
> struct clk *tv_dac_clk;
>
> struct videomode vm;
> @@ -671,12 +673,12 @@ static int venc_init_regulator(void)
> return 0;
> }
>
> -static void venc_dump_regs(struct seq_file *s)
> +static int venc_dump_regs(struct seq_file *s, void *p)
> {
> #define DUMPREG(r) seq_printf(s, "%-35s %08x\n", #r, venc_read_reg(r))
>
> if (venc_runtime_get())
> - return;
> + return 0;
>
> DUMPREG(VENC_F_CONTROL);
> DUMPREG(VENC_VIDOUT_CTRL);
> @@ -723,6 +725,7 @@ static void venc_dump_regs(struct seq_file *s)
> venc_runtime_put();
>
> #undef DUMPREG
> + return 0;
> }
>
> static int venc_get_clocks(struct platform_device *pdev)
> @@ -913,7 +916,7 @@ static int venc_bind(struct device *dev, struct device
> *master, void *data)
> goto err_probe_of;
> }
>
> - dss_debugfs_create_file("venc", venc_dump_regs);
> + venc.debugfs = dss_debugfs_create_file("venc", venc_dump_regs, &venc);
>
> venc_init_output(pdev);
>
> @@ -929,6 +932,8 @@ static void venc_unbind(struct device *dev, struct device
> *master, void *data)
> {
> struct platform_device *pdev = to_platform_device(dev);
>
> + dss_debugfs_remove_file(venc.debugfs);
> +
> venc_uninit_output(pdev);
>
> pm_runtime_disable(&pdev->dev);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
