[PATCH 1/2] usb: gadget: f_fs: resurect usb_functionfs_descs_head structure

[PATCH 1/2] 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.  It's 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.

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..92c94e8 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));
 
 /*
  * Descriptors format:
-- 
2.0.0.526.g5318336

[PATCH 2/2] tools: ffs-test: convert to new descriptor format fixing compilation error

[Michal Nazarewicz]

Commit [ac8dde11: “usb: gadget: f_fs: Add flags to descriptors block”]
which introduced a new descriptor format for FunctionFS removed the
usb_functionfs_descs_head structure, which is still used by ffs-test.
tool.

Convert ffs-test by converting it to use the new header format.  For
testing kernels prior to 3.14 (when the new format was introduced) and
parsing of the legacy headers in the new kernels, provide a compilation
flag to make the tool use the old format.

Finally, include information as to when the legacy FunctionFS headers
format has been deprecated (which is also when the new one has been
introduced).

Reported-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Reported-by: Krzysztof Opasiak <k.opasiak@samsung.com>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/uapi/linux/usb/functionfs.h |  2 +-
 tools/usb/Makefile                  |  6 +++++-
 tools/usb/ffs-test.c                | 20 ++++++++++++++++++--
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 92c94e8..d54ba5d 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -60,7 +60,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/Makefile b/tools/usb/Makefile
index acf2165..d576b3b 100644
--- a/tools/usb/Makefile
+++ b/tools/usb/Makefile
@@ -6,7 +6,11 @@ WARNINGS = -Wall -Wextra
 CFLAGS = $(WARNINGS) -g -I../include
 LDFLAGS = $(PTHREAD_LIBS)
 
-all: testusb ffs-test
+all: testusb ffs-test ffs-test-legacy
+
+ffs-test-legacy: ffs-test.c
+	$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) -DUSE_LEGACY_DESC_HEAD
+
 %: %.c
 	$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS)
 
diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
index fe1e66b..74b353d 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>
@@ -21,6 +21,8 @@
 
 /* $(CROSS_COMPILE)cc -Wall -Wextra -g -o ffs-test ffs-test.c -lpthread */
 
+/* Uncomment to make the tool use legacy FFS descriptor headers. */
+/* #define USE_LEGACY_DESC_HEAD */
 
 #define _BSD_SOURCE /* for endian.h */
 
@@ -106,7 +108,15 @@ static void _msg(unsigned level, const char *fmt, ...)
 /******************** Descriptors and Strings *******************************/
 
 static const struct {
-	struct usb_functionfs_descs_head header;
+	struct {
+		__le32 magic;
+		__le32 length;
+#ifndef USE_LEGACY_DESC_HEAD
+		__le32 flags;
+#endif
+		__le32 fs_count;
+		__le32 hs_count;
+	} __attribute__((packed)) header;
 	struct {
 		struct usb_interface_descriptor intf;
 		struct usb_endpoint_descriptor_no_audio sink;
@@ -114,7 +124,13 @@ static const struct {
 	} __attribute__((packed)) fs_descs, hs_descs;
 } __attribute__((packed)) descriptors = {
 	.header = {
+#ifdef USE_LEGACY_DESC_HEAD
 		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC),
+#else
+		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC_V2),
+		.flags = cpu_to_le32(FUNCTIONFS_HAS_FS_DESC |
+				     FUNCTIONFS_HAS_HS_DESC),
+#endif
 		.length = cpu_to_le32(sizeof descriptors),
 		.fs_count = 3,
 		.hs_count = 3,
-- 
2.0.0.526.g5318336

RE: [PATCH 2/2] tools: ffs-test: convert to new descriptor format fixing compilation error

[Krzysztof Opasiak]

Hi,

> -----Original Message-----
> From: Michal Nazarewicz [mailto:mina86@mina86.com]
> Sent: Thursday, June 05, 2014 9:43 AM
> To: Felipe Balbi
> Cc: Krzysztof Opasiak; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; Michal Nazarewicz
> Subject: [PATCH 2/2] tools: ffs-test: convert to new descriptor
> format fixing compilation error
> 
> Commit [ac8dde11: “usb: gadget: f_fs: Add flags to descriptors
> block”]
> which introduced a new descriptor format for FunctionFS removed the
> usb_functionfs_descs_head structure, which is still used by ffs-
> test.
> tool.
> 
> Convert ffs-test by converting it to use the new header format.
> For
> testing kernels prior to 3.14 (when the new format was introduced)
> and
> parsing of the legacy headers in the new kernels, provide a
> compilation
> flag to make the tool use the old format.
> 
> Finally, include information as to when the legacy FunctionFS
> headers
> format has been deprecated (which is also when the new one has been
> introduced).
> 
> Reported-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Reported-by: Krzysztof Opasiak <k.opasiak@samsung.com>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

(...)

> 
>  static const struct {
> -	struct usb_functionfs_descs_head header;
> +	struct {
> +		__le32 magic;
> +		__le32 length;
> +#ifndef USE_LEGACY_DESC_HEAD
> +		__le32 flags;
> +#endif
> +		__le32 fs_count;
> +		__le32 hs_count;
> +	} __attribute__((packed)) header;

I would suggest adding a suitable structure as you described in previous discussion[1]. Writing first 3 fields in each userspace program doesn't look quite good. Using:

#ifndef USE_LEGACY_DESC_HEAD
struct {
	struct usb_functionfs_desc_head2 header;
	__le32 fs_count
	(... and rest according to flags ...)
} __attribute__((packed)) header;
#else ...

Would be shorter, more flexible and user friendly. Moreover it gives less places for mistake (writing fields in wrong order).

Footnotes:
1 - http://marc.info/?l=linux-usb&m=140190878901586&w=2


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

Re: [PATCH 2/2] tools: ffs-test: convert to new descriptor format fixing compilation error

[Michal Nazarewicz]

On Thu, Jun 05 2014, Krzysztof Opasiak <k.opasiak@samsung.com> wrote:
> I would suggest adding a suitable structure as you described in
> previous discussion[1]. Writing first 3 fields in each userspace
> program doesn't look quite good. Using:
>
> #ifndef USE_LEGACY_DESC_HEAD
> struct {
> 	struct usb_functionfs_desc_head2 header;
> 	__le32 fs_count
> 	(... and rest according to flags ...)
> } __attribute__((packed)) header;
> #else ...
>
> Would be shorter, more flexible and user friendly. Moreover it gives
> less places for mistake (writing fields in wrong order).

For ffs-test with USE_LEGACY_DESC_HEAD support it would be longer.
Compare:

----------- >8 ---------------------------------------------------------
static const struct {
	struct {
		__le32 magic;
		__le32 length;
#ifndef USE_LEGACY_DESC_HEAD
		__le32 flags;
#endif
		__le32 fs_count;
		__le32 hs_count;
	} __attribute__((packed)) header;
	/* … */
} __attribute__((packed)) descriptors = {
	.header = {
#ifdef USE_LEGACY_DESC_HEAD
		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC),
#else
		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC_V2),
		.flags = cpu_to_le32(FUNCTIONFS_HAS_FS_DESC |
				     FUNCTIONFS_HAS_HS_DESC),
#endif
		.length = cpu_to_le32(sizeof descriptors),
		.fs_count = 3,
		.hs_count = 3,
	},
	/* … */
};
----------- >8 ---------------------------------------------------------

with

----------- >8 ---------------------------------------------------------
static const struct {
#ifdef USE_LEGACY_DESC_HEAD
	struct usb_functionfs_descs_head header;
#else
	struct usb_functionfs_descs_head2 header;
	__le32 fs_count;
	__le32 hs_count;
#endif
	/* … */
} __attribute__((packed)) descriptors = {
#ifndef USE_LEGACY_DESC_HEAD
	.fs_count = 3,
	.hs_count = 3,
	.header = {
		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC_V2),
		.flags = cpu_to_le32(FUNCTIONFS_HAS_FS_DESC |
				     FUNCTIONFS_HAS_HS_DESC),
#else
		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC),
		.fs_count = 3,
		.hs_count = 3,
#endif
		.length = cpu_to_le32(sizeof descriptors),
	},
	/* … */
};
----------- >8 ---------------------------------------------------------

or with

----------- >8 ---------------------------------------------------------
static const struct {
#ifdef USE_LEGACY_DESC_HEAD
	struct usb_functionfs_descs_head header;
#else
	struct {
		struct usb_functionfs_descs_head2 header;
		__le32 fs_count;
		__le32 hs_count;
	} header;
#endif
	/* … */
} __attribute__((packed)) descriptors = {
	.header = {
#ifdef USE_LEGACY_DESC_HEAD
		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC),
		.length = cpu_to_le32(sizeof descriptors),
#else
		.header = {
			.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),
		},
#endif
		.fs_count = 3,
		.hs_count = 3,
	},
	/* … */
};
----------- >8 ---------------------------------------------------------

The second one uses an unreadable hack to match length of the first one
and the third one is two lines longer.  On top of that, the second and
third produce different structures, use more complicated preprocessing
directives, and repeat value of fs_count and hs_count twice.

Without legacy support, it would indeed be shorter (by two lines), but
would lead to awkward structures:

----------- >8 ---------------------------------------------------------
struct {
	struct usb_functionfs_descs_head2 header;
	/* Why aren't the two below fields inside of a header? */
	__le32 fs_count;  
	__le32 hs_count;
};

struct {
	struct {
		/* Why is there a header.header field? */
		struct usb_functionfs_descs_head2 header;
		__le32 fs_count;  
		__le32 hs_count;
	};
};
----------- >8 ---------------------------------------------------------

And I don't see how it would be more flexible.  If anything, it would be
less.

I understand, and share, your sentiment, but I don't think adding 
usb_functionfs_descs_head2 structure with magic, flags and length would
improve the situation.

Something I thought about shortly was:

----------- >8 ---------------------------------------------------------
#define USB_FFS_DESCS_HEAD(_flags) struct {				\
		__le32 magic, length, flags;				\
		__le32 data[!!(_flags & FUNCTIONFS_HAS_FS_DESC) +	\
			    !!(_flags & FUNCTIONFS_HAS_HS_DESC) +	\
			    !!(_flags & FUNCTIONFS_HAS_SS_DESC)];	\
	} __attribute__((packed))

#define USB_FFS_DESCS_HEAD_INIT(_flags, length, ...) {			\
		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC_V2),	\
		.flags = cpu_to_le32(flags),				\
		.langth = cpu_to_le32(length),				\
		.data = { __VA_ARGS__ }					\
	}

static const struct {
	USB_FFS_DESCS_HEAD(FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC),
	/* … */
} __attribute__((packed)) descriptors = {
	USB_FFS_DESCS_HEAD_INIT(FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC,
				sizeof(descriptors), 3, 3),
	/* … */
};
----------- >8 ---------------------------------------------------------

But whether this is nicer is arguable as well.

-- 
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: [PATCH 2/2] tools: ffs-test: convert to new descriptor format fixing compilation error

[Krzysztof Opasiak]

Hi,

> -----Original Message-----
> From: Michal Nazarewicz [mailto:mpn@google.com] On Behalf Of Michal
> Nazarewicz
> Sent: Thursday, June 05, 2014 1:56 PM
> To: Krzysztof Opasiak; 'Felipe Balbi'
> Cc: Krzysztof Opasiak; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] tools: ffs-test: convert to new descriptor
> format fixing compilation error
> 
> On Thu, Jun 05 2014, Krzysztof Opasiak <k.opasiak@samsung.com>
> wrote:
> > I would suggest adding a suitable structure as you described in
> > previous discussion[1]. Writing first 3 fields in each userspace
> > program doesn't look quite good. Using:
> >
> > #ifndef USE_LEGACY_DESC_HEAD
> > struct {
> > 	struct usb_functionfs_desc_head2 header;
> > 	__le32 fs_count
> > 	(... and rest according to flags ...)
> > } __attribute__((packed)) header;
> > #else ...
> >
> > Would be shorter, more flexible and user friendly. Moreover it
> gives
> > less places for mistake (writing fields in wrong order).
> 
> For ffs-test with USE_LEGACY_DESC_HEAD support it would be longer.
> Compare:

(... snip ...)

> 
> The second one uses an unreadable hack to match length of the first
> one
> and the third one is two lines longer.  On top of that, the second
> and
> third produce different structures, use more complicated
> preprocessing
> directives, and repeat value of fs_count and hs_count twice.
> 
> Without legacy support, it would indeed be shorter (by two lines),

Yes and I was talking about apps which use only new API without compatibility layer.

I agree that when providing compatibility layer it is more complicated but as always this is a cost of compatibility layer. 


> but
> would lead to awkward structures:
> 
> ----------- >8 ----------------------------------------------------
> -----
> struct {
> 	struct usb_functionfs_descs_head2 header;
> 	/* Why aren't the two below fields inside of a header? */
> 	__le32 fs_count;
> 	__le32 hs_count;
> };
> 
> struct {
> 	struct {
> 		/* Why is there a header.header field? */
> 		struct usb_functionfs_descs_head2 header;
> 		__le32 fs_count;
> 		__le32 hs_count;
> 	};
> };
> ----------- >8 ----------------------------------------------------
> -----
> 

Please consider this:

struct {
	struct usb_functionfs_descs_head2 header;
	__le32 fs_count;
	__le32 hs_count;
	struct {
		(...)
	} fs_desc;
	struct {
		(...) /* Other than previous */
	} hs_desc;
} = {
	.header = (...),
	.fs_count = X,
	.fs_desc = {
		(...)
	},
	.hs_count = Y,
	.hs_desc = {
		(...)
	},
}

This approach looks quite reasonable for me and don't cause any confusion. Numbers of fs and hs descriptors don't go to headers because they are more connected with fs_desc and hs_desc than with header. Using this allows you to initialize them as close to fs_desc and hs_desc as possible. BTW yes I see that when we would like to have backward compatibility this will be few lines longer.

> And I don't see how it would be more flexible.  If anything, it
> would be
> less.

When I was writing about flexibility I was talking about use case described above.

> 
> I understand, and share, your sentiment, but I don't think adding
> usb_functionfs_descs_head2 structure with magic, flags and length
> would
> improve the situation.
> 
> Something I thought about shortly was:
> 
> ----------- >8 ----------------------------------------------------
> -----
> #define USB_FFS_DESCS_HEAD(_flags) struct {				\
> 		__le32 magic, length, flags;				\
> 		__le32 data[!!(_flags & FUNCTIONFS_HAS_FS_DESC) +	\
> 			    !!(_flags & FUNCTIONFS_HAS_HS_DESC) +	\
> 			    !!(_flags & FUNCTIONFS_HAS_SS_DESC)];	\
> 	} __attribute__((packed))
> 
> #define USB_FFS_DESCS_HEAD_INIT(_flags, length, ...) {
> 	\
> 		.magic = cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC_V2),
> 	\
> 		.flags = cpu_to_le32(flags),				\
> 		.langth = cpu_to_le32(length),				\
> 		.data = { __VA_ARGS__ }					\
> 	}
> 
> static const struct {
> 	USB_FFS_DESCS_HEAD(FUNCTIONFS_HAS_FS_DESC |
> FUNCTIONFS_HAS_HS_DESC),
> 	/* … */
> } __attribute__((packed)) descriptors = {
> 	USB_FFS_DESCS_HEAD_INIT(FUNCTIONFS_HAS_FS_DESC |
> FUNCTIONFS_HAS_HS_DESC,
> 				sizeof(descriptors), 3, 3),
> 	/* … */
> };
> ----------- >8 ----------------------------------------------------
> -----
> 
> But whether this is nicer is arguable as well.

In my opinion those macros are quite nice and should appear in functionfs.h. Maybe it would be suitable to provide ifdef __USB_FFS_USE_LEGACY_API and two versions of those macros? This will make backward compatibility to be handled by one switch before including a header instead of directly by user with ifndefs. Then the whole code with binary backward compatibility would be as simple as:

#ifdef MY_USE_LEGACY_FLAG
#define __USB_FFS_USE_LEGACY_API
#endif

#include <linux/usb/functionfs.h>

#define FFS_FLAGS (FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC)

struct {
	USB_FFS_DESCS_HEAD(FFS_FLAGS) header;
	struct {
		(...) /* descriptors */
	} fs_desc, hs_desc;
} __attribute__((packed)) my_desc = {
	.header = USB_FFS_DESCS_HEAD_INIT(FFS_FLAGS, sizeof(my_desc), 3, 3),
	(...) /* rest of initialization */
};

Of course two more suitable ifndef will be required for ss_desc. Taking all into account this approach looks very good to me and needs only minimal user action to provide support for both legacy and new API. This approach have also some disadvantage because we assume that developer has new kernel headers which provide such macros and only place where such app run has old kernel. I'm also considering approach with two macros because this would allow app to detect in runtime whether current kernel has new ffs version and use legacy macro as fallback procedure. 


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

Re: [PATCH 1/2] usb: gadget: f_fs: resurect usb_functionfs_descs_head structure

[Sergei Shtylyov]

Hello.

On 06/05/2014 11:43 AM, Michal Nazarewicz wrote:

> Even though usb_functionfs_descs_head structure is now deprecated,
> it has been used by some user space tools.  It's removel in commit

    s/It's removel/Its removal/.

> [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.

> Cc: <stable@vger.kernel.org>  # 3.14
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

WBR, Sergei