From mboxrd@z Thu Jan 1 00:00:00 1970 From: Manu Gautam Subject: Re: [PATCH v3 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode Date: Mon, 23 Dec 2013 17:16:30 +0530 Message-ID: <52B82296.9070103@codeaurora.org> References: <1387533625-21363-1-git-send-email-mgautam@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Michal Nazarewicz Cc: balbi-l0cyMroinI0@public.gmane.org, jackp-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, pheatwol-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, andrzej.p-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org List-Id: linux-arm-msm@vger.kernel.org On 12/20/2013 8:47 PM, Michal Nazarewicz wrote: > On Fri, Dec 20 2013, Manu Gautam wrote: >=20 > 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. >=20 > do { > ds =3D ep->descs[desc_idx]; > } while (!ds && --desc_idx >=3D 0); > if (desc_idx < 0) { > ret =3D -EINVAL; > break; > } >=20 > Or something similar. Point is, why aren't we failing dawn to high/l= ow > speed if ep->descs[2] is NULL? > agreed. >> 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; >> + } >=20 > data +=3D hs_len; > len -=3D hs_len; >=20 >> + } else { >> + hs_len =3D 0; >> + } >> + >> + if ((len >=3D hs_len + 8)) { >=20 > With the above len -=3D hs_len, this just becomes =E2=80=9Clen >=3D 8= =E2=80=9D. >=20 > Nit: too many parenthesise. Having =E2=80=9C((=E2=80=A6))=E2=80=9D m= akes 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 =3D get_unaligned_le32(data + hs_len); >> + if (ss_magic !=3D FUNCTIONFS_SS_DESC_MAGIC) >> + goto einval; >=20 > The temporary variable doesn't really serve any purpose here, and wit= h > the above =E2=80=9Cdata +=3D hs_len=E2=80=9D this becomes: >=20 > if (get_unaligned_le32(data) !=3D FUNCTIONFS_SS_DESC_MAGIC) > goto einval; >=20 Agreed. I will correct this and below assignments. >> + >> + 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; >> + } >> + 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; >=20 > Nit: I would keep this consistent in the way that I'd just reference > local variables: >=20 > ffs->raw_descs_length =3D 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", >=20 > is_ss ? "super" : (is_hs "high" : "full"), >=20 will change this. >> - /* Only high speed but not supported by gadget? */ >> - if (unlikely(!(full | high))) >> + /* Only high/super speed but not supported by gadget? */ >=20 > The comment cloud be improved, e.g.: >=20 > /* Has descriptions only for speeds gadgets does not support. */ >=20 ok. >> + 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); >=20 > Nit: indention of the arguments. I will fix this. --=20 The Qualcomm Innovation Center, Inc. is a member of the Code Aurora For= um, 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