[PATCH 0/2] Google VPD sysfs driver

[PATCH 0/2] Google VPD sysfs driver

[Thierry Escande]

Hi,

This patchset adds support for accessing Google Vital Product Data (VPD)
through the sysfs interface under /sys/firmware/vpd.

This patchset contains the VPD decoding routines read from coreboot
table entries and the driver responsible for creating the sysfs
interface.

This patchset depends on [1] for coreboot support in the Google memory
console.

Regards,
 Thierry
 
[1] https://lkml.org/lkml/2017/3/28/902

Wei-Ning Huang (2):
  firmware: Google Vital Product Data: import lib_vpd source files
  firmware: Google VPD sysfs driver

 drivers/firmware/google/Kconfig      |   7 +
 drivers/firmware/google/Makefile     |   3 +
 drivers/firmware/google/vpd.c        | 333 +++++++++++++++++++++++++++++++++++
 drivers/firmware/google/vpd_decode.c | 102 +++++++++++
 drivers/firmware/google/vpd_decode.h |  59 +++++++
 5 files changed, 504 insertions(+)
 create mode 100644 drivers/firmware/google/vpd.c
 create mode 100644 drivers/firmware/google/vpd_decode.c
 create mode 100644 drivers/firmware/google/vpd_decode.h

-- 
2.7.4

[PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Thierry Escande]

From: Wei-Ning Huang <wnhuang@google.com>

This patch imports lib_vpd.h and vpd_decode.c from the Chromium Vital
Product Data project.

This library is used to parse VPD sections obtained from coreboot table
entries describing Chromebook devices product data. Only the sections of
type VPD_TYPE_STRING are decoded.

The VPD string sections in the coreboot tables contain the type (1 byte
set to 0x01 for strings), the key length, the key ascii array, the value
length, and the value ascii array. The key and value arrays are not null
terminated.

Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/firmware/google/vpd_decode.c | 99 ++++++++++++++++++++++++++++++++++++
 drivers/firmware/google/vpd_decode.h | 59 +++++++++++++++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 drivers/firmware/google/vpd_decode.c
 create mode 100644 drivers/firmware/google/vpd_decode.h

diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
new file mode 100644
index 0000000..75e4027
--- /dev/null
+++ b/drivers/firmware/google/vpd_decode.c
@@ -0,0 +1,99 @@
+/*
+ * vpd_decode.c
+ *
+ * Google VPD decoding routines.
+ *
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/export.h>
+
+#include "vpd_decode.h"
+
+static int decode_len(const int32_t max_len, const uint8_t *in,
+		      int32_t *length, int32_t *decoded_len)
+{
+	uint8_t more;
+	int i = 0;
+
+	if (!length || !decoded_len)
+		return VPD_FAIL;
+
+	*length = 0;
+	do {
+		if (i >= max_len)
+			return VPD_FAIL;
+
+		more = in[i] & 0x80;
+		*length <<= 7;
+		*length |= in[i] & 0x7f;
+		++i;
+	} while (more);
+
+	*decoded_len = i;
+
+	return VPD_OK;
+}
+
+int decode_vpd_string(const int32_t max_len, const uint8_t *input_buf,
+		      int32_t *consumed, vpd_decode_callback callback,
+		      void *callback_arg)
+{
+	int type;
+	int res;
+	int32_t key_len, value_len;
+	int32_t decoded_len;
+	const uint8_t *key, *value;
+
+	/* type */
+	if (*consumed >= max_len)
+		return VPD_FAIL;
+
+	type = input_buf[*consumed];
+
+	switch (type) {
+	case VPD_TYPE_INFO:
+	case VPD_TYPE_STRING:
+		(*consumed)++;
+
+		/* key */
+		res = decode_len(max_len - *consumed, &input_buf[*consumed],
+				 &key_len, &decoded_len);
+		if (res != VPD_OK || *consumed + decoded_len >= max_len)
+			return VPD_FAIL;
+
+		*consumed += decoded_len;
+		key = &input_buf[*consumed];
+		*consumed += key_len;
+
+		/* value */
+		res = decode_len(max_len - *consumed, &input_buf[*consumed],
+				 &value_len, &decoded_len);
+		if (res != VPD_OK || *consumed + decoded_len > max_len)
+			return VPD_FAIL;
+
+		*consumed += decoded_len;
+		value = &input_buf[*consumed];
+		*consumed += value_len;
+
+		if (type == VPD_TYPE_STRING)
+			return callback(key, key_len, value, value_len,
+					callback_arg);
+		break;
+
+	default:
+		return VPD_FAIL;
+	}
+
+	return VPD_OK;
+}
+EXPORT_SYMBOL(decode_vpd_string);
diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/google/vpd_decode.h
new file mode 100644
index 0000000..c6c9b75
--- /dev/null
+++ b/drivers/firmware/google/vpd_decode.h
@@ -0,0 +1,59 @@
+/*
+ * vpd_decode.h
+ *
+ * Google VPD decoding routines.
+ *
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __VPD_DECODE_H
+#define __VPD_DECODE_H
+
+#include <linux/types.h>
+
+enum {
+	VPD_OK = 0,
+	VPD_FAIL,
+};
+
+enum {
+	VPD_TYPE_TERMINATOR = 0,
+	VPD_TYPE_STRING,
+	VPD_TYPE_INFO                = 0xfe,
+	VPD_TYPE_IMPLICIT_TERMINATOR = 0xff,
+};
+
+/* Callback for decode_vpd_string to invoke. */
+typedef int vpd_decode_callback(const uint8_t *key, int32_t key_len,
+				const uint8_t *value, int32_t value_len,
+				void *arg);
+
+/*
+ * decode_vpd_string
+ *
+ * Given the encoded string, this function invokes callback with extracted
+ * (key, value). The *consumed will be plused the number of bytes consumed in
+ * this function.
+ *
+ * The input_buf points to the first byte of the input buffer.
+ *
+ * The *consumed starts from 0, which is actually the next byte to be decoded.
+ * It can be non-zero to be used in multiple calls.
+ *
+ * If one entry is successfully decoded, sends it to callback and returns the
+ * result.
+ */
+int decode_vpd_string(const int32_t max_len, const uint8_t *input_buf,
+		      int32_t *consumed, vpd_decode_callback callback,
+		      void *callback_arg);
+
+#endif  /* __VPD_DECODE_H */
-- 
2.7.4

[PATCH 2/2] firmware: Google VPD sysfs driver

[Thierry Escande]

From: Wei-Ning Huang <wnhuang@google.com>

This patch introduces the Google Vital Product Data driver.

This driver reads Vital Product Data from coreboot tables and then
creates the corresponding sysfs entries under /sys/firmware/vpd to
provide easy access for userspace programs (does not require flashrom).

The sysfs is structured as follow:

 /sys/firmware/vpd
 |-- ro
 |   |-- key1
 |   `-- key2
 |-- ro_raw
 |-- rw
 |   `-- key1
 `-- rw_raw

Where ro_raw and rw_raw contain the raw VPD partition. The files under
ro and rw correspond to the key name in the VPD and the the file content
is the value for the key.

Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/firmware/google/Kconfig  |   7 +
 drivers/firmware/google/Makefile |   3 +
 drivers/firmware/google/vpd.c    | 333 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 343 insertions(+)
 create mode 100644 drivers/firmware/google/vpd.c

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 00000e0..f16b381 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -64,4 +64,11 @@ config GOOGLE_MEMCONSOLE_COREBOOT
 	  the coreboot table.  If found, this log is exported to userland
 	  in the file /sys/firmware/log.
 
+config GOOGLE_VPD
+	tristate "Vital Product Data"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  This option enables the kernel to expose the content of Google VPD
+	  under /sys/firmware/vpd.
+
 endif # GOOGLE_FIRMWARE
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index bb952c6..bc4de02 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -6,3 +6,6 @@ obj-$(CONFIG_GOOGLE_COREBOOT_TABLE_OF)     += coreboot_table-of.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
+
+vpd-sysfs-y := vpd.o vpd_decode.o
+obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
new file mode 100644
index 0000000..e167f33
--- /dev/null
+++ b/drivers/firmware/google/vpd.c
@@ -0,0 +1,333 @@
+/*
+ * vpd.c
+ *
+ * Driver for exporting VPD content to sysfs.
+ *
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/ctype.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include "coreboot_table.h"
+#include "vpd_decode.h"
+
+#define CB_TAG_VPD      0x2c
+#define VPD_CBMEM_MAGIC 0x43524f53
+
+static struct kobject *vpd_kobj;
+
+struct vpd_cbmem {
+	uint32_t magic;
+	uint32_t version;
+	uint32_t ro_size;
+	uint32_t rw_size;
+	uint8_t blob[0];
+};
+
+struct vpd_section {
+	bool enabled;
+	const char *name;
+	char *raw_name;                /* the string name_raw */
+	struct kobject *kobj;          /* vpd/name directory */
+	char *baseaddr;
+	struct bin_attribute bin_attr; /* vpd/name_raw bin_attribute */
+	struct list_head attribs;      /* key/value in vpd_attrib_info list */
+};
+
+struct vpd_attrib_info {
+	char *key;
+	const char *value;
+	struct bin_attribute bin_attr;
+	struct list_head list;
+};
+
+static struct vpd_section ro_vpd;
+static struct vpd_section rw_vpd;
+
+static ssize_t vpd_attrib_read(struct file *filp, struct kobject *kobp,
+			       struct bin_attribute *bin_attr, char *buf,
+			       loff_t pos, size_t count)
+{
+	struct vpd_attrib_info *info = bin_attr->private;
+
+	return memory_read_from_buffer(buf, count, &pos, info->value,
+				       info->bin_attr.size);
+}
+
+/*
+ * vpd_section_check_key_name()
+ *
+ * The VPD specification supports only [a-zA-Z0-9_]+ characters in key names but
+ * old firmware versions may have entries like "S/N" which are problematic when
+ * exporting them as sysfs attributes. These keys present in old firmwares are
+ * ignored.
+ *
+ * Returns VPD_OK for a valid key name, VPD_FAIL otherwise.
+ *
+ * @key: The key name to check
+ * @key_len: key name length
+ */
+static int vpd_section_check_key_name(const uint8_t *key, int32_t key_len)
+{
+	int c;
+
+	while (key_len-- > 0) {
+		c = *key++;
+
+		if (!isalnum(c) && c != '_')
+			return VPD_FAIL;
+	}
+
+	return VPD_OK;
+}
+
+static int vpd_section_attrib_add(const uint8_t *key, int32_t key_len,
+				  const uint8_t *value, int32_t value_len,
+				  void *arg)
+{
+	int ret;
+	struct vpd_section *sec = arg;
+	struct vpd_attrib_info *info;
+
+	/*
+	 * Return VPD_OK immediately to decode next entry if the current key
+	 * name contains invalid characters.
+	 */
+	if (vpd_section_check_key_name(key, key_len) != VPD_OK)
+		return VPD_OK;
+
+	info = kzalloc(sizeof(struct vpd_attrib_info), GFP_KERNEL);
+	info->key = kzalloc(key_len + 1, GFP_KERNEL);
+	if (!info->key)
+		return -ENOMEM;
+
+	memcpy(info->key, key, key_len);
+
+	sysfs_bin_attr_init(&info->bin_attr);
+	info->bin_attr.attr.name = info->key;
+	info->bin_attr.attr.mode = 0444;
+	info->bin_attr.size = value_len;
+	info->bin_attr.read = vpd_attrib_read;
+	info->bin_attr.private = info;
+
+	info->value = value;
+
+	INIT_LIST_HEAD(&info->list);
+	list_add_tail(&info->list, &sec->attribs);
+
+	ret = sysfs_create_bin_file(sec->kobj, &info->bin_attr);
+	if (ret) {
+		kfree(info->key);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void vpd_section_attrib_destroy(struct vpd_section *sec)
+{
+	struct vpd_attrib_info *info;
+	struct vpd_attrib_info *temp;
+
+	list_for_each_entry_safe(info, temp, &sec->attribs, list) {
+		kfree(info->key);
+		sysfs_remove_bin_file(sec->kobj, &info->bin_attr);
+		kfree(info);
+	}
+}
+
+static ssize_t vpd_section_read(struct file *filp, struct kobject *kobp,
+		struct bin_attribute *bin_attr, char *buf,
+		loff_t pos, size_t count)
+{
+	struct vpd_section *sec = bin_attr->private;
+
+	return memory_read_from_buffer(buf, count, &pos, sec->baseaddr,
+				       sec->bin_attr.size);
+}
+
+
+static int vpd_section_create_attribs(struct vpd_section *sec)
+{
+	int32_t consumed;
+	int ret;
+
+	consumed = 0;
+	do {
+		ret = decode_vpd_string(sec->bin_attr.size, sec->baseaddr,
+					&consumed, vpd_section_attrib_add, sec);
+	} while (ret == VPD_OK);
+
+	return 0;
+}
+
+static int vpd_section_init(const char *name, struct vpd_section *sec,
+		phys_addr_t physaddr, size_t size)
+{
+	int ret;
+	int raw_len;
+
+	sec->baseaddr = memremap(physaddr, size, MEMREMAP_WB);
+	if (sec->baseaddr == NULL)
+		return -ENOMEM;
+
+	sec->name = name;
+
+	/* We want to export the raw partion with name ${name}_raw */
+	raw_len = strlen(name) + 5;
+	sec->raw_name = kzalloc(raw_len, GFP_KERNEL);
+	strncpy(sec->raw_name, name, raw_len);
+	strncat(sec->raw_name, "_raw", raw_len);
+
+	sysfs_bin_attr_init(&sec->bin_attr);
+	sec->bin_attr.attr.name = sec->raw_name;
+	sec->bin_attr.attr.mode = 0444;
+	sec->bin_attr.size = size;
+	sec->bin_attr.read = vpd_section_read;
+	sec->bin_attr.private = sec;
+
+	ret = sysfs_create_bin_file(vpd_kobj, &sec->bin_attr);
+	if (ret)
+		goto free_sec;
+
+	sec->kobj = kobject_create_and_add(name, vpd_kobj);
+	if (!sec->kobj) {
+		ret = -EINVAL;
+		goto sysfs_remove;
+	}
+
+	INIT_LIST_HEAD(&sec->attribs);
+	vpd_section_create_attribs(sec);
+
+	sec->enabled = true;
+
+	return 0;
+
+sysfs_remove:
+	sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
+
+free_sec:
+	kfree(sec->raw_name);
+	iounmap(sec->baseaddr);
+
+	return ret;
+}
+
+static int vpd_section_destroy(struct vpd_section *sec)
+{
+	if (sec->enabled) {
+		vpd_section_attrib_destroy(sec);
+		kobject_del(sec->kobj);
+		sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
+		kfree(sec->raw_name);
+		iounmap(sec->baseaddr);
+	}
+
+	return 0;
+}
+
+static int init_vpd_sections(phys_addr_t physaddr)
+{
+	struct vpd_cbmem __iomem *temp;
+	struct vpd_cbmem header;
+	int ret = 0;
+
+	temp = memremap(physaddr, sizeof(struct vpd_cbmem), MEMREMAP_WB);
+	if (!temp)
+		return -ENOMEM;
+
+	memcpy_fromio(&header, temp, sizeof(struct vpd_cbmem));
+	iounmap(temp);
+
+	if (header.magic != VPD_CBMEM_MAGIC)
+		return -ENODEV;
+
+	if (header.ro_size) {
+		ret = vpd_section_init("ro", &ro_vpd,
+				physaddr + sizeof(struct vpd_cbmem),
+				header.ro_size);
+		if (ret)
+			return ret;
+	}
+
+	if (header.rw_size) {
+		ret = vpd_section_init("rw", &rw_vpd,
+				physaddr + sizeof(struct vpd_cbmem) +
+				header.ro_size, header.rw_size);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int vpd_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct lb_cbmem_ref entry;
+
+	ret = coreboot_table_find(CB_TAG_VPD, &entry, sizeof(entry));
+	if (ret)
+		return ret;
+
+	return init_vpd_sections(entry.cbmem_addr);
+}
+
+static struct platform_driver vpd_driver = {
+	.probe = vpd_probe,
+	.driver = {
+		.name = "vpd",
+	},
+};
+
+static int __init platform_vpd_init(void)
+{
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_simple("vpd", -1, NULL, 0);
+	if (pdev == NULL)
+		return -ENODEV;
+
+	vpd_kobj = kobject_create_and_add("vpd", firmware_kobj);
+	if (!vpd_kobj)
+		return -ENOMEM;
+
+	memset(&ro_vpd, 0, sizeof(ro_vpd));
+	memset(&rw_vpd, 0, sizeof(rw_vpd));
+
+	platform_driver_register(&vpd_driver);
+
+	return 0;
+}
+
+static void __exit platform_vpd_exit(void)
+{
+	vpd_section_destroy(&ro_vpd);
+	vpd_section_destroy(&rw_vpd);
+	kobject_del(vpd_kobj);
+}
+
+module_init(platform_vpd_init);
+module_exit(platform_vpd_exit);
+
+MODULE_AUTHOR("Google, Inc.");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Greg Kroah-Hartman]

On Tue, Apr 11, 2017 at 11:14:30AM +0200, Thierry Escande wrote:
> From: Wei-Ning Huang <wnhuang@google.com>
> 
> This patch imports lib_vpd.h and vpd_decode.c from the Chromium Vital
> Product Data project.
> 
> This library is used to parse VPD sections obtained from coreboot table
> entries describing Chromebook devices product data. Only the sections of
> type VPD_TYPE_STRING are decoded.
> 
> The VPD string sections in the coreboot tables contain the type (1 byte
> set to 0x01 for strings), the key length, the key ascii array, the value
> length, and the value ascii array. The key and value arrays are not null
> terminated.
> 
> Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/firmware/google/vpd_decode.c | 99 ++++++++++++++++++++++++++++++++++++
>  drivers/firmware/google/vpd_decode.h | 59 +++++++++++++++++++++
>  2 files changed, 158 insertions(+)
>  create mode 100644 drivers/firmware/google/vpd_decode.c
>  create mode 100644 drivers/firmware/google/vpd_decode.h
> 
> diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
> new file mode 100644
> index 0000000..75e4027
> --- /dev/null
> +++ b/drivers/firmware/google/vpd_decode.c
> @@ -0,0 +1,99 @@
> +/*
> + * vpd_decode.c
> + *
> + * Google VPD decoding routines.
> + *
> + * Copyright 2017 Google Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2.0 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/export.h>
> +
> +#include "vpd_decode.h"
> +
> +static int decode_len(const int32_t max_len, const uint8_t *in,
> +		      int32_t *length, int32_t *decoded_len)
> +{
> +	uint8_t more;

Care to use "real" kernel variable types please?  u8, u16, and others
are you friend, uint8_t really isn't what we prefer, and checkpatch
should tell you that...

thanks,

greg k-h

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Greg Kroah-Hartman]

On Tue, Apr 11, 2017 at 11:14:30AM +0200, Thierry Escande wrote:
> From: Wei-Ning Huang <wnhuang@google.com>
> 
> This patch imports lib_vpd.h and vpd_decode.c from the Chromium Vital
> Product Data project.
> 
> This library is used to parse VPD sections obtained from coreboot table
> entries describing Chromebook devices product data. Only the sections of
> type VPD_TYPE_STRING are decoded.
> 
> The VPD string sections in the coreboot tables contain the type (1 byte
> set to 0x01 for strings), the key length, the key ascii array, the value
> length, and the value ascii array. The key and value arrays are not null
> terminated.
> 
> Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/firmware/google/vpd_decode.c | 99 ++++++++++++++++++++++++++++++++++++
>  drivers/firmware/google/vpd_decode.h | 59 +++++++++++++++++++++
>  2 files changed, 158 insertions(+)
>  create mode 100644 drivers/firmware/google/vpd_decode.c
>  create mode 100644 drivers/firmware/google/vpd_decode.h
> 
> diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
> new file mode 100644
> index 0000000..75e4027
> --- /dev/null
> +++ b/drivers/firmware/google/vpd_decode.c
> @@ -0,0 +1,99 @@
> +/*
> + * vpd_decode.c
> + *
> + * Google VPD decoding routines.
> + *
> + * Copyright 2017 Google Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2.0 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/export.h>
> +
> +#include "vpd_decode.h"
> +
> +static int decode_len(const int32_t max_len, const uint8_t *in,
> +		      int32_t *length, int32_t *decoded_len)
> +{
> +	uint8_t more;
> +	int i = 0;
> +
> +	if (!length || !decoded_len)
> +		return VPD_FAIL;
> +
> +	*length = 0;
> +	do {
> +		if (i >= max_len)
> +			return VPD_FAIL;
> +
> +		more = in[i] & 0x80;
> +		*length <<= 7;
> +		*length |= in[i] & 0x7f;
> +		++i;
> +	} while (more);
> +
> +	*decoded_len = i;
> +
> +	return VPD_OK;
> +}
> +
> +int decode_vpd_string(const int32_t max_len, const uint8_t *input_buf,
> +		      int32_t *consumed, vpd_decode_callback callback,
> +		      void *callback_arg)
> +{
> +	int type;
> +	int res;
> +	int32_t key_len, value_len;
> +	int32_t decoded_len;
> +	const uint8_t *key, *value;
> +
> +	/* type */
> +	if (*consumed >= max_len)
> +		return VPD_FAIL;
> +
> +	type = input_buf[*consumed];
> +
> +	switch (type) {
> +	case VPD_TYPE_INFO:
> +	case VPD_TYPE_STRING:
> +		(*consumed)++;
> +
> +		/* key */
> +		res = decode_len(max_len - *consumed, &input_buf[*consumed],
> +				 &key_len, &decoded_len);
> +		if (res != VPD_OK || *consumed + decoded_len >= max_len)
> +			return VPD_FAIL;
> +
> +		*consumed += decoded_len;
> +		key = &input_buf[*consumed];
> +		*consumed += key_len;
> +
> +		/* value */
> +		res = decode_len(max_len - *consumed, &input_buf[*consumed],
> +				 &value_len, &decoded_len);
> +		if (res != VPD_OK || *consumed + decoded_len > max_len)
> +			return VPD_FAIL;
> +
> +		*consumed += decoded_len;
> +		value = &input_buf[*consumed];
> +		*consumed += value_len;
> +
> +		if (type == VPD_TYPE_STRING)
> +			return callback(key, key_len, value, value_len,
> +					callback_arg);
> +		break;
> +
> +	default:
> +		return VPD_FAIL;
> +	}
> +
> +	return VPD_OK;
> +}
> +EXPORT_SYMBOL(decode_vpd_string);

Normally put the prefix for the driver you are exporting from:
	vpd_string_decode()?

And EXPORT_SYMBOL_GPL() perhaps?

thanks,

greg k-h

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Greg Kroah-Hartman]

On Tue, Apr 11, 2017 at 04:10:12PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 11, 2017 at 11:14:30AM +0200, Thierry Escande wrote:
> > From: Wei-Ning Huang <wnhuang@google.com>
> > 
> > This patch imports lib_vpd.h and vpd_decode.c from the Chromium Vital
> > Product Data project.
> > 
> > This library is used to parse VPD sections obtained from coreboot table
> > entries describing Chromebook devices product data. Only the sections of
> > type VPD_TYPE_STRING are decoded.
> > 
> > The VPD string sections in the coreboot tables contain the type (1 byte
> > set to 0x01 for strings), the key length, the key ascii array, the value
> > length, and the value ascii array. The key and value arrays are not null
> > terminated.
> > 
> > Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> > ---
> >  drivers/firmware/google/vpd_decode.c | 99 ++++++++++++++++++++++++++++++++++++
> >  drivers/firmware/google/vpd_decode.h | 59 +++++++++++++++++++++
> >  2 files changed, 158 insertions(+)
> >  create mode 100644 drivers/firmware/google/vpd_decode.c
> >  create mode 100644 drivers/firmware/google/vpd_decode.h
> > 
> > diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
> > new file mode 100644
> > index 0000000..75e4027
> > --- /dev/null
> > +++ b/drivers/firmware/google/vpd_decode.c
> > @@ -0,0 +1,99 @@
> > +/*
> > + * vpd_decode.c
> > + *
> > + * Google VPD decoding routines.
> > + *
> > + * Copyright 2017 Google Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License v2.0 as published by
> > + * the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/export.h>
> > +
> > +#include "vpd_decode.h"
> > +
> > +static int decode_len(const int32_t max_len, const uint8_t *in,
> > +		      int32_t *length, int32_t *decoded_len)
> > +{
> > +	uint8_t more;
> > +	int i = 0;
> > +
> > +	if (!length || !decoded_len)
> > +		return VPD_FAIL;
> > +
> > +	*length = 0;
> > +	do {
> > +		if (i >= max_len)
> > +			return VPD_FAIL;
> > +
> > +		more = in[i] & 0x80;
> > +		*length <<= 7;
> > +		*length |= in[i] & 0x7f;
> > +		++i;
> > +	} while (more);
> > +
> > +	*decoded_len = i;
> > +
> > +	return VPD_OK;
> > +}
> > +
> > +int decode_vpd_string(const int32_t max_len, const uint8_t *input_buf,
> > +		      int32_t *consumed, vpd_decode_callback callback,
> > +		      void *callback_arg)
> > +{
> > +	int type;
> > +	int res;
> > +	int32_t key_len, value_len;
> > +	int32_t decoded_len;
> > +	const uint8_t *key, *value;
> > +
> > +	/* type */
> > +	if (*consumed >= max_len)
> > +		return VPD_FAIL;
> > +
> > +	type = input_buf[*consumed];
> > +
> > +	switch (type) {
> > +	case VPD_TYPE_INFO:
> > +	case VPD_TYPE_STRING:
> > +		(*consumed)++;
> > +
> > +		/* key */
> > +		res = decode_len(max_len - *consumed, &input_buf[*consumed],
> > +				 &key_len, &decoded_len);
> > +		if (res != VPD_OK || *consumed + decoded_len >= max_len)
> > +			return VPD_FAIL;
> > +
> > +		*consumed += decoded_len;
> > +		key = &input_buf[*consumed];
> > +		*consumed += key_len;
> > +
> > +		/* value */
> > +		res = decode_len(max_len - *consumed, &input_buf[*consumed],
> > +				 &value_len, &decoded_len);
> > +		if (res != VPD_OK || *consumed + decoded_len > max_len)
> > +			return VPD_FAIL;
> > +
> > +		*consumed += decoded_len;
> > +		value = &input_buf[*consumed];
> > +		*consumed += value_len;
> > +
> > +		if (type == VPD_TYPE_STRING)
> > +			return callback(key, key_len, value, value_len,
> > +					callback_arg);
> > +		break;
> > +
> > +	default:
> > +		return VPD_FAIL;
> > +	}
> > +
> > +	return VPD_OK;
> > +}
> > +EXPORT_SYMBOL(decode_vpd_string);
> 
> Normally put the prefix for the driver you are exporting from:
> 	vpd_string_decode()?
> 
> And EXPORT_SYMBOL_GPL() perhaps?

Wait, why is this even exported?  You build this into the module that
needs it...

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Thierry Escande]

Hi Greg

On 11/04/2017 16:09, Greg Kroah-Hartman wrote:
> On Tue, Apr 11, 2017 at 11:14:30AM +0200, Thierry Escande wrote:
>> +#include <linux/export.h>
>> +
>> +#include "vpd_decode.h"
>> +
>> +static int decode_len(const int32_t max_len, const uint8_t *in,
>> +		      int32_t *length, int32_t *decoded_len)
>> +{
>> +	uint8_t more;
>
> Care to use "real" kernel variable types please?  u8, u16, and others
> are you friend, uint8_t really isn't what we prefer, and checkpatch
> should tell you that...

By default checkpatch doesn't warn about types unless --strict is 
specified... Sorry about that.

Regards,
  Thierry

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Thierry Escande]

On 11/04/2017 16:11, Greg Kroah-Hartman wrote:
> On Tue, Apr 11, 2017 at 04:10:12PM +0200, Greg Kroah-Hartman wrote:
>> On Tue, Apr 11, 2017 at 11:14:30AM +0200, Thierry Escande wrote:
...
>>> +	}
>>> +
>>> +	return VPD_OK;
>>> +}
>>> +EXPORT_SYMBOL(decode_vpd_string);
>>
>> Normally put the prefix for the driver you are exporting from:
>> 	vpd_string_decode()?
>>
>> And EXPORT_SYMBOL_GPL() perhaps?
>
> Wait, why is this even exported?  You build this into the module that
> needs it...
>

Right. Will fix that as well.

Regards,
  Thierry

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Joe Perches]

On Tue, 2017-04-11 at 16:09 +0200, Greg Kroah-Hartman wrote:
> Care to use "real" kernel variable types please?  u8, u16, and others
> are you friend, uint8_t really isn't what we prefer, and checkpatch
> should tell you that...

checkpatch doesn't warn about "u?int\d+_t" types unless
--strict is enabled and most likely it shouldn't.

There are about 100k uses of those types in the kernel.

Many are uapi and are perhaps properly used in drivers and lib.

checkpatch can't tell what type a particular use should have.

Maybe coccinelle could based on whether the include comes
from a uapi directory or prototype or some such.

[Cocci] [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Joe Perches]

On Tue, 2017-04-11 at 16:09 +0200, Greg Kroah-Hartman wrote:
> Care to use "real" kernel variable types please?  u8, u16, and others
> are you friend, uint8_t really isn't what we prefer, and checkpatch
> should tell you that...

checkpatch doesn't warn about "u?int\d+_t" types unless
--strict is enabled and most likely it shouldn't.

There are about 100k uses of those types in the kernel.

Many are uapi and are perhaps properly used in drivers and lib.

checkpatch can't tell what type a particular use should have.

Maybe coccinelle could based on whether the include comes
from a uapi directory or prototype or some such.

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Greg Kroah-Hartman]

On Tue, Apr 11, 2017 at 11:15:54AM -0700, Joe Perches wrote:
> On Tue, 2017-04-11 at 16:09 +0200, Greg Kroah-Hartman wrote:
> > Care to use "real" kernel variable types please?  u8, u16, and others
> > are you friend, uint8_t really isn't what we prefer, and checkpatch
> > should tell you that...
> 
> checkpatch doesn't warn about "u?int\d+_t" types unless
> --strict is enabled and most likely it shouldn't.

For brand new drivers, it's a good thing to run, to keep maintainers
from complaining about obvious things :)

thanks,

greg k-h

[Cocci] [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Greg Kroah-Hartman]

On Tue, Apr 11, 2017 at 11:15:54AM -0700, Joe Perches wrote:
> On Tue, 2017-04-11 at 16:09 +0200, Greg Kroah-Hartman wrote:
> > Care to use "real" kernel variable types please?  u8, u16, and others
> > are you friend, uint8_t really isn't what we prefer, and checkpatch
> > should tell you that...
> 
> checkpatch doesn't warn about "u?int\d+_t" types unless
> --strict is enabled and most likely it shouldn't.

For brand new drivers, it's a good thing to run, to keep maintainers
from complaining about obvious things :)

thanks,

greg k-h

Re: [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Joe Perches]

On Tue, 2017-04-11 at 20:42 +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 11, 2017 at 11:15:54AM -0700, Joe Perches wrote:
> > On Tue, 2017-04-11 at 16:09 +0200, Greg Kroah-Hartman wrote:
> > > Care to use "real" kernel variable types please?  u8, u16, and others
> > > are you friend, uint8_t really isn't what we prefer, and checkpatch
> > > should tell you that...
> > 
> > checkpatch doesn't warn about "u?int\d+_t" types unless
> > --strict is enabled and most likely it shouldn't.
> 
> For brand new drivers, it's a good thing to run, to keep maintainers
> from complaining about obvious things :)

--strict emits some messages that are either unacceptable
or unnecessary to some maintainers.

'course checkpatch itself is unacceptable and unnecessary
to some maintainers too.

cheers, Joe

[Cocci] [PATCH 1/2] firmware: Google VPD: import lib_vpd source files

[Joe Perches]

On Tue, 2017-04-11 at 20:42 +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 11, 2017 at 11:15:54AM -0700, Joe Perches wrote:
> > On Tue, 2017-04-11 at 16:09 +0200, Greg Kroah-Hartman wrote:
> > > Care to use "real" kernel variable types please?  u8, u16, and others
> > > are you friend, uint8_t really isn't what we prefer, and checkpatch
> > > should tell you that...
> > 
> > checkpatch doesn't warn about "u?int\d+_t" types unless
> > --strict is enabled and most likely it shouldn't.
> 
> For brand new drivers, it's a good thing to run, to keep maintainers
> from complaining about obvious things :)

--strict emits some messages that are either unacceptable
or unnecessary to some maintainers.

'course checkpatch itself is unacceptable and unnecessary
to some maintainers too.

cheers, Joe