From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758152AbcEFGqg (ORCPT ); Fri, 6 May 2016 02:46:36 -0400 Received: from mga02.intel.com ([134.134.136.20]:11565 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758087AbcEFGqe (ORCPT ); Fri, 6 May 2016 02:46:34 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,585,1455004800"; d="asc'?scan'208";a="98242685" From: Felipe Balbi To: Jim Lin Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed In-Reply-To: <572B2208.7010804@nvidia.com> References: <1461321780-3226-1-git-send-email-jilin@nvidia.com> <87bn51uagb.fsf@intel.com> <571E0058.6020007@nvidia.com> <87r3dtrj7b.fsf@intel.com> <571F2BA3.6040209@nvidia.com> <871t5plya5.fsf@intel.com> <57234522.9000607@nvidia.com> <87bn4siq5c.fsf@intel.com> <5729ADC9.3030704@nvidia.com> <87shxyds7o.fsf@intel.com> <572B2208.7010804@nvidia.com> User-Agent: Notmuch/0.21+96~g9bbc54b (http://notmuchmail.org) Emacs/25.0.90.3 (x86_64-pc-linux-gnu) Date: Fri, 06 May 2016 09:44:25 +0300 Message-ID: <87mvo3it2u.fsf@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; 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; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Jim, Jim Lin writes: > On 2016=E5=B9=B405=E6=9C=8804=E6=97=A5 18:37, Felipe Balbi wrote: >> * PGP Signed by an unknown key >> >> >> Hi, >> >> Jim Lin writes: >> >> >> >>>>> In f_fs.c >>>>> " >>>>> static int __ffs_data_do_os_desc(enum ffs_os_desc_type type, >>>>> struct usb_os_desc_header *h, void *data, >>>>> unsigned len, void *priv) >>>>> { >>>>> struct ffs_data *ffs =3D priv; >>>>> u8 length; >>>>> >>>>> ENTER(); >>>>> >>>>> switch (type) { >>>>> case FFS_OS_DESC_EXT_COMPAT: { >>>>> struct usb_ext_compat_desc *d =3D data; >>>>> int i; >>>>> >>>>> if (len < sizeof(*d) || >>>>> d->bFirstInterfaceNumber >=3D ffs->interfaces_count || >>>>> d->Reserved1) >>>>> return -EINVAL; >>>>> " >>>> that's fine, but this is only failing because something else is >>>> returning the wrong set of descriptors (SS vs HS). That's the bug we >>>> want to fix, not work around it. >>>> >>> Thanks. >> you're welcome, but to fix that bug we need more information. Why is >> composite.c using the wrong set of descriptors ? What is your setup ? >> >> Are you using an in-kernel gadget ? which one ? > No, our gadget driver is on the way to submit. >> Using configfs or legacy >> gadgets ? gadgetfs ? f_fs ? > >> How to trigger this ? Can you provide >> instructions and (in case of gadgetfs/ffs) code to create a gadget that >> hits this problem ? >> > Please refer to > https://android.googlesource.com/platform/system/core/+/master/adb/usb_li= nux_client.cpp according to this, there is a set of SuperSpeed descriptors starting on linux 169: https://android.googlesource.com/platform/system/core/+/master/adb/usb_linu= x_client.cpp#169 I don't get what the problem is. You mentioned something about SS vs HS descriptors at some point, but that shouldn't be a problem seen that ADB provides SS descriptors. > Also this is a thought coming from another engineer for your reference. > " > > I think Microsoft and linux are contradicting the requirements.=20 > According MSFT's os descriptor definition, one of the reserved fields=20 > needs to be set to 1 whereas seems like f_fs.c expects them to be 0.=20 > (copy pasting from the spec downloaded from:=20 > https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx)= =20 I see.. > What does upstream think ? Requires some conflict resolution I guess !!=20 > Since the OS descriptors are from MSFT, I believe upstream has to drop=20 > the check and I think this patch might be valid.. If we difer from the spec, we need to remain compliant. I can see adb sets this to a 1 as the spec requires: https://android.googlesource.com/platform/system/core/+/master/adb/usb_linu= x_client.cpp#206 Now I understand the problem, it's not related to SS vs HS, it's just us using the wrong check for Reserved1. Here's one thing though, the patch isn't exactly correct. Instead of removing the check completely, we *must* force the correct check. IOW: if (len < sizeof(*d) || d->bFirstInterfaceNumber >=3D ffs->interfaces_count || =2D d->Reserved1) + !d->Reserved1) Heh, now your commit log makes more sense as well, but it could use some rewording. It appears, from that commit, that the problem is writing without SS descriptors, which it isn't. The real problem is the wrong check of the Reserved1 field in MSFT OS Descriptor. cheers =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXLD1JAAoJEIaOsuA1yqREousP/R3fQRGf4qp2rKCO2mwigLbr EC046LTe6d2LqXCOC4IwJPfyl4TCew/aNxZbW3xw/pWRw3v1lVo1W0YPqPGIScYv hGQTxj9uEK9r/uAf+2vkNGfQLy6Dg3TqWSpwA7nWtpur+TrwavU25KfW/ApdObkU voToYPlVvs+BF4XWK+gv+iupwlbFKXtLTcoix6c/QGkr/RiJV0dy3tmwwqvSvfNJ 4VDwhSwvK4BhZ1r08uq+VGpNKJDCgOFIKU8y/5cOygHRXlZZvBdZxMkHHI/xaSHH TgcJ4tcl+ioU9bL7NNYyzEHwmjhIPr7yB/njsdCxdIrb+U57nNzKGSnACGEJ/vWT i8p0GzGQ7zSBt8ehrJt5ymVy1YmxJWAyEEE1YjQOgmdnY4IIzGiYe78u2A10YrW1 Qb6vIR35RSQp7IzksdTzAZBiGwRVqtKTF8t3PU/spuDBuKefS5xrpkr+6JinlFjo /zkIgnq1VWmX+JEgGd8NBhHFuY4KYILrCAdG5BtH3B6DNyG5okz1cPT5gX7F0yOS AiH66qb7SiaEI3bQWcR+WXkArZb7oOKn4taO1XHJ7/UzzrQiRqaEa84XehhY4Ewu 48eVnxFOnN6+08YLb5U6+1SgMxr2urhnN/ZeDrs12iFU4g6bhlZcYn9rQTrkXS/D 9KbCauqb02kyyq5os5xA =Cj6e -----END PGP SIGNATURE----- --=-=-=--