From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web10.8176.1589184838995260518 for ; Mon, 11 May 2020 01:13:59 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=CJskNgFJ; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: guomin.jiang@intel.com) IronPort-SDR: lE9qT91V0SIGRp9gWcUM+YdMN91u6jzht0t8RfyDYBYplfUuhoK54W3Taih/lF2614sRXlaq8+ vGS0qkqQO4JQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 May 2020 01:13:58 -0700 IronPort-SDR: AHHwunVBN/uje5fjcIXTlwA+Hxio1du43QpY2yqmc4CNe/AuCrTB/baPiJyVe1+bps1M4yJzqI uJ/ZrtCyN+1A== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,379,1583222400"; d="scan'208";a="408845609" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga004.jf.intel.com with ESMTP; 11 May 2020 01:13:58 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 11 May 2020 01:13:58 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 11 May 2020 01:13:57 -0700 Received: from FMSEDG001.ED.cps.intel.com (10.1.192.133) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 11 May 2020 01:13:57 -0700 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (104.47.70.106) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 11 May 2020 01:13:57 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CgzRva53lhCjF8w28sDIfmNlh8K0nSoFowCkmxCwWQWGHZ+QHe95eeW3pm3C76GT5XcBSZhvKf66PM3DSWPC/jRW9C1RA/NTF7lL4OB1TK178Jd1dFSnLKT75ZJAUgPyQh8oUvHpdQsV8mZoW5izofqaqJXLwuLE9WF9eaLrxRhqUy0vEfr3cGrY1M1gtQehsGD5Ywe7/b1RAotvuu6kH2ZIMVCLUh946ly1nIXfMgWSpVd3AogKhU4kzS/VjfefK5CPradrgjaNTL9mWbENHm/SiG9qayBc2tU7HLVzaOo7BQSiaGhS809JphUwjXC3k0InceABv2ZBiOO/rBZhWg== 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=n0yKbcvF+M8N35Z9nm4mMZWMQ6bklP7nSz1yq0Sjgwc=; b=hj7k6xb3haEsEc1xTH2dbMmE64Gokn/7/bsiz1aqQ+WSXurjXhkhCLNZQzWu0WRCXio7qO3wws+aMvmvrMWjAMz1PyDen6lHUdrs/ufmAxnLFbf84mU6xmBEOjx3xvP7sUSrAhbWAJMzk7ZBNUkJSeyenCi6m9pLbGOWpgo9mBxos0nnKziWeOuQgmk7TciIQnJoNMZJELVwt1R1t7BVbSbBTMJpHRkASf88OIhCSzgWQSiDuWy2R0+EpDt1ecmU86BfPJGueNP3x6ZMQBEuDr4gWhZI69zE1rQngkzHI1SZyFOcmO75+QSdHZs3cFtLRxpJOyA1uZN47gqCmOIDjQ== 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=n0yKbcvF+M8N35Z9nm4mMZWMQ6bklP7nSz1yq0Sjgwc=; b=CJskNgFJwcDzVT/v8O4JvxqY3WHU/BDBZ2vcrJfVCzaIT0FIIvaYJ5eCQcTxTH+aCYJUiX/wFTjDLIoUM/wbt6uDNwuVds6m31Nomx3tRkBQFLMBH0yZts1/vJMh/uaF5hYMXf/eV9TalsVGP3UxEIxni9r/XPqbXj675nACebY= Received: from DM6PR11MB2955.namprd11.prod.outlook.com (2603:10b6:5:65::31) by DM6PR11MB3532.namprd11.prod.outlook.com (2603:10b6:5:70::25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2979.35; Mon, 11 May 2020 08:13:54 +0000 Received: from DM6PR11MB2955.namprd11.prod.outlook.com ([fe80::ccd4:4b0d:535a:58be]) by DM6PR11MB2955.namprd11.prod.outlook.com ([fe80::ccd4:4b0d:535a:58be%7]) with mapi id 15.20.2979.033; Mon, 11 May 2020 08:13:54 +0000 From: "Guomin Jiang" To: "Wu, Hao A" , "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/aiiRT9ggABDr9A= Date: Mon, 11 May 2020 08:13:54 +0000 Message-ID: References: <20200509082651.327-1-guomin.jiang@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-version: 11.2.0.6 dlp-product: dlpe-windows dlp-reaction: no-action 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.196] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 7225262b-67f6-4066-cdf9-08d7f5833f4d x-ms-traffictypediagnostic: DM6PR11MB3532: 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: sgjc8k1Y25JXl1/si5Su/mAgZoeci0hwz9xGI7PiGWy8os130gTetZz1AmOk7jnmc36qC9FEAh3H+aBMty3blpbyOKtIfJO/3LwTGVAcqJIU2z3wEOXRG1jeIuY8S8dtQWItqNRxzQOLofW782uhWLW4bTpuwQN2Hy44TMr7LYdOP34+K26sY+X65L3SsVzafswawsZUewpiSsxiadxMsx7A2l+qQsjyczu8sEupxEyRKmAuvp7Wj8hD3ITnBWWKYZVM6D6fWki5bSptYP545jAFRb1yrcmqqWOdsOpr8GeCNj4iKUArXOV2Sq6GebEzDAgX7s/hCLshB7GgNZyN6Nrnc00ZjgUQi3DXgbmlwk70Oo3nliYk+wMkGOKKpHzyEX2Ewo3+PZNHMCs0q5hJDydWfqtLopDMg4oryFlQ8O1wphUidiHl6/D/xdKMlf+TRiKBxfY8gHjWUcacBTDxFa56ePtrDuuZKFDqhN85D04JYy1o9bONuMkq4WlKc/TFG1PyrUaz9e/e56e6u9lHnxe7gSyrya1NyxH4YPCLnIqVg1DSobDKsKY3fbnuXNsa5bHV9bi3LF3ytoD/ztPwYDWd1/Nv1uldNN1nTTe25/k= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR11MB2955.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(39860400002)(396003)(136003)(366004)(346002)(376002)(33430700001)(66446008)(316002)(8676002)(86362001)(9686003)(5660300002)(33656002)(186003)(6506007)(53546011)(52536014)(7696005)(33440700001)(54906003)(26005)(110136005)(76116006)(71200400001)(107886003)(66476007)(66946007)(64756008)(55016002)(8936002)(66556008)(4326008)(966005)(478600001)(2906002);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: nKQyU2xujANYV7AEMkUhd++mLqMxTmzcSIP8uYm3vLP7twrOAcnXmKTRgflLr9GzdcJyvL8QkTAQI80x72JeCWsRw0oHBj5IvOJIlhqjhvRMz7PdfmON9SHBbdUw8hWoc825mauhY93sXPkPqIW+qwZYQFigJhrgLIMzsBck73xk6PDreNxFQYO5p9gK6hepJc/GFJopxQ+DAnzPNix3xM1NKGeeOpo8MJSqz6F/Wf/S7H9tYARYeda2+OSYDwCXhc0N5b84RPgUGbpaM5WGUPlOCu/XqAU/ITUXSg+NjovCzfijWCq3S0UQNphjSc61CrIqGLbJShr3adpptOO3pxAKpIP6z3qb8jd5LyPVPCEaO3r10EBgBzuNFmL5OhZkaADIhrV6bT2pv2MJ1WWHwWsPG4PyaJAByv9dxIwLIR3ajDora9+ZVQcgIHzHMERQrtkvlTM244HreJAepVaVdLWrGx+gzJJbvv4G7vdtsmA= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 7225262b-67f6-4066-cdf9-08d7f5833f4d X-MS-Exchange-CrossTenant-originalarrivaltime: 11 May 2020 08:13:54.5860 (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: 5nbLwgfRRV22Sbzp/fEAM+NDIYOAr4bFbv6yxh59iIFbnsX4MgXkHqWj0Xhpl7IQhlWbS28Mzk+QZpLtYQGOpw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB3532 Return-Path: guomin.jiang@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Hao, Please see the comments inline. > -----Original Message----- > From: Wu, Hao A > Sent: Monday, May 11, 2020 1:25 PM > 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 >=20 > > -----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 > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2694 > > > > When the USB fail and then Reset Device, it should rebuild description. >=20 >=20 > Hello Guomin, >=20 > Could you help to add a brief summary of the relationship between the > proposed fix and the issue reported in the above BZ tracker? It would be > helpful for clarifying the purpose of the commit. Thanks. I will add the brief summary in next version patch. >=20 > One more inline comments below: >=20 >=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(+) > > > > 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 > > + table - > > %r\n", Status)); > > + goto ON_EXIT; > > + } > > + > > Status =3D UsbSetConfig (Dev, > > Dev->ActiveConfig->Desc.ConfigurationValue); > > > > 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; > > } > > > > +/** > > + 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.NumEndpoints; > > + 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); >=20 >=20 > For the above codes in the 'else' block, how about: > 1. Free the origin descriptor in UsbDev->DevDesc->Configs 2. Assign the n= ew > descriptor 'ConfigDesc' to UsbDev->DevDesc->Configs By doing so, I think > the memory copy of the descriptors can be skipped. Current memory layout as below: UsbDev->DevDesc --> [many configs] --> Configs[0] --> [many interfaces] --> interface[0] --> [many endpoints] --> endpoint[0] --> endpoint[1] ... --> endpoint[NumEndpoints-1] --> Configs[1] --> [many interfaces] --> endpoint[0] --> endpoint[1] ... --> endpoint[NumEndpoints-1] ... --> Configs[NumConfig-1] --> [many interfaces] --> endpoint[0] --> endpoint[1] ... --> endpoint[NumEndpoints-1] Some code may refer the original interface and endpoint, if free it, it may= be dereferenced. In previous practice, I use UsbFreeDevDesc() function to free the buffer bu= t observe the issue. >=20 > The reminder of the patch looks good to me. >=20 > Best Regards, > Hao Wu >=20 >=20 > > + } > > + } > > + > > + return EFI_SUCCESS; > > +} > > > > /** > > 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 > > ); > > > > +/** > > + 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. > > > > -- > > 2.25.1.windows.1