From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Nazarewicz Subject: Re: [PATCH v4 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode Date: Tue, 24 Dec 2013 03:50:54 +0100 Message-ID: References: <1387799322-26737-1-git-send-email-mgautam@codeaurora.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from mail-ee0-f49.google.com ([74.125.83.49]:39512 "EHLO mail-ee0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758207Ab3LXCvF (ORCPT ); Mon, 23 Dec 2013 21:51:05 -0500 Received: by mail-ee0-f49.google.com with SMTP id c41so2631177eek.8 for ; Mon, 23 Dec 2013 18:51:03 -0800 (PST) In-Reply-To: <1387799322-26737-1-git-send-email-mgautam@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: balbi@ti.com, jackp@codeaurora.org, pheatwol@codeaurora.org Cc: linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org, benoit@android.com, andrzej.p@samsung.com, gregkh@linuxfoundation.org, Manu Gautam --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On Mon, Dec 23 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. 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 The other two minor issues are up to you. I don't have strong feelings. > Signed-off-by: Manu Gautam > --- > 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, > } >=20=20 > 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; > + } > + > + data +=3D hs_len; > + len -=3D hs_len; > + } else { > + hs_len =3D 0; > + } > + > + if (len >=3D 8) { > + /* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */ > + if (get_unaligned_le32(data) !=3D FUNCTIONFS_SS_DESC_MAGIC) > + goto einval; > + > + ss_count =3D get_unaligned_le32(data + 4); > + data +=3D 8; > + len -=3D 8; > + } > + > + if (!fs_count && !hs_count && !ss_count) > + goto einval; > + > + if (ss_count) { > + ss_len =3D ffs_do_descs(ss_count, data, len, > + __ffs_data_do_entity, ffs); > + if (unlikely(ss_len < 0)) { > + ret =3D ss_len; > goto error; > + } > + ret =3D ss_len; > } else { > + ss_len =3D 0; > ret =3D 0; > } >=20=20 > if (unlikely(len !=3D ret)) > goto einval; >=20=20 > - ffs->raw_fs_descs_length =3D fs_len; > - ffs->raw_descs_length =3D fs_len + ret; > - ffs->raw_descs =3D _data; > - ffs->fs_descs_count =3D fs_count; > - ffs->hs_descs_count =3D hs_count; > + ffs->raw_fs_hs_descs_length =3D fs_len + hs_len; > + ffs->raw_ss_descs_length =3D ss_len; > + ffs->raw_descs_length =3D 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 =3D _data; > + ffs->fs_descs_count =3D fs_count; > + ffs->hs_descs_count =3D hs_count; > + ffs->ss_descs_count =3D ss_count; > + /* ss_descs? present @ header + hs_fs_descs + ss_magic + ss_count */ > + if (ffs->ss_descs_count) > + ffs->raw_ss_descs_offset =3D 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? >=20=20 > return 0; >=20=20 > @@ -1850,16 +1909,23 @@ static int __ffs_func_bind_do_descs(enum ffs_enti= ty_type type, u8 *valuep, > * If hs_descriptors is not NULL then we are reading hs > * descriptors now > */ > - const int isHS =3D func->function.hs_descriptors !=3D NULL; > - unsigned idx; > + const int is_hs =3D func->function.hs_descriptors !=3D NULL; > + const int is_ss =3D func->function.ss_descriptors !=3D NULL; > + unsigned ep_desc_id, idx; >=20=20 > if (type !=3D FFS_DESCRIPTOR) > return 0; >=20=20 > - if (isHS) > + if (is_ss) { > + func->function.ss_descriptors[(long)valuep] =3D desc; > + ep_desc_id =3D 2; > + } else if (is_hs) { > func->function.hs_descriptors[(long)valuep] =3D desc; > - else > + ep_desc_id =3D 1; > + } else { > func->function.fs_descriptors[(long)valuep] =3D desc; > + ep_desc_id =3D 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 !=3D 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 =3D 2; func->function.ss_descriptors[(long)valuep] =3D desc; } else if (func->function.hs_descriptors) { ed_desc_id =3D 1; func->function.ss_descriptors[(long)valuep] =3D desc; } else { ed_desc_id =3D 0; func->function.fs_descriptors[(long)valuep] =3D desc; } >=20=20 > if (!desc || desc->bDescriptorType !=3D USB_DT_ENDPOINT) > return 0; > @@ -1867,13 +1933,13 @@ static int __ffs_func_bind_do_descs(enum ffs_enti= ty_type type, u8 *valuep, > idx =3D (ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) - 1; > ffs_ep =3D func->eps + idx; >=20=20 > - 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[] =3D { "full", "high", "super" }; and a simple speed_names[ep_desc_id], > ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK); > return -EINVAL; > } > - ffs_ep->descs[isHS] =3D ds; > + ffs_ep->descs[ep_desc_id] =3D ds; >=20=20 > ffs_dump_mem(": Original ep desc", ds, ds->bLength); > if (ffs_ep->ep) { > @@ -2027,15 +2095,16 @@ static int _ffs_func_bind(struct usb_configuratio= n *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; >=20=20 > ENTER(); >=20=20 > - /* 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; >=20=20 > /* Allocate a single chunk, less management later on */ --=20 Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=3D./ `o ..o | Computer Science, Micha=C5=82 =E2=80=9Cmina86=E2=80=9D Nazarewicz = (o o) ooo +------ooO--(_)--Ooo-- --=-=-= Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" --==-=-= Content-Type: text/plain --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJSuPaOAAoJECBgQBJQdR/07JMQAIVP8ajmZbNfKp6g4yZmFCcn b4Bb3YFuqnUNu6rHLljpxDUB++PIIaXh+rgmCb1al1l0wogZb3asRpTCwGySVFdp CEMV1zrMwEevSV3G7vJtBtyrzFcbbjf7eafRLjyF85oWG0zKu2MA3pT/10vnPLaw 6yoKyaJ0nIUiq0d5dXmi++Z9kZz6PFc1yHb8/eL7PiVMGwhOdV9BaxOgpyuopCq9 wZOQJzXk5KNGJqjVq8GCQkL4JJNwoGmyHxuGTXIOjuAVRp6vtB73W0U1iwac5Bjd bIDaKdQ6ilncjNSr1RTMPKzB5wiSSVrbR1FICTgD0uPWokLyBwDz9fVdNZqy8DU/ JRoPuvlONBw+GJeJ8H0/YB0PkwHbw4zSZtieoWM+gy8VN62xPuD2CR9bDRMALnLN 1Ea5dU7nz0yraTNtu6pftLsaQTqMr/x2eQuGMsltpVOxme3EpK9VKs/lfPOjiWjI XIlHOzKd6jahHRm4dGTkud8IcuPFMkdRO6EmNxgJxuL14wwdOv6N1VEJNTNiMpeh HxuD0lPMFXBXbeyQKLlAOxDRWWn8K2wexnXaX/6IqXE7EfuN14Am4P0niMzTUVuI LnVmfMwcqkyNrKI1Jf6ZQo4zesyyBraYBycaCdlxRK8W4o0ziPQGmPSxMu53E99f 46IY6W6DJhoY4I/GspJ7 =L0SP -----END PGP SIGNATURE----- --==-=-=-- --=-=-=--