Re: [PATCH resend 2] x86, olpc: add debugfs interface for EC commands

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 29 Nov 2011 22:27:20 +0000 (GMT)
Daniel Drake <dsd@laptop.org> wrote:

> Add a debugfs interface for sending commands to the OLPC EC and reading
> their responses. This is useful for EC development and debugging.

It would be nice to expand on what an "EC" is.  For those people who
don't know.  This group doesn't include me, of course.

> Based on code by Paul Fox.

It would be good solicit and include Paul's signed-off-by:.  And/or to cc
him on the patch.

> diff --git a/arch/x86/platform/olpc/olpc.c b/arch/x86/platform/olpc/olpc.c
> index 7cce722..860aff9 100644
> --- a/arch/x86/platform/olpc/olpc.c
> +++ b/arch/x86/platform/olpc/olpc.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/debugfs.h>
>  
>  #include <asm/geode.h>
>  #include <asm/setup.h>
> @@ -31,6 +32,13 @@ EXPORT_SYMBOL_GPL(olpc_platform_info);
>  
>  static DEFINE_SPINLOCK(ec_lock);
>  
> +/* debugfs interface to EC commands */
> +#define EC_MAX_CMD_ARGS (5 + 1)	/* cmd byte + 5 args */
> +#define EC_MAX_CMD_REPLY (8)
> +static struct dentry *ec_debugfs_dir;
> +static unsigned char ec_debugfs_resp[EC_MAX_CMD_REPLY];
> +static unsigned int ec_debugfs_resp_bytes;
> +
>  /* EC event mask to be applied during suspend (defining wakeup sources). */
>  static u16 ec_wakeup_mask;
>  
> @@ -269,6 +277,98 @@ int olpc_ec_sci_query(u16 *sci_value)
>  }
>  EXPORT_SYMBOL_GPL(olpc_ec_sci_query);
>  
> +static ssize_t ec_gen_write(struct file *file, const char __user *buf,
> +			    size_t size, loff_t *ppos)
> +{
> +	int i, m;
> +	unsigned char ec_cmd[EC_MAX_CMD_ARGS];
> +	unsigned int ec_cmd_int[EC_MAX_CMD_ARGS];
> +	char cmdbuf[64];
> +	int ec_cmd_bytes;
> +
> +	size = simple_write_to_buffer(cmdbuf, sizeof(cmdbuf), ppos, buf, size);
> +
> +	m = sscanf(cmdbuf, "%x:%u %x %x %x %x %x", &ec_cmd_int[0],
> +		   &ec_debugfs_resp_bytes,
> +		   &ec_cmd_int[1], &ec_cmd_int[2], &ec_cmd_int[3],
> +		   &ec_cmd_int[4], &ec_cmd_int[5]);
> +	if (m <= 0 || ec_debugfs_resp_bytes > EC_MAX_CMD_REPLY) {

I think you need "if (m < 2" here.  We require that
ec_debugfs_resp_bytes was written to.

> +		printk(KERN_DEBUG "olpc-ec: bad ec cmd:  "
> +		       "cmd:response-count [arg1 [arg2 ...]]\n");
> +		return -EINVAL;
> +	}
> +
> +	/* convert scanf'd ints to char */
> +	for (i = 0; i < EC_MAX_CMD_ARGS; i++)
> +		ec_cmd[i] = ec_cmd_int[i];

Here we're potentially reading uninitialised stack slots.  It would be
better to constrain this loop to just the number of items we know are
present and valid.

> +	ec_cmd_bytes = m - 2;
> +
> +	printk(KERN_DEBUG "olpc-ec: debugfs cmd 0x%02x with %d args "
> +	       "%02x %02x %02x %02x %02x, want %d returns\n",
> +	       ec_cmd[0], ec_cmd_bytes, ec_cmd[1], ec_cmd[2], ec_cmd[3],
> +	       ec_cmd[4], ec_cmd[5], ec_debugfs_resp_bytes);
> +
> +	olpc_ec_cmd((unsigned char) ec_cmd[0],
> +		(ec_cmd_bytes == 0) ? NULL : &ec_cmd[1],
> +		ec_cmd_bytes, ec_debugfs_resp, ec_debugfs_resp_bytes);
> +
> +	printk(KERN_DEBUG "olpc-ec: response "
> +	       "%02x %02x %02x %02x %02x %02x %02x %02x (%d bytes expected)\n",
> +	       ec_debugfs_resp[0], ec_debugfs_resp[1], ec_debugfs_resp[2],
> +	       ec_debugfs_resp[3], ec_debugfs_resp[4], ec_debugfs_resp[5],
> +	       ec_debugfs_resp[6], ec_debugfs_resp[7], ec_debugfs_resp_bytes);
> +
> +	return size;
> +}
> +
> +static ssize_t ec_gen_read(struct file *file, char __user *buf,
> +			   size_t size, loff_t *ppos)
> +{
> +	unsigned int i, r;
> +	char *rp;
> +	char respbuf[64];
> +
> +	rp = respbuf;
> +	rp += sprintf(rp, "%02x", ec_debugfs_resp[0]);
> +	for (i = 1; i < ec_debugfs_resp_bytes; i++)
> +		rp += sprintf(rp, ", %02x", ec_debugfs_resp[i]);
> +	rp += sprintf(rp, "\n");
> +
> +	r = rp - respbuf;
> +
> +	return simple_read_from_buffer(buf, size, ppos, respbuf, r);
> +}
> +
> +static const struct file_operations ec_debugfs_genops = {
> +	.write	 = ec_gen_write,
> +	.read	 = ec_gen_read,
> +};
> +
> +static char ec_debugfs_help_text[] =
> +	"generic write:   CC:N A A A A ...\n"
> +	" CC is the (hex) command, N is the (optional) count of expected\n"
> +	" reply bytes, and A A A A are optional (hex) arguments.\n"
> +	"generic read:  will show hex reply bytes, *whether or not* they\n"
> +	" came from the immediately previous command\n";

erk, this is a bit silly.  Surely we don't document debugfs files by
embedding that documentation into other debugfs files.  It would be
saner (and smaller!) to document it under Documentation/ somewhere. 
Plus we can provide much better documentation in that context.