Re: [PATCH v2 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4812 bytes --]

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 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              |

any comments here ? nobody ? Manu, if nobody complains in another week,
please send this hunk as a formal patch.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]