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

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

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