greybus: start improving manifest parsing

Currently the module manifest parsing code is sort of representative
only and is not really very useful.

This patch begins doing "real" parsing of the module manifest.
It scans the module manifest to identify the descriptors it holds.
It then verifies there's only one module descriptor found, and
initializes new some fields in the gb_module structure based on what
it contains (converting what's found to native byte order).
Note that if anything unexpected is found or other errors occur when
parsing the manifest, the parse fails.

Because we now save this converted information when it's parsed we
no longer have a greybus_descriptor_module struct within a struct
gb_module.  And because we've already converted these values, we can
do a little less work displaying values in sysfs.  (We also now show
vendor, product, and version values in the right byte order.)  This
eliminates the need for greybus_string(), so get rid of it.

It also slightly simplifies the greybus module matching code.

Move some existing parsing code into a new file, "manifest.c".
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

drivers/staging/greybus/Makefile

@@ -3,6 +3,7 @@ greybus-y :=	core.o		\
 		sysfs.o		\
 		debugfs.o	\
 		ap.o		\
+		manifest.o	\
 		module.o	\
 		interface.o	\
 		function.o	\

drivers/staging/greybus/core.c

@@ -126,22 +126,6 @@ static void greybus_module_release(struct device *dev)
 }
 
 
-const u8 *greybus_string(struct gb_module *gmod, int id)
-{
-	int i;
-	struct gmod_string *string;
-
-	if (!gmod)
-		return NULL;
-
-	for (i = 0; i < gmod->num_strings; ++i) {
-		string = gmod->string[i];
-		if (string->id == id)
-			return &string->string[0];
-	}
-	return NULL;
-}
-
 static struct device_type greybus_module_type = {
 	.name =		"greybus_module",
 	.release =	greybus_module_release,
@@ -204,19 +188,6 @@ static const struct greybus_module_id fake_greybus_module_id = {
 	GREYBUS_DEVICE(0x42, 0x42)
 };
 
-static int create_module(struct gb_module *gmod,
-			    struct greybus_descriptor_module *module,
-			    size_t desc_size)
-{
-	if (desc_size != sizeof(*module)) {
-		dev_err(gmod->dev.parent, "invalid module header size %zu\n",
-			desc_size);
-		return -EINVAL;
-	}
-	memcpy(&gmod->module, module, desc_size);
-	return 0;
-}
-
 static int create_string(struct gb_module *gmod,
 			 struct greybus_descriptor_string *string,
 			 size_t desc_size)
@@ -284,15 +255,16 @@ void gb_add_module(struct greybus_host_device *hd, u8 module_id,
 	struct gb_module *gmod;
 	struct greybus_manifest *manifest;
 	int retval;
-	int overall_size;
-
-	/* we have to have at _least_ the manifest header */
-	if (size <= sizeof(manifest->header))
-		return;
 
-	gmod = gb_module_create(hd, module_id);
-	if (!gmod)
+	/*
+	 * Parse the manifest and build up our data structures
+	 * representing what's in it.
+	 */
+	gmod = gb_manifest_parse(data, size);
+	if (!gmod) {
+		dev_err(hd->parent, "manifest error\n");
 		return;
+	}
 
 	gmod->dev.parent = hd->parent;
 	gmod->dev.driver = NULL;
@@ -303,24 +275,6 @@ void gb_add_module(struct greybus_host_device *hd, u8 module_id,
 	device_initialize(&gmod->dev);
 	dev_set_name(&gmod->dev, "%d", module_id);
 
-	manifest = (struct greybus_manifest *)data;
-	overall_size = le16_to_cpu(manifest->header.size);
-	if (overall_size != size) {
-		dev_err(hd->parent, "size != manifest header size, %d != %d\n",
-			size, overall_size);
-		goto error;
-	}
-
-	/* Validate major/minor number */
-	if (manifest->header.version_major > GREYBUS_VERSION_MAJOR) {
-		dev_err(hd->parent,
-			"Manifest version too new (%hhu.%hhu > %hhu.%hhu)\n",
-			manifest->header.version_major,
-			manifest->header.version_minor,
-			GREYBUS_VERSION_MAJOR, GREYBUS_VERSION_MINOR);
-		goto error;
-	}
-
 	size -= sizeof(manifest->header);
 	data += sizeof(manifest->header);
 	while (size > 0) {
@@ -347,8 +301,6 @@ void gb_add_module(struct greybus_host_device *hd, u8 module_id,
 			break;
 
 		case GREYBUS_TYPE_MODULE:
-			retval = create_module(gmod, &desc->module,
-						  data_size);
 			break;
 
 		case GREYBUS_TYPE_STRING:

drivers/staging/greybus/greybus.h

@@ -20,6 +20,7 @@
 
 #include "greybus_id.h"
 #include "greybus_manifest.h"
+#include "manifest.h"
 #include "module.h"
 #include "interface.h"
 #include "function.h"
@@ -246,8 +247,6 @@ int greybus_disabled(void);
 
 void greybus_remove_device(struct gb_module *gmod);
 
-const u8 *greybus_string(struct gb_module *gmod, int id);
-
 /* Internal functions to gb module, move to internal .h file eventually. */
 
 void gb_add_module(struct greybus_host_device *hd, u8 module_id,

drivers/staging/greybus/manifest.c

+/*
+ * Greybus module manifest parsing
+ *
+ * Copyright 2014 Google Inc.
+ *
+ * Released under the GPLv2 only.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/err.h>
+
+#include "greybus.h"
+
+/*
+ * We scan the manifest once to identify where all the descriptors
+ * are.  The result is a list of these manifest_desc structures.  We
+ * then pick through them for what we're looking for (starting with
+ * the module descriptor).  As each is processed we remove it from
+ * the list.  When we're done the list should (probably) be empty.
+ */
+struct manifest_desc {
+	struct list_head		links;
+
+	size_t				size;
+	void				*data;
+	enum greybus_descriptor_type	type;
+};
+
+static LIST_HEAD(manifest_descs);
+
+static void release_manifest_descriptor(struct manifest_desc *descriptor)
+{
+	list_del(&descriptor->links);
+	kfree(descriptor);
+}
+
+static void release_manifest_descriptors(void)
+{
+	struct manifest_desc *descriptor;
+	struct manifest_desc *next;
+
+	list_for_each_entry_safe(descriptor, next, &manifest_descs, links)
+		release_manifest_descriptor(descriptor);
+}
+
+/*
+ * Validate the given descriptor.  Its reported size must fit within
+ * the number of bytes reamining, and it must have a recognized
+ * type.  Check that the reported size is at least as big as what
+ * we expect to see.  (It could be bigger, perhaps for a new version
+ * of the format.)
+ *
+ * Returns the number of bytes consumed by the descriptor, or a
+ * negative errno.
+ */
+static int identify_descriptor(struct greybus_descriptor *desc, size_t size)
+{
+	struct greybus_descriptor_header *desc_header = &desc->header;
+	struct manifest_desc *descriptor;
+	int desc_size;
+	size_t expected_size;
+
+	if (size < sizeof(*desc_header)) {
+		pr_err("manifest too small\n");
+		return -EINVAL;		/* Must at least have header */
+	}
+
+	desc_size = (int)le16_to_cpu(desc_header->size);
+	if ((size_t)desc_size > size) {
+		pr_err("descriptor too big\n");
+		return -EINVAL;
+	}
+
+	switch (desc_header->type) {
+	case GREYBUS_TYPE_MODULE:
+		if (desc_size < sizeof(struct greybus_descriptor_module)) {
+			pr_err("module descriptor too small (%u)\n",
+				desc_size);
+			return -EINVAL;
+		}
+		break;
+	case GREYBUS_TYPE_DEVICE:
+		break;
+	case GREYBUS_TYPE_CLASS:
+		pr_err("class descriptor found (ignoring)\n");
+		break;
+	case GREYBUS_TYPE_STRING:
+		expected_size = sizeof(struct greybus_descriptor_header);
+		expected_size += sizeof(struct greybus_descriptor_string);
+		expected_size += (size_t)desc->string.length;
+		if (desc_size < expected_size) {
+			pr_err("string descriptor too small (%u)\n",
+				desc_size);
+			return -EINVAL;
+		}
+		break;
+	case GREYBUS_TYPE_CPORT:
+		if (desc_size < sizeof(struct greybus_descriptor_cport)) {
+			pr_err("cport descriptor too small (%u)\n",
+				desc_size);
+			return -EINVAL;
+		}
+		break;
+	case GREYBUS_TYPE_INVALID:
+	default:
+		pr_err("invalid descriptor type (%hhu)\n", desc_header->type);
+		return -EINVAL;
+	}
+
+	descriptor = kzalloc(sizeof(*descriptor), GFP_KERNEL);
+	if (!descriptor)
+		return -ENOMEM;
+
+	descriptor->size = desc_size;
+	descriptor->data = desc;
+	descriptor->type = desc_header->type;
+	list_add_tail(&descriptor->links, &manifest_descs);
+
+	return desc_size;
+}
+
+/*
+ * Find the string descriptor having the given id, validate it, and
+ * allocate a duplicate copy of it.  The duplicate has an extra byte
+ * which guarantees the returned string is NUL-terminated.
+ *
+ * String index 0 is valid (it represents "no string"), and for
+ * that a null pointer is returned.
+ *
+ * Otherwise returns a pointer to a newly-allocated copy of the
+ * descriptor string, or an error-coded pointer on failure.
+ */
+static char *gb_string_get(u8 string_id)
+{
+	struct greybus_descriptor_string *desc_string;
+	struct manifest_desc *descriptor;
+	bool found = false;
+	char *string;
+
+	/* A zero string id means no string (but no error) */
+	if (!string_id)
+		return NULL;
+
+	list_for_each_entry(descriptor, &manifest_descs, links) {
+		struct greybus_descriptor *desc;
+
+		if (descriptor->type != GREYBUS_TYPE_STRING)
+			continue;
+
+		desc = descriptor->data;
+		desc_string = &desc->string;
+		if (desc_string->id == string_id) {
+			found = true;
+			break;
+		}
+	}
+	if (!found)
+		return ERR_PTR(-ENOENT);
+
+	/* Allocate an extra byte so we can guarantee it's NUL-terminated */
+	string = kmemdup(&desc_string->string, (size_t)desc_string->length + 1,
+				GFP_KERNEL);
+	if (!string)
+		return ERR_PTR(-ENOMEM);
+	string[desc_string->length] = '\0';
+
+	/* Ok we've used this string, so we're done with it */
+	release_manifest_descriptor(descriptor);
+
+	return string;
+}
+
+struct gb_module *gb_manifest_parse_module(struct manifest_desc *module_desc)
+{
+	struct greybus_descriptor *desc = module_desc->data;
+	struct greybus_descriptor_module *desc_module = &desc->module;
+	struct gb_module *gmod;
+
+	gmod = kzalloc(sizeof(*gmod), GFP_KERNEL);
+	if (!gmod)
+		return NULL;
+
+	/* Handle the strings first--they can fail */
+	gmod->vendor_string = gb_string_get(desc_module->vendor_stringid);
+	if (IS_ERR(gmod->vendor_string)) {
+		kfree(gmod);
+		return NULL;
+	}
+	gmod->product_string = gb_string_get(desc_module->product_stringid);
+	if (IS_ERR(gmod->product_string)) {
+		kfree(gmod->vendor_string);
+		kfree(gmod);
+		return NULL;
+	}
+
+	gmod->vendor = le16_to_cpu(desc_module->vendor);
+	gmod->product = le16_to_cpu(desc_module->product);
+	gmod->version = le16_to_cpu(desc_module->version);
+	gmod->serial_number = le64_to_cpu(desc_module->serial_number);
+
+	/* Release the module descriptor, now that we're done with it */
+	release_manifest_descriptor(module_desc);
+
+	return gmod;
+}
+
+/*
+ * Parse a buffer containing a module manifest.
+ *
+ * If we find anything wrong with the content/format of the buffer
+ * we reject it.
+ *
+ * The first requirement is that the manifest's version is
+ * one we can parse.
+ *
+ * We make an initial pass through the buffer and identify all of
+ * the descriptors it contains, keeping track for each its type
+ * and the location size of its data in the buffer.
+ *
+ * Next we scan the descriptors, looking for a module descriptor;
+ * there must be exactly one of those.  When found, we record the
+ * information it contains, and then remove that descriptor (and any
+ * string descriptors it refers to) from further consideration.
+ *
+ * After that we look for the module's interfaces--there must be at
+ * least one of those.
+ *
+ * Return a pointer to an initialized gb_module structure
+ * representing the content of the module manifest, or a null
+ * pointer if an error occurs.
+ */
+struct gb_module *gb_manifest_parse(void *data, size_t size)
+{
+	struct greybus_manifest *manifest;
+	struct greybus_manifest_header *header;
+	struct greybus_descriptor *desc;
+	struct manifest_desc *descriptor;
+	struct manifest_desc *module_desc = NULL;
+	struct gb_module *gmod;
+	u16 manifest_size;
+	u32 found = 0;
+
+	/* we have to have at _least_ the manifest header */
+	if (size <= sizeof(manifest->header)) {
+		pr_err("short manifest (%zu)\n", size);
+		return NULL;
+	}
+
+	/* Make sure the size is right */
+	manifest = data;
+	header = &manifest->header;
+	manifest_size = le16_to_cpu(header->size);
+	if (manifest_size != size) {
+		pr_err("manifest size mismatch %zu != %hu\n",
+			size, manifest_size);
+		return NULL;
+	}
+
+	/* Validate major/minor number */
+	if (header->version_major > GREYBUS_VERSION_MAJOR) {
+		pr_err("manifest version too new (%hhu.%hhu > %hhu.%hhu)\n",
+			header->version_major, header->version_minor,
+			GREYBUS_VERSION_MAJOR, GREYBUS_VERSION_MINOR);
+		return NULL;
+	}
+
+	/* OK, find all the descriptors */
+	desc = (struct greybus_descriptor *)(header + 1);
+	size -= sizeof(*header);
+	while (size) {
+		int desc_size;
+
+		desc_size = identify_descriptor(desc, size);
+		if (desc_size <= 0) {
+			if (!desc_size)
+				pr_err("zero-sized manifest descriptor\n");
+			goto out_err;
+		}
+		desc = (struct greybus_descriptor *)((char *)desc + desc_size);
+		size -= desc_size;
+	}
+
+	/* There must be a single module descriptor */
+	list_for_each_entry(descriptor, &manifest_descs, links) {
+		if (descriptor->type == GREYBUS_TYPE_MODULE)
+			if (!found++)
+				module_desc = descriptor;
+	}
+	if (found != 1) {
+		pr_err("manifest must have 1 module descriptor (%u found)\n",
+			found);
+		goto out_err;
+	}
+
+	/* Parse the module manifest, starting with the module descriptor */
+	gmod = gb_manifest_parse_module(module_desc);
+
+	/*
+	 * We really should have no remaining descriptors, but we
+	 * don't know what newer format manifests might leave.
+	 */
+	if (!list_empty(&manifest_descs)) {
+		pr_info("excess descriptors in module manifest\n");
+		release_manifest_descriptors();
+	}
+
+	return gmod;
+out_err:
+	release_manifest_descriptors();
+
+	return NULL;
+}

drivers/staging/greybus/manifest.h

+/*
+ * Greybus module manifest parsing
+ *
+ * Copyright 2014 Google Inc.
+ *
+ * Released under the GPLv2 only.
+ */
+
+#ifndef __MANIFEST_H
+#define __MANIFEST_H
+
+struct gb_module *gb_manifest_parse(void *data, size_t size);
+
+#endif /* __MANIFEST_H */

drivers/staging/greybus/module.c

@@ -14,20 +14,16 @@ static DEFINE_SPINLOCK(gb_modules_lock);
 static int gb_module_match_one_id(struct gb_module *gmod,
 				const struct greybus_module_id *id)
 {
-	struct greybus_descriptor_module *module;
-
-	module = &gmod->module;
-
 	if ((id->match_flags & GREYBUS_DEVICE_ID_MATCH_VENDOR) &&
-	    (id->vendor != le16_to_cpu(module->vendor)))
+	    (id->vendor != gmod->vendor))
 		return 0;
 
 	if ((id->match_flags & GREYBUS_DEVICE_ID_MATCH_PRODUCT) &&
-	    (id->product != le16_to_cpu(module->product)))
+	    (id->product != gmod->product))
 		return 0;
 
 	if ((id->match_flags & GREYBUS_DEVICE_ID_MATCH_SERIAL) &&
-	    (id->serial_number != le64_to_cpu(module->serial_number)))
+	    (id->serial_number != gmod->serial_number))
 		return 0;
 
 	return 1;
@@ -84,6 +80,9 @@ void gb_module_destroy(struct gb_module *module)
 	if (WARN_ON(!module))
 		return;
 
+	kfree(module->product_string);
+	kfree(module->vendor_string);
+
 	spin_lock_irq(&gb_modules_lock);
 	list_del(&module->links);
 	spin_unlock_irq(&gb_modules_lock);

drivers/staging/greybus/module.h

@@ -20,7 +20,14 @@ struct gb_module {
 	struct list_head links;	/* greybus_host_device->modules */
 	u8 module_id;		/* Physical location within the Endo */
 
-	struct greybus_descriptor_module module;
+	/* Information taken from the manifest module descriptor */
+	u16 vendor;
+	u16 product;
+	u16 version;
+	u64 serial_number;
+	char *vendor_string;
+	char *product_string;
+
 	int num_cports;
 	int num_strings;
 	u16 cport_ids[MAX_CPORTS_PER_MODULE];

drivers/staging/greybus/sysfs.c

@@ -26,7 +26,7 @@ static ssize_t module_##field##_show(struct device *dev,		\
 				     char *buf)				\
 {									\
 	struct gb_module *gmod = to_gb_module(dev);			\
-	return sprintf(buf, "%x\n", gmod->module.field);		\
+	return sprintf(buf, "%x\n", gmod->field);			\
 }									\
 static DEVICE_ATTR_RO(module_##field)
 
@@ -40,8 +40,7 @@ static ssize_t module_serial_number_show(struct device *dev,
 {
 	struct gb_module *gmod = to_gb_module(dev);
 
-	return sprintf(buf, "%llX\n",
-		      (unsigned long long)le64_to_cpu(gmod->module.serial_number));
+	return sprintf(buf, "%llX\n", (unsigned long long)gmod->serial_number);
 }
 static DEVICE_ATTR_RO(module_serial_number);
 
@@ -51,8 +50,7 @@ static ssize_t module_vendor_string_show(struct device *dev,
 {
 	struct gb_module *gmod = to_gb_module(dev);
 
-	return sprintf(buf, "%s",
-		       greybus_string(gmod, gmod->module.vendor_stringid));
+	return sprintf(buf, "%s", gmod->vendor_string);
 }
 static DEVICE_ATTR_RO(module_vendor_string);
 
@@ -62,8 +60,7 @@ static ssize_t module_product_string_show(struct device *dev,
 {
 	struct gb_module *gmod = to_gb_module(dev);
 
-	return sprintf(buf, "%s",
-		       greybus_string(gmod, gmod->module.product_stringid));
+	return sprintf(buf, "%s", gmod->product_string);
 }
 static DEVICE_ATTR_RO(module_product_string);
 
@@ -81,21 +78,17 @@ static umode_t module_attrs_are_visible(struct kobject *kobj,
 					struct attribute *a, int n)
 {
 	struct gb_module *gmod = to_gb_module(kobj_to_dev(kobj));
+	umode_t mode = a->mode;
+
+	if (a == &dev_attr_module_vendor_string.attr && gmod->vendor_string)
+		return mode;
+	if (a == &dev_attr_module_product_string.attr && gmod->product_string)
+		return mode;
+	if (gmod->vendor || gmod->product || gmod->version)
+		return mode;
+	if (gmod->serial_number)
+		return mode;
 
-	if ((a == &dev_attr_module_vendor_string.attr) &&
-	    (gmod->module.vendor_stringid))
-		return a->mode;
-	if ((a == &dev_attr_module_product_string.attr) &&
-	    (gmod->module.product_stringid))
-		return a->mode;
-
-	// FIXME - make this a dynamic structure to "know" if it really is here
-	// or not easier?
-	if (gmod->module.vendor ||
-	    gmod->module.product ||
-	    gmod->module.version ||
-	    gmod->module.serial_number)
-		return a->mode;
 	return 0;
 }
 
@@ -104,9 +97,6 @@ static struct attribute_group module_attr_grp = {
 	.is_visible =	module_attrs_are_visible,
 };
 
-
-
-
 const struct attribute_group *greybus_module_groups[] = {
 	&module_attr_grp,
 	NULL,