Re: [RFC Patch 1/1] debugfs_printk and debugfs_dump interface

[David Wilder]

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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/