| Message ID | 5243DD3A.8080603@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
> 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?
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? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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?
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h index d6b0128..b8cb740 100644 --- a/include/uapi/linux/usb/functionfs.h +++ b/include/uapi/linux/usb/functionfs.h @@ -9,8 +9,9 @@ enum { - FUNCTIONFS_DESCRIPTORS_MAGIC = 1, - FUNCTIONFS_STRINGS_MAGIC = 2 + FUNCTIONFS_DESCRIPTORS_MAGIC = 1, + FUNCTIONFS_STRINGS_MAGIC = 2, + FUNCTIONFS_SS_DESCRIPTORS_MAGIC = 3 }; @@ -60,6 +61,25 @@ struct usb_functionfs_descs_head { * | 2 | payload | | descriptor's payload | */ +struct usb_functionfs_ss_descs_head { + __le32 magic; + __le32 length; + __le32 reserved; + __le32 ss_count; +} __attribute__((packed)); + +/* + * SS Descriptors format: + * + * | off | name | type | description | + * |-----+-----------+--------------+--------------------------------------| + * | 0 | magic | LE32 | FUNCTIONFS_SS_DESCRIPTORS_MAGIC | + * | 4 | length | LE32 | length of the whole data chunk | + * | 8 | ss_count | LE32 | number of super-speed descriptors | + * | 12 | reserved field |