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 <mgautam@xxxxxxxxxxxxxx>
> ---
> 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 */
>
> - /* [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,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 = 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;
> +
> + ds = ep->descs[desc_idx];
> + if (!ds) {
> + ret = -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 = ep->descs[desc_idx];
} while (!ds && --desc_idx >= 0);
if (desc_idx < 0) {
ret = -EINVAL;
break;
}
Or something similar. Point is, why aren't we failing dawn to high/low
speed if ep->descs[2] is NULL?
>
> ep->ep->driver_data = ep;
> ep->ep->desc = ds;
> @@ -1464,6 +1480,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 +1596,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, ss_magic, ret = -EINVAL;
> char *data = _data;
>
> ENTER();
> @@ -1586,9 +1608,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 +1626,59 @@ 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 >= hs_len + 8)) {
With the above len -= hs_len, this just becomes “len >= 8”.
Nit: too many parenthesise. Having “((…))” makes me think there's
assignment inside which there's no.
> + /* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */
> + ss_magic = get_unaligned_le32(data + hs_len);
> + if (ss_magic != FUNCTIONFS_SS_DESC_MAGIC)
> + goto einval;
The temporary variable doesn't really serve any purpose here, and with
the above “data += hs_len” this becomes:
if (get_unaligned_le32(data) != FUNCTIONFS_SS_DESC_MAGIC)
goto einval;
> +
> + ss_count = get_unaligned_le32(data + hs_len + 4);
> + data += hs_len + 8;
> + len -= hs_len + 8;
> + } else {
> + data += hs_len;
> + len -= hs_len;
> + }
> +
> + 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 = 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 = fs_len + hs_len + ss_len;
> + 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;
>
> return 0;
>
> @@ -1850,16 +1906,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;
> + }
>
> if (!desc || desc->bDescriptorType != USB_DT_ENDPOINT)
> return 0;
> @@ -1867,13 +1930,13 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
> idx = (ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) - 1;
> ffs_ep = func->eps + idx;
>
> - 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] = ds;
> + ffs_ep->descs[ep_desc_id] = ds;
>
> 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 = !!func->ffs->fs_descs_count;
> const int high = gadget_is_dualspeed(func->gadget) &&
> func->ffs->hs_descs_count;
> + const int super = gadget_is_superspeed(func->gadget) &&
> + func->ffs->ss_descs_count;
>
> - int ret;
> + int fs_len, hs_len, ret;
>
> /* Make it a single chunk, less management later on */
> vla_group(d);
> @@ -2027,15 +2092,16 @@ static int _ffs_func_bind(struct usb_configuration *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;
>
> ENTER();
>
> - /* 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;
>
> /* Allocate a single chunk, less management later on */
> @@ -2045,8 +2111,16 @@ static int _ffs_func_bind(struct usb_configuration *c,
>
> /* 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 = ffs->eps_count; ret; --ret) {
> struct ffs_ep *ptr;
> @@ -2068,33 +2142,51 @@ static int _ffs_func_bind(struct usb_configuration *c,
> */
> if (likely(full)) {
> func->function.fs_descriptors = vla_ptr(vlabuf, d, fs_descs);
> - ret = ffs_do_descs(ffs->fs_descs_count,
> + fs_len = 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 = fs_len;
> goto error;
> + }
> } else {
> - ret = 0;
> + fs_len = 0;
> }
>
> if (likely(high)) {
> func->function.hs_descriptors = vla_ptr(vlabuf, d, hs_descs);
> - ret = ffs_do_descs(ffs->hs_descs_count,
> - vla_ptr(vlabuf, d, raw_descs) + ret,
> - d_raw_descs__sz - ret,
> + hs_len = 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 = hs_len;
> + goto error;
> + }
> + } else {
> + hs_len = 0;
> + }
> +
> + if (likely(super)) {
> + func->function.ss_descriptors = vla_ptr(vlabuf, d, ss_descs);
> + ret = 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;
> }
>
> +
> /*
> * Now handle interface numbers allocation and interface and
> * endpoint numbers rewriting. We can do that in one go
> * now.
> */
> ret = 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_configuration *c,
> */
> func->function.fs_descriptors = NULL;
> func->function.hs_descriptors = NULL;
> + func->function.ss_descriptors = NULL;
> func->interfaces_nums = NULL;
>
> 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;
>
> 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 = 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 |
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature
