2014-06-13 13:38:26

by Michal Nazarewicz

[permalink] [raw]
Subject: [PATCHv4 0/6] Various small USB fixes

Michal Nazarewicz (6):
usb: gadget: f_fs: resurect usb_functionfs_descs_head structure
tools: ffs-test: fix header values endianess
usb: gadget: f_fs: add usb_functionfs_descs_head_v2 structure
tools: ffs-test: convert to new descriptor format
tools: ffs-test: add compatibility code for old kernels
usb: gadget: f_mass_storage: Fix a warning while loading
g_mass_storage

drivers/usb/gadget/f_mass_storage.c | 54 ++++++++++------
include/uapi/linux/usb/functionfs.h | 19 +++++-
tools/usb/ffs-test.c | 124 +++++++++++++++++++++++++++++++++---
3 files changed, 166 insertions(+), 31 deletions(-)

--
2.0.0.526.g5318336


2014-06-13 13:38:25

by Michal Nazarewicz

[permalink] [raw]
Subject: [PATCHv4 3/6] usb: gadget: f_fs: add usb_functionfs_descs_head_v2 structure

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 <[email protected]>
---
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

2014-06-13 13:38:24

by Michal Nazarewicz

[permalink] [raw]
Subject: [PATCHv4 6/6] usb: gadget: f_mass_storage: Fix a warning while loading g_mass_storage

While loading g_mass_storage module, the following warning can
trigger:

WARNING: at drivers/usb/gadget/composite.c:
usb_composite_setup_continue: Unexpected call
Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
[<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
[<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
[<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
[<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
[<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
[<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
[<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
[<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)

The root cause is that the existing code fails to take into account
the possibility that common->new_fsg can change while
do_set_interface() is running, because the spinlock does not protect
it.

Change the code so that the common->new_fsg field is protected by the
common->lock spin lock.

Reported-by: Yang Wei <[email protected]>
Signed-off-by: Michal Nazarewicz <[email protected]>
---
drivers/usb/gadget/f_mass_storage.c | 54 +++++++++++++++++++++++--------------
1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index b963939..bd663c2 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -264,7 +264,7 @@ struct fsg_common {
/* filesem protects: backing files in use */
struct rw_semaphore filesem;

- /* lock protects: state, all the req_busy's */
+ /* lock protects: state, all the req_busy's, and new_fsg */
spinlock_t lock;

struct usb_ep *ep0; /* Copy of gadget->ep0 */
@@ -407,23 +407,39 @@ static void wakeup_thread(struct fsg_common *common)
wake_up_process(common->thread_task);
}

-static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
+static void __raise_exception(struct fsg_common *common,
+ enum fsg_state new_state)
{
- unsigned long flags;
-
/*
* Do nothing if a higher-priority exception is already in progress.
* If a lower-or-equal priority exception is in progress, preempt it
* and notify the main thread by sending it a signal.
*/
+ if (common->state > new_state)
+ return;
+
+ common->exception_req_tag = common->ep0_req_tag;
+ common->state = new_state;
+ if (common->thread_task)
+ send_sig_info(SIGUSR1, SEND_SIG_FORCED, common->thread_task);
+}
+
+static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
+{
+ unsigned long flags;
spin_lock_irqsave(&common->lock, flags);
- if (common->state <= new_state) {
- common->exception_req_tag = common->ep0_req_tag;
- common->state = new_state;
- if (common->thread_task)
- send_sig_info(SIGUSR1, SEND_SIG_FORCED,
- common->thread_task);
- }
+ __raise_exception(common, new_state);
+ spin_unlock_irqrestore(&common->lock, flags);
+}
+
+static void raise_config_change_exception(struct fsg_common *common,
+ struct fsg_dev *new_fsg)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&common->lock, flags);
+ common->new_fsg = new_fsg;
+ __raise_exception(common, FSG_STATE_CONFIG_CHANGE);
spin_unlock_irqrestore(&common->lock, flags);
}

@@ -2320,16 +2336,14 @@ reset:
static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
{
struct fsg_dev *fsg = fsg_from_func(f);
- fsg->common->new_fsg = fsg;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ raise_config_change_exception(fsg->common, fsg);
return USB_GADGET_DELAYED_STATUS;
}

static void fsg_disable(struct usb_function *f)
{
struct fsg_dev *fsg = fsg_from_func(f);
- fsg->common->new_fsg = NULL;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ raise_config_change_exception(fsg->common, NULL);
}


@@ -2342,6 +2356,7 @@ static void handle_exception(struct fsg_common *common)
struct fsg_buffhd *bh;
enum fsg_state old_state;
struct fsg_lun *curlun;
+ struct fsg_dev *new_fsg;
unsigned int exception_req_tag;

/*
@@ -2405,6 +2420,7 @@ static void handle_exception(struct fsg_common *common)
common->next_buffhd_to_drain = &common->buffhds[0];
exception_req_tag = common->exception_req_tag;
old_state = common->state;
+ new_fsg = common->new_fsg;

if (old_state == FSG_STATE_ABORT_BULK_OUT)
common->state = FSG_STATE_STATUS_PHASE;
@@ -2460,8 +2476,8 @@ static void handle_exception(struct fsg_common *common)
break;

case FSG_STATE_CONFIG_CHANGE:
- do_set_interface(common, common->new_fsg);
- if (common->new_fsg)
+ do_set_interface(common, new_fsg);
+ if (new_fsg)
usb_composite_setup_continue(common->cdev);
break;

@@ -3178,8 +3194,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)

DBG(fsg, "unbind\n");
if (fsg->common->fsg == fsg) {
- fsg->common->new_fsg = NULL;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ raise_config_change_exception(fsg->common, NULL);
/* FIXME: make interruptible or killable somehow? */
wait_event(common->fsg_wait, common->fsg != fsg);
}
@@ -3665,4 +3680,3 @@ void fsg_config_from_params(struct fsg_config *cfg,
cfg->fsg_num_buffers = fsg_num_buffers;
}
EXPORT_SYMBOL_GPL(fsg_config_from_params);
-
--
2.0.0.526.g5318336

2014-06-13 13:38:22

by Michal Nazarewicz

[permalink] [raw]
Subject: [PATCHv4 2/6] tools: ffs-test: fix header values endianess

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: [email protected]
Signed-off-by: Michal Nazarewicz <[email protected]>
---
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

2014-06-13 13:38:21

by Michal Nazarewicz

[permalink] [raw]
Subject: [PATCHv4 5/6] tools: ffs-test: add compatibility code for old kernels

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 <[email protected]>
---
tools/usb/ffs-test.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 105 insertions(+), 5 deletions(-)

diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
index 708d317..88d5e71 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,89 @@ static const struct {
},
};

+static size_t descs_to_legacy(void **legacy, const void *descriptors_v2)
+{
+ const unsigned char *descs_end, *descs_start;
+ __u32 length, fs_count = 0, hs_count = 0, count;
+
+ /* Read v2 header */
+ {
+ const struct {
+ const struct usb_functionfs_descs_head_v2 header;
+ const __le32 counts[];
+ } __attribute__((packed)) *const in = descriptors_v2;
+ 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 = (const void *)counts;
+
+#undef GET_NEXT_COUNT_IF_FLAG
+ }
+
+ /*
+ * Find the end of FS and HS USB descriptors. SS descriptors
+ * are ignored since legacy format does not support them.
+ */
+ descs_end = descs_start;
+ do {
+ if (length < *descs_end)
+ return 0;
+ length -= *descs_end;
+ descs_end += *descs_end;
+ } while (--count);
+
+ /* Allocate legacy descriptors and copy the data. */
+ {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+ struct {
+ struct usb_functionfs_descs_head header;
+ __u8 descriptors[];
+ } __attribute__((packed)) *out;
+#pragma GCC diagnostic pop
+
+ 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;
+ }
+
+ return length;
+}
+

#define STR_INTERFACE_ "Source/Sink"

@@ -491,12 +575,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 +608,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

2014-06-13 13:44:41

by Michal Nazarewicz

[permalink] [raw]
Subject: [PATCHv4 4/6] tools: ffs-test: convert to new descriptor format

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 <[email protected]>
---
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 <[email protected]>
@@ -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

2014-06-13 13:45:25

by Michal Nazarewicz

[permalink] [raw]
Subject: [PATCHv4 1/6] usb: gadget: f_fs: resurect usb_functionfs_descs_head structure

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.

Cc: <[email protected]> # 3.14
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 | 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

2014-06-13 14:22:13

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCHv4 6/6] usb: gadget: f_mass_storage: Fix a warning while loading g_mass_storage

On Fri, 13 Jun 2014, Michal Nazarewicz wrote:

> While loading g_mass_storage module, the following warning can
> trigger:
>
> WARNING: at drivers/usb/gadget/composite.c:
> usb_composite_setup_continue: Unexpected call
> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>
> The root cause is that the existing code fails to take into account
> the possibility that common->new_fsg can change while
> do_set_interface() is running, because the spinlock does not protect
> it.
>
> Change the code so that the common->new_fsg field is protected by the
> common->lock spin lock.

NAK. common->new_fsg does not need to be protected. Wei's patch is
much simpler and will work well.

Alan Stern