From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Nazarewicz Subject: Re: [PATCH v3 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode Date: Fri, 20 Dec 2013 16:17:08 +0100 Message-ID: References: <1387533625-21363-1-git-send-email-mgautam@codeaurora.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from mail-wi0-f177.google.com ([209.85.212.177]:46348 "EHLO mail-wi0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753948Ab3LTPRU (ORCPT ); Fri, 20 Dec 2013 10:17:20 -0500 Received: by mail-wi0-f177.google.com with SMTP id cc10so3830235wib.4 for ; Fri, 20 Dec 2013 07:17:19 -0800 (PST) In-Reply-To: <1387533625-21363-1-git-send-email-mgautam@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: balbi@ti.com, jackp@codeaurora.org, pheatwol@codeaurora.org Cc: linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org, andrzej.p@samsung.com, gregkh@linuxfoundation.org, Manu Gautam --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On Fri, Dec 20 2013, 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. > > Signed-off-by: Manu Gautam > --- > drivers/usb/gadget/f_fs.c | 165 ++++++++++++++++++++++++++++--= ------ > drivers/usb/gadget/u_fs.h | 8 +- > include/uapi/linux/usb/functionfs.h | 5 ++ > 3 files changed, 140 insertions(+), 38 deletions(-) > > diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c > index 306a2b5..65bc861 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 */ >=20=20 > - /* [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]; >=20=20 > u8 num; >=20=20 > @@ -1184,9 +1184,12 @@ static void ffs_data_reset(struct ffs_data *ffs) > ffs->stringtabs =3D NULL; >=20=20 > ffs->raw_descs_length =3D 0; > - ffs->raw_fs_descs_length =3D 0; > + ffs->raw_fs_hs_descs_length =3D 0; > + ffs->raw_ss_descs_offset =3D 0; > + ffs->raw_ss_descs_length =3D 0; > ffs->fs_descs_count =3D 0; > ffs->hs_descs_count =3D 0; > + ffs->ss_descs_count =3D 0; >=20=20 > ffs->strings_count =3D 0; > ffs->interfaces_count =3D 0; > @@ -1329,7 +1332,20 @@ 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 =3D ep->descs[ep->descs[1] ? 1 : 0]; > + int desc_idx; > + > + if (ffs->gadget->speed =3D=3D USB_SPEED_SUPER) > + desc_idx =3D 2; > + else if (ffs->gadget->speed =3D=3D USB_SPEED_HIGH) > + desc_idx =3D 1; > + else > + desc_idx =3D 0; > + > + ds =3D ep->descs[desc_idx]; > + if (!ds) { > + ret =3D -EINVAL; > + break; > + } I don't like this. Why are we failing if descriptors for given speed are missing? If they are, we should fall back to lower speed. do { ds =3D ep->descs[desc_idx]; } while (!ds && --desc_idx >=3D 0); if (desc_idx < 0) { ret =3D -EINVAL; break; } Or something similar. Point is, why aren't we failing dawn to high/low speed if ep->descs[2] is NULL? >=20=20 > ep->ep->driver_data =3D ep; > ep->ep->desc =3D ds; > @@ -1464,6 +1480,12 @@ static int __must_check ffs_do_desc(char *data, un= signed len, > } > break; >=20=20 > + case USB_DT_SS_ENDPOINT_COMP: > + pr_vdebug("EP SS companion descriptor\n"); > + if (length !=3D 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 +1596,8 @@ static int __ffs_data_do_entity(enum ffs_entity_typ= e 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 =3D -EINVAL; > + unsigned fs_count, hs_count, ss_count =3D 0; > + int fs_len, hs_len, ss_len, ss_magic, ret =3D -EINVAL; > char *data =3D _data; >=20=20 > ENTER(); > @@ -1586,9 +1608,6 @@ static int __ffs_data_got_descs(struct ffs_data *ff= s, > fs_count =3D get_unaligned_le32(data + 8); > hs_count =3D get_unaligned_le32(data + 12); >=20=20 > - if (!fs_count && !hs_count) > - goto einval; > - > data +=3D 16; > len -=3D 16; >=20=20 > @@ -1607,22 +1626,59 @@ static int __ffs_data_got_descs(struct ffs_data *= ffs, > } >=20=20 > if (likely(hs_count)) { > - ret =3D ffs_do_descs(hs_count, data, len, > + hs_len =3D ffs_do_descs(hs_count, data, len, > __ffs_data_do_entity, ffs); > - if (unlikely(ret < 0)) > + if (unlikely(hs_len < 0)) { > + ret =3D hs_len; > + goto error; > + } data +=3D hs_len; len -=3D hs_len; > + } else { > + hs_len =3D 0; > + } > + > + if ((len >=3D hs_len + 8)) { With the above len -=3D hs_len, this just becomes =E2=80=9Clen >=3D 8=E2=80= =9D. Nit: too many parenthesise. Having =E2=80=9C((=E2=80=A6))=E2=80=9D makes m= e think there's assignment inside which there's no. > + /* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */ > + ss_magic =3D get_unaligned_le32(data + hs_len); > + if (ss_magic !=3D FUNCTIONFS_SS_DESC_MAGIC) > + goto einval; The temporary variable doesn't really serve any purpose here, and with the above =E2=80=9Cdata +=3D hs_len=E2=80=9D this becomes: if (get_unaligned_le32(data) !=3D FUNCTIONFS_SS_DESC_MAGIC) goto einval; > + > + ss_count =3D get_unaligned_le32(data + hs_len + 4); > + data +=3D hs_len + 8; > + len -=3D hs_len + 8; > + } else { > + data +=3D hs_len; > + len -=3D hs_len; > + } > + > + if (!fs_count && !hs_count && !ss_count) > + goto einval; > + > + if (ss_count) { > + ss_len =3D ffs_do_descs(ss_count, data, len, > + __ffs_data_do_entity, ffs); > + if (unlikely(ss_len < 0)) { > + ret =3D ss_len; > goto error; > + } > + ret =3D ss_len; > } else { > + ss_len =3D 0; > ret =3D 0; > } >=20=20 > if (unlikely(len !=3D ret)) > goto einval; >=20=20 > - ffs->raw_fs_descs_length =3D fs_len; > - ffs->raw_descs_length =3D fs_len + ret; > - ffs->raw_descs =3D _data; > - ffs->fs_descs_count =3D fs_count; > - ffs->hs_descs_count =3D hs_count; > + ffs->raw_fs_hs_descs_length =3D fs_len + hs_len; > + ffs->raw_ss_descs_length =3D ss_len; > + ffs->raw_descs_length =3D ffs->raw_fs_hs_descs_length + ss_len; Nit: I would keep this consistent in the way that I'd just reference local variables: ffs->raw_descs_length =3D fs_len + hs_len + ss_len; > + ffs->raw_descs =3D _data; > + ffs->fs_descs_count =3D fs_count; > + ffs->hs_descs_count =3D hs_count; > + ffs->ss_descs_count =3D ss_count; > + /* ss_descs? present @ header + hs_fs_descs + ss_magic + ss_count */ > + if (ffs->ss_descs_count) > + ffs->raw_ss_descs_offset =3D 16 + ffs->raw_fs_hs_descs_length + 8; >=20=20 > return 0; >=20=20 > @@ -1850,16 +1906,23 @@ static int __ffs_func_bind_do_descs(enum ffs_enti= ty_type type, u8 *valuep, > * If hs_descriptors is not NULL then we are reading hs > * descriptors now > */ > - const int isHS =3D func->function.hs_descriptors !=3D NULL; > - unsigned idx; > + const int is_hs =3D func->function.hs_descriptors !=3D NULL; > + const int is_ss =3D func->function.ss_descriptors !=3D NULL; > + unsigned ep_desc_id, idx; >=20=20 > if (type !=3D FFS_DESCRIPTOR) > return 0; >=20=20 > - if (isHS) > + if (is_ss) { > + func->function.ss_descriptors[(long)valuep] =3D desc; > + ep_desc_id =3D 2; > + } else if (is_hs) { > func->function.hs_descriptors[(long)valuep] =3D desc; > - else > + ep_desc_id =3D 1; > + } else { > func->function.fs_descriptors[(long)valuep] =3D desc; > + ep_desc_id =3D 0; > + } >=20=20 > if (!desc || desc->bDescriptorType !=3D USB_DT_ENDPOINT) > return 0; > @@ -1867,13 +1930,13 @@ static int __ffs_func_bind_do_descs(enum ffs_enti= ty_type type, u8 *valuep, > idx =3D (ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) - 1; > ffs_ep =3D func->eps + idx; >=20=20 > - if (unlikely(ffs_ep->descs[isHS])) { > + if (unlikely(ffs_ep->descs[ep_desc_id])) { > pr_vdebug("two %sspeed descriptors for EP %d\n", > - isHS ? "high" : "full", > + is_ss ? "super" : "high/full", is_ss ? "super" : (is_hs "high" : "full"), > ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK); > return -EINVAL; > } > - ffs_ep->descs[isHS] =3D ds; > + ffs_ep->descs[ep_desc_id] =3D ds; >=20=20 > ffs_dump_mem(": Original ep desc", ds, ds->bLength); > if (ffs_ep->ep) { > @@ -2017,8 +2080,10 @@ static int _ffs_func_bind(struct usb_configuration= *c, > const int full =3D !!func->ffs->fs_descs_count; > const int high =3D gadget_is_dualspeed(func->gadget) && > func->ffs->hs_descs_count; > + const int super =3D gadget_is_superspeed(func->gadget) && > + func->ffs->ss_descs_count; >=20=20 > - int ret; > + int fs_len, hs_len, ret; >=20=20 > /* Make it a single chunk, less management later on */ > vla_group(d); > @@ -2027,15 +2092,16 @@ static int _ffs_func_bind(struct usb_configuratio= n *c, > full ? ffs->fs_descs_count + 1 : 0); > vla_item_with_sz(d, struct usb_descriptor_header *, hs_descs, > high ? ffs->hs_descs_count + 1 : 0); > + vla_item_with_sz(d, struct usb_descriptor_header *, ss_descs, > + super ? ffs->ss_descs_count + 1 : 0); > vla_item_with_sz(d, short, inums, ffs->interfaces_count); > - vla_item_with_sz(d, char, raw_descs, > - high ? ffs->raw_descs_length : ffs->raw_fs_descs_length); > + vla_item_with_sz(d, char, raw_descs, ffs->raw_descs_length); > char *vlabuf; >=20=20 > ENTER(); >=20=20 > - /* Only high speed but not supported by gadget? */ > - if (unlikely(!(full | high))) > + /* Only high/super speed but not supported by gadget? */ The comment cloud be improved, e.g.: /* Has descriptions only for speeds gadgets does not support. */ > + if (unlikely(!(full | high | super))) > return -ENOTSUPP; >=20=20 > /* Allocate a single chunk, less management later on */ > @@ -2045,8 +2111,16 @@ static int _ffs_func_bind(struct usb_configuration= *c, >=20=20 > /* Zero */ > memset(vla_ptr(vlabuf, d, eps), 0, d_eps__sz); > + /* Copy only raw (hs,fs) descriptors (until ss_magic and ss_count) */ > memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs + 16, > - d_raw_descs__sz); > + ffs->raw_fs_hs_descs_length); > + /* Copy SS descriptors */ > + if (func->ffs->ss_descs_count) > + memcpy(vla_ptr(vlabuf, d, raw_descs) + > + ffs->raw_fs_hs_descs_length, > + ffs->raw_descs + ffs->raw_ss_descs_offset, > + ffs->raw_ss_descs_length); > + > memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz); > for (ret =3D ffs->eps_count; ret; --ret) { > struct ffs_ep *ptr; > @@ -2068,33 +2142,51 @@ static int _ffs_func_bind(struct usb_configuratio= n *c, > */ > if (likely(full)) { > func->function.fs_descriptors =3D vla_ptr(vlabuf, d, fs_descs); > - ret =3D ffs_do_descs(ffs->fs_descs_count, > + fs_len =3D ffs_do_descs(ffs->fs_descs_count, > vla_ptr(vlabuf, d, raw_descs), > d_raw_descs__sz, > __ffs_func_bind_do_descs, func); Nit: indention of the arguments. > - if (unlikely(ret < 0)) > + if (unlikely(fs_len < 0)) { > + ret =3D fs_len; > goto error; > + } > } else { > - ret =3D 0; > + fs_len =3D 0; > } >=20=20 > if (likely(high)) { > func->function.hs_descriptors =3D vla_ptr(vlabuf, d, hs_descs); > - ret =3D ffs_do_descs(ffs->hs_descs_count, > - vla_ptr(vlabuf, d, raw_descs) + ret, > - d_raw_descs__sz - ret, > + hs_len =3D ffs_do_descs(ffs->hs_descs_count, > + vla_ptr(vlabuf, d, raw_descs) + fs_len, > + d_raw_descs__sz - fs_len, > __ffs_func_bind_do_descs, func); > + if (unlikely(hs_len < 0)) { > + ret =3D hs_len; > + goto error; > + } > + } else { > + hs_len =3D 0; > + } > + > + if (likely(super)) { > + func->function.ss_descriptors =3D vla_ptr(vlabuf, d, ss_descs); > + ret =3D ffs_do_descs(ffs->ss_descs_count, > + vla_ptr(vlabuf, d, raw_descs) + fs_len + hs_len, > + d_raw_descs__sz - fs_len - hs_len, > + __ffs_func_bind_do_descs, func); > if (unlikely(ret < 0)) > goto error; > } >=20=20 > + > /* > * Now handle interface numbers allocation and interface and > * endpoint numbers rewriting. We can do that in one go > * now. > */ > ret =3D ffs_do_descs(ffs->fs_descs_count + > - (high ? ffs->hs_descs_count : 0), > + (high ? ffs->hs_descs_count : 0) + > + (super ? ffs->ss_descs_count : 0), > vla_ptr(vlabuf, d, raw_descs), d_raw_descs__sz, > __ffs_func_bind_do_nums, func); > if (unlikely(ret < 0)) > @@ -2441,6 +2533,7 @@ static void ffs_func_unbind(struct usb_configuratio= n *c, > */ > func->function.fs_descriptors =3D NULL; > func->function.hs_descriptors =3D NULL; > + func->function.ss_descriptors =3D NULL; > func->interfaces_nums =3D NULL; >=20=20 > ffs_event_add(ffs, FUNCTIONFS_UNBIND); > diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h > index bc2d371..1580194 100644 > --- a/drivers/usb/gadget/u_fs.h > +++ b/drivers/usb/gadget/u_fs.h > @@ -213,13 +213,17 @@ struct ffs_data { > * Real descriptors are 16 bytes after raw_descs (so you need > * to skip 16 bytes (ie. ffs->raw_descs + 16) to get to the > * first full speed descriptor). raw_descs_length and > - * raw_fs_descs_length do not have those 16 bytes added. > + * raw_fs_hs_descs_length do not have those 16 bytes added. > + * ss_desc are 8 bytes (ss_magic + count) pass the hs_descs > */ > const void *raw_descs; > unsigned raw_descs_length; > - unsigned raw_fs_descs_length; > + unsigned raw_fs_hs_descs_length; > + unsigned raw_ss_descs_offset; > + unsigned raw_ss_descs_length; > unsigned fs_descs_count; > unsigned hs_descs_count; > + unsigned ss_descs_count; >=20=20 > unsigned short strings_count; > unsigned short interfaces_count; > diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb= /functionfs.h > index d6b0128..1ab79a2 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=20 > +#define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C >=20=20 > #ifndef __KERNEL__ >=20=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 | > --=20 > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation --=20 Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=3D./ `o ..o | Computer Science, Micha=C5=82 =E2=80=9Cmina86=E2=80=9D Nazarewicz = (o o) ooo +------ooO--(_)--Ooo-- --=-=-= Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" --==-=-= Content-Type: text/plain --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJStF91AAoJECBgQBJQdR/0dXkP/Aq6DAjCo9k6DXCTNj1Jek1B eialI+1Is2+LPiwJ0feKI+4uP5qGZmN+X27EuDrCcxanScu0V/az6vw21o77SQ3o aSzw3tA8ibHsXzfs6Gvm2+bBpYMUs3sHBfjkzmhkdEyXP8kPmh84nnzrflLXvN/d 24BWIc+idEqwNSWY/DV5hjNzrdELw5/BFDcC9A2YdbpbjAQ4sagireCPMccJYaW7 4M3Z1ZfJO25aR1D/XLKbNPUNzW1YyB6bgqtXqBSZDwg/5/bsOF4GUJkX3SmLA2TY V9CBm0oLELGA+ivkgV6yn4dbY5TgaZhhOfLuZRIx2gtWqC6DnLfM9GNm5DUkjJ8o NplMr9LgBLiBXg6G49bFX1cuz5q0qBoyBWVkRefsoFPeWttc8eC89p1KQjHVQ7wU 9AlWLPIaipuJHmskfLQByTqS06TZ3PLbCs0BHa06bmILkJTsZUllwxNZ6Ckm3ovG qDwe9EVlwIaaQLXCo2OLbwYNh928cRB+ppKc6/PmGDmj0LMVwCszedJ2LHCQpJ+6 U8FCwB5hqkHmH34S3GRGfVK/7CoOC6yDAKx4hcLdWjzZmLBx2arJHQtss6LEjInm Bm0LtESx76hFYEujkOYEJjmar/Jm0BzgMxaqdBVGqOcPvkb6Y7G8KFCBjwOOrfEF poZOUkleP6eeP921CaWS =Odnj -----END PGP SIGNATURE----- --==-=-=-- --=-=-=--