Messages in this thread | | | From | Nick Crews <> | Date | Fri, 8 Feb 2019 12:45:58 -0700 | Subject | Re: [PATCH v6 3/4] platform/chrome: Add support for raw commands in debugfs |
| |
I just sent out a fixed version. Thanks!
On Fri, Feb 8, 2019 at 10:18 AM Enric Balletbo Serra <eballetbo@gmail.com> wrote: > > Hi Nick, > Missatge de Nick Crews <ncrews@chromium.org> del dia dv., 8 de febr. > 2019 a les 2:24: > > > > Add a debugfs attribute that allows sending raw commands to the EC. > > This is useful for development and debug but should not be enabled > > in a production environment. > > > > To test: > > Get the EC firmware build date > > First send the request command > > > echo 00 f0 38 00 03 00 > raw > > Then read the result. "12/21/18" is in the middle of the response > > > cat raw > > 00 31 32 2f 32 31 2f 31 38 00 00 0f 01 00 01 00 .12/21/18....... > > > > Get the EC firmware build date > > First send the request command > > > echo 00 f0 38 00 03 00 > raw > > Then read the result. "12/21/18" is in the middle of the response > > > cat raw > > 00 31 32 2f 32 31 2f 31 38 00 00 0f 01 00 01 00 .12/21/18....... > > > > Signed-off-by: Duncan Laurie <dlaurie@google.com> > > Signed-off-by: Nick Crews <ncrews@chromium.org> > > --- > > > > Changes in v6: > > - s/4.19/5.1/ for kernel version in documentation, since that is > > the version this patch should land in. > > - Instead of requiring at least 3 bytes for msg type and command, > > now just require two for msg type. We can skip the command. > > - Fixed error checking in probe() so that errors are hidden, without > > causing more errors or unextpected behavior. > > - Some comment polishing. > > - In the core, actually unregister the debugfs child platform_device > > > > Changes in v5: > > - core now always continues regardless of debugfs failure > > - mv documentation to file header > > - Check for OOM > > - rm unneeded check if debug_info is allocqated > > - rm bogus comment > > - add space around "+" > > - rm WILCO_EC_FLAG_EXTENDED_DATA, that is already obvious from > > wilco_ec_message.response_size > > > > Changes in v4: > > - Change debugfs driver to be a separate module > > - Change me email to @chromium.org from @google.com > > - Change CONFIG_WILCO_EC_SYSFS_RAW to > > CONFIG_WILCO_EC_DEBUGFS > > > > Changes in v3: > > - Move the attribute to the debugfs system > > - Move the implementation to debugfs.c > > - Improve the raw hex parsing > > - Encapsulate global variables in one object > > - Add safety check when passing less than 3 bytes > > - Move documentation to debugfs-wilco-ec > > > > Changes in v2: > > - Add sysfs documentation > > - rm duplicate EC_MAILBOX_DATA_SIZE defs > > - Make docstrings follow kernel style > > - Fix tags in commit msg > > - Move Kconfig to subdirectory > > - Reading raw now includes ASCII translation > > > > Documentation/ABI/testing/debugfs-wilco-ec | 23 ++ > > drivers/platform/chrome/wilco_ec/Kconfig | 10 + > > drivers/platform/chrome/wilco_ec/Makefile | 2 + > > drivers/platform/chrome/wilco_ec/core.c | 14 ++ > > drivers/platform/chrome/wilco_ec/debugfs.c | 238 +++++++++++++++++++++ > > include/linux/platform_data/wilco-ec.h | 2 + > > 6 files changed, 289 insertions(+) > > create mode 100644 Documentation/ABI/testing/debugfs-wilco-ec > > create mode 100644 drivers/platform/chrome/wilco_ec/debugfs.c > > > > diff --git a/Documentation/ABI/testing/debugfs-wilco-ec b/Documentation/ABI/testing/debugfs-wilco-ec > > new file mode 100644 > > index 000000000000..f814f112e213 > > --- /dev/null > > +++ b/Documentation/ABI/testing/debugfs-wilco-ec > > @@ -0,0 +1,23 @@ > > +What: /sys/kernel/debug/wilco_ec/raw > > +Date: January 2019 > > +KernelVersion: 5.1 > > +Description: > > + Write and read raw mailbox commands to the EC. > > + > > + For writing: > > + Bytes 0-1 indicate the message type: > > + 00 F0 = Execute Legacy Command > > + 00 F2 = Read/Write NVRAM Property > > + Byte 2 provides the command code > > + Bytes 3+ consist of the data passed in the request > > + > > + At least three bytes are required, for the msg type and command, > > + with additional bytes optional for additional data. > > + > > + Example: > > + // Request EC info type 3 (EC firmware build date) > > + $ echo 00 f0 38 00 03 00 > raw > > + // View the result. The decoded ASCII result "12/21/18" is > > + // included after the raw hex. > > + $ cat raw > > + 00 31 32 2f 32 31 2f 31 38 00 38 00 01 00 2f 00 .12/21/18.8... > > diff --git a/drivers/platform/chrome/wilco_ec/Kconfig b/drivers/platform/chrome/wilco_ec/Kconfig > > index 20945a301ec6..fb6d9ff33e41 100644 > > --- a/drivers/platform/chrome/wilco_ec/Kconfig > > +++ b/drivers/platform/chrome/wilco_ec/Kconfig > > @@ -9,3 +9,13 @@ config WILCO_EC > > > > To compile this driver as a module, choose M here: the > > module will be called wilco_ec. > > + > > +config WILCO_EC_DEBUGFS > > + tristate "Enable raw access to EC via debugfs" > > + depends on WILCO_EC > > + help > > + If you say Y here, you get support for sending raw commands to > > + the Wilco EC via debugfs. These commands do not do any byte > > + manipulation and allow for testing arbitrary commands. This > > + interface is intended for debug only and will not be present > > + on production devices. > > diff --git a/drivers/platform/chrome/wilco_ec/Makefile b/drivers/platform/chrome/wilco_ec/Makefile > > index 03b32301dc61..063e7fb4ea17 100644 > > --- a/drivers/platform/chrome/wilco_ec/Makefile > > +++ b/drivers/platform/chrome/wilco_ec/Makefile > > @@ -2,3 +2,5 @@ > > > > wilco_ec-objs := core.o mailbox.o > > obj-$(CONFIG_WILCO_EC) += wilco_ec.o > > +wilco_ec_debugfs-objs := debugfs.o > > +obj-$(CONFIG_WILCO_EC_DEBUGFS) += wilco_ec_debugfs.o > > diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c > > index 20ecc580d108..ae86cae216fd 100644 > > --- a/drivers/platform/chrome/wilco_ec/core.c > > +++ b/drivers/platform/chrome/wilco_ec/core.c > > @@ -69,11 +69,25 @@ static int wilco_ec_probe(struct platform_device *pdev) > > cros_ec_lpc_mec_init(ec->io_packet->start, > > ec->io_packet->start + EC_MAILBOX_DATA_SIZE); > > > > + /* > > + * Register a debugfs platform device that will get picked up by the > > + * debugfs driver. Ignore failure. > > + */ > > + ec->debugfs_pdev = platform_device_register_data(dev, > > + "wilco-ec-debugfs", > > + PLATFORM_DEVID_AUTO, > > + NULL, 0); > > + > > return 0; > > } > > > > static int wilco_ec_remove(struct platform_device *pdev) > > { > > + struct wilco_ec_device *ec = platform_get_drvdata(pdev); > > + > > + if (ec->debugfs_pdev) > > + platform_device_unregister(ec->debugfs_pdev); > > + > > /* Teardown cros_ec interface */ > > cros_ec_lpc_mec_destroy(); > > > > diff --git a/drivers/platform/chrome/wilco_ec/debugfs.c b/drivers/platform/chrome/wilco_ec/debugfs.c > > new file mode 100644 > > index 000000000000..8b324bfe42cd > > --- /dev/null > > +++ b/drivers/platform/chrome/wilco_ec/debugfs.c > > @@ -0,0 +1,238 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * debugfs attributes for Wilco EC > > + * > > + * Copyright 2019 Google LLC > > + * > > + * There is only one attribute used for debugging, called raw. > > + * You can write a hexadecimal sentence to raw, and that series of bytes > > + * will be sent to the EC. Then, you can read the bytes of response > > + * by reading from raw. > > + * > > + * For writing: > > + * Bytes 0-1 indicate the message type: > > + * 00 F0 = Execute Legacy Command > > + * 00 F2 = Read/Write NVRAM Property > > + * Byte 2 provides the command code > > + * Bytes 3+ consist of the data passed in the request > > + * > > + * When referencing the EC interface spec, byte 2 corresponds to MBOX[0], > > + * byte 3 corresponds to MBOX[1], etc. > > + * > > + * At least three bytes are required, for the msg type and command, > > + * with additional bytes optional for additional data. > > + * > > + * Example: > > + * // Request EC info type 3 (EC firmware build date) > > + * $ echo 00 f0 38 00 03 00 > raw > > + * // View the result. The decoded ASCII result "12/21/18" is > > + * // included after the raw hex. > > + * $ cat raw > > + * 00 31 32 2f 32 31 2f 31 38 00 38 00 01 00 2f 00 .12/21/18.8... > > + */ > > + > > +#include <linux/ctype.h> > > +#include <linux/debugfs.h> > > +#include <linux/fs.h> > > +#include <linux/module.h> > > +#include <linux/platform_data/wilco-ec.h> > > +#include <linux/platform_device.h> > > + > > +#define DRV_NAME "wilco-ec-debugfs" > > + > > +/* The 256 raw bytes will take up more space when represented as a hex string */ > > +#define FORMATTED_BUFFER_SIZE (EC_MAILBOX_DATA_SIZE_EXTENDED * 4) > > + > > +struct wilco_ec_debugfs { > > + struct wilco_ec_device *ec; > > + struct dentry *dir; > > + size_t response_size; > > + u8 raw_data[EC_MAILBOX_DATA_SIZE_EXTENDED]; > > + u8 formatted_data[FORMATTED_BUFFER_SIZE]; > > +}; > > +static struct wilco_ec_debugfs *debug_info; > > + > > +/** > > + * parse_hex_sentence() - Convert a ascii hex representation into byte array. > > + * @in: Input buffer of ascii. > > + * @isize: Length of input buffer. > > + * @out: Output buffer. > > + * @osize: Length of output buffer, e.g. max number of bytes to parse. > > + * > > + * An valid input is a series of ascii hexadecimal numbers, separated by spaces. > > + * An example valid input is > > + * " 00 f2 0 000076 6 0 ff" > > + * > > + * If an individual "word" within the hex sentence is longer than MAX_WORD_SIZE, > > + * then the sentence is illegal, and parsing will fail. > > + * > > + * Return: Number of bytes parsed, or negative error code on failure. > > + */ > > +static int parse_hex_sentence(const char *in, int isize, u8 *out, int osize) > > +{ > > + int n_parsed = 0; > > + int word_start = 0; > > + int word_end; > > + int word_len; > > + /* Temp buffer for holding a "word" of chars that represents one byte */ > > + const static int MAX_WORD_SIZE = 16; > > + char tmp[MAX_WORD_SIZE + 1]; > > I got this warning: > > CC [M] drivers/platform/chrome/wilco_ec/debugfs.o > drivers/platform/chrome/wilco_ec/debugfs.c: In function ‘parse_hex_sentence’: > drivers/platform/chrome/wilco_ec/debugfs.c:79:2: warning: ISO C90 > forbids array ‘tmp’ whose size can’t be evaluated [-Wvla] > char tmp[MAX_WORD_SIZE + 1]; > ^~~~ > > Could you kindly fix and resend, please. > > Thanks, > Enric > > > + u8 byte; > > + > > + while (word_start < isize && n_parsed < osize) { > > + /* Find the start of the next word */ > > + while (word_start < isize && isspace(in[word_start])) > > + word_start++; > > + /* reached the end of the input before next word? */ > > + if (word_start >= isize) > > + break; > > + > > + /* Find the end of this word */ > > + word_end = word_start; > > + while (word_end < isize && !isspace(in[word_end])) > > + word_end++; > > + > > + /* Copy to a tmp NULL terminated string */ > > + word_len = word_end - word_start; > > + if (word_len > MAX_WORD_SIZE) > > + return -EINVAL; > > + memcpy(tmp, in + word_start, word_len); > > + tmp[word_len] = '\0'; > > + > > + /* > > + * Convert from hex string, place in output. If fails to parse, > > + * just return -EINVAL because specific error code is only > > + * relevant for this one word, returning it would be confusing. > > + */ > > + if (kstrtou8(tmp, 16, &byte)) > > + return -EINVAL; > > + out[n_parsed++] = byte; > > + > > + word_start = word_end; > > + } > > + return n_parsed; > > +} > > + > > +/* The message type takes up two bytes*/ > > +#define TYPE_AND_DATA_SIZE ((EC_MAILBOX_DATA_SIZE) + 2) > > + > > +static ssize_t raw_write(struct file *file, const char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + char *buf = debug_info->formatted_data; > > + struct wilco_ec_message msg; > > + u8 request_data[TYPE_AND_DATA_SIZE]; > > + ssize_t kcount; > > + int ret; > > + > > + if (count > FORMATTED_BUFFER_SIZE) > > + return -EINVAL; > > + > > + kcount = simple_write_to_buffer(buf, FORMATTED_BUFFER_SIZE, ppos, > > + user_buf, count); > > + if (kcount < 0) > > + return kcount; > > + > > + ret = parse_hex_sentence(buf, kcount, request_data, TYPE_AND_DATA_SIZE); > > + if (ret < 0) > > + return ret; > > + /* Need at least two bytes for message type */ > > + if (ret < 2) > > + return -EINVAL; > > + > > + /* Clear response data buffer */ > > + memset(debug_info->raw_data, '\0', EC_MAILBOX_DATA_SIZE_EXTENDED); > > + > > + msg.type = request_data[0] << 8 | request_data[1]; > > + msg.flags = WILCO_EC_FLAG_RAW; > > + msg.command = ret > 2 ? request_data[2] : 0; > > + msg.request_data = ret > 3 ? request_data + 3 : 0; > > + msg.request_size = ret - 3; > > + msg.response_data = debug_info->raw_data; > > + msg.response_size = EC_MAILBOX_DATA_SIZE; > > + > > + /* Telemetry commands use extended response data */ > > + if (msg.type == WILCO_EC_MSG_TELEMETRY_LONG) { > > + msg.flags |= WILCO_EC_FLAG_EXTENDED_DATA; > > + msg.response_size = EC_MAILBOX_DATA_SIZE_EXTENDED; > > + } > > + > > + ret = wilco_ec_mailbox(debug_info->ec, &msg); > > + if (ret < 0) > > + return ret; > > + debug_info->response_size = ret; > > + > > + return count; > > +} > > + > > +static ssize_t raw_read(struct file *file, char __user *user_buf, size_t count, > > + loff_t *ppos) > > +{ > > + int fmt_len = 0; > > + > > + if (debug_info->response_size) { > > + fmt_len = hex_dump_to_buffer(debug_info->raw_data, > > + debug_info->response_size, > > + 16, 1, debug_info->formatted_data, > > + FORMATTED_BUFFER_SIZE, true); > > + /* Only return response the first time it is read */ > > + debug_info->response_size = 0; > > + } > > + > > + return simple_read_from_buffer(user_buf, count, ppos, > > + debug_info->formatted_data, fmt_len); > > +} > > + > > +static const struct file_operations fops_raw = { > > + .owner = THIS_MODULE, > > + .read = raw_read, > > + .write = raw_write, > > + .llseek = no_llseek, > > +}; > > + > > +/** > > + * wilco_ec_debugfs_probe() - Create the debugfs node > > + * @pdev: The platform device, probably created in core.c > > + * > > + * Try to create a debugfs node. If it fails, then we don't want to change > > + * behavior at all, this is for debugging after all. Just fail silently. > > + * > > + * Return: 0 always. > > + */ > > +static int wilco_ec_debugfs_probe(struct platform_device *pdev) > > +{ > > + struct wilco_ec_device *ec = dev_get_drvdata(pdev->dev.parent); > > + > > + debug_info = devm_kzalloc(&pdev->dev, sizeof(*debug_info), GFP_KERNEL); > > + if (!debug_info) > > + return 0; > > + debug_info->ec = ec; > > + debug_info->dir = debugfs_create_dir("wilco_ec", NULL); > > + if (!debug_info->dir) > > + return 0; > > + debugfs_create_file("raw", 0644, debug_info->dir, NULL, &fops_raw); > > + > > + return 0; > > +} > > + > > +static int wilco_ec_debugfs_remove(struct platform_device *pdev) > > +{ > > + debugfs_remove_recursive(debug_info->dir); > > + > > + return 0; > > +} > > + > > +static struct platform_driver wilco_ec_debugfs_driver = { > > + .driver = { > > + .name = DRV_NAME, > > + }, > > + .probe = wilco_ec_debugfs_probe, > > + .remove = wilco_ec_debugfs_remove, > > +}; > > + > > +module_platform_driver(wilco_ec_debugfs_driver); > > + > > +MODULE_ALIAS("platform:" DRV_NAME); > > +MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("Wilco EC debugfs driver"); > > diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h > > index 0feb4b520a54..5344975afa1a 100644 > > --- a/include/linux/platform_data/wilco-ec.h > > +++ b/include/linux/platform_data/wilco-ec.h > > @@ -34,6 +34,7 @@ > > * @data_buffer: Buffer used for EC communication. The same buffer > > * is used to hold the request and the response. > > * @data_size: Size of the data buffer used for EC communication. > > + * @debugfs_pdev: The child platform_device used by the debugfs sub-driver. > > */ > > struct wilco_ec_device { > > struct device *dev; > > @@ -43,6 +44,7 @@ struct wilco_ec_device { > > struct resource *io_packet; > > void *data_buffer; > > size_t data_size; > > + struct platform_device *debugfs_pdev; > > }; > > > > /** > > -- > > 2.20.1.611.gfbb209baf1-goog > >
| |