From patchwork Tue Oct 8 04:22:55 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Manu Gautam X-Patchwork-Id: 3000321 Return-Path: X-Original-To: patchwork-linux-arm-msm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id A4455BF924 for ; Tue, 8 Oct 2013 04:23:21 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A217B201B3 for ; Tue, 8 Oct 2013 04:23:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 688B6201B4 for ; Tue, 8 Oct 2013 04:23:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751635Ab3JHEXE (ORCPT ); Tue, 8 Oct 2013 00:23:04 -0400 Received: from smtp.codeaurora.org ([198.145.11.231]:59774 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751529Ab3JHEXD (ORCPT ); Tue, 8 Oct 2013 00:23:03 -0400 Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id C631213EEE3; Tue, 8 Oct 2013 04:23:02 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 486) id B681713F279; Tue, 8 Oct 2013 04:23:02 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Received: from [10.44.23.105] (unknown [202.46.23.62]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: mgautam@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 85C6413EEE3; Tue, 8 Oct 2013 04:22:58 +0000 (UTC) Message-ID: <5253889F.8010005@codeaurora.org> Date: Tue, 08 Oct 2013 09:52:55 +0530 From: Manu Gautam User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 MIME-Version: 1.0 To: balbi@ti.com CC: 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 Subject: Re: [PATCH v2 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode 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> In-Reply-To: <524BA2C3.5050007@codeaurora.org> X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 |