From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web10.6577.1589174724419808439 for ; Sun, 10 May 2020 22:25:24 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=Oc43wYvC; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: hao.a.wu@intel.com) IronPort-SDR: c8EzWU2O0QaZkFTxX06cSNeW4YAjlq2JY3hpFKndMDrNtnkmg4r6o28jmcQHu5JzIMzpGUBoNC FTmbUUydW8/g== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 May 2020 22:25:24 -0700 IronPort-SDR: wihAmrzVMdC2ZhzHzNe/0YssDOHJblkB+BL5HZcZMWsG35jLa/13DgI6q6vrusu36mDyx1+Mjv dY9Ho+dpthDA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,378,1583222400"; d="scan'208";a="463259854" Received: from orsmsx102.amr.corp.intel.com ([10.22.225.129]) by fmsmga006.fm.intel.com with ESMTP; 10 May 2020 22:25:23 -0700 Received: from orsmsx157.amr.corp.intel.com (10.22.240.23) by ORSMSX102.amr.corp.intel.com (10.22.225.129) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 10 May 2020 22:25:23 -0700 Received: from ORSEDG002.ED.cps.intel.com (10.7.248.5) by ORSMSX157.amr.corp.intel.com (10.22.240.23) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 10 May 2020 22:25:23 -0700 Received: from NAM04-CO1-obe.outbound.protection.outlook.com (104.47.45.51) by edgegateway.intel.com (134.134.137.101) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 10 May 2020 22:25:23 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=D5Vnl4J2DDn7z4UKrnMJQb8Tp4nqgY/7YOzzBpLBbBsTnD/4GE1edPGCsfS/zDK7PEJ4hpsyETzBp/7umm/tOoPqknhGR6c5fg1QfhgTAgae+jtq2x6I5QKBpoHXw0w1ZmeiGPlG5oEwQu3/qcvNvhy8RMibdRTYkqHRlWjaGNGgBRbq61nURS/T4YFXB0h7AFlCqB8tagCR24tpFO3gSjezRU103EeH01Nbapqkt1jGDcZDsDd+Hp0HLkq3Xj+HY4Bfj2euRNxuCx4NZDk/r0lYtJ31X7rTuv3tZ6IeYxUBixJGyaQBuxxgAFNfts0xIhnnsbHbWuMtWEjsieZ5QA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=ADShdvbtvUmoeCIK097MZgNJHt+RiATHdPgDidkctzU=; b=MHsEbAwGssBE5OCuPQ7dIQ9TG9vgDKeegKQjVYSnjoijzb1/2Kj8zDUiEOYtoPe4WUYmN+Kk7tYv5VyExXquBvx4ml9Sebi/OZjz+Cgx07X6JwmzmYzT4DDH5PC2IvAcfwCV8F7h6qzavzUND/M8g7XzGJEXW0XkZ1Vj14gZSUEftQXZamLeb98/5rrwFlqxUGOCXw/CW07zJgCjGbtEK+aIBe6shWiFj86mVATfQCqq42jZQMuSat/W2hND+jsIEG3wEl4ENW7g/6t/PBOHh4g2SuC0/kavDHKwxk1yrbvJmG4H9ZMy/aREOs3W5BKWRcfcK3Kl46EwiTT/3er5nQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=ADShdvbtvUmoeCIK097MZgNJHt+RiATHdPgDidkctzU=; b=Oc43wYvCX8LdUFtsbNTi5jW9H7kyiNyqjtdPDaWh4Zr7N/RtD3CPb2Kuja6El6Dw3yZEKXGt+3P2q2ktyAlrWsccPKovE1+y8N9wcIap3+6b1PO2gx+5FOIxEB2TYBAXaLM7KpEyhKqzBHC2tZrpfdX5TEOdZG3pj/3YNnTsIJY= Received: from BN8PR11MB3666.namprd11.prod.outlook.com (2603:10b6:408:8c::19) by BN8PR11MB3746.namprd11.prod.outlook.com (2603:10b6:408:8c::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2979.26; Mon, 11 May 2020 05:25:19 +0000 Received: from BN8PR11MB3666.namprd11.prod.outlook.com ([fe80::24c2:ec4b:91aa:d77e]) by BN8PR11MB3666.namprd11.prod.outlook.com ([fe80::24c2:ec4b:91aa:d77e%5]) with mapi id 15.20.2979.033; Mon, 11 May 2020 05:25:19 +0000 From: "Wu, Hao A" To: "Jiang, Guomin" , "devel@edk2.groups.io" CC: "jeremy.linton@arm.com" , "Wang, Jian J" , "Ni, Ray" Subject: Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device Thread-Topic: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device Thread-Index: AQHWJdunQEHxu/ajEU+2DtGyWSVA/aiiRT9g Date: Mon, 11 May 2020 05:25:18 +0000 Message-ID: References: <20200509082651.327-1-guomin.jiang@intel.com> In-Reply-To: <20200509082651.327-1-guomin.jiang@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.55.52.218] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1bbb4321-64c0-43c2-1029-08d7f56bb1df x-ms-traffictypediagnostic: BN8PR11MB3746: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 04004D94E2 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: U5ZbGmiUIkS7v4LfO/c01A0ZTo100AQuVRpV5iTr82UqGJo8+uz5K+qul8M5nT5j4M09T2yhGuGPRskBW5NaKVhenLhI02RnA7BCS6nFNqZ4UYAGNUL3MoOkra7OJtyHF/laMC8KYlxWgyVCqG47YSmOSoSacxGnKZ3a75FKWmrW3JDKbLioAZOLfY9F69GTwoMP2oYA4MwgPBKYwcEgJKcn6QKReke7dWj8oJzqxRWGRh4d+WEWPEaVJVq9MwNP7j7eSFzZQxE70f4tWomnCUOkVlVkAVfrSkEpufQRMJoi0jwB1EUk+5WTzu1tqv2Jb5WW5DRQZBsFvyeyF9E+XAWv+MYmPKHT6mzx4Zpo3RTeYN3CaiVddn+Lp8XDMswvBsVUwCSnLbSGA8L+QP8rYMj5+2b6gvyLTh97PUuANEZ9DsMBbwVIoc2oyyeP3jZfDZABCC5ibs97KhNBxM0DBD2uEoYTekW5m19KWSUi7NJ9pwryzYl8mQ9OfN/AfpyiCv8ifM3ruxmtEgY//OPgC/z7SNYJBRh09H/pHLLkAmUTMS2MI3QKtgieeHsf7stcSl0GwkgyS19JG5sU6g0rrVzGHbs9rBLEJpKnpSbU0eo= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN8PR11MB3666.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(136003)(396003)(39860400002)(366004)(346002)(376002)(33430700001)(5660300002)(8936002)(71200400001)(33440700001)(66556008)(66946007)(64756008)(66476007)(76116006)(66446008)(86362001)(4326008)(186003)(26005)(107886003)(9686003)(478600001)(316002)(33656002)(8676002)(55016002)(54906003)(110136005)(2906002)(53546011)(6506007)(52536014)(966005)(7696005);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: ukWAVlRW7o0FOnuZtudu1Ab3MW2DYyZZRxFH0WRJFxqhepB3GnQ6V7f/NA19B8p4TFVPu9jDrMzrnqodDPu/xLcO+9WMoNlmSqj9IZzRQfbBbd7mxq8ymKAy8No09Bd0/nckGJu6DuEmgxm63jRDKtEFVokmSx7vgpKK4ZqqKFSB7rsFnTtjBweGOOVqI7C+dutxmam/5S72BoHZQRpv4B87sOCQbkynAdjTAfplCsCPqd7Xo6QtlSkpIKZxRJy1rqUkDZea8d+0fesmP4hAgePga4YM/WDgy6X4skyKR/7Ard9OB7JV6oGjzni/g7LEt4sP9J2rKupvjhXUF4T1gBo5vZBJpVzYyLmftGxtWZ7MNTqwLxEqA3BGntczHwJeOnUmtnOBSvGOZ3UIahYNvVgCdFZQ+7wMazcTSFs5a8hJEE3CVXKeufg3C0yoVvjxVjxz9MahkzTBY/dc6TCvmRWO52lp7wTDMvvIlffDOw0= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 1bbb4321-64c0-43c2-1029-08d7f56bb1df X-MS-Exchange-CrossTenant-originalarrivaltime: 11 May 2020 05:25:18.9307 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: GHirtWzMZfqXpxH8sT4ByXKBcWCw9DUkLRHlFANojlhUZ250WO4B+CmZ5Zin36kdv8rKfAxgSPRZTi4zEAO8nA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR11MB3746 Return-Path: hao.a.wu@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Jiang, Guomin > Sent: Saturday, May 9, 2020 4:27 PM > To: devel@edk2.groups.io > Cc: jeremy.linton@arm.com; Wang, Jian J ; Wu, Hao = A > ; Ni, Ray > Subject: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description > table after Reset Device >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2694 >=20 > When the USB fail and then Reset Device, it should rebuild description. Hello Guomin, Could you help to add a brief summary of the relationship between the propo= sed fix and the issue reported in the above BZ tracker? It would be helpful for clarifying the purpose of the commit. Thanks. One more inline comments below: >=20 > Signed-off-by: Guomin Jiang > Cc: Jian J Wang > Cc: Hao A Wu > Cc: Ray Ni > --- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 7 ++ > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 152 > +++++++++++++++++++++++ MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h > | 14 +++ > 3 files changed, 173 insertions(+) >=20 > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c > index 4b4915c019ad..7bb9373a6adf 100644 > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c > @@ -874,6 +874,13 @@ UsbIoPortReset ( > // is in CONFIGURED state. > // > if (Dev->ActiveConfig !=3D NULL) { > + Status =3D UsbRebuildDescTable (Dev); > + > + if (EFI_ERROR (Status)) { > + DEBUG (( DEBUG_ERROR, "UsbIoPortReset: failed to rebuild desc tabl= e - > %r\n", Status)); > + goto ON_EXIT; > + } > + > Status =3D UsbSetConfig (Dev, Dev->ActiveConfig->Desc.ConfigurationV= alue); >=20 > if (EFI_ERROR (Status)) { > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c > index b08188b1bc78..d8e5e50b7c5a 100644 > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c > @@ -900,6 +900,158 @@ UsbBuildDescTable ( > return EFI_SUCCESS; > } >=20 > +/** > + Copy the interface descriptor > + > + @param[out] DescBuf The buffer of raw descriptor. > + @param[in] SrcBuf The buffer of raw descriptor > +**/ > +VOID > +UsbCopyInterfaceDesc ( > + OUT USB_INTERFACE_DESC *DescBuf, > + IN USB_INTERFACE_DESC *SrcBuf > + ) > +{ > + UINTN Index, IndexI; > + > + ASSERT ((DescBuf !=3D NULL) && (SrcBuf !=3D NULL)); > + > + if (DescBuf->NumOfSetting =3D=3D SrcBuf->NumOfSetting) { > + DescBuf->ActiveIndex =3D SrcBuf->ActiveIndex; > + > + for (Index =3D 0; Index < DescBuf->NumOfSetting; Index++) { > + CopyMem (&DescBuf->Settings[Index]->Desc, > + &SrcBuf->Settings[Index]->Desc, > + sizeof(EFI_USB_INTERFACE_DESCRIPTOR)); > + > + if (DescBuf->Settings[Index]->Desc.NumEndpoints =3D=3D > + SrcBuf->Settings[Index]->Desc.NumEndpoints) { > + > + for (IndexI =3D 0; IndexI < DescBuf->Settings[Index]->Desc.NumEn= dpoints; > + IndexI++) { > + CopyMem (DescBuf->Settings[Index]->Endpoints[IndexI], > + SrcBuf->Settings[Index]->Endpoints[IndexI], > + sizeof(USB_ENDPOINT_DESC)); > + } > + } > + } > + } > +} > + > +/** > + Copy the configuration descriptor and its interfaces. > + > + @param[out] DescBuf The buffer of raw descriptor. > + @param[in] SrcBuf The buffer of raw descriptor > +**/ > +VOID > +UsbCopyConfigDesc ( > + OUT USB_CONFIG_DESC *DescBuf, > + IN USB_CONFIG_DESC *SrcBuf > + ) > +{ > + UINTN Index; > + > + ASSERT ((DescBuf !=3D NULL) && (SrcBuf !=3D NULL)); > + > + if (DescBuf->Desc.NumInterfaces =3D=3D SrcBuf->Desc.NumInterfaces) { > + CopyMem (&DescBuf->Desc, &SrcBuf->Desc, > + sizeof(EFI_USB_CONFIG_DESCRIPTOR)); > + > + for (Index =3D 0; Index < DescBuf->Desc.NumInterfaces; Index++) { > + UsbCopyInterfaceDesc (DescBuf->Interfaces[Index], SrcBuf- > >Interfaces[Index]); > + } > + } > +} > + > +/** > + Rebuild the whole array of descriptors. > + > + @param[in] UsbDev The Usb device. > + > + @retval EFI_SUCCESS The descriptor table is build. > + @retval EFI_DEVICE_ERROR Invalid config > + @retval Others Command error when get descriptor. > +**/ > +EFI_STATUS > +UsbRebuildDescTable ( > + IN USB_DEVICE *UsbDev > + ) > +{ > + EFI_USB_CONFIG_DESCRIPTOR *Config; > + USB_CONFIG_DESC *ConfigDesc; > + UINT8 NumConfig; > + EFI_STATUS Status; > + UINT8 Index; > + > + // > + // Override the device descriptor, Current device fail but the > + original // descriptor pointer array is still exist. > + // > + Status =3D UsbCtrlGetDesc ( > + UsbDev, > + USB_DESC_TYPE_DEVICE, > + 0, > + 0, > + UsbDev->DevDesc, > + sizeof (EFI_USB_DEVICE_DESCRIPTOR) > + ); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "UsbRebuildDescTable: failed to override device > descriptor - %r\n", Status)); > + return Status; > + } > + > + NumConfig =3D UsbDev->DevDesc->Desc.NumConfigurations; > + if (NumConfig =3D=3D 0) { > + return EFI_DEVICE_ERROR; > + } > + > + DEBUG ((DEBUG_INFO, "UsbReuildDescTable: device has %d configures\n", > + NumConfig)); > + > + // > + // Read each configurations, then parse them // for (Index =3D 0; > + Index < NumConfig; Index++) { > + Config =3D UsbGetOneConfig (UsbDev, Index); > + > + if (Config =3D=3D NULL) { > + DEBUG ((DEBUG_INFO, "UsbRebuildDescTable: failed to get configure > + (index %d)\n", Index)); > + > + // > + // If we can get the default descriptor, it is likely that the > + // device is still operational. > + // > + if (Index =3D=3D 0) { > + return EFI_DEVICE_ERROR; > + } > + > + break; > + } > + > + ConfigDesc =3D UsbParseConfigDesc ((UINT8 *) Config, > + Config->TotalLength); > + > + FreePool (Config); > + > + if (ConfigDesc =3D=3D NULL) { > + DEBUG ((DEBUG_ERROR, "UsbRebuildDescTable: failed to parse > + configure (index %d)\n", Index)); > + > + // > + // If we can get the default descriptor, it is likely that the > + // device is still operational. > + // > + if (Index =3D=3D 0) { > + return EFI_DEVICE_ERROR; > + } > + > + break; > + } else { > + UsbCopyConfigDesc (UsbDev->DevDesc->Configs[Index], ConfigDesc); > + UsbFreeConfigDesc (ConfigDesc); For the above codes in the 'else' block, how about: 1. Free the origin descriptor in UsbDev->DevDesc->Configs 2. Assign the new descriptor 'ConfigDesc' to UsbDev->DevDesc->Configs By doing so, I think the memory copy of the descriptors can be skipped. The reminder of the patch looks good to me. Best Regards, Hao Wu > + } > + } > + > + return EFI_SUCCESS; > +} >=20 > /** > Set the device's address. > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h > index 7b0c77fdc79c..54367133241a 100644 > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h > @@ -169,6 +169,20 @@ UsbBuildDescTable ( > IN USB_DEVICE *UsbDev > ); >=20 > +/** > + Rebuild the whole array of descriptors. > + > + @param[in] UsbDev The Usb device. > + > + @retval EFI_SUCCESS The descriptor table is build. > + @retval EFI_DEVICE_ERROR Invalid config > + @retval Others Command error when get descriptor. > +**/ > +EFI_STATUS > +UsbRebuildDescTable ( > + IN USB_DEVICE *UsbDev > + ); > + > /** > Set the device's address. >=20 > -- > 2.25.1.windows.1