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-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.
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
signature.asc
Description: Digital signature
