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

[Michal Nazarewicz <mina86@mina86.com>]

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

On Mon, Dec 23 2013, Manu Gautam <mgautam@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 <mina86@mina86.com>

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

> Signed-off-by: Manu Gautam <mgautam@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 speed
	 * descriptors; if hs_descriptors is not NULL, we are reading high
	 * speed descriptors; otherwise, we are reading full speed
	 * descriptors.
	 */
	if (func->function.ss_descriptors) {
		ed_desc_id = 2;
		func->function.ss_descriptors[(long)valuep] = desc;
	} else if (func->function.hs_descriptors) {
		ed_desc_id = 1;
		func->function.ss_descriptors[(long)valuep] = desc;
	} else {
		ed_desc_id = 0;
		func->function.fs_descriptors[(long)valuep] = desc;
	}

>  
>  	if (!desc || desc->bDescriptorType != USB_DT_ENDPOINT)
>  		return 0;
> @@ -1867,13 +1933,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" : (is_hs ? "high" : "full"),

With ep_desc_id this would be solved by a static array at the top of the
function:

	static const char *const speed_names[] = { "full", "high", "super" };

and a simple

			   speed_names[ep_desc_id],

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

> @@ -2027,15 +2095,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);

This means we might allocate space for descriptors that the gadget will
never use.  For instance if user space provides super speed descriptors
but gadget only supports high speed.  I'm not saying this should stop
the patch, it's a mere observation.  The amount of wasted memory is
probably not that big so perhaps it's not worth complicating the code
to save it after all.

>  	char *vlabuf;
>  
>  	ENTER();
>  
> -	/* Only high speed but not supported by gadget? */
> -	if (unlikely(!(full | high)))
> +	/* Has descriptors only for speeds gadget does not support */
> +	if (unlikely(!(full | high | super)))
>  		return -ENOTSUPP;
>  
>  	/* Allocate a single chunk, less management later on */

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]