From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felipe Balbi Subject: Re: [PATCH v2 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode Date: Tue, 26 Nov 2013 11:41:29 -0600 Message-ID: <20131126174129.GR24310@saruman.home> References: <1379678152-9617-1-git-send-email-mgautam@codeaurora.org> <52415BAC.6030708@codeaurora.org> <20130925204030.GW10746@radagast> <5243DD3A.8080603@codeaurora.org> <52493DFE.503@codeaurora.org> <20131001143731.GV1476@radagast> <524BA2C3.5050007@codeaurora.org> <5253889F.8010005@codeaurora.org> Reply-To: Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="e7jIye1Ygp5H0AIi" Return-path: Received: from bear.ext.ti.com ([192.94.94.41]:49506 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753440Ab3KZRmF (ORCPT ); Tue, 26 Nov 2013 12:42:05 -0500 Content-Disposition: inline In-Reply-To: <5253889F.8010005@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Manu Gautam Cc: balbi@ti.com, Paul Zimmerman , Jack Pham , "pheatwol@codeaurora.org" , "linux-usb@vger.kernel.org" , "linux-arm-msm@vger.kernel.org" , "gregkh@linuxfoundation.org" , Andrzej Pietrasiewicz , Michal Nazarewicz , Benoit Goby --e7jIye1Ygp5H0AIi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 descript= ors > >>>>>>> count) in desc_header (which is passed from userspace) make the d= river > >>>>>>> incompatible with existing userspace applications compiled agains= t old > >>>>>>> header file. Let me know if that is acceptable. We are using this > >>>>>>> driver with Android for adbd (android debug bridge) and these cha= nges > >>>>>>> are required to support adb over Super Speed controllers e.g. DWC3 > >>>>>>> along with changed in adbd to pass SS EP and companion descriptor= s. > >>>>>> > >>>>>> Good you mentioned, it saves me the trouble of reviewing this patc= h :-) > >>>>>> > >>>>>> 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 w= ith > >>>>>> superspeed. While at that, we might want to add a few bytes of res= erved, > >>>>>> 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 imple= ment > >>>>> 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' val= ue > >>>> passed in, and only handle the SuperSpeed stuff if the length indica= tes > >>>> 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 c= an > >>> 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 foll= owing. > >>> This can be used by kernel driver to check if userspace is trying pass > >>> ss desc only or some invalid data.=20 > >>> 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. > >> > >=20 > > Can you please add appropriate folks to this thread who can check this = as > > well? > >=20 >=20 > I have CC'ed Michal and Andrzej as they have contributed to this driver. > Followed is the interface enhancement that I am suggesting: >=20 > 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 =3D 2 > }; >=20 > +#define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C >=20 > #ifndef __KERNEL__ >=20 > @@ -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. --=20 balbi --e7jIye1Ygp5H0AIi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) iQIcBAEBAgAGBQJSlN1JAAoJEIaOsuA1yqRE36kP/2aklysPfojexop/rP6q+IFR hY/CptM+q8+UlrPZsR6Q2jW63NAl/ycLrv0qJ4EXnERsCsZpvYaj1QB9hjMUhHx/ 1WzFzr1G77w6bK5qkvR3Cwm+tKpdBTUGloIyU1PB3E+4oE6iZdRa9deyp7n2QHW3 K06nOS0dszpaDVlPlBTDvUVZGNW9NHCD7GBwUuRm5t7/GeD5Q4a0QJDn4oGALuPx flj1JPcrVF5Ejo8+2kK/rkVEEe69WLOL0HFNXeu8DXbgr4heq3QYWXjKFtINi8y8 V0u722V1Ot82WhS6mCsaCtHu5TSFuM4L2923PfplC7ktmUb7C7yr643HZKZUN0a7 bqfA5wAIBrka3Ugirjawd/jNEj1nzG4ewIc1UUm8RTy07hHuSwBUH+7K0BgfrLow JDqJYkNiWaWnrd+wYtn/Q5QO/4W0FPKo322aCWXANz5QtrK3Og/+0cKOr/Bjclcr CCcf8hmJE2bYUU+Hg37XdXmooLs51m4e9mVQOCrLmPmmMum+/puj5Xboq82Dbw+O rih+MlCo2ZQ27YXisPVZLmonZ//9y1DWcduikuWrdt0i+iY1jWvkDYJXtRAPNQG0 HVCz1gx2/RRxkhOQsUfPkprtsJm4Mh3VgJzp3U1DK5iktX+H4AOaAkreeJU/7qkt a3OOTI7Dnj+NuC1ZseKO =dHIH -----END PGP SIGNATURE----- --e7jIye1Ygp5H0AIi--