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

[Manu Gautam <mgautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 12/20/2013 8:47 PM, Michal Nazarewicz wrote:
> On Fri, Dec 20 2013, Manu Gautam wrote:
> 
> 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?
>

agreed.

>>  	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.
>

I will correct this.

>> +		/* 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;
> 

Agreed. I will correct this and below assignments.

>> +
>> +		ss_count = get_unaligned_le32(data + hs_len + 4);
>> +		data += hs_len + 8;
>> +		len  -= hs_len + 8;
>> +	} else {
>> +		data += hs_len;
>> +		len  -= hs_len;
>> +	}


>> +	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;
>

Agreed.

>> +	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"),
> 

will change this.

>> -	/* 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. */
> 

ok.

>> +		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.

I will fix this.




-- 
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-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html