[PATCHv2 1/5] usb: gadget: f_fs: resurect usb_functionfs_descs_head structure

[PATCHv2 1/5] usb: gadget: f_fs: resurect usb_functionfs_descs_head structure

[Michal Nazarewicz]

Even though usb_functionfs_descs_head structure is now deprecated,
it has been used by some user space tools.  Its removel in commit
[ac8dde1: “Add flags to descriptors block”] was an oversight
leading to build breakage for such tools.

Bring it back so that old user space tools can still be build
without problems on newer kernel versions.

Reported-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Reported-by: Krzysztof Opasiak <k.opasiak@samsung.com>
Cc: <stable@vger.kernel.org>  # 3.14
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/uapi/linux/usb/functionfs.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 2a4b4a7..24b68c5 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -33,6 +33,13 @@ struct usb_endpoint_descriptor_no_audio {
 	__u8  bInterval;
 } __attribute__((packed));
 
+/* Legacy format, deprecated as of 3.14. */
+struct usb_functionfs_descs_head {
+	__le32 magic;
+	__le32 length;
+	__le32 fs_count;
+	__le32 hs_count;
+} __attribute__((packed, deprecated));
 
 /*
  * Descriptors format:
-- 
2.0.0.526.g5318336

[PATCHv2 2/5] tools: ffs-test: fix header values endianess

[Michal Nazarewicz]

It appears that no one ever run ffs-test on a big-endian machine,
since it used cpu-endianess for fs_count and hs_count fields which
should be in little-endian format.  Fix by wrapping the numbers in
cpu_to_le32.

Cc: stable@vger.kernel.org
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 tools/usb/ffs-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
index fe1e66b..a87e99f 100644
--- a/tools/usb/ffs-test.c
+++ b/tools/usb/ffs-test.c
@@ -116,8 +116,8 @@ static const struct {
 	.header = {
 		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC),
 		.length = cpu_to_le32(sizeof descriptors),
-		.fs_count = 3,
-		.hs_count = 3,
+		.fs_count = cpu_to_le32(3),
+		.hs_count = cpu_to_le32(3),
 	},
 	.fs_descs = {
 		.intf = {
-- 
2.0.0.526.g5318336

[PATCHv2 3/5] usb: gadget: f_fs: add usb_functionfs_descs_head_v2 structure

[Michal Nazarewicz]

The structure can be used with user space tools that use the new
functionfs description format, for example as follows:

static const struct {
	struct usb_functionfs_descs_head_v2 header;
	__le32 fs_count;
	__le32 hs_count;
	struct {
		…
	} fs_desc;
	struct {
		…
	} hs_desc;
} descriptors = {
	.header = {
		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC_V2),
		.length = cpu_to_le32(sizeof(descriptors)),
		.flags = cpu_to_le32(FUNCTIONFS_HAS_FS_DESC |
				     FUNCTIONFS_HAS_HS_DESC)
	},
	.fs_count = cpu_to_le32(X),
	.fs_desc = {
		…
	},
	.hs_count = cpu_to_le32(Y),
	.hs_desc = {
		…
	}
};

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/uapi/linux/usb/functionfs.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 24b68c5..2592d86 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -33,6 +33,16 @@ struct usb_endpoint_descriptor_no_audio {
 	__u8  bInterval;
 } __attribute__((packed));
 
+struct usb_functionfs_descs_head_v2 {
+	__le32 magic;
+	__le32 length;
+	__le32 flags;
+	/*
+	 * __le32 fs_count, hs_count, fs_count; must be included manually in
+	 * the structure taking flags into consideration.
+	 */
+} __attribute__((packed));
+
 /* Legacy format, deprecated as of 3.14. */
 struct usb_functionfs_descs_head {
 	__le32 magic;
-- 
2.0.0.526.g5318336

[PATCHv2 4/5] tools: ffs-test: convert to new descriptor format

[Michal Nazarewicz]

Since commit [ac8dde11: “Add flags to descriptors block”] functionfs
supports a new, more powerful and extensible, descriptor format.
Since ffs-test is probably the first thing users of the functionfs
interface see when they start writing functionfs user space daemons,
convert it to use the new format thus promoting it.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/uapi/linux/usb/functionfs.h |  2 +-
 tools/usb/ffs-test.c                | 14 +++++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 2592d86..1dc473a 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -70,7 +70,7 @@ struct usb_functionfs_descs_head {
  * structure.  Any flags that are not recognised cause the whole block to be
  * rejected with -ENOSYS.
  *
- * Legacy descriptors format:
+ * Legacy descriptors format (deprecated as of 3.14):
  *
  * | off | name      | type         | description                          |
  * |-----+-----------+--------------+--------------------------------------|
diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
index a87e99f..708d317 100644
--- a/tools/usb/ffs-test.c
+++ b/tools/usb/ffs-test.c
@@ -1,5 +1,5 @@
 /*
- * ffs-test.c.c -- user mode filesystem api for usb composite function
+ * ffs-test.c -- user mode filesystem api for usb composite function
  *
  * Copyright (C) 2010 Samsung Electronics
  *                    Author: Michal Nazarewicz <mina86@mina86.com>
@@ -106,7 +106,9 @@ static void _msg(unsigned level, const char *fmt, ...)
 /******************** Descriptors and Strings *******************************/
 
 static const struct {
-	struct usb_functionfs_descs_head header;
+	struct usb_functionfs_descs_head_v2 header;
+	__le32 fs_count;
+	__le32 hs_count;
 	struct {
 		struct usb_interface_descriptor intf;
 		struct usb_endpoint_descriptor_no_audio sink;
@@ -114,11 +116,12 @@ static const struct {
 	} __attribute__((packed)) fs_descs, hs_descs;
 } __attribute__((packed)) descriptors = {
 	.header = {
-		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC),
+		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC_V2),
+		.flags = cpu_to_le32(FUNCTIONFS_HAS_FS_DESC |
+				     FUNCTIONFS_HAS_HS_DESC),
 		.length = cpu_to_le32(sizeof descriptors),
-		.fs_count = cpu_to_le32(3),
-		.hs_count = cpu_to_le32(3),
 	},
+	.fs_count = cpu_to_le32(3),
 	.fs_descs = {
 		.intf = {
 			.bLength = sizeof descriptors.fs_descs.intf,
@@ -142,6 +145,7 @@ static const struct {
 			/* .wMaxPacketSize = autoconfiguration (kernel) */
 		},
 	},
+	.hs_count = cpu_to_le32(3),
 	.hs_descs = {
 		.intf = {
 			.bLength = sizeof descriptors.fs_descs.intf,
-- 
2.0.0.526.g5318336

[PATCHv2 5/5] tools: ffs-test: add compatibility code for old kernels

[Michal Nazarewicz]

If ffs-test is used with a kernel prior to 3.14, which do not
support the new descriptors format, it will fail when trying to
write the descriptors.  Add a function that converts the new
descriptors to the legacy ones and use it to retry writing the
descriptors using the legacy format.

Also add “-l” flag to ffs-test which will cause the tool to
never try the new format and instead immediatelly try the
legacy one.  This should be useful to test whether parsing
of the old format still works on given 3.14+ kernel.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 tools/usb/ffs-test.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 79 insertions(+), 5 deletions(-)

diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
index 708d317..407c828 100644
--- a/tools/usb/ffs-test.c
+++ b/tools/usb/ffs-test.c
@@ -29,6 +29,7 @@
 #include <fcntl.h>
 #include <pthread.h>
 #include <stdarg.h>
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -172,6 +173,61 @@ static const struct {
 	},
 };
 
+static size_t descs_to_legacy(void **legacy, const void *descriptors) {
+	__u32 length, flags, fs_count = 0, hs_count = 0, count;
+	const unsigned char *descs = descriptors, *usb_descs;
+	unsigned char *out;
+
+	if (get_unaligned_le32(descs) != FUNCTIONFS_DESCRIPTORS_MAGIC_V2)
+		return 0;
+
+	length = get_unaligned_le32(descs + 4);
+	if (length < 8)
+		return 0;
+	descs += 8;
+	length -= 8;
+
+#define GET_LE32(ret) do {				\
+		if (length < 4)				\
+			return 0;			\
+		ret = get_unaligned_le32(descs);	\
+		descs += 4;				\
+		length -= 4;				\
+	} while (0)
+
+	GET_LE32(flags);
+	if (flags & FUNCTIONFS_HAS_FS_DESC)
+		GET_LE32(fs_count);
+	if (flags & FUNCTIONFS_HAS_HS_DESC)
+		GET_LE32(hs_count);
+	if (!fs_count && !hs_count)
+		return 0;
+	if (flags & FUNCTIONFS_HAS_SS_DESC)
+		GET_LE32(count);  /* The value is ignored later on anyway. */
+	if (flags)
+		return 0;
+
+#undef GET_LE32
+
+	usb_descs = descs;
+	for (count = fs_count + hs_count; count; --count) {
+		if (length < *descs)
+			return 0;
+		length -= *descs;
+		descs += *descs;
+	}
+
+	length = 16 + (descs - usb_descs);
+	out = malloc(length);
+	put_unaligned_le32(FUNCTIONFS_DESCRIPTORS_MAGIC, out);
+	put_unaligned_le32(length, out + 4);
+	put_unaligned_le32(fs_count, out + 8);
+	put_unaligned_le32(hs_count, out + 12);
+	memcpy(out + 16, usb_descs, length - 16);
+	*legacy = out;
+	return length;
+}
+
 
 #define STR_INTERFACE_ "Source/Sink"
 
@@ -491,12 +547,29 @@ ep0_consume(struct thread *ignore, const void *buf, size_t nbytes)
 	return nbytes;
 }
 
-static void ep0_init(struct thread *t)
+static void ep0_init(struct thread *t, bool legacy_descriptors)
 {
+	void *legacy;
 	ssize_t ret;
+	size_t len;
+
+	if (legacy_descriptors) {
+		info("%s: writing descriptors\n", t->filename);
+		goto legacy;
+	}
 
-	info("%s: writing descriptors\n", t->filename);
+	info("%s: writing descriptors (in v2 format)\n", t->filename);
 	ret = write(t->fd, &descriptors, sizeof descriptors);
+
+	if (ret < 0 && errno == EINVAL) {
+		warn("%s: new format rejected, trying legacy\n", t->filename);
+legacy:
+		len = descs_to_legacy(&legacy, &descriptors);
+		if (len) {
+			ret = write(t->fd, legacy, len);
+			free(legacy);
+		}
+	}
 	die_on(ret < 0, "%s: write: descriptors", t->filename);
 
 	info("%s: writing strings\n", t->filename);
@@ -507,14 +580,15 @@ static void ep0_init(struct thread *t)
 
 /******************** Main **************************************************/
 
-int main(void)
+int main(int argc, char **argv)
 {
+	bool legacy_descriptors;
 	unsigned i;
 
-	/* XXX TODO: Argument parsing missing */
+	legacy_descriptors = argc > 2 && !strcmp(argv[1], "-l");
 
 	init_thread(threads);
-	ep0_init(threads);
+	ep0_init(threads, legacy_descriptors);
 
 	for (i = 1; i < sizeof threads / sizeof *threads; ++i)
 		init_thread(threads + i);
-- 
2.0.0.526.g5318336

RE: [PATCHv2 5/5] tools: ffs-test: add compatibility code for old kernels

[Krzysztof Opasiak]

Hi Michal,

> -----Original Message-----
> From: Michal Nazarewicz [mailto:mina86@mina86.com]
> Sent: Friday, June 06, 2014 11:13 AM
> To: Felipe Balbi; Krzysztof Opasiak
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Michal
> Nazarewicz
> Subject: [PATCHv2 5/5] tools: ffs-test: add compatibility code for
> old kernels

(... snip ...)

> +static size_t descs_to_legacy(void **legacy, const void
> *descriptors) {
> +	__u32 length, flags, fs_count = 0, hs_count = 0, count;
> +	const unsigned char *descs = descriptors, *usb_descs;
> +	unsigned char *out;
> +
> +	if (get_unaligned_le32(descs) !=
> FUNCTIONFS_DESCRIPTORS_MAGIC_V2)
> +		return 0;
> +
> +	length = get_unaligned_le32(descs + 4);
> +	if (length < 8)
> +		return 0;
> +	descs += 8;
> +	length -= 8;
> +
> +#define GET_LE32(ret) do {				\
> +		if (length < 4)				\
> +			return 0;			\
> +		ret = get_unaligned_le32(descs);	\
> +		descs += 4;				\
> +		length -= 4;				\
> +	} while (0)
> +
> +	GET_LE32(flags);
> +	if (flags & FUNCTIONFS_HAS_FS_DESC)
> +		GET_LE32(fs_count);
> +	if (flags & FUNCTIONFS_HAS_HS_DESC)
> +		GET_LE32(hs_count);
> +	if (!fs_count && !hs_count)
> +		return 0;
> +	if (flags & FUNCTIONFS_HAS_SS_DESC)
> +		GET_LE32(count);  /* The value is ignored later on
> anyway. */
> +	if (flags)
> +		return 0;

As far as I understand you are taking the flags from descriptor and then test them with possible ffs speed flags, after getting flags your are not assigning anything to this variable so in this place it will be != 0  unless none of the flags has been provided. I don't think this is intended behavior.

> +
> +#undef GET_LE32
> +
> +	usb_descs = descs;
> +	for (count = fs_count + hs_count; count; --count) {
> +		if (length < *descs)
> +			return 0;
> +		length -= *descs;
> +		descs += *descs;
> +	}
> +
> +	length = 16 + (descs - usb_descs);
> +	out = malloc(length);
> +	put_unaligned_le32(FUNCTIONFS_DESCRIPTORS_MAGIC, out);
> +	put_unaligned_le32(length, out + 4);
> +	put_unaligned_le32(fs_count, out + 8);
> +	put_unaligned_le32(hs_count, out + 12);
> +	memcpy(out + 16, usb_descs, length - 16);
> +	*legacy = out;
> +	return length;
> +}
> +

As this is an example which will be copy-paste by many users maybe you should you struct usb_functionfs_descs_head and struct usb_functionfs_descs_head_v2 instead of direct operations using hard-coded offsets to make this function more readable?

--
BR's
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak@samsung.com

[PATCHv3] tools: ffs-test: add compatibility code for old kernels

[Michal Nazarewicz]

If ffs-test is used with a kernel prior to 3.14, which do not
support the new descriptors format, it will fail when trying to
write the descriptors.  Add a function that converts the new
descriptors to the legacy ones and use it to retry writing the
descriptors using the legacy format.

Also add “-l” flag to ffs-test which will cause the tool to
never try the new format and instead immediatelly try the
legacy one.  This should be useful to test whether parsing
of the old format still works on given 3.14+ kernel.
---
 tools/usb/ffs-test.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 5 deletions(-)

diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
index 708d317..afd69ea 100644
--- a/tools/usb/ffs-test.c
+++ b/tools/usb/ffs-test.c
@@ -29,6 +29,7 @@
 #include <fcntl.h>
 #include <pthread.h>
 #include <stdarg.h>
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -172,6 +173,82 @@ static const struct {
 	},
 };
 
+static size_t descs_to_legacy(void **legacy, const void *descriptors) {
+	__u32 length, flags, fs_count = 0, hs_count = 0, count;
+	const unsigned char *descs, *usb_descs;
+
+	/* Read v2 header */
+	{
+		const struct usb_functionfs_descs_head_v2 *head = descriptors;
+
+		if (le32_to_cpu(head->magic) !=
+		    FUNCTIONFS_DESCRIPTORS_MAGIC_V2)
+			return 0;
+		length = le32_to_cpu(head->length);
+		if (length <= sizeof *head)
+			return 0;
+		flags = le32_to_cpu(head->flags);
+		descs = (const void *)(head + 1);
+		length -= sizeof *head;
+	}
+
+	/* Read v2 counts */
+#define GET_LE32(ret) do {					\
+		if (length < 4)					\
+			return 0;				\
+		ret = le32_to_cpu(*(const __le32 *)descs);	\
+		descs += 4;					\
+		length -= 4;					\
+	} while (0)
+
+	if (flags & FUNCTIONFS_HAS_FS_DESC)
+		GET_LE32(fs_count);
+	if (flags & FUNCTIONFS_HAS_HS_DESC)
+		GET_LE32(hs_count);
+	if (!fs_count && !hs_count)
+		return 0;
+	if (flags & FUNCTIONFS_HAS_SS_DESC)
+		/* Legacy does not support SS so this is ignored. */
+		GET_LE32(count);
+	if (flags & ~(FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC |
+		      FUNCTIONFS_HAS_SS_DESC))
+		return 0;
+
+#undef GET_LE32
+
+	/*
+	 * Find the end of FS and HS USB descriptors.  SS descriptors
+	 * are ignored since legacy format does not support them.
+	 */
+	count = fs_count + hs_count;
+	usb_descs = descs;
+	do {
+		if (length < *descs)
+			return 0;
+		length -= *descs;
+		descs += *descs;
+	} while (--count);
+
+	/* Allocate legacy descriptors and copy the data. */
+	{
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+		struct usb_functionfs_descs_head *head;
+#pragma GCC diagnostic pop
+
+		length = sizeof *head + (descs - usb_descs);
+		head = malloc(length);
+		head->magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC);
+		head->length = cpu_to_le32(length);
+		head->fs_count = cpu_to_le32(fs_count);
+		head->hs_count = cpu_to_le32(hs_count);
+		memcpy(head + 1, usb_descs, descs - usb_descs);
+		*legacy = head;
+	}
+
+	return length;
+}
+
 
 #define STR_INTERFACE_ "Source/Sink"
 
@@ -491,12 +568,29 @@ ep0_consume(struct thread *ignore, const void *buf, size_t nbytes)
 	return nbytes;
 }
 
-static void ep0_init(struct thread *t)
+static void ep0_init(struct thread *t, bool legacy_descriptors)
 {
+	void *legacy;
 	ssize_t ret;
+	size_t len;
+
+	if (legacy_descriptors) {
+		info("%s: writing descriptors\n", t->filename);
+		goto legacy;
+	}
 
-	info("%s: writing descriptors\n", t->filename);
+	info("%s: writing descriptors (in v2 format)\n", t->filename);
 	ret = write(t->fd, &descriptors, sizeof descriptors);
+
+	if (ret < 0 && errno == EINVAL) {
+		warn("%s: new format rejected, trying legacy\n", t->filename);
+legacy:
+		len = descs_to_legacy(&legacy, &descriptors);
+		if (len) {
+			ret = write(t->fd, legacy, len);
+			free(legacy);
+		}
+	}
 	die_on(ret < 0, "%s: write: descriptors", t->filename);
 
 	info("%s: writing strings\n", t->filename);
@@ -507,14 +601,15 @@ static void ep0_init(struct thread *t)
 
 /******************** Main **************************************************/
 
-int main(void)
+int main(int argc, char **argv)
 {
+	bool legacy_descriptors;
 	unsigned i;
 
-	/* XXX TODO: Argument parsing missing */
+	legacy_descriptors = argc > 2 && !strcmp(argv[1], "-l");
 
 	init_thread(threads);
-	ep0_init(threads);
+	ep0_init(threads, legacy_descriptors);
 
 	for (i = 1; i < sizeof threads / sizeof *threads; ++i)
 		init_thread(threads + i);
-- 
2.0.0.526.g5318336

Re: [PATCHv2 5/5] tools: ffs-test: add compatibility code for old kernels

[Michal Nazarewicz]

On Mon, Jun 09 2014, Krzysztof Opasiak wrote:
> As this is an example which will be copy-paste by many users maybe you
> should you struct usb_functionfs_descs_head and struct
> usb_functionfs_descs_head_v2 instead of direct operations using
> hard-coded offsets to make this function more readable?

v3 uses usb_functionfs_descs_head{_v2,} instead of hard-coded offsets.
I also started wondering if it would make sense to go one step further
and define a temporary structure holding the headers.  Something like:

----------- >8 ---------------------------------------------------------
	/* Read v2 header */
	{
		const struct {
			const struct usb_functionfs_descs_head_v2 header;
			const __le32 counts[];
		} __attribute__((packed)) *const in = descriptors;
		const __le32 *counts = in->counts;
		__u32 flags;

		if (le32_to_cpu(in->header.magic) !=
		    FUNCTIONFS_DESCRIPTORS_MAGIC_V2)
			return 0;
		length = le32_to_cpu(in->header.length);
		if (length <= sizeof in->header)
			return 0;
		length -= sizeof in->header;
		flags = le32_to_cpu(in->header.flags);
		if (flags & ~(FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC |
			      FUNCTIONFS_HAS_SS_DESC))
			return 0;

#define GET_NEXT_COUNT_IF_FLAG(ret, flg) do {		\
			if (!(flags & (flg)))		\
				break;			\
			if (length < 4)			\
				return 0;		\
			ret = le32_to_cpu(*counts);	\
			length -= 4;			\
			++counts;			\
		} while (0)

		GET_NEXT_COUNT_IF_FLAG(fs_count, FUNCTIONFS_HAS_FS_DESC);
		GET_NEXT_COUNT_IF_FLAG(hs_count, FUNCTIONFS_HAS_HS_DESC);
		GET_NEXT_COUNT_IF_FLAG(count, FUNCTIONFS_HAS_SS_DESC);

		count = fs_count + hs_count;
		if (!count)
			return 0;
		descs_start = descs_end = (const void *)counts;

#undef GET_NEXT_COUNT_IF_FLAG
	}

----------- >8 ---------------------------------------------------------

	/* Allocate legacy descriptors and copy the data. */
	{
		struct {
			struct usb_functionfs_descs_head header;
			__u8 descriptors[];
		} __attribute__((packed)) *out;

		length = sizeof out->header + (descs_end - descs_start);
		out = malloc(length);
		out->header.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC);
		out->header.length = cpu_to_le32(length);
		out->header.fs_count = cpu_to_le32(fs_count);
		out->header.hs_count = cpu_to_le32(hs_count);
		memcpy(out->descriptors, descs_start, descs_end - descs_start);
		*legacy = out;
	}
----------- >8 ---------------------------------------------------------

Thoughts?

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

RE: [PATCHv2 5/5] tools: ffs-test: add compatibility code for old kernels

[Krzysztof Opasiak]

Hi,

> -----Original Message-----
> From: Michal Nazarewicz [mailto:mpn@google.com] On Behalf Of Michal
> Nazarewicz
> Sent: Monday, June 09, 2014 3:28 PM
> To: Krzysztof Opasiak; 'Felipe Balbi'; Krzysztof Opasiak
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCHv2 5/5] tools: ffs-test: add compatibility code
> for old kernels
> 
> On Mon, Jun 09 2014, Krzysztof Opasiak wrote:
> > As this is an example which will be copy-paste by many users
> maybe you
> > should you struct usb_functionfs_descs_head and struct
> > usb_functionfs_descs_head_v2 instead of direct operations using
> > hard-coded offsets to make this function more readable?
> 
> v3 uses usb_functionfs_descs_head{_v2,} instead of hard-coded
> offsets.
> I also started wondering if it would make sense to go one step
> further
> and define a temporary structure holding the headers.  Something
> like:
> 
> ----------- >8 ----------------------------------------------------
> -----
> 	/* Read v2 header */
> 	{
> 		const struct {
> 			const struct usb_functionfs_descs_head_v2 header;
> 			const __le32 counts[];
> 		} __attribute__((packed)) *const in = descriptors;
> 		const __le32 *counts = in->counts;
> 		__u32 flags;
> 
> 		if (le32_to_cpu(in->header.magic) !=
> 		    FUNCTIONFS_DESCRIPTORS_MAGIC_V2)
> 			return 0;
> 		length = le32_to_cpu(in->header.length);
> 		if (length <= sizeof in->header)
> 			return 0;
> 		length -= sizeof in->header;
> 		flags = le32_to_cpu(in->header.flags);
> 		if (flags & ~(FUNCTIONFS_HAS_FS_DESC |
> FUNCTIONFS_HAS_HS_DESC |
> 			      FUNCTIONFS_HAS_SS_DESC))
> 			return 0;
> 
> #define GET_NEXT_COUNT_IF_FLAG(ret, flg) do {		\
> 			if (!(flags & (flg)))		\
> 				break;			\
> 			if (length < 4)			\
> 				return 0;		\
> 			ret = le32_to_cpu(*counts);	\
> 			length -= 4;			\
> 			++counts;			\
> 		} while (0)
> 
> 		GET_NEXT_COUNT_IF_FLAG(fs_count,
> FUNCTIONFS_HAS_FS_DESC);
> 		GET_NEXT_COUNT_IF_FLAG(hs_count,
> FUNCTIONFS_HAS_HS_DESC);
> 		GET_NEXT_COUNT_IF_FLAG(count, FUNCTIONFS_HAS_SS_DESC);
> 
> 		count = fs_count + hs_count;
> 		if (!count)
> 			return 0;
> 		descs_start = descs_end = (const void *)counts;
> 
> #undef GET_NEXT_COUNT_IF_FLAG
> 	}
> 
> ----------- >8 ----------------------------------------------------
> -----
> 
> 	/* Allocate legacy descriptors and copy the data. */
> 	{
> 		struct {
> 			struct usb_functionfs_descs_head header;
> 			__u8 descriptors[];
> 		} __attribute__((packed)) *out;
> 
> 		length = sizeof out->header + (descs_end -
> descs_start);
> 		out = malloc(length);
> 		out->header.magic =
> cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC);
> 		out->header.length = cpu_to_le32(length);
> 		out->header.fs_count = cpu_to_le32(fs_count);
> 		out->header.hs_count = cpu_to_le32(hs_count);
> 		memcpy(out->descriptors, descs_start, descs_end -
> descs_start);
> 		*legacy = out;
> 	}
> ----------- >8 ----------------------------------------------------
> -----
> 
> Thoughts?

Looks very good to me! In my opinion this one should be used, it emphasizes your intentions very well so it's perfect code for sample app.

--
BR's
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak@samsung.com