| Message ID | 5253889F.8010005@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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
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 |