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: <[email protected]> # 3.14
Signed-off-by: Michal Nazarewicz <[email protected]>
---
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
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 <[email protected]>
Reported-by: Krzysztof Opasiak <[email protected]>
Signed-off-by: Michal Nazarewicz <[email protected]>
---
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 <[email protected]>
@@ -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
Hi,
> -----Original Message-----
> From: Michal Nazarewicz [mailto:[email protected]]
> Sent: Thursday, June 05, 2014 9:43 AM
> To: Felipe Balbi
> Cc: Krzysztof Opasiak; [email protected]; linux-
> [email protected]; 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 <[email protected]>
> Reported-by: Krzysztof Opasiak <[email protected]>
> Signed-off-by: Michal Nazarewicz <[email protected]>
(...)
>
> 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
[email protected]
On Thu, Jun 05 2014, Krzysztof Opasiak <[email protected]> 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 +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
Hi,
> -----Original Message-----
> From: Michal Nazarewicz [mailto:[email protected]] On Behalf Of Michal
> Nazarewicz
> Sent: Thursday, June 05, 2014 1:56 PM
> To: Krzysztof Opasiak; 'Felipe Balbi'
> Cc: Krzysztof Opasiak; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 2/2] tools: ffs-test: convert to new descriptor
> format fixing compilation error
>
> On Thu, Jun 05 2014, Krzysztof Opasiak <[email protected]>
> 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
[email protected]
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: <[email protected]> # 3.14
> Signed-off-by: Michal Nazarewicz <[email protected]>
WBR, Sergei