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@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
