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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
