| Message ID | 20171013145944.26557-22-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.pinchart@ideasonboard.com> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@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
Hi Sebastian, On Monday, 16 October 2017 00:10:19 EEST Sebastian Reichel wrote: > 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.pinchart@ideasonboard.com> > > --- > > Reviewed-by: Sebastian Reichel <sebastian.reichel@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(-) [snip] > > 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. Indeed, it's a leftover from a previous version where I was still exposing struct dentry. I'll remove it in v2. > > +struct dss_debugfs_entry; > > +struct platform_device; > > +struct seq_file; [snip]
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; +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);
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.pinchart@ideasonboard.com> --- 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(-)