Messages in this thread | | | Date | Thu, 24 Apr 2008 14:23:16 -0700 | From | David Wilder <> | Subject | Re: [RFC Patch 1/1] debugfs_printk and debugfs_dump interface |
| |
debugfs_print makes a great improvement to the trace interface. I missing a couple of bugs the first time I reviewed it. Please see my comments in-line.
Dave..
> +static inline struct trace_info *init_trace_interface(struct > + debugfs_printk_data *dpk) > +{ > + struct trace_info *ti; > + > + dpk->exists = trace_exists(dpk->parent_dir, dpk->dir, &ti); > + > + switch(dpk->exists) { > + > + case TRACE_PARENT_DIR_EXISTS: > + case TRACE_PARENT_DIR_ABSENT: > + if(!dpk->buf_size) > + dpk->buf_size = DEFAULT_TRACE_BUF_SIZE; > + if(!dpk->sub_buf_size) > + dpk->sub_buf_size = DEFAULT_TRACE_SUB_BUF_NR; > + if(!dpk->flags) > + dpk->flags = TRACE_FLIGHT_CHANNEL; > + ti = trace_setup(dpk->parent_dir, dpk->dir, > + dpk->buf_size, dpk->sub_buf_size, dpk->flags); > + printk(KERN_INFO "Trace interface %s setup through " > + "debugfs_printk\n", > + ti->dir->d_iname); > + if (IS_ERR(ti)) { > + printk(KERN_ERR "Trace interface could not be " > + "initialised\n"); > + return PTR_ERR(ti); > + } > + /* Fall through */ > + case TRACE_DIR_EXISTS: > + if (ti->state != TRACE_RUNNING) {
You should start the trace only if the current state is TRACE_SETUP (I think). See my comment in debugfs_print()
if (ti->state == TRACE_SETUP) > + trace_start(ti); > + } > + } > + return ti; > +} > + > +/* > + * debugfs_printk - A function to write into the debugfs mount 'directly' > + * using 'trace' infrastructure > + * @dpk - Structure containing info such as parent_dir and directory > + * @format - String containing format string specifiers > + * @ap - List of arguments > + */ > +int debugfs_printk(struct debugfs_printk_data *dpk, char *format, ...) > +{ > + int ret; > + struct trace_info *ti; > + va_list(ap); > + unsigned long flags; > + > + va_start(ap, format); > + > + ti = init_trace_interface(dpk);
init_trace_interface() alway sets trace->state to TRACE_RUNNING . The results is that the user is prevented from stopping the trace. You can see this in fork_new_trace.
$ cat debug/trace_new_example/do_fork/state running $ echo stop > debug/trace_new_example/do_fork/state $ cat debug/trace_new_example/do_fork/state running
> + > + /* Now do the actual printing */ > + /* Take an RCU Lock over the trace_info state */ > + rcu_read_lock(); > + /* Take a spinlock for the global buffer used by relay */ > + if (dpk->flags & TRACE_GLOBAL_CHANNEL) > + spin_lock_irqsave(&trace_lock, flags); > + ret = trace_printf(ti, format, ap); > + if (dpk->flags & TRACE_GLOBAL_CHANNEL) > + spin_unlock_irqrestore(&trace_lock, flags); > + rcu_read_unlock(); > + > + va_end(ap); > + return ret; > +} > +EXPORT_SYMBOL(debugfs_printk); > + > +/* > + * debugfs_printk - A function to write into the debugfs mount 'directly' > + * using 'trace' infrastructure > + * @dpk - Structure containing info such as parent_dir and directory > + * @output - Data that needs to be output > + * @output_len - Length of the output data > + */ > +int debugfs_dump(struct debugfs_printk_data *dpk, const void *output, > + const int output_len) > +{ > + struct trace_info *ti; > + char *record; > + > + ti = init_trace_interface(dpk);
Same issue as debugfs_printf,
You should also check for trace_running as you did for trace_printf.
> + > + /* Now do the actual printing */ > + rcu_read_lock(); > + record = relay_reserve(ti->rchan, output_len); > + if (record) > + memcpy(record, output, output_len); > + else > + return -ENOMEM; > + rcu_read_unlock(); > + > + return 0; > +} > +EXPORT_SYMBOL(debugfs_dump);
| |