From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751600AbcEMGiR (ORCPT ); Fri, 13 May 2016 02:38:17 -0400 Received: from mga14.intel.com ([192.55.52.115]:33410 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750961AbcEMGiO (ORCPT ); Fri, 13 May 2016 02:38:14 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,612,1455004800"; d="asc'?scan'208";a="805364922" From: Felipe Balbi To: "Du\, Changbin" , Al Viro Cc: "gregkh\@linuxfoundation.org" , "mina86\@mina86.com" , "rui.silva\@linaro.org" , "k.opasiak\@samsung.com" , "lars\@metafoo.de" , "linux-usb\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" Subject: RE: [PATCH] usb: gadget: f_fs: report error if excess data received In-Reply-To: <0C18FE92A7765D4EB9EE5D38D86A563A05D2F5C3@SHSMSX103.ccr.corp.intel.com> References: <1462961970-2001-1-git-send-email-changbin.du@intel.com> <87twi4g8s2.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F01F@SHSMSX103.ccr.corp.intel.com> <87a8jvg43q.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F156@SHSMSX103.ccr.corp.intel.com> <874ma3g1lq.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F183@SHSMSX103.ccr.corp.intel.com> <87zirveixx.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F2C8@SHSMSX103.ccr.corp.intel.com> <87shxneg6t.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F35C@SHSMSX103.ccr.corp.intel.com> <878tzfed17.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F5C3@SHSMSX103.ccr.corp.intel.com> User-Agent: Notmuch/0.22+11~g124a67e (http://notmuchmail.org) Emacs/25.0.93.2 (x86_64-pc-linux-gnu) Date: Fri, 13 May 2016 09:36:03 +0300 Message-ID: <8737pmcvn0.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, "Du, Changbin" writes: >> "Du, Changbin" writes: >> >> right, and that was my point: if we copy more to userspace, then we h= ave >> >> a real big problem. >> >> >> > Yes, we drop the data because we userspace buffer is not enough this t= ime. >> > The problem here is that really can we just drop it silently? Maybe no= t. >>=20 >> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending >> more data than it should, is another problem altogether which needs to >> be addressed at the host. >>=20 >> Adding a print to aid debugging is a good idea, but bailing out on the >> peripheral side is not :-s >>=20 > Ok, if we think this is a problem at host side that the transfer is not d= evice > expected, then device side should not accept the data or deliver the > transferred data to userspace. But now we take part of the data to usersp= ace > and says it is ok. > Do you agree with this point? We deliver to userspace the part userspace requested, right? So that's okay. The USB details WRT e.g. babble or host trying to send more data than expected, needs to be handled within the kernel. > IMO, we expose usb transfer as a file on device side. But file read() doe= sn't > have a requirement that "sorry, you cannot read so little! you need read = all > once, else we may drop data for you. :) ". but that's not how read() semantics work. When userspace asks to read(x) bytes, we have three possible outcomes: i. We have x bytes to return, so we copy_to_user(x) ii. We have y < x bytes to return, so we copy_to_user(y) iii. We have y > x bytes to return, so we copy_to_user(x) This is exactly how the kernel is behaving. The only "detail" we have is that, for some reason, host is sending too much data. what I still don't know is if this extra data is garbage or something userspace genuinely cares about. Do you know the answer to this? > And some library that may retry read() until get enough data (which is > normal For a general read). Then sometimes the buffer size for > sys_read may not as expected. This is why I think ioctl approach is > more appropriate for usb transfer. no, this won't change anything. Besides, it's a pointless discussion as cannot break userspace ABI. GadgetFS and FunctionFS have been shipping in kernel for many years. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXNXXTAAoJEIaOsuA1yqRE2KgP/24vx51D7vFnSlyX8TZ+/4bA Q+ykNbvK/RfUOKTWs8KwIIp+Bk671i0alF9NVWGvCnLCaGe42SGtCzXLZiDIRELp LzlahCv8SiPvbhm/BQE8RievMxe65L0m1Er6T4oRKpufCwYtWQU55NbVVnoPcBKN LY984VNTgpoqMa0gLs9+VmgwKqNq6jccioYCid/slw019SMQWeMaZVwG1WxFG5DF /eQ3AelNVY9BeRZCvL0IrXLnlYocwNN4k2IrWGPEPJQQ8TU10fFF6nqsAs2hUWp1 nBSd23SDrTgaCdPbEOKBkM3L9vV2vcMvHccl21Y1oS4iwHLi7s6IpX476QgYtwQ7 xaF6qBkLvHENngxbZ6WbQpq4dmemqYu+WVlR06b7futt3fQJ0oizAeiR1UaveG9L PqKaTPed9IDR/n3ipu2pKmnNh4z13ehJtpo8ZEWnwRi36IOWcO60Rt2MI5t2OFFg qPh4GYjUJhlAXPkxkCKrXjdXJatl7iIgd2ZE/GDm/59uHCu6jMT4VPQAzjTdRnnA r6i8Kz3CqJKmRED0U++bECnugKxOleB7JpR90oFLu+EZ4TjWii+vSL49CYBJpqnB BHC9T2zSjZFijLlnV/7SZ6drFDfkjPTP9NgjVDR6PR4FiRo3HTFr/4Fegx0Pu+xF m6PYjQwzQQv+PF6EmvL9 =QepQ -----END PGP SIGNATURE----- --=-=-=--