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

[PATCH v2 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 also requires changing usb_functionfs_descs_head
to accommodate ss_count i.e. SuperSpeed Descriptors
count.

Signed-off-by: Manu Gautam <mgautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/usb/gadget/f_fs.c           | 148 +++++++++++++++++++++++++++---------
 include/uapi/linux/usb/functionfs.h |   5 +-
 2 files changed, 115 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index f394f29..95a5746 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -194,16 +194,18 @@ 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_descs_length and
-	 * raw_fs_descs_length do not have those 16 bytes added.
+	 * Real descriptors are 20 bytes after raw_descs (so you need
+	 * to skip 20 bytes (ie. ffs->raw_descs + 20) to get to the
+	 * first full speed descriptor).  raw_(fs|hs|ss)descs_length
+	 * do not have those 20 bytes added.
 	 */
 	const void			*raw_descs;
-	unsigned			raw_descs_length;
+	unsigned			raw_hs_descs_length;
 	unsigned			raw_fs_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;
@@ -301,8 +303,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;
 
@@ -1358,10 +1360,12 @@ static void ffs_data_reset(struct ffs_data *ffs)
 	ffs->raw_strings = NULL;
 	ffs->stringtabs = NULL;
 
-	ffs->raw_descs_length = 0;
+	ffs->raw_hs_descs_length = 0;
 	ffs->raw_fs_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;
@@ -1569,7 +1573,20 @@ 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;
+
+		ds = ep->descs[desc_idx];
+		if (!ds) {
+			ret = -EINVAL;
+			break;
+		}
 
 		ep->ep->driver_data = ep;
 		ep->ep->desc = ds;
@@ -1704,6 +1721,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:
@@ -1814,8 +1837,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;
+	int fs_len, hs_len, ss_len, ret = -EINVAL;
 	char *data = _data;
 
 	ENTER();
@@ -1825,12 +1848,13 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 		goto error;
 	fs_count = get_unaligned_le32(data +  8);
 	hs_count = get_unaligned_le32(data + 12);
+	ss_count = get_unaligned_le32(data + 16);
 
-	if (!fs_count && !hs_count)
+	if (!fs_count && !hs_count && !ss_count)
 		goto einval;
 
-	data += 16;
-	len  -= 16;
+	data += 20;
+	len  -= 20;
 
 	if (likely(fs_count)) {
 		fs_len = ffs_do_descs(fs_count, data, len,
@@ -1847,11 +1871,29 @@ 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 (likely(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;
 	}
 
@@ -1859,10 +1901,12 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 		goto einval;
 
 	ffs->raw_fs_descs_length = fs_len;
-	ffs->raw_descs_length    = fs_len + ret;
+	ffs->raw_hs_descs_length    = ffs->raw_fs_descs_length + hs_len;
+	ffs->raw_ss_descs_length = ffs->raw_hs_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;
 
@@ -2086,16 +2130,23 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 	 * If hs_descriptors is not NULL then we are reading hs
 	 * descriptors now
 	 */
-	const int isHS = func->function.hs_descriptors != NULL;
-	unsigned idx;
+	const int is_hs = func->function.hs_descriptors != NULL;
+	const int is_ss = func->function.ss_descriptors != NULL;
+	unsigned ep_desc_id, idx;
 
 	if (type != FFS_DESCRIPTOR)
 		return 0;
 
-	if (isHS)
+	if (is_ss) {
+		func->function.ss_descriptors[(long)valuep] = desc;
+		ep_desc_id = 2;
+	} else if (is_hs) {
 		func->function.hs_descriptors[(long)valuep] = desc;
-	else
+		ep_desc_id = 1;
+	} else {
 		func->function.fs_descriptors[(long)valuep]    = desc;
+		ep_desc_id = 0;
+	}
 
 	if (!desc || desc->bDescriptorType != USB_DT_ENDPOINT)
 		return 0;
@@ -2103,13 +2154,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])) {
+	if (unlikely(ffs_ep->descs[ep_desc_id])) {
 		pr_vdebug("two %sspeed descriptors for EP %d\n",
-			  isHS ? "high" : "full",
+			  is_ss ? "super" : "high/full",
 			  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) {
@@ -2204,8 +2255,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 */
 	struct {
@@ -2214,9 +2267,10 @@ static int ffs_func_bind(struct usb_configuration *c,
 			*fs_descs[full ? ffs->fs_descs_count + 1 : 0];
 		struct usb_descriptor_header
 			*hs_descs[high ? ffs->hs_descs_count + 1 : 0];
+		struct usb_descriptor_header
+			*ss_descs[super ? ffs->ss_descs_count + 1 : 0];
 		short inums[ffs->interfaces_count];
-		char raw_descs[high ? ffs->raw_descs_length
-				    : ffs->raw_fs_descs_length];
+		char raw_descs[ffs->raw_ss_descs_length];
 	} *data;
 
 	ENTER();
@@ -2232,7 +2286,7 @@ static int ffs_func_bind(struct usb_configuration *c,
 
 	/* Zero */
 	memset(data->eps, 0, sizeof data->eps);
-	memcpy(data->raw_descs, ffs->raw_descs + 16, sizeof data->raw_descs);
+	memcpy(data->raw_descs, ffs->raw_descs + 20, sizeof(data->raw_descs));
 	memset(data->inums, 0xff, sizeof data->inums);
 	for (ret = ffs->eps_count; ret; --ret)
 		data->eps[ret].num = -1;
@@ -2248,32 +2302,52 @@ static int ffs_func_bind(struct usb_configuration *c,
 	 */
 	if (likely(full)) {
 		func->function.fs_descriptors = data->fs_descs;
-		ret = ffs_do_descs(ffs->fs_descs_count,
+		fs_len = ffs_do_descs(ffs->fs_descs_count,
 				   data->raw_descs,
-				   sizeof data->raw_descs,
+				   sizeof(data->raw_descs),
 				   __ffs_func_bind_do_descs, func);
-		if (unlikely(ret < 0))
+		if (unlikely(fs_len < 0)) {
+			ret = fs_len;
 			goto error;
+		}
 	} else {
-		ret = 0;
+		fs_len = 0;
 	}
 
 	if (likely(high)) {
 		func->function.hs_descriptors = data->hs_descs;
-		ret = ffs_do_descs(ffs->hs_descs_count,
-				   data->raw_descs + ret,
-				   (sizeof data->raw_descs) - ret,
+		hs_len = ffs_do_descs(ffs->hs_descs_count,
+				   data->raw_descs + fs_len,
+				   (sizeof(data->raw_descs)) - 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 = data->ss_descs;
+		ret = ffs_do_descs(ffs->ss_descs_count,
+				   data->raw_descs + fs_len + hs_len,
+				   (sizeof(data->raw_descs)) - fs_len - hs_len,
+				   __ffs_func_bind_do_descs, func);
+		if (unlikely(ret < 0))
+			goto error;
+	}
+
+
 	/*
 	 * Now handle interface numbers allocation and interface and
 	 * endpoint numbers rewriting.  We can do that in one go
 	 * now.
 	 */
 	ret = ffs_do_descs(ffs->fs_descs_count +
-			   (high ? ffs->hs_descs_count : 0),
-			   data->raw_descs, sizeof data->raw_descs,
+			   (high ? ffs->hs_descs_count : 0) +
+			   (super ? ffs->ss_descs_count : 0),
+			   data->raw_descs, sizeof(data->raw_descs),
 			   __ffs_func_bind_do_nums, func);
 	if (unlikely(ret < 0))
 		goto error;
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index d6b0128..d6940d7 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -37,6 +37,7 @@ struct usb_functionfs_descs_head {
 	__le32 length;
 	__le32 fs_count;
 	__le32 hs_count;
+	__le32 ss_count;
 } __attribute__((packed));
 
 /*
@@ -48,8 +49,10 @@ struct usb_functionfs_descs_head {
  * |   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       |
+ * |  16 | ss_count  | LE32         | number of super-speed descriptors    |
+ * |  20 | fs_descrs | Descriptor[] | list of full-speed descriptors       |
  * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
+ * |     | ss_descrs | Descriptor[] | list of super-speed descriptors      |
  *
  * descs are just valid USB descriptors and have the following format:
  *
-- 
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

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

[Manu Gautam]

Hi Felipe,

I wanted to mention one point with respect to this patch:
Below changes in the functionfs.h to add ss_count (super speed 
descriptors count) in desc_header (which is passed from userspace) make 
the driver incompatible with existing userspace applications compiled 
against old header file. Let me know if that is acceptable.
We are using this driver with Android for adbd (android debug bridge) 
and these changes are required to support adb over Super Speed 
controllers e.g. DWC3 along with changed in adbd to pass SS EP and 
companion descriptors.

Regards,
Manu

On 9/20/2013 5:25 PM, Manu Gautam wrote:
> diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
> index d6b0128..d6940d7 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -37,6 +37,7 @@ struct usb_functionfs_descs_head {
>   	__le32 length;
>   	__le32 fs_count;
>   	__le32 hs_count;
> +	__le32 ss_count;
>   } __attribute__((packed));
>   
>   /*
> @@ -48,8 +49,10 @@ struct usb_functionfs_descs_head {
>    * |   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       |
> + * |  16 | ss_count  | LE32         | number of super-speed descriptors    |
> + * |  20 | fs_descrs | Descriptor[] | list of full-speed descriptors       |
>    * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
> + * |     | ss_descrs | Descriptor[] | list of super-speed descriptors      |

   

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

[Felipe Balbi]

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

Hi,

(please avoid top-posting)

On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
> Hi Felipe,
> 
> I wanted to mention one point with respect to this patch: Below
> changes in the functionfs.h to add ss_count (super speed descriptors
> count) in desc_header (which is passed from userspace) make the driver
> incompatible with existing userspace applications compiled against old
> header file. Let me know if that is acceptable.  We are using this
> driver with Android for adbd (android debug bridge) and these changes
> are required to support adb over Super Speed controllers e.g. DWC3
> along with changed in adbd to pass SS EP and companion descriptors.

Good you mentioned, it saves me the trouble of reviewing this patch :-)

It's not acceptable to break userspace ABI at all. If you want
SuperSpeed support on function fs, we need to figure out a way to do so
without breaking userspace.

This might mean adding a separate userspace interface to be used with
superspeed. While at that, we might want to add a few bytes of reserved,
unused space in our structures for situations where we need to add more
data into it, just to make it slightly future proof.

-- 
balbi

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

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

[Manu Gautam]

On 9/26/2013 2:10 AM, Felipe Balbi wrote:
> Hi,
>
> (please avoid top-posting)
>
> On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
>> Hi Felipe,
>>
>> I wanted to mention one point with respect to this patch: Below
>> changes in the functionfs.h to add ss_count (super speed descriptors
>> count) in desc_header (which is passed from userspace) make the driver
>> incompatible with existing userspace applications compiled against old
>> header file. Let me know if that is acceptable.  We are using this
>> driver with Android for adbd (android debug bridge) and these changes
>> are required to support adb over Super Speed controllers e.g. DWC3
>> along with changed in adbd to pass SS EP and companion descriptors.
>
> Good you mentioned, it saves me the trouble of reviewing this patch :-)
>
> It's not acceptable to break userspace ABI at all. If you want
> SuperSpeed support on function fs, we need to figure out a way to do so
> without breaking userspace.
>
> This might mean adding a separate userspace interface to be used with
> superspeed. While at that, we might want to add a few bytes of reserved,
> unused space in our structures for situations where we need to add more
> data into it, just to make it slightly future proof.
>

Thanks for your reply.
As you suggested we can have a different interface for super speed
which would be optional to workaround ABI compatibility issue.
Let me know if below interface looks fine to you, I will then implement
accordingly:

diff --git a/include/uapi/linux/usb/functionfs.h 
b/include/uapi/linux/usb/functionfs.h
index d6b0128..b8cb740 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -9,8 +9,9 @@


  enum {
-       FUNCTIONFS_DESCRIPTORS_MAGIC = 1,
-       FUNCTIONFS_STRINGS_MAGIC     = 2
+       FUNCTIONFS_DESCRIPTORS_MAGIC     = 1,
+       FUNCTIONFS_STRINGS_MAGIC         = 2,
+       FUNCTIONFS_SS_DESCRIPTORS_MAGIC  = 3
  };


@@ -60,6 +61,25 @@ struct usb_functionfs_descs_head {
   * |   2 | payload         |      | descriptor's payload     |
   */

+struct usb_functionfs_ss_descs_head {
+       __le32 magic;
+       __le32 length;
+       __le32 reserved;
+       __le32 ss_count;
+} __attribute__((packed));
+
+/*
+ * SS Descriptors format:
+ *
+ * | off | name      | type         | description 
     |
+ * 
|-----+-----------+--------------+--------------------------------------|
+ * |   0 | magic     | LE32         | FUNCTIONFS_SS_DESCRIPTORS_MAGIC 
     |
+ * |   4 | length    | LE32         | length of the whole data chunk 
     |
+ * |   8 | ss_count  | LE32         | number of super-speed descriptors 
    |
+ * |  12 | reserved field 
     |
+ * |  16 | ss_descrs | Descriptor[] | list of super-speed descriptors 
     |
+ */
+
  struct usb_functionfs_strings_head {
         __le32 magic;
         __le32 length;

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

[Paul Zimmerman]

> From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Manu Gautam
> Sent: Thursday, September 26, 2013 12:08 AM
> 
> On 9/26/2013 2:10 AM, Felipe Balbi wrote:
> >
> > On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
> >> Hi Felipe,
> >>
> >> I wanted to mention one point with respect to this patch: Below
> >> changes in the functionfs.h to add ss_count (super speed descriptors
> >> count) in desc_header (which is passed from userspace) make the driver
> >> incompatible with existing userspace applications compiled against old
> >> header file. Let me know if that is acceptable.  We are using this
> >> driver with Android for adbd (android debug bridge) and these changes
> >> are required to support adb over Super Speed controllers e.g. DWC3
> >> along with changed in adbd to pass SS EP and companion descriptors.
> >
> > Good you mentioned, it saves me the trouble of reviewing this patch :-)
> >
> > It's not acceptable to break userspace ABI at all. If you want
> > SuperSpeed support on function fs, we need to figure out a way to do so
> > without breaking userspace.
> >
> > This might mean adding a separate userspace interface to be used with
> > superspeed. While at that, we might want to add a few bytes of reserved,
> > unused space in our structures for situations where we need to add more
> > data into it, just to make it slightly future proof.
> >
> 
> Thanks for your reply.
> As you suggested we can have a different interface for super speed
> which would be optional to workaround ABI compatibility issue.
> Let me know if below interface looks fine to you, I will then implement
> accordingly:

Just a suggestion: Instead of a new interface for SuperSpeed, why not
just add the new fields to the end of the ffs_data struct? And have the
functions that copy the struct to/from userspace check the 'len' value
passed in, and only handle the SuperSpeed stuff if the length indicates
it is new userspace?

-- 
Paul

--
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: [PATCH v2 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode

[Manu Gautam]

On 9/28/2013 1:52 AM, Paul Zimmerman wrote:
>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Manu Gautam
>> Sent: Thursday, September 26, 2013 12:08 AM
>>
>> On 9/26/2013 2:10 AM, Felipe Balbi wrote:
>>>
>>> On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
>>>> Hi Felipe,
>>>>
>>>> I wanted to mention one point with respect to this patch: Below
>>>> changes in the functionfs.h to add ss_count (super speed descriptors
>>>> count) in desc_header (which is passed from userspace) make the driver
>>>> incompatible with existing userspace applications compiled against old
>>>> header file. Let me know if that is acceptable.  We are using this
>>>> driver with Android for adbd (android debug bridge) and these changes
>>>> are required to support adb over Super Speed controllers e.g. DWC3
>>>> along with changed in adbd to pass SS EP and companion descriptors.
>>>
>>> Good you mentioned, it saves me the trouble of reviewing this patch :-)
>>>
>>> It's not acceptable to break userspace ABI at all. If you want
>>> SuperSpeed support on function fs, we need to figure out a way to do so
>>> without breaking userspace.
>>>
>>> This might mean adding a separate userspace interface to be used with
>>> superspeed. While at that, we might want to add a few bytes of reserved,
>>> unused space in our structures for situations where we need to add more
>>> data into it, just to make it slightly future proof.
>>>
>>
>> Thanks for your reply.
>> As you suggested we can have a different interface for super speed
>> which would be optional to workaround ABI compatibility issue.
>> Let me know if below interface looks fine to you, I will then implement
>> accordingly:
> 
> Just a suggestion: Instead of a new interface for SuperSpeed, why not
> just add the new fields to the end of the ffs_data struct? And have the
> functions that copy the struct to/from userspace check the 'len' value
> passed in, and only handle the SuperSpeed stuff if the length indicates
> it is new userspace?
> 

Initially I thought on similar lines but then adding a new interface for
SS looked cleaner to me. But, your suggestion also make sense as we can
avoid extra system call and the same interface can be extended later.
One more thing we can do is to add a magic number after hs_desc (i.e. at
the end of existing ffs_data) to specify that ss_descriptors are following.
This can be used by kernel driver to check if userspace is trying pass
ss desc only or some invalid data. 
Felipe: Your recommendation on this?

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

[Felipe Balbi]

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

Hi,

On Mon, Sep 30, 2013 at 02:31:50PM +0530, Manu Gautam wrote:
> On 9/28/2013 1:52 AM, Paul Zimmerman wrote:
> >> From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-owner-u79uwXL29TY@public.gmane.orgnel.org] On Behalf Of Manu Gautam
> >> Sent: Thursday, September 26, 2013 12:08 AM
> >>
> >> On 9/26/2013 2:10 AM, Felipe Balbi wrote:
> >>>
> >>> On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
> >>>> Hi Felipe,
> >>>>
> >>>> I wanted to mention one point with respect to this patch: Below
> >>>> changes in the functionfs.h to add ss_count (super speed descriptors
> >>>> count) in desc_header (which is passed from userspace) make the driver
> >>>> incompatible with existing userspace applications compiled against old
> >>>> header file. Let me know if that is acceptable.  We are using this
> >>>> driver with Android for adbd (android debug bridge) and these changes
> >>>> are required to support adb over Super Speed controllers e.g. DWC3
> >>>> along with changed in adbd to pass SS EP and companion descriptors.
> >>>
> >>> Good you mentioned, it saves me the trouble of reviewing this patch :-)
> >>>
> >>> It's not acceptable to break userspace ABI at all. If you want
> >>> SuperSpeed support on function fs, we need to figure out a way to do so
> >>> without breaking userspace.
> >>>
> >>> This might mean adding a separate userspace interface to be used with
> >>> superspeed. While at that, we might want to add a few bytes of reserved,
> >>> unused space in our structures for situations where we need to add more
> >>> data into it, just to make it slightly future proof.
> >>>
> >>
> >> Thanks for your reply.
> >> As you suggested we can have a different interface for super speed
> >> which would be optional to workaround ABI compatibility issue.
> >> Let me know if below interface looks fine to you, I will then implement
> >> accordingly:
> > 
> > Just a suggestion: Instead of a new interface for SuperSpeed, why not
> > just add the new fields to the end of the ffs_data struct? And have the
> > functions that copy the struct to/from userspace check the 'len' value
> > passed in, and only handle the SuperSpeed stuff if the length indicates
> > it is new userspace?
> > 
> 
> Initially I thought on similar lines but then adding a new interface for
> SS looked cleaner to me. But, your suggestion also make sense as we can
> avoid extra system call and the same interface can be extended later.
> One more thing we can do is to add a magic number after hs_desc (i.e. at
> the end of existing ffs_data) to specify that ss_descriptors are following.
> This can be used by kernel driver to check if userspace is trying pass
> ss desc only or some invalid data. 
> Felipe: Your recommendation on this?

We need to have some more people look at this. I remember there were
always some concerns about Chris architecture when doing such changes.

-- 
balbi

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

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

[Manu Gautam]

On 10/1/2013 8:07 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Sep 30, 2013 at 02:31:50PM +0530, Manu Gautam wrote:
>> On 9/28/2013 1:52 AM, Paul Zimmerman wrote:
>>>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Manu Gautam
>>>> Sent: Thursday, September 26, 2013 12:08 AM
>>>>
>>>> On 9/26/2013 2:10 AM, Felipe Balbi wrote:
>>>>>
>>>>> On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
>>>>>> Hi Felipe,
>>>>>>
>>>>>> I wanted to mention one point with respect to this patch: Below
>>>>>> changes in the functionfs.h to add ss_count (super speed descriptors
>>>>>> count) in desc_header (which is passed from userspace) make the driver
>>>>>> incompatible with existing userspace applications compiled against old
>>>>>> header file. Let me know if that is acceptable.  We are using this
>>>>>> driver with Android for adbd (android debug bridge) and these changes
>>>>>> are required to support adb over Super Speed controllers e.g. DWC3
>>>>>> along with changed in adbd to pass SS EP and companion descriptors.
>>>>>
>>>>> Good you mentioned, it saves me the trouble of reviewing this patch :-)
>>>>>
>>>>> It's not acceptable to break userspace ABI at all. If you want
>>>>> SuperSpeed support on function fs, we need to figure out a way to do so
>>>>> without breaking userspace.
>>>>>
>>>>> This might mean adding a separate userspace interface to be used with
>>>>> superspeed. While at that, we might want to add a few bytes of reserved,
>>>>> unused space in our structures for situations where we need to add more
>>>>> data into it, just to make it slightly future proof.
>>>>>
>>>>
>>>> Thanks for your reply.
>>>> As you suggested we can have a different interface for super speed
>>>> which would be optional to workaround ABI compatibility issue.
>>>> Let me know if below interface looks fine to you, I will then implement
>>>> accordingly:
>>>
>>> Just a suggestion: Instead of a new interface for SuperSpeed, why not
>>> just add the new fields to the end of the ffs_data struct? And have the
>>> functions that copy the struct to/from userspace check the 'len' value
>>> passed in, and only handle the SuperSpeed stuff if the length indicates
>>> it is new userspace?
>>>
>>
>> Initially I thought on similar lines but then adding a new interface for
>> SS looked cleaner to me. But, your suggestion also make sense as we can
>> avoid extra system call and the same interface can be extended later.
>> One more thing we can do is to add a magic number after hs_desc (i.e. at
>> the end of existing ffs_data) to specify that ss_descriptors are following.
>> This can be used by kernel driver to check if userspace is trying pass
>> ss desc only or some invalid data. 
>> Felipe: Your recommendation on this?
> 
> We need to have some more people look at this. I remember there were
> always some concerns about Chris architecture when doing such changes.
> 

Can you please add appropriate folks to this thread who can check this as
well?

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

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

[Manu Gautam]

On 10/2/2013 10:06 AM, Manu Gautam wrote:
> On 10/1/2013 8:07 PM, Felipe Balbi wrote:
>> Hi,
>>
>> On Mon, Sep 30, 2013 at 02:31:50PM +0530, Manu Gautam wrote:
>>> On 9/28/2013 1:52 AM, Paul Zimmerman wrote:
>>>>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Manu Gautam
>>>>> Sent: Thursday, September 26, 2013 12:08 AM
>>>>>
>>>>> On 9/26/2013 2:10 AM, Felipe Balbi wrote:
>>>>>>
>>>>>> On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
>>>>>>> Hi Felipe,
>>>>>>>
>>>>>>> I wanted to mention one point with respect to this patch: Below
>>>>>>> changes in the functionfs.h to add ss_count (super speed descriptors
>>>>>>> count) in desc_header (which is passed from userspace) make the driver
>>>>>>> incompatible with existing userspace applications compiled against old
>>>>>>> header file. Let me know if that is acceptable.  We are using this
>>>>>>> driver with Android for adbd (android debug bridge) and these changes
>>>>>>> are required to support adb over Super Speed controllers e.g. DWC3
>>>>>>> along with changed in adbd to pass SS EP and companion descriptors.
>>>>>>
>>>>>> Good you mentioned, it saves me the trouble of reviewing this patch :-)
>>>>>>
>>>>>> It's not acceptable to break userspace ABI at all. If you want
>>>>>> SuperSpeed support on function fs, we need to figure out a way to do so
>>>>>> without breaking userspace.
>>>>>>
>>>>>> This might mean adding a separate userspace interface to be used with
>>>>>> superspeed. While at that, we might want to add a few bytes of reserved,
>>>>>> unused space in our structures for situations where we need to add more
>>>>>> data into it, just to make it slightly future proof.
>>>>>>
>>>>>
>>>>> Thanks for your reply.
>>>>> As you suggested we can have a different interface for super speed
>>>>> which would be optional to workaround ABI compatibility issue.
>>>>> Let me know if below interface looks fine to you, I will then implement
>>>>> accordingly:
>>>>
>>>> Just a suggestion: Instead of a new interface for SuperSpeed, why not
>>>> just add the new fields to the end of the ffs_data struct? And have the
>>>> functions that copy the struct to/from userspace check the 'len' value
>>>> passed in, and only handle the SuperSpeed stuff if the length indicates
>>>> it is new userspace?
>>>>
>>>
>>> Initially I thought on similar lines but then adding a new interface for
>>> SS looked cleaner to me. But, your suggestion also make sense as we can
>>> avoid extra system call and the same interface can be extended later.
>>> One more thing we can do is to add a magic number after hs_desc (i.e. at
>>> the end of existing ffs_data) to specify that ss_descriptors are following.
>>> This can be used by kernel driver to check if userspace is trying pass
>>> ss desc only or some invalid data. 
>>> Felipe: Your recommendation on this?
>>
>> We need to have some more people look at this. I remember there were
>> always some concerns about Chris architecture when doing such changes.
>>
> 
> Can you please add appropriate folks to this thread who can check this as
> well?
> 

I have CC'ed  Michal and Andrzej as they have contributed to this driver.
Followed is the interface enhancement that I am suggesting:

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 v2 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode

[Felipe Balbi]

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

Hi,

On Tue, Oct 08, 2013 at 09:52:55AM +0530, Manu Gautam wrote:
> On 10/2/2013 10:06 AM, Manu Gautam wrote:
> > On 10/1/2013 8:07 PM, Felipe Balbi wrote:
> >> Hi,
> >>
> >> On Mon, Sep 30, 2013 at 02:31:50PM +0530, Manu Gautam wrote:
> >>> On 9/28/2013 1:52 AM, Paul Zimmerman wrote:
> >>>>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Manu Gautam
> >>>>> Sent: Thursday, September 26, 2013 12:08 AM
> >>>>>
> >>>>> On 9/26/2013 2:10 AM, Felipe Balbi wrote:
> >>>>>>
> >>>>>> On Tue, Sep 24, 2013 at 03:00:20PM +0530, Manu Gautam wrote:
> >>>>>>> Hi Felipe,
> >>>>>>>
> >>>>>>> I wanted to mention one point with respect to this patch: Below
> >>>>>>> changes in the functionfs.h to add ss_count (super speed descriptors
> >>>>>>> count) in desc_header (which is passed from userspace) make the driver
> >>>>>>> incompatible with existing userspace applications compiled against old
> >>>>>>> header file. Let me know if that is acceptable.  We are using this
> >>>>>>> driver with Android for adbd (android debug bridge) and these changes
> >>>>>>> are required to support adb over Super Speed controllers e.g. DWC3
> >>>>>>> along with changed in adbd to pass SS EP and companion descriptors.
> >>>>>>
> >>>>>> Good you mentioned, it saves me the trouble of reviewing this patch :-)
> >>>>>>
> >>>>>> It's not acceptable to break userspace ABI at all. If you want
> >>>>>> SuperSpeed support on function fs, we need to figure out a way to do so
> >>>>>> without breaking userspace.
> >>>>>>
> >>>>>> This might mean adding a separate userspace interface to be used with
> >>>>>> superspeed. While at that, we might want to add a few bytes of reserved,
> >>>>>> unused space in our structures for situations where we need to add more
> >>>>>> data into it, just to make it slightly future proof.
> >>>>>>
> >>>>>
> >>>>> Thanks for your reply.
> >>>>> As you suggested we can have a different interface for super speed
> >>>>> which would be optional to workaround ABI compatibility issue.
> >>>>> Let me know if below interface looks fine to you, I will then implement
> >>>>> accordingly:
> >>>>
> >>>> Just a suggestion: Instead of a new interface for SuperSpeed, why not
> >>>> just add the new fields to the end of the ffs_data struct? And have the
> >>>> functions that copy the struct to/from userspace check the 'len' value
> >>>> passed in, and only handle the SuperSpeed stuff if the length indicates
> >>>> it is new userspace?
> >>>>
> >>>
> >>> Initially I thought on similar lines but then adding a new interface for
> >>> SS looked cleaner to me. But, your suggestion also make sense as we can
> >>> avoid extra system call and the same interface can be extended later.
> >>> One more thing we can do is to add a magic number after hs_desc (i.e. at
> >>> the end of existing ffs_data) to specify that ss_descriptors are following.
> >>> This can be used by kernel driver to check if userspace is trying pass
> >>> ss desc only or some invalid data. 
> >>> Felipe: Your recommendation on this?
> >>
> >> We need to have some more people look at this. I remember there were
> >> always some concerns about Chris architecture when doing such changes.
> >>
> > 
> > Can you please add appropriate folks to this thread who can check this as
> > well?
> > 
> 
> I have CC'ed  Michal and Andrzej as they have contributed to this driver.
> Followed is the interface enhancement that I am suggesting:
> 
> 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              |

any comments here ? nobody ? Manu, if nobody complains in another week,
please send this hunk as a formal patch.

-- 
balbi

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

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

[Manu Gautam]

On 11/26/2013 11:11 PM, Felipe Balbi wrote:
> 
> any comments here ? nobody ? Manu, if nobody complains in another week,
> please send this hunk as a formal patch.
> 

I have just sent a Patch (v3).

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