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-ow...@vger.kernel.org > >> [mailto:linux-usb-ow...@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. -- balbi
signature.asc
Description: Digital signature
