Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751957AbcDVLy5 (ORCPT ); Fri, 22 Apr 2016 07:54:57 -0400 Received: from mga01.intel.com ([192.55.52.88]:56225 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751254AbcDVLyz (ORCPT ); Fri, 22 Apr 2016 07:54:55 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,517,1455004800"; d="asc'?scan'208";a="950689660" From: Felipe Balbi To: Jim Lin Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Jim Lin Subject: Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed In-Reply-To: <1461321780-3226-1-git-send-email-jilin@nvidia.com> References: <1461321780-3226-1-git-send-email-jilin@nvidia.com> User-Agent: Notmuch/0.21+96~g9bbc54b (http://notmuchmail.org) Emacs/25.0.90.3 (x86_64-pc-linux-gnu) Date: Fri, 22 Apr 2016 14:52:52 +0300 Message-ID: <87bn51uagb.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-Length: 3637 Lines: 110 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi Jim, Jim Lin writes: > Android N adds os_desc_compat in v2_descriptor by init_functionfs() > (system/core/adb/usb_linux_client.cpp) to support automatic install > of MTP driver on Windows for USB device mode. > > Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field > and return -EINVAL. > This results in a second adb_write of usb_linux_client.cpp > (system/core/adb/) which doesn't have ss_descriptors filled. > Then later kernel_panic (composite.c) occurs when ss_descriptors > as a pointer with NULL is being accessed. where exactly in composite.c are we dereferencing a NULL pointer ? Is this happening on set_config() ? If that's the case, why is gadget->speed set to USB_SPEED_SUPER to start with ? Your controller should already have negotiated highspeed which means function_descriptors() should have returned highspeed descriptors, not a NULL superspeed. Care to explain why you haven't negotiated Highspeed ? The only thing I can think of is that you're using a Superspeed-capable peripheral controller (dwc3?) with maximum-speed set to Superspeed, with a Superspeed-capable cable connected to an XHCI PC, but loading a high-speed gadget driver (which you got from Android, written with f_fs) and this gadget doesn't tell composite that its maximum speed is Highspeed, instead of super-speed. We can add a check, sure, to avoid a kernel oops; however, you should really fix up the gadget implementation and/or set dwc3's maximum-speed property accordingly. Can you check if this patch makes your scenario work while still being fully functional ? diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index de9ffd60fcfa..3d3cdc5ed20d 100644 =2D-- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f, { struct usb_descriptor_header **descriptors; =20 + /* + * NOTE: we try to help gadget drivers which might not be setting + * max_speed appropriately. + */ + switch (speed) { case USB_SPEED_SUPER_PLUS: descriptors =3D f->ssp_descriptors; =2D break; + if (descriptors) + break; + /* FALLTHROUGH */ case USB_SPEED_SUPER: descriptors =3D f->ss_descriptors; =2D break; + if (descriptors) + break; + /* FALLTHROUGH */ case USB_SPEED_HIGH: descriptors =3D f->hs_descriptors; =2D break; + if (descriptors) + break; + /* FALLTHROUGH */ default: descriptors =3D f->fs_descriptors; } =20 + /* + * if we can't find any descriptors at all, then this gadget deserves to + * Oops with a NULL pointer dereference + */ + return descriptors; } =20 =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXGhCVAAoJEIaOsuA1yqREPVkP/jedbpz4eZ/qfm2LNkntEQTL RQgCMAPYa4/oUvGK/ahc9rwOX2yRfnmPJgOHCSaDQ89qiraeEEG3rwEUpX9em/JM IV2VGIIz9IN1ECFPIGoHuDwZo2l/6VEnsc5bKS38jylVAa+FQQ1OXttKCIWyUwoh qpPJ9w9IMiZQm0S1+nCaUaimMVhTCseDGtoeUOxSpvyAyXXy08z9RQab9BizNgJS amHJqFj3Dgowhoh8Ths6vv9eFQ713o4ysTstVeXupoZ3JS4VQH9KNxVstnyT0uY9 utC37xK68bMFlrYGMhdALh/9pTB49/VcUuzN2uowKGqrjmgCLpa7U/raLBlzKSvG Mr7cC4yPcwPGFUfbjGwkAr71b4BIzfej1RvtqUn00FIJ6im54v/BegGFVZWDzVH9 6/dZvcI6exIUyWt9KhKJP28GBpPf/ROc+VSq1weWSAmfoQsmoCjReOeVC/KCThtV Tr21wTWr22GeK/F1XjbRLaAQd6E89imlQJ/tXUHV4VQqrOz9TrHaitlPAj+gEWFd dwrp6D7XseT04nDLPdYWvdpJNMPS3U6FrETFmwB7RqKXU05KBrEw6hlih+R/iQAA k5RLDw8eyLDN1ypA5LY5qxONmt2PG0GjuUMDrxNuk3n1c/iqo+WUANlOgYjhJ7mt 7oJQBSorIbNKHUcXAETB =tWUS -----END PGP SIGNATURE----- --=-=-=--