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

[Manu Gautam <mgautam@codeaurora.org>]

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