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

[Manu Gautam <mgautam@codeaurora.org>]

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