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

2013-12-24 Thread Manu Gautam 
On 12/24/2013 8:20 AM, Michal Nazarewicz wrote:
 On Mon, Dec 23 2013, Manu Gautam mgau...@codeaurora.org wrote:
 Allow userspace to pass SuperSpeed descriptors and
 handle them in the driver accordingly.
 This change doesn't modify existing desc_header and thereby
 keeps the ABI changes backward compatible i.e. existing
 userspace drivers compiled with old header (functionfs.h)
 would continue to work with the updated kernel.
 
 I'm mostly fine with this patch.  If you change __ffs_func_bind_do_descs
 as I've described inline, feel free to resend with:
 
 Acked-by: Michal Nazarewicz min...@mina86.com
 
 The other two minor issues are up to you.  I don't have strong feelings.
 

Thanks. I have tried to address your comments in ver-5 of the patch
that I just sent.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-12-23 Thread Manu Gautam 
Allow userspace to pass SuperSpeed descriptors and
handle them in the driver accordingly.
This change doesn't modify existing desc_header and thereby
keeps the ABI changes backward compatible i.e. existing
userspace drivers compiled with old header (functionfs.h)
would continue to work with the updated kernel.

Change-Id: Ic27035fdef2a83828024348d75be1518e9f8c5c6
Signed-off-by: Manu Gautam mgau...@codeaurora.org
---
 drivers/usb/gadget/f_fs.c | 176 +++---
 drivers/usb/gadget/u_fs.h |   8 ++-
 2 files changed, 142 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 306a2b5..8c7bf04 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -122,8 +122,8 @@ struct ffs_ep {
struct usb_ep   *ep;/* P: ffs-eps_lock */
struct usb_request  *req;   /* P: epfile-mutex */
 
-   /* [0]: full speed, [1]: high speed */
-   struct usb_endpoint_descriptor  *descs[2];
+   /* [0]: full speed, [1]: high speed, [2]: super speed */
+   struct usb_endpoint_descriptor  *descs[3];
 
u8  num;
 
@@ -1184,9 +1184,12 @@ static void ffs_data_reset(struct ffs_data *ffs)
ffs-stringtabs = NULL;
 
ffs-raw_descs_length = 0;
-   ffs-raw_fs_descs_length = 0;
+   ffs-raw_fs_hs_descs_length = 0;
+   ffs-raw_ss_descs_offset = 0;
+   ffs-raw_ss_descs_length = 0;
ffs-fs_descs_count = 0;
ffs-hs_descs_count = 0;
+   ffs-ss_descs_count = 0;
 
ffs-strings_count = 0;
ffs-interfaces_count = 0;
@@ -1329,7 +1332,24 @@ static int ffs_func_eps_enable(struct ffs_function *func)
spin_lock_irqsave(func-ffs-eps_lock, flags);
do {
struct usb_endpoint_descriptor *ds;
-   ds = ep-descs[ep-descs[1] ? 1 : 0];
+   int desc_idx;
+
+   if (ffs-gadget-speed == USB_SPEED_SUPER)
+   desc_idx = 2;
+   else if (ffs-gadget-speed == USB_SPEED_HIGH)
+   desc_idx = 1;
+   else
+   desc_idx = 0;
+
+   /* fall-back to lower speed if desc missing for current speed */
+   do {
+   ds = ep-descs[desc_idx];
+   } while (!ds  --desc_idx = 0);
+
+   if (!ds) {
+   ret = -EINVAL;
+   break;
+   }
 
ep-ep-driver_data = ep;
ep-ep-desc = ds;
@@ -1464,6 +1484,12 @@ static int __must_check ffs_do_desc(char *data, unsigned 
len,
}
break;
 
+   case USB_DT_SS_ENDPOINT_COMP:
+   pr_vdebug(EP SS companion descriptor\n);
+   if (length != sizeof(struct usb_ss_ep_comp_descriptor))
+   goto inv_length;
+   break;
+
case USB_DT_OTHER_SPEED_CONFIG:
case USB_DT_INTERFACE_POWER:
case USB_DT_DEBUG:
@@ -1574,8 +1600,8 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 static int __ffs_data_got_descs(struct ffs_data *ffs,
char *const _data, size_t len)
 {
-   unsigned fs_count, hs_count;
-   int fs_len, ret = -EINVAL;
+   unsigned fs_count, hs_count, ss_count = 0;
+   int fs_len, hs_len, ss_len, ret = -EINVAL;
char *data = _data;
 
ENTER();
@@ -1586,9 +1612,6 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
fs_count = get_unaligned_le32(data +  8);
hs_count = get_unaligned_le32(data + 12);
 
-   if (!fs_count  !hs_count)
-   goto einval;
-
data += 16;
len  -= 16;
 
@@ -1607,22 +1630,58 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
}
 
if (likely(hs_count)) {
-   ret = ffs_do_descs(hs_count, data, len,
+   hs_len = ffs_do_descs(hs_count, data, len,
   __ffs_data_do_entity, ffs);
-   if (unlikely(ret  0))
+   if (unlikely(hs_len  0)) {
+   ret = hs_len;
+   goto error;
+   }
+
+   data += hs_len;
+   len  -= hs_len;
+   } else {
+   hs_len = 0;
+   }
+
+   if (len = 8) {
+   /* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */
+   if (get_unaligned_le32(data) != FUNCTIONFS_SS_DESC_MAGIC)
+   goto einval;
+
+   ss_count = get_unaligned_le32(data + 4);
+   data += 8;
+   len  -= 8;
+   }
+
+   if (!fs_count  !hs_count  !ss_count)
+   goto einval;
+
+   if (ss_count) {
+   ss_len = ffs_do_descs(ss_count, data, len,
+  __ffs_data_do_entity, ffs);
+   if (unlikely(ss_len  0)) {
+   ret = ss_len;

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

2013-12-23 Thread Felipe Balbi 
On Mon, Dec 23, 2013 at 05:18:42PM +0530, Manu Gautam wrote:
 Allow userspace to pass SuperSpeed descriptors and
 handle them in the driver accordingly.
 This change doesn't modify existing desc_header and thereby
 keeps the ABI changes backward compatible i.e. existing
 userspace drivers compiled with old header (functionfs.h)
 would continue to work with the updated kernel.
 
 Change-Id: Ic27035fdef2a83828024348d75be1518e9f8c5c6

when sending patches upstream, please remove gerrit garbage from commit
log ;-)

I'll wait for Michal to review this one.

-- 
balbi


signature.asc
Description: Digital signature

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

2013-12-23 Thread Michal Nazarewicz 
On Mon, Dec 23 2013, Manu Gautam mgau...@codeaurora.org wrote:
 Allow userspace to pass SuperSpeed descriptors and
 handle them in the driver accordingly.
 This change doesn't modify existing desc_header and thereby
 keeps the ABI changes backward compatible i.e. existing
 userspace drivers compiled with old header (functionfs.h)
 would continue to work with the updated kernel.

I'm mostly fine with this patch.  If you change __ffs_func_bind_do_descs
as I've described inline, feel free to resend with:

Acked-by: Michal Nazarewicz min...@mina86.com

The other two minor issues are up to you.  I don't have strong feelings.

 Signed-off-by: Manu Gautam mgau...@codeaurora.org
 ---
  drivers/usb/gadget/f_fs.c | 176 
 +++---
  drivers/usb/gadget/u_fs.h |   8 ++-
  2 files changed, 142 insertions(+), 42 deletions(-)

 diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
 index 306a2b5..8c7bf04 100644
 --- a/drivers/usb/gadget/f_fs.c
 +++ b/drivers/usb/gadget/f_fs.c
 @@ -1607,22 +1630,58 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
   }
  
   if (likely(hs_count)) {
 - ret = ffs_do_descs(hs_count, data, len,
 + hs_len = ffs_do_descs(hs_count, data, len,
  __ffs_data_do_entity, ffs);
 - if (unlikely(ret  0))
 + if (unlikely(hs_len  0)) {
 + ret = hs_len;
 + goto error;
 + }
 +
 + data += hs_len;
 + len  -= hs_len;
 + } else {
 + hs_len = 0;
 + }
 +
 + if (len = 8) {
 + /* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */
 + if (get_unaligned_le32(data) != FUNCTIONFS_SS_DESC_MAGIC)
 + goto einval;
 +
 + ss_count = get_unaligned_le32(data + 4);
 + data += 8;
 + len  -= 8;
 + }
 +
 + if (!fs_count  !hs_count  !ss_count)
 + goto einval;
 +
 + if (ss_count) {
 + ss_len = ffs_do_descs(ss_count, data, len,
 +__ffs_data_do_entity, ffs);
 + if (unlikely(ss_len  0)) {
 + ret = ss_len;
   goto error;
 + }
 + ret = ss_len;
   } else {
 + ss_len = 0;
   ret = 0;
   }
  
   if (unlikely(len != ret))
   goto einval;
  
 - ffs-raw_fs_descs_length = fs_len;
 - ffs-raw_descs_length= fs_len + ret;
 - ffs-raw_descs   = _data;
 - ffs-fs_descs_count  = fs_count;
 - ffs-hs_descs_count  = hs_count;
 + ffs-raw_fs_hs_descs_length  = fs_len + hs_len;
 + ffs-raw_ss_descs_length = ss_len;
 + ffs-raw_descs_length= fs_len + hs_len + ss_len;

This can always be calculated as the sum of the other two fields, so
perhaps the redundancy in the structure is not needed, especially as
this is only used in _ffs_func_bind?

 + ffs-raw_descs   = _data;
 + ffs-fs_descs_count  = fs_count;
 + ffs-hs_descs_count  = hs_count;
 + ffs-ss_descs_count  = ss_count;
 + /* ss_descs? present @ header + hs_fs_descs + ss_magic + ss_count */
 + if (ffs-ss_descs_count)
 + ffs-raw_ss_descs_offset = 16 +
 ffs-raw_fs_hs_descs_length + 8;

Similarly here, the offset is easily calculated from the other fields.

I don't have strong feelings, but perhaps it would end up being cleaner
if the values were calculated on demand in _ffs_func_bind?

  
   return 0;
  
 @@ -1850,16 +1909,23 @@ static int __ffs_func_bind_do_descs(enum 
 ffs_entity_type type, u8 *valuep,
* If hs_descriptors is not NULL then we are reading hs
* descriptors now
*/
 - const int isHS = func-function.hs_descriptors != NULL;
 - unsigned idx;
 + const int is_hs = func-function.hs_descriptors != NULL;
 + const int is_ss = func-function.ss_descriptors != NULL;
 + unsigned ep_desc_id, idx;
  
   if (type != FFS_DESCRIPTOR)
   return 0;
  
 - if (isHS)
 + if (is_ss) {
 + func-function.ss_descriptors[(long)valuep] = desc;
 + ep_desc_id = 2;
 + } else if (is_hs) {
   func-function.hs_descriptors[(long)valuep] = desc;
 - else
 + ep_desc_id = 1;
 + } else {
   func-function.fs_descriptors[(long)valuep]= desc;
 + ep_desc_id = 0;
 + }

I should have caught it in my previous review.  Since is_hs and is_ss
are mutually exclusive, it would be better to use ep_desc_id as a single
3-state variable than having two 2-state variables, which may cause some
confusion as to what they actually mean.  So how about:

unsigned ed_desc_id, idx;

if (type != FFS_DESCRIPTOR)
return 0;

/*
 * If ss_descriptors is not NULL, we are reading super