[PATCH v5 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode

[PATCH v5 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode

[Manu Gautam]

Allow userspace to pass SuperSpeed descriptors and
handle them in the driver accordingly.
This change doesn't modify existing desc_header and thereby
keeps the ABI changes backward compatible i.e. existing
userspace drivers compiled with old header (functionfs.h)
would continue to work with the updated kernel.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/f_fs.c           | 182 +++++++++++++++++++++++++++---------
 drivers/usb/gadget/u_fs.h           |  10 +-
 include/uapi/linux/usb/functionfs.h |   5 +
 3 files changed, 147 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 306a2b5..928959d 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -122,8 +122,8 @@ struct ffs_ep {
 	struct usb_ep			*ep;	/* P: ffs->eps_lock */
 	struct usb_request		*req;	/* P: epfile->mutex */
 
-	/* [0]: full speed, [1]: high speed */
-	struct usb_endpoint_descriptor	*descs[2];
+	/* [0]: full speed, [1]: high speed, [2]: super speed */
+	struct usb_endpoint_descriptor	*descs[3];
 
 	u8				num;
 
@@ -1183,10 +1183,11 @@ static void ffs_data_reset(struct ffs_data *ffs)
 	ffs->raw_strings = NULL;
 	ffs->stringtabs = NULL;
 
-	ffs->raw_descs_length = 0;
-	ffs->raw_fs_descs_length = 0;
+	ffs->raw_fs_hs_descs_length = 0;
+	ffs->raw_ss_descs_length = 0;
 	ffs->fs_descs_count = 0;
 	ffs->hs_descs_count = 0;
+	ffs->ss_descs_count = 0;
 
 	ffs->strings_count = 0;
 	ffs->interfaces_count = 0;
@@ -1329,7 +1330,24 @@ static int ffs_func_eps_enable(struct ffs_function *func)
 	spin_lock_irqsave(&func->ffs->eps_lock, flags);
 	do {
 		struct usb_endpoint_descriptor *ds;
-		ds = ep->descs[ep->descs[1] ? 1 : 0];
+		int desc_idx;
+
+		if (ffs->gadget->speed == USB_SPEED_SUPER)
+			desc_idx = 2;
+		else if (ffs->gadget->speed == USB_SPEED_HIGH)
+			desc_idx = 1;
+		else
+			desc_idx = 0;
+
+		/* fall-back to lower speed if desc missing for current speed */
+		do {
+			ds = ep->descs[desc_idx];
+		} while (!ds && --desc_idx >= 0);
+
+		if (!ds) {
+			ret = -EINVAL;
+			break;
+		}
 
 		ep->ep->driver_data = ep;
 		ep->ep->desc = ds;
@@ -1464,6 +1482,12 @@ static int __must_check ffs_do_desc(char *data, unsigned len,
 	}
 		break;
 
+	case USB_DT_SS_ENDPOINT_COMP:
+		pr_vdebug("EP SS companion descriptor\n");
+		if (length != sizeof(struct usb_ss_ep_comp_descriptor))
+			goto inv_length;
+		break;
+
 	case USB_DT_OTHER_SPEED_CONFIG:
 	case USB_DT_INTERFACE_POWER:
 	case USB_DT_DEBUG:
@@ -1574,8 +1598,8 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 static int __ffs_data_got_descs(struct ffs_data *ffs,
 				char *const _data, size_t len)
 {
-	unsigned fs_count, hs_count;
-	int fs_len, ret = -EINVAL;
+	unsigned fs_count, hs_count, ss_count = 0;
+	int fs_len, hs_len, ss_len, ret = -EINVAL;
 	char *data = _data;
 
 	ENTER();
@@ -1586,9 +1610,6 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 	fs_count = get_unaligned_le32(data +  8);
 	hs_count = get_unaligned_le32(data + 12);
 
-	if (!fs_count && !hs_count)
-		goto einval;
-
 	data += 16;
 	len  -= 16;
 
@@ -1607,22 +1628,54 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 	}
 
 	if (likely(hs_count)) {
-		ret = ffs_do_descs(hs_count, data, len,
+		hs_len = ffs_do_descs(hs_count, data, len,
 				   __ffs_data_do_entity, ffs);
-		if (unlikely(ret < 0))
+		if (unlikely(hs_len < 0)) {
+			ret = hs_len;
+			goto error;
+		}
+
+		data += hs_len;
+		len  -= hs_len;
+	} else {
+		hs_len = 0;
+	}
+
+	if (len >= 8) {
+		/* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */
+		if (get_unaligned_le32(data) != FUNCTIONFS_SS_DESC_MAGIC)
+			goto einval;
+
+		ss_count = get_unaligned_le32(data + 4);
+		data += 8;
+		len  -= 8;
+	}
+
+	if (!fs_count && !hs_count && !ss_count)
+		goto einval;
+
+	if (ss_count) {
+		ss_len = ffs_do_descs(ss_count, data, len,
+				   __ffs_data_do_entity, ffs);
+		if (unlikely(ss_len < 0)) {
+			ret = ss_len;
 			goto error;
+		}
+		ret = ss_len;
 	} else {
+		ss_len = 0;
 		ret = 0;
 	}
 
 	if (unlikely(len != ret))
 		goto einval;
 
-	ffs->raw_fs_descs_length = fs_len;
-	ffs->raw_descs_length    = fs_len + ret;
-	ffs->raw_descs           = _data;
-	ffs->fs_descs_count      = fs_count;
-	ffs->hs_descs_count      = hs_count;
+	ffs->raw_fs_hs_descs_length	 = fs_len + hs_len;
+	ffs->raw_ss_descs_length	 = ss_len;
+	ffs->raw_descs			 = _data;
+	ffs->fs_descs_count		 = fs_count;
+	ffs->hs_descs_count		 = hs_count;
+	ffs->ss_descs_count		 = ss_count;
 
 	return 0;
 
@@ -1845,21 +1898,28 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 	struct usb_endpoint_descriptor *ds = (void *)desc;
 	struct ffs_function *func = priv;
 	struct ffs_ep *ffs_ep;
-
-	/*
-	 * If hs_descriptors is not NULL then we are reading hs
-	 * descriptors now
-	 */
-	const int isHS = func->function.hs_descriptors != NULL;
-	unsigned idx;
+	unsigned ep_desc_id, idx;
+	static const char *speed_names[] = { "full", "high", "super" };
 
 	if (type != FFS_DESCRIPTOR)
 		return 0;
 
-	if (isHS)
+	/*
+	 * If ss_descriptors is not NULL, we are reading super speed
+	 * descriptors; if hs_descriptors is not NULL, we are reading high
+	 * speed descriptors; otherwise, we are reading full speed
+	 * descriptors.
+	 */
+	if (func->function.ss_descriptors) {
+		ep_desc_id = 2;
+		func->function.ss_descriptors[(long)valuep] = desc;
+	} else if (func->function.hs_descriptors) {
+		ep_desc_id = 1;
 		func->function.hs_descriptors[(long)valuep] = desc;
-	else
+	} else {
+		ep_desc_id = 0;
 		func->function.fs_descriptors[(long)valuep]    = desc;
+	}
 
 	if (!desc || desc->bDescriptorType != USB_DT_ENDPOINT)
 		return 0;
@@ -1867,13 +1927,13 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 	idx = (ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) - 1;
 	ffs_ep = func->eps + idx;
 
-	if (unlikely(ffs_ep->descs[isHS])) {
-		pr_vdebug("two %sspeed descriptors for EP %d\n",
-			  isHS ? "high" : "full",
+	if (unlikely(ffs_ep->descs[ep_desc_id])) {
+		pr_err("two %sspeed descriptors for EP %d\n",
+			  speed_names[ep_desc_id],
 			  ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
 		return -EINVAL;
 	}
-	ffs_ep->descs[isHS] = ds;
+	ffs_ep->descs[ep_desc_id] = ds;
 
 	ffs_dump_mem(": Original  ep desc", ds, ds->bLength);
 	if (ffs_ep->ep) {
@@ -2017,8 +2077,10 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	const int full = !!func->ffs->fs_descs_count;
 	const int high = gadget_is_dualspeed(func->gadget) &&
 		func->ffs->hs_descs_count;
+	const int super = gadget_is_superspeed(func->gadget) &&
+		func->ffs->ss_descs_count;
 
-	int ret;
+	int fs_len, hs_len, ret;
 
 	/* Make it a single chunk, less management later on */
 	vla_group(d);
@@ -2027,15 +2089,17 @@ static int _ffs_func_bind(struct usb_configuration *c,
 		full ? ffs->fs_descs_count + 1 : 0);
 	vla_item_with_sz(d, struct usb_descriptor_header *, hs_descs,
 		high ? ffs->hs_descs_count + 1 : 0);
+	vla_item_with_sz(d, struct usb_descriptor_header *, ss_descs,
+		super ? ffs->ss_descs_count + 1 : 0);
 	vla_item_with_sz(d, short, inums, ffs->interfaces_count);
 	vla_item_with_sz(d, char, raw_descs,
-		high ? ffs->raw_descs_length : ffs->raw_fs_descs_length);
+			ffs->raw_fs_hs_descs_length + ffs->raw_ss_descs_length);
 	char *vlabuf;
 
 	ENTER();
 
-	/* Only high speed but not supported by gadget? */
-	if (unlikely(!(full | high)))
+	/* Has descriptors only for speeds gadget does not support */
+	if (unlikely(!(full | high | super)))
 		return -ENOTSUPP;
 
 	/* Allocate a single chunk, less management later on */
@@ -2045,8 +2109,16 @@ static int _ffs_func_bind(struct usb_configuration *c,
 
 	/* Zero */
 	memset(vla_ptr(vlabuf, d, eps), 0, d_eps__sz);
+	/* Copy only raw (hs,fs) descriptors (until ss_magic and ss_count) */
 	memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs + 16,
-	       d_raw_descs__sz);
+		ffs->raw_fs_hs_descs_length);
+	/* Copy SS descs present @ header + hs_fs_descs + ss_magic + ss_count */
+	if (func->ffs->ss_descs_count)
+		memcpy(vla_ptr(vlabuf, d, raw_descs) +
+				ffs->raw_fs_hs_descs_length,
+		       ffs->raw_descs + 16 + ffs->raw_fs_hs_descs_length + 8,
+		       ffs->raw_ss_descs_length);
+
 	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
 	for (ret = ffs->eps_count; ret; --ret) {
 		struct ffs_ep *ptr;
@@ -2068,22 +2140,38 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	 */
 	if (likely(full)) {
 		func->function.fs_descriptors = vla_ptr(vlabuf, d, fs_descs);
-		ret = ffs_do_descs(ffs->fs_descs_count,
-				   vla_ptr(vlabuf, d, raw_descs),
-				   d_raw_descs__sz,
-				   __ffs_func_bind_do_descs, func);
-		if (unlikely(ret < 0))
+		fs_len = ffs_do_descs(ffs->fs_descs_count,
+				      vla_ptr(vlabuf, d, raw_descs),
+				      d_raw_descs__sz,
+				      __ffs_func_bind_do_descs, func);
+		if (unlikely(fs_len < 0)) {
+			ret = fs_len;
 			goto error;
+		}
 	} else {
-		ret = 0;
+		fs_len = 0;
 	}
 
 	if (likely(high)) {
 		func->function.hs_descriptors = vla_ptr(vlabuf, d, hs_descs);
-		ret = ffs_do_descs(ffs->hs_descs_count,
-				   vla_ptr(vlabuf, d, raw_descs) + ret,
-				   d_raw_descs__sz - ret,
-				   __ffs_func_bind_do_descs, func);
+		hs_len = ffs_do_descs(ffs->hs_descs_count,
+				      vla_ptr(vlabuf, d, raw_descs) + fs_len,
+				      d_raw_descs__sz - fs_len,
+				      __ffs_func_bind_do_descs, func);
+		if (unlikely(hs_len < 0)) {
+			ret = hs_len;
+			goto error;
+		}
+	} else {
+		hs_len = 0;
+	}
+
+	if (likely(super)) {
+		func->function.ss_descriptors = vla_ptr(vlabuf, d, ss_descs);
+		ret = ffs_do_descs(ffs->ss_descs_count,
+				vla_ptr(vlabuf, d, raw_descs) + fs_len + hs_len,
+				d_raw_descs__sz - fs_len - hs_len,
+				__ffs_func_bind_do_descs, func);
 		if (unlikely(ret < 0))
 			goto error;
 	}
@@ -2094,7 +2182,8 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	 * now.
 	 */
 	ret = ffs_do_descs(ffs->fs_descs_count +
-			   (high ? ffs->hs_descs_count : 0),
+			   (high ? ffs->hs_descs_count : 0) +
+			   (super ? ffs->ss_descs_count : 0),
 			   vla_ptr(vlabuf, d, raw_descs), d_raw_descs__sz,
 			   __ffs_func_bind_do_nums, func);
 	if (unlikely(ret < 0))
@@ -2441,6 +2530,7 @@ static void ffs_func_unbind(struct usb_configuration *c,
 	 */
 	func->function.fs_descriptors = NULL;
 	func->function.hs_descriptors = NULL;
+	func->function.ss_descriptors = NULL;
 	func->interfaces_nums = NULL;
 
 	ffs_event_add(ffs, FUNCTIONFS_UNBIND);
diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h
index bc2d371..08bf26e 100644
--- a/drivers/usb/gadget/u_fs.h
+++ b/drivers/usb/gadget/u_fs.h
@@ -212,14 +212,16 @@ struct ffs_data {
 	/*
 	 * Real descriptors are 16 bytes after raw_descs (so you need
 	 * to skip 16 bytes (ie. ffs->raw_descs + 16) to get to the
-	 * first full speed descriptor).  raw_descs_length and
-	 * raw_fs_descs_length do not have those 16 bytes added.
+	 * first full speed descriptor).
+	 * raw_fs_hs_descs_length does not have those 16 bytes added.
+	 * ss_descs are 8 bytes (ss_magic + count) pass the hs_descs
 	 */
 	const void			*raw_descs;
-	unsigned			raw_descs_length;
-	unsigned			raw_fs_descs_length;
+	unsigned			raw_fs_hs_descs_length;
+	unsigned			raw_ss_descs_length;
 	unsigned			fs_descs_count;
 	unsigned			hs_descs_count;
+	unsigned			ss_descs_count;
 
 	unsigned short			strings_count;
 	unsigned short			interfaces_count;
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index d6b0128..0f8f7be 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -13,6 +13,7 @@ enum {
 	FUNCTIONFS_STRINGS_MAGIC     = 2
 };
 
+#define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C
 
 #ifndef __KERNEL__
 
@@ -50,7 +51,11 @@ struct usb_functionfs_descs_head {
  * |  12 | hs_count  | LE32         | number of high-speed descriptors     |
  * |  16 | fs_descrs | Descriptor[] | list of full-speed descriptors       |
  * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
+ * |     | ss_magic  | LE32         | FUNCTIONFS_SS_DESC_MAGIC             |
+ * |     | ss_count  | LE32         | number of super-speed descriptors    |
+ * |     | ss_descrs | Descriptor[] | list of super-speed descriptors      |
  *
+ * ss_magic: if present then it implies that SS_DESCs are also present
  * descs are just valid USB descriptors and have the following format:
  *
  * | off | name            | type | description              |
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode

[Manu Gautam]

On 12/24/2013 3:00 PM, Manu Gautam wrote:
> Allow userspace to pass SuperSpeed descriptors and
> handle them in the driver accordingly.
> This change doesn't modify existing desc_header and thereby
> keeps the ABI changes backward compatible i.e. existing
> userspace drivers compiled with old header (functionfs.h)
> would continue to work with the updated kernel.
> 
> Signed-off-by: Manu Gautam <mgautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Acked-by: Michal Nazarewicz <mina86-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>
> ---

Hi Felipe,

Is this patch good now to be added to your queue (if not already in)?
It applied cleanly on your next branch.

Thanks,
Manu


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] usb: gadget: f_fs: Add flags to descriptors block

[Michal Nazarewicz]

This reworks the way SuperSpeed descriptors are added and instead of
having a magick after full and high speed descriptors, it reworks the
whole descriptors block to include a flags field which lists which
descriptors are present and makes future extensions possible.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
Just to repeat what I wrote on my phone which is unable to send
text/plain messages:

On Tue, Feb 25 2014, Michał Nazarewicz wrote:
> On Feb 25, 2014 5:47 AM, "Manu Gautam" <mgautam@codeaurora.org> wrote:
>> Is this patch good now to be added to your queue (if not already in)?
>> It applied cleanly on your next branch.
>
> Actually, recent LWN article about flags in syscalls got me thinking and
> maybe it's good this patch is not yet in Felipe's queue.
>
> I admit I screwed up when I designed the headers format, but now that we
> change it to accommodate SuperSpeed, I think we should go ahead and change
> the format by adding flags field.
>
> […]
>
> Sorry to mention this only now, but I believe that's the right thing to do.

This patch implements this new format and in doing so simplifies, in
my opinion, the code a bit.

It needs to be applied on top of Manu's patch. 

It has not been tested at all, not even compile-tested.  This is
because whenever I try to compile the kernel, I get this nice message
in my logs:

    mce: [Hardware Error]: Machine check events logged
    thermal_sys: Critical temperature reached(100 C),shutting down
    thermal_sys: Critical temperature reached(100 C),shutting down

Sorry about that.

 drivers/usb/gadget/f_fs.c           | 136 +++++++++++++++---------------------
 drivers/usb/gadget/u_fs.h           |  12 ++--
 include/uapi/linux/usb/functionfs.h |  49 ++++++++-----
 3 files changed, 94 insertions(+), 103 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 928959d..7d01bd6 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1167,7 +1167,7 @@ static void ffs_data_clear(struct ffs_data *ffs)
 	if (ffs->epfiles)
 		ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
 
-	kfree(ffs->raw_descs);
+	kfree(ffs->raw_descs_data);
 	kfree(ffs->raw_strings);
 	kfree(ffs->stringtabs);
 }
@@ -1179,12 +1179,12 @@ static void ffs_data_reset(struct ffs_data *ffs)
 	ffs_data_clear(ffs);
 
 	ffs->epfiles = NULL;
+	ffs->raw_descs_data = NULL;
 	ffs->raw_descs = NULL;
 	ffs->raw_strings = NULL;
 	ffs->stringtabs = NULL;
 
-	ffs->raw_fs_hs_descs_length = 0;
-	ffs->raw_ss_descs_length = 0;
+	ffs->real_descs_length = 0;
 	ffs->fs_descs_count = 0;
 	ffs->hs_descs_count = 0;
 	ffs->ss_descs_count = 0;
@@ -1598,89 +1598,76 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 static int __ffs_data_got_descs(struct ffs_data *ffs,
 				char *const _data, size_t len)
 {
-	unsigned fs_count, hs_count, ss_count = 0;
-	int fs_len, hs_len, ss_len, ret = -EINVAL;
-	char *data = _data;
+	char *data = _data, *raw_descs;
+	unsigned counts[3], flags;
+	int ret = -EINVAL, i;
 
 	ENTER();
 
-	if (unlikely(get_unaligned_le32(data) != FUNCTIONFS_DESCRIPTORS_MAGIC ||
-		     get_unaligned_le32(data + 4) != len))
+	if (get_unaligned_le32(data + 4) != len)
 		goto error;
-	fs_count = get_unaligned_le32(data +  8);
-	hs_count = get_unaligned_le32(data + 12);
-
-	data += 16;
-	len  -= 16;
 
-	if (likely(fs_count)) {
-		fs_len = ffs_do_descs(fs_count, data, len,
-				      __ffs_data_do_entity, ffs);
-		if (unlikely(fs_len < 0)) {
-			ret = fs_len;
+	switch (get_unaligned_le32(data)) {
+	case FUNCTIONFS_DESCRIPTORS_MAGIC:
+		flags = FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC;
+		data += 8;
+		len  -= 8;
+		break;
+	case FUNCTIONFS_DESCRIPTORS_MAGIC_V2:
+		flags = get_unaligned_le32(data + 8);
+		if (flags & ~(FUNCTIONFS_HAS_FS_DESC |
+			      FUNCTIONFS_HAS_HS_DESC |
+			      FUNCTIONFS_HAS_SS_DESC)) {
+			ret = -ENOSYS;
 			goto error;
 		}
-
-		data += fs_len;
-		len  -= fs_len;
-	} else {
-		fs_len = 0;
+		data += 12;
+		len  -= 12;
+		break;
+	default:
+		goto error;
 	}
 
-	if (likely(hs_count)) {
-		hs_len = ffs_do_descs(hs_count, data, len,
-				   __ffs_data_do_entity, ffs);
-		if (unlikely(hs_len < 0)) {
-			ret = hs_len;
+	/* Read fs_count, hs_count and ss_count (if present) */
+	for (i = 0; i < 3; ++i) {
+		if (!(flags & (1 << i))) {
+			counts[i] = 0;
+		} else if (len < 4) {
 			goto error;
+		} else {
+			counts[i] = get_unaligned_le32(data);
+			data += 4;
+			len  -= 4;
 		}
-
-		data += hs_len;
-		len  -= hs_len;
-	} else {
-		hs_len = 0;
 	}
 
-	if (len >= 8) {
-		/* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */
-		if (get_unaligned_le32(data) != FUNCTIONFS_SS_DESC_MAGIC)
-			goto einval;
-
-		ss_count = get_unaligned_le32(data + 4);
-		data += 8;
-		len  -= 8;
-	}
-
-	if (!fs_count && !hs_count && !ss_count)
-		goto einval;
-
-	if (ss_count) {
-		ss_len = ffs_do_descs(ss_count, data, len,
+	/* Read descriptors */
+	raw_descs = data;
+	for (i = 0; i < 3; ++i) {
+		if (!counts[i])
+			continue;
+		ret = ffs_do_descs(descs[i].count, data, len,
 				   __ffs_data_do_entity, ffs);
-		if (unlikely(ss_len < 0)) {
-			ret = ss_len;
+		if (ret < 0)
 			goto error;
-		}
-		ret = ss_len;
-	} else {
-		ss_len = 0;
-		ret = 0;
+		data += ret;
+		len  -= ret;
 	}
 
-	if (unlikely(len != ret))
-		goto einval;
+	if (raw_descs == data || len) {
+		ret = -EINVAL;
+		goto error;
+	}
 
-	ffs->raw_fs_hs_descs_length	 = fs_len + hs_len;
-	ffs->raw_ss_descs_length	 = ss_len;
-	ffs->raw_descs			 = _data;
-	ffs->fs_descs_count		 = fs_count;
-	ffs->hs_descs_count		 = hs_count;
-	ffs->ss_descs_count		 = ss_count;
+	ffs->raw_descs_data	= _data;
+	ffs->raw_descs		= raw_descs;
+	ffs->raw_descs_length	= data - raw_descs;
+	ffs->fs_descs_count	= counts[0];
+	ffs->hs_descs_count	= counts[1];
+	ffs->ss_descs_count	= counts[2];
 
 	return 0;
 
-einval:
-	ret = -EINVAL;
 error:
 	kfree(_data);
 	return ret;
@@ -2092,8 +2079,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	vla_item_with_sz(d, struct usb_descriptor_header *, ss_descs,
 		super ? ffs->ss_descs_count + 1 : 0);
 	vla_item_with_sz(d, short, inums, ffs->interfaces_count);
-	vla_item_with_sz(d, char, raw_descs,
-			ffs->raw_fs_hs_descs_length + ffs->raw_ss_descs_length);
+	vla_item_with_sz(d, char, raw_descs, ffs->raw_raw_descs_length);
 	char *vlabuf;
 
 	ENTER();
@@ -2109,15 +2095,9 @@ static int _ffs_func_bind(struct usb_configuration *c,
 
 	/* Zero */
 	memset(vla_ptr(vlabuf, d, eps), 0, d_eps__sz);
-	/* Copy only raw (hs,fs) descriptors (until ss_magic and ss_count) */
-	memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs + 16,
-		ffs->raw_fs_hs_descs_length);
-	/* Copy SS descs present @ header + hs_fs_descs + ss_magic + ss_count */
-	if (func->ffs->ss_descs_count)
-		memcpy(vla_ptr(vlabuf, d, raw_descs) +
-				ffs->raw_fs_hs_descs_length,
-		       ffs->raw_descs + 16 + ffs->raw_fs_hs_descs_length + 8,
-		       ffs->raw_ss_descs_length);
+	/* Copy descriptors  */
+	memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs,
+	       ffs->raw_descs_length);
 
 	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
 	for (ret = ffs->eps_count; ret; --ret) {
@@ -2599,7 +2579,7 @@ static int _ffs_name_dev(struct ffs_dev *dev, const char *name)
 	existing = _ffs_find_dev(name);
 	if (existing)
 		return -EBUSY;
-	
+
 	dev->name = name;
 
 	return 0;
@@ -2682,7 +2662,7 @@ static void ffs_release_dev(struct ffs_data *ffs_data)
 	ffs_dev = ffs_data->private_data;
 	if (ffs_dev)
 		ffs_dev->mounted = false;
-	
+
 	if (ffs_dev->ffs_release_dev_callback)
 		ffs_dev->ffs_release_dev_callback(ffs_dev);
 
diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h
index 08bf26e..866c848 100644
--- a/drivers/usb/gadget/u_fs.h
+++ b/drivers/usb/gadget/u_fs.h
@@ -210,15 +210,13 @@ struct ffs_data {
 
 	/* filled by __ffs_data_got_descs() */
 	/*
-	 * Real descriptors are 16 bytes after raw_descs (so you need
-	 * to skip 16 bytes (ie. ffs->raw_descs + 16) to get to the
-	 * first full speed descriptor).
-	 * raw_fs_hs_descs_length does not have those 16 bytes added.
-	 * ss_descs are 8 bytes (ss_magic + count) pass the hs_descs
+	 * raw_descs is what you kfree, real_descs points inside of raw_descs,
+	 * where full speed, high speed and super speed descriptors start.
+	 * real_descs_length is the length of all those descriptors.
 	 */
 	const void			*raw_descs;
-	unsigned			raw_fs_hs_descs_length;
-	unsigned			raw_ss_descs_length;
+	const void			*real_descs;
+	unsigned			real_descs_length;
 	unsigned			fs_descs_count;
 	unsigned			hs_descs_count;
 	unsigned			ss_descs_count;
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 0f8f7be..ee6fcbc 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -10,7 +10,14 @@
 
 enum {
 	FUNCTIONFS_DESCRIPTORS_MAGIC = 1,
-	FUNCTIONFS_STRINGS_MAGIC     = 2
+	FUNCTIONFS_STRINGS_MAGIC = 2,
+	FUNCTIONFS_DESCRIPTORS_MAGIC_V2 = 3,
+};
+
+enum functionfs_flags {
+	FUNCTIONFS_HAS_FS_DESC = 1,
+	FUNCTIONFS_HAS_HS_DESC = 2,
+	FUNCTIONFS_HAS_SS_DESC = 4,
 };
 
 #define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C
@@ -30,33 +37,39 @@ struct usb_endpoint_descriptor_no_audio {
 
 
 /*
- * All numbers must be in little endian order.
- */
-
-struct usb_functionfs_descs_head {
-	__le32 magic;
-	__le32 length;
-	__le32 fs_count;
-	__le32 hs_count;
-} __attribute__((packed));
-
-/*
  * Descriptors format:
  *
  * | off | name      | type         | description                          |
  * |-----+-----------+--------------+--------------------------------------|
- * |   0 | magic     | LE32         | FUNCTIONFS_{FS,HS}_DESCRIPTORS_MAGIC |
+ * |   0 | magic     | LE32         | FUNCTIONFS_DESCRIPTORS_MAGIC_V2      |
+ * |   4 | length    | LE32         | length of the whole data chunk       |
+ * |   8 | flags     | LE32         | combination of functionfs_flags      |
+ * |     | fs_count  | LE32         | number of full-speed descriptors     |
+ * |     | hs_count  | LE32         | number of high-speed descriptors     |
+ * |     | ss_count  | LE32         | number of high-speed descriptors     |
+ * |     | fs_descrs | Descriptor[] | list of full-speed descriptors       |
+ * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
+ * |     | ss_descrs | Descriptor[] | list of high-speed descriptors       |
+ *
+ * Depending on which flags are set, various fields may be missing in the
+ * structure.  Any flags that are not recognised cause the whole block to be
+ * rejected with -ENOSYS.
+ *
+ * Legacy descriptors format:
+ *
+ * | off | name      | type         | description                          |
+ * |-----+-----------+--------------+--------------------------------------|
+ * |   0 | magic     | LE32         | FUNCTIONFS_DESCRIPTORS_MAGIC         |
  * |   4 | length    | LE32         | length of the whole data chunk       |
  * |   8 | fs_count  | LE32         | number of full-speed descriptors     |
  * |  12 | hs_count  | LE32         | number of high-speed descriptors     |
  * |  16 | fs_descrs | Descriptor[] | list of full-speed descriptors       |
  * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
- * |     | ss_magic  | LE32         | FUNCTIONFS_SS_DESC_MAGIC             |
- * |     | ss_count  | LE32         | number of super-speed descriptors    |
- * |     | ss_descrs | Descriptor[] | list of super-speed descriptors      |
  *
- * ss_magic: if present then it implies that SS_DESCs are also present
- * descs are just valid USB descriptors and have the following format:
+ * All numbers must be in little endian order.
+ *
+ * Descriptor[] is an array of valid USB descriptors which have the following
+ * format:
  *
  * | off | name            | type | description              |
  * |-----+-----------------+------+--------------------------|
-- 
1.9.0.rc1.175.g0b1dcb5

[PATCHv2] usb: gadget: f_fs: Add flags to descriptors block

[Michal Nazarewicz]

This reworks the way SuperSpeed descriptors are added and instead of
having a magick after full and high speed descriptors, it reworks the
whole descriptors block to include a flags field which lists which
descriptors are present and makes future extensions possible.

Signed-off-by: Michal Nazarewicz <mina86-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/gadget/f_fs.c           | 136 +++++++++++++++---------------------
 drivers/usb/gadget/u_fs.h           |  12 ++--
 include/uapi/linux/usb/functionfs.h |  49 ++++++++-----
 3 files changed, 94 insertions(+), 103 deletions(-)

All right, with some fidling with my fan and limiting CPU frequency to
the mininimum, I've managed to compile this patch and fixed all the
typos.

Manu, if you could include it in your series, adjust your user space
client and test it, it would be wonderful. :]

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 928959d..fab63d3 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1167,7 +1167,7 @@ static void ffs_data_clear(struct ffs_data *ffs)
 	if (ffs->epfiles)
 		ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
 
-	kfree(ffs->raw_descs);
+	kfree(ffs->raw_descs_data);
 	kfree(ffs->raw_strings);
 	kfree(ffs->stringtabs);
 }
@@ -1179,12 +1179,12 @@ static void ffs_data_reset(struct ffs_data *ffs)
 	ffs_data_clear(ffs);
 
 	ffs->epfiles = NULL;
+	ffs->raw_descs_data = NULL;
 	ffs->raw_descs = NULL;
 	ffs->raw_strings = NULL;
 	ffs->stringtabs = NULL;
 
-	ffs->raw_fs_hs_descs_length = 0;
-	ffs->raw_ss_descs_length = 0;
+	ffs->raw_descs_length = 0;
 	ffs->fs_descs_count = 0;
 	ffs->hs_descs_count = 0;
 	ffs->ss_descs_count = 0;
@@ -1598,89 +1598,76 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 static int __ffs_data_got_descs(struct ffs_data *ffs,
 				char *const _data, size_t len)
 {
-	unsigned fs_count, hs_count, ss_count = 0;
-	int fs_len, hs_len, ss_len, ret = -EINVAL;
-	char *data = _data;
+	char *data = _data, *raw_descs;
+	unsigned counts[3], flags;
+	int ret = -EINVAL, i;
 
 	ENTER();
 
-	if (unlikely(get_unaligned_le32(data) != FUNCTIONFS_DESCRIPTORS_MAGIC ||
-		     get_unaligned_le32(data + 4) != len))
+	if (get_unaligned_le32(data + 4) != len)
 		goto error;
-	fs_count = get_unaligned_le32(data +  8);
-	hs_count = get_unaligned_le32(data + 12);
-
-	data += 16;
-	len  -= 16;
 
-	if (likely(fs_count)) {
-		fs_len = ffs_do_descs(fs_count, data, len,
-				      __ffs_data_do_entity, ffs);
-		if (unlikely(fs_len < 0)) {
-			ret = fs_len;
+	switch (get_unaligned_le32(data)) {
+	case FUNCTIONFS_DESCRIPTORS_MAGIC:
+		flags = FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC;
+		data += 8;
+		len  -= 8;
+		break;
+	case FUNCTIONFS_DESCRIPTORS_MAGIC_V2:
+		flags = get_unaligned_le32(data + 8);
+		if (flags & ~(FUNCTIONFS_HAS_FS_DESC |
+			      FUNCTIONFS_HAS_HS_DESC |
+			      FUNCTIONFS_HAS_SS_DESC)) {
+			ret = -ENOSYS;
 			goto error;
 		}
-
-		data += fs_len;
-		len  -= fs_len;
-	} else {
-		fs_len = 0;
+		data += 12;
+		len  -= 12;
+		break;
+	default:
+		goto error;
 	}
 
-	if (likely(hs_count)) {
-		hs_len = ffs_do_descs(hs_count, data, len,
-				   __ffs_data_do_entity, ffs);
-		if (unlikely(hs_len < 0)) {
-			ret = hs_len;
+	/* Read fs_count, hs_count and ss_count (if present) */
+	for (i = 0; i < 3; ++i) {
+		if (!(flags & (1 << i))) {
+			counts[i] = 0;
+		} else if (len < 4) {
 			goto error;
+		} else {
+			counts[i] = get_unaligned_le32(data);
+			data += 4;
+			len  -= 4;
 		}
-
-		data += hs_len;
-		len  -= hs_len;
-	} else {
-		hs_len = 0;
 	}
 
-	if (len >= 8) {
-		/* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */
-		if (get_unaligned_le32(data) != FUNCTIONFS_SS_DESC_MAGIC)
-			goto einval;
-
-		ss_count = get_unaligned_le32(data + 4);
-		data += 8;
-		len  -= 8;
-	}
-
-	if (!fs_count && !hs_count && !ss_count)
-		goto einval;
-
-	if (ss_count) {
-		ss_len = ffs_do_descs(ss_count, data, len,
+	/* Read descriptors */
+	raw_descs = data;
+	for (i = 0; i < 3; ++i) {
+		if (!counts[i])
+			continue;
+		ret = ffs_do_descs(counts[i], data, len,
 				   __ffs_data_do_entity, ffs);
-		if (unlikely(ss_len < 0)) {
-			ret = ss_len;
+		if (ret < 0)
 			goto error;
-		}
-		ret = ss_len;
-	} else {
-		ss_len = 0;
-		ret = 0;
+		data += ret;
+		len  -= ret;
 	}
 
-	if (unlikely(len != ret))
-		goto einval;
+	if (raw_descs == data || len) {
+		ret = -EINVAL;
+		goto error;
+	}
 
-	ffs->raw_fs_hs_descs_length	 = fs_len + hs_len;
-	ffs->raw_ss_descs_length	 = ss_len;
-	ffs->raw_descs			 = _data;
-	ffs->fs_descs_count		 = fs_count;
-	ffs->hs_descs_count		 = hs_count;
-	ffs->ss_descs_count		 = ss_count;
+	ffs->raw_descs_data	= _data;
+	ffs->raw_descs		= raw_descs;
+	ffs->raw_descs_length	= data - raw_descs;
+	ffs->fs_descs_count	= counts[0];
+	ffs->hs_descs_count	= counts[1];
+	ffs->ss_descs_count	= counts[2];
 
 	return 0;
 
-einval:
-	ret = -EINVAL;
 error:
 	kfree(_data);
 	return ret;
@@ -2092,8 +2079,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	vla_item_with_sz(d, struct usb_descriptor_header *, ss_descs,
 		super ? ffs->ss_descs_count + 1 : 0);
 	vla_item_with_sz(d, short, inums, ffs->interfaces_count);
-	vla_item_with_sz(d, char, raw_descs,
-			ffs->raw_fs_hs_descs_length + ffs->raw_ss_descs_length);
+	vla_item_with_sz(d, char, raw_descs, ffs->raw_descs_length);
 	char *vlabuf;
 
 	ENTER();
@@ -2109,15 +2095,9 @@ static int _ffs_func_bind(struct usb_configuration *c,
 
 	/* Zero */
 	memset(vla_ptr(vlabuf, d, eps), 0, d_eps__sz);
-	/* Copy only raw (hs,fs) descriptors (until ss_magic and ss_count) */
-	memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs + 16,
-		ffs->raw_fs_hs_descs_length);
-	/* Copy SS descs present @ header + hs_fs_descs + ss_magic + ss_count */
-	if (func->ffs->ss_descs_count)
-		memcpy(vla_ptr(vlabuf, d, raw_descs) +
-				ffs->raw_fs_hs_descs_length,
-		       ffs->raw_descs + 16 + ffs->raw_fs_hs_descs_length + 8,
-		       ffs->raw_ss_descs_length);
+	/* Copy descriptors  */
+	memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs,
+	       ffs->raw_descs_length);
 
 	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
 	for (ret = ffs->eps_count; ret; --ret) {
@@ -2599,7 +2579,7 @@ static int _ffs_name_dev(struct ffs_dev *dev, const char *name)
 	existing = _ffs_find_dev(name);
 	if (existing)
 		return -EBUSY;
-	
+
 	dev->name = name;
 
 	return 0;
@@ -2682,7 +2662,7 @@ static void ffs_release_dev(struct ffs_data *ffs_data)
 	ffs_dev = ffs_data->private_data;
 	if (ffs_dev)
 		ffs_dev->mounted = false;
-	
+
 	if (ffs_dev->ffs_release_dev_callback)
 		ffs_dev->ffs_release_dev_callback(ffs_dev);
 
diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h
index 08bf26e..46ddda9 100644
--- a/drivers/usb/gadget/u_fs.h
+++ b/drivers/usb/gadget/u_fs.h
@@ -210,15 +210,13 @@ struct ffs_data {
 
 	/* filled by __ffs_data_got_descs() */
 	/*
-	 * Real descriptors are 16 bytes after raw_descs (so you need
-	 * to skip 16 bytes (ie. ffs->raw_descs + 16) to get to the
-	 * first full speed descriptor).
-	 * raw_fs_hs_descs_length does not have those 16 bytes added.
-	 * ss_descs are 8 bytes (ss_magic + count) pass the hs_descs
+	 * raw_descs is what you kfree, real_descs points inside of raw_descs,
+	 * where full speed, high speed and super speed descriptors start.
+	 * real_descs_length is the length of all those descriptors.
 	 */
+	const void			*raw_descs_data;
 	const void			*raw_descs;
-	unsigned			raw_fs_hs_descs_length;
-	unsigned			raw_ss_descs_length;
+	unsigned			raw_descs_length;
 	unsigned			fs_descs_count;
 	unsigned			hs_descs_count;
 	unsigned			ss_descs_count;
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 0f8f7be..ee6fcbc 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -10,7 +10,14 @@
 
 enum {
 	FUNCTIONFS_DESCRIPTORS_MAGIC = 1,
-	FUNCTIONFS_STRINGS_MAGIC     = 2
+	FUNCTIONFS_STRINGS_MAGIC = 2,
+	FUNCTIONFS_DESCRIPTORS_MAGIC_V2 = 3,
+};
+
+enum functionfs_flags {
+	FUNCTIONFS_HAS_FS_DESC = 1,
+	FUNCTIONFS_HAS_HS_DESC = 2,
+	FUNCTIONFS_HAS_SS_DESC = 4,
 };
 
 #define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C
@@ -30,33 +37,39 @@ struct usb_endpoint_descriptor_no_audio {
 
 
 /*
- * All numbers must be in little endian order.
- */
-
-struct usb_functionfs_descs_head {
-	__le32 magic;
-	__le32 length;
-	__le32 fs_count;
-	__le32 hs_count;
-} __attribute__((packed));
-
-/*
  * Descriptors format:
  *
  * | off | name      | type         | description                          |
  * |-----+-----------+--------------+--------------------------------------|
- * |   0 | magic     | LE32         | FUNCTIONFS_{FS,HS}_DESCRIPTORS_MAGIC |
+ * |   0 | magic     | LE32         | FUNCTIONFS_DESCRIPTORS_MAGIC_V2      |
+ * |   4 | length    | LE32         | length of the whole data chunk       |
+ * |   8 | flags     | LE32         | combination of functionfs_flags      |
+ * |     | fs_count  | LE32         | number of full-speed descriptors     |
+ * |     | hs_count  | LE32         | number of high-speed descriptors     |
+ * |     | ss_count  | LE32         | number of high-speed descriptors     |
+ * |     | fs_descrs | Descriptor[] | list of full-speed descriptors       |
+ * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
+ * |     | ss_descrs | Descriptor[] | list of high-speed descriptors       |
+ *
+ * Depending on which flags are set, various fields may be missing in the
+ * structure.  Any flags that are not recognised cause the whole block to be
+ * rejected with -ENOSYS.
+ *
+ * Legacy descriptors format:
+ *
+ * | off | name      | type         | description                          |
+ * |-----+-----------+--------------+--------------------------------------|
+ * |   0 | magic     | LE32         | FUNCTIONFS_DESCRIPTORS_MAGIC         |
  * |   4 | length    | LE32         | length of the whole data chunk       |
  * |   8 | fs_count  | LE32         | number of full-speed descriptors     |
  * |  12 | hs_count  | LE32         | number of high-speed descriptors     |
  * |  16 | fs_descrs | Descriptor[] | list of full-speed descriptors       |
  * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
- * |     | ss_magic  | LE32         | FUNCTIONFS_SS_DESC_MAGIC             |
- * |     | ss_count  | LE32         | number of super-speed descriptors    |
- * |     | ss_descrs | Descriptor[] | list of super-speed descriptors      |
  *
- * ss_magic: if present then it implies that SS_DESCs are also present
- * descs are just valid USB descriptors and have the following format:
+ * All numbers must be in little endian order.
+ *
+ * Descriptor[] is an array of valid USB descriptors which have the following
+ * format:
  *
  * | off | name            | type | description              |
  * |-----+-----------------+------+--------------------------|
-- 
1.9.0.rc1.175.g0b1dcb5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: gadget: f_fs: Add flags to descriptors block

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

Hi,

On Tue, Feb 25, 2014 at 06:02:18PM +0100, Michal Nazarewicz wrote:
> This reworks the way SuperSpeed descriptors are added and instead of
> having a magick after full and high speed descriptors, it reworks the
> whole descriptors block to include a flags field which lists which
> descriptors are present and makes future extensions possible.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
>  drivers/usb/gadget/f_fs.c           | 136 +++++++++++++++---------------------
>  drivers/usb/gadget/u_fs.h           |  12 ++--
>  include/uapi/linux/usb/functionfs.h |  49 ++++++++-----
>  3 files changed, 94 insertions(+), 103 deletions(-)
> 
> All right, with some fidling with my fan and limiting CPU frequency to
> the mininimum, I've managed to compile this patch and fixed all the
> typos.
> 
> Manu, if you could include it in your series, adjust your user space
> client and test it, it would be wonderful. :]

I'll wait for that then. Note that I want to close my tree next
wednesday tops.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv2] usb: gadget: f_fs: Add flags to descriptors block

[Manu Gautam]

On 2/25/2014 10:32 PM, Michal Nazarewicz wrote:
> This reworks the way SuperSpeed descriptors are added and instead of
> having a magick after full and high speed descriptors, it reworks the
> whole descriptors block to include a flags field which lists which
> descriptors are present and makes future extensions possible.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---

> Manu, if you could include it in your series, adjust your user space
> client and test it, it would be wonderful. :]
> 

I have tested this with my userspace client. It works fine. I checked
both DESC_MAGIC and DESC_MAGIC_V2 with and without passing ss_descs.
I faced one issue when I was using MAGIC_V2 and specified ss_count as 0.
This resulted in driver treating ss_count as start of descs blob.
Later noticed that this is now changed and depending on the flags
(descriptors) passed, these fields may or may not be present. I see that
you have explicitly mentioned the same in functionfs.h. This looks fine
to me.
Additionally, there were could of typos (mentioned below) which I have
fixed in the patch.

>  
>  #define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C

can be removed.

> + * |     | fs_count  | LE32         | number of full-speed descriptors     |
> + * |     | hs_count  | LE32         | number of high-speed descriptors     |
> + * |     | ss_count  | LE32         | number of high-speed descriptors     |

s/full/super

> + * |     | fs_descrs | Descriptor[] | list of full-speed descriptors       |
> + * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
> + * |     | ss_descrs | Descriptor[] | list of high-speed descriptors       |

s/full/super

> + *
> + * Depending on which flags are set, various fields may be missing in the
> + * structure.  Any flags that are not recognised cause the whole block to be
> + * rejected with -ENOSYS.

This is what I was talking about.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

RE: [PATCHv2] usb: gadget: f_fs: Add flags to descriptors block

[Krzysztof Opasiak]

Hello Michal,

> -----Original Message-----
> From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Michal Nazarewicz
> Sent: Tuesday, February 25, 2014 6:02 PM
> To: Manu Gautam
> Cc: balbi-l0cyMroinI0@public.gmane.org; jackp-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org; pheatwol-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org;
> linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> benoit-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org; Andrzej Pietrasiewicz;
> gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org; Manu Gautam
> Subject: [PATCHv2] usb: gadget: f_fs: Add flags to descriptors
> block
> 
> This reworks the way SuperSpeed descriptors are added and instead
> of
> having a magick after full and high speed descriptors, it reworks
> the
> whole descriptors block to include a flags field which lists which
> descriptors are present and makes future extensions possible.
> 
> Signed-off-by: Michal Nazarewicz <mina86-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/usb/gadget/f_fs.c           | 136 +++++++++++++++---------
> ------------
>  drivers/usb/gadget/u_fs.h           |  12 ++--
>  include/uapi/linux/usb/functionfs.h |  49 ++++++++-----
>  3 files changed, 94 insertions(+), 103 deletions(-)
> 
> All right, with some fidling with my fan and limiting CPU frequency
> to
> the mininimum, I've managed to compile this patch and fixed all the
> typos.
> 
> Manu, if you could include it in your series, adjust your user
> space
> client and test it, it would be wonderful. :]
> 

(...) snip

> diff --git a/include/uapi/linux/usb/functionfs.h
> b/include/uapi/linux/usb/functionfs.h
> index 0f8f7be..ee6fcbc 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -10,7 +10,14 @@
> 
>  enum {
>  	FUNCTIONFS_DESCRIPTORS_MAGIC = 1,
> -	FUNCTIONFS_STRINGS_MAGIC     = 2
> +	FUNCTIONFS_STRINGS_MAGIC = 2,
> +	FUNCTIONFS_DESCRIPTORS_MAGIC_V2 = 3,
> +};
> +
> +enum functionfs_flags {
> +	FUNCTIONFS_HAS_FS_DESC = 1,
> +	FUNCTIONFS_HAS_HS_DESC = 2,
> +	FUNCTIONFS_HAS_SS_DESC = 4,
>  };
> 
>  #define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C
> @@ -30,33 +37,39 @@ struct usb_endpoint_descriptor_no_audio {
> 
> 
>  /*
> - * All numbers must be in little endian order.
> - */
> -
> -struct usb_functionfs_descs_head {
> -	__le32 magic;
> -	__le32 length;
> -	__le32 fs_count;
> -	__le32 hs_count;
> -} __attribute__((packed));
> -
> -/*

I have tried to compile FFS examples with v3.15-rc8 but I have faced an
issue that they are unable to build due to missing definition of this
structure. The same is with adb, sdb and all userspace apps which use
legacy API. What is the reason of removing it? Was your intentions?

Maybe would be a good idea to leave this structure untouched as legacy
userspace API?

There is also no structure definition for new API, maybe suitable
structure should be defined (struct usb_functionfs_descs_head2 for
example) to make userspace life easier?


--
BR's
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: gadget: f_fs: Add flags to descriptors block

[Michal Nazarewicz]

>> -struct usb_functionfs_descs_head {
>> -	__le32 magic;
>> -	__le32 length;
>> -	__le32 fs_count;
>> -	__le32 hs_count;
>> -} __attribute__((packed));

On Wed, Jun 04 2014, Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> I have tried to compile FFS examples with v3.15-rc8 but I have faced an
> issue that they are unable to build due to missing definition of this
> structure.

https://lkml.org/lkml/2014/5/21/522

> The same is with adb, sdb and all userspace apps which use legacy
> API. What is the reason of removing it? Was your intentions?

It was a mistake on my part.

> Maybe would be a good idea to leave this structure untouched as legacy
> userspace API?

I don't care either way to be honest.  I guess if there's non-negligible
number of usb_functionfs_descs_head structure users out there, I can
prepare a CL adding the structure back.

> There is also no structure definition for new API, maybe suitable
> structure should be defined (struct usb_functionfs_descs_head2 for
> example) to make userspace life easier?

That structure would not have many fields though, since what exactly the
header contains depends on the flags.  Whether fs_count, hs_count and
ss_count fields are present depends on which bits in the flags are set.
So the usb_functionfs_descs_head2 structure could only contain:

	struct usb_functionfs_descs_head2 {
		__le32 magic;
		__le32 length;
		__le32 flags;
	};

I'm not sure if that would be particularly helpful.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>--<xmpp:mina86-/eSpBmjxGS4dnm+yROfE0A@public.gmane.org>--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv2] usb: gadget: f_fs: Add flags to descriptors block

[Krzysztof Opasiak]

Hi,

> -----Original Message-----
> From: Michal Nazarewicz [mailto:mpn@google.com] On Behalf Of Michal
> Nazarewicz
> Sent: Wednesday, June 04, 2014 9:06 PM
> To: Krzysztof Opasiak; 'Manu Gautam'
> Cc: balbi@ti.com; jackp@codeaurora.org; pheatwol@codeaurora.org;
> linux-usb@vger.kernel.org; linux-arm-msm@vger.kernel.org;
> benoit@android.com; Andrzej Pietrasiewicz;
> gregkh@linuxfoundation.org; 'Manu Gautam'; Karol Lewandowski; Marek
> Szyprowski
> Subject: Re: [PATCHv2] usb: gadget: f_fs: Add flags to descriptors
> block
> 
> >> -struct usb_functionfs_descs_head {
> >> -	__le32 magic;
> >> -	__le32 length;
> >> -	__le32 fs_count;
> >> -	__le32 hs_count;
> >> -} __attribute__((packed));
> 
> On Wed, Jun 04 2014, Krzysztof Opasiak <k.opasiak@samsung.com>
> wrote:
> > I have tried to compile FFS examples with v3.15-rc8 but I have
> faced an
> > issue that they are unable to build due to missing definition of
> this
> > structure.
> 
> https://lkml.org/lkml/2014/5/21/522

Thank you for this link, I have missed that patch.

> I don't care either way to be honest.  I guess if there's non-
> negligible
> number of usb_functionfs_descs_head structure users out there, I
> can
> prepare a CL adding the structure back.

The number of FFS users which I know is limited to 3 maybe 4, so it's not too much. I can port all of them to new API but I'm not sure if we would like to do this?

> 
> > There is also no structure definition for new API, maybe suitable
> > structure should be defined (struct usb_functionfs_descs_head2
> for
> > example) to make userspace life easier?
> 
> That structure would not have many fields though, since what
> exactly the
> header contains depends on the flags.  Whether fs_count, hs_count
> and
> ss_count fields are present depends on which bits in the flags are
> set.
> So the usb_functionfs_descs_head2 structure could only contain:
> 
> 	struct usb_functionfs_descs_head2 {
> 		__le32 magic;
> 		__le32 length;
> 		__le32 flags;
> 	};
> 
> I'm not sure if that would be particularly helpful.

I think that this structure is much more user friendly than copy-paste of those 3 fields in each ffs program.

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