On 3/19/2019 1:56 PM, Wu, Hao A wrote:
-----Original Message----- From: Ming Huang [mailto:ming.huang@linaro.org] Sent: Tuesday, March 19, 2019 12:14 PM To: Wu, Hao A; Leif Lindholm Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong, Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org; Kinney, Michael D; Gao, Liming; wanghuiqiang@huawei.com; huangming23@huawei.com; zhangjinsong2@huawei.com; huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J Subject: Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue
On 3/19/2019 10:25 AM, Wu, Hao A wrote:
-----Original Message----- From: Leif Lindholm [mailto:leif.lindholm@linaro.org] Sent: Monday, March 18, 2019 8:43 PM To: Ming Huang Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong, Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org;
Kinney,
Michael D; Gao, Liming; wanghuiqiang@huawei.com; huangming23@huawei.com; zhangjinsong2@huawei.com; huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J; Wu, Hao A;
Ni,
Ray Subject: Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue
+MdeModulePkg maintainers (you added MdePkg maintainers to cc)
This looks like an improvement to me.
Am I correct in guessing this behaviour refers to some specific corner case of a USB CDROM emulated from a BMC?
On Mon, Feb 25, 2019 at 05:10:52PM +0800, Ming Huang wrote:
The system environment: virtual-CDROM(USB interface) via BMC, insert
a
iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM to first boot option. With release version bios, disconnecting CDROM when boot to "1 seconds left, Press Esc or F2 to enter Setup" then system will get a exception.
The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be
uninstalled
in this situation after print some transfer error. The status will be invalid parameter. This line will get a exception for BlockIo not point
Do you mean 'EFI_INVALID_PARAMETER' is returned from: Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
**) &BlockIo);
Yes.
If so, my guess is that 'Handle' is NULL at this point. An improvement can be adding a previous check for 'Status' after the ASSERT at:
Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid,
&TempDevicePath, &Handle);
ASSERT_EFI_ERROR (Status);
As my debug output, this 'Status' is seccuss and Handle is not NULL, but gBS->ConnectController return:Not Found
Debug output: [BmExpandMediaDevicePath]:[1056L] Handle=3E3F3D18 BlockIo=3B2757B6 Media=AFAF6C617470AFAF Status=Success EhcExecTransfer: transfer failed with 40 EhcBulkTransfer: error - Device Error, transfer - 40 ......... [UsbOnHubInterrupt]:[632L] SignalEvent (HubIf->HubNotify) UsbBotExecCommand: UsbBotSendCommand (Device Error) UsbBootExecCmd: Device Error to Exec 0x0 Cmd (Result = 1) EhcExecTransfer: transfer failed with 40 ............... [USBMassDriverBindingStop]:[1010L] Uninstall USB block io, free: 3E44F218(F0) [BmExpandMediaDevicePath]:[1064L] Connect Not Found [BmExpandMediaDevicePath]:[1076L] Handle=3E3F3D18 BlockIo=3B2757B6 Media=AFAF6C617470AFAF Status=Invalid Parameter
Thanks for the debug information, I got it now.
The call to the gBS->ConnectController() leads to protocols being uninstalled from 'Handle' and removing 'Handle' from the database. Then within the call to gBS->HandleProtocol(), CoreValidateHandle() returns EFI_INVALID_PARAMETER since the handle cannot be found.
I am good with this patch, please help to address Leif's previous comment to keep the ASSERT.
I will add ASSERT back in v2.
Also, I have filed a Bugzilla tracker for this: https://bugzilla.tianocore.org/show_bug.cgi?id=1631
Could you help to add the reference to the above BZ in the commit log message? Thanks.
Sure, add it in v2.
Thanks
Best Regards, Hao Wu
Thanks
And leave:
Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
**) &BlockIo);
ASSERT_EFI_ERROR (Status);
unchanged.
Best Regards, Hao Wu
to right address: AllocatePool (BlockIo->Media->BlockSize) So, here need to judge the status not using ASSERT_EFI_ERROR.
Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ming Huang ming.huang@linaro.org
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index d5957db610d9..c2f1c651b02f 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c @@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath ( // Block IO read/write will success. // Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid,
(VOID
**) &BlockIo);
- ASSERT_EFI_ERROR (Status);
- if (EFI_ERROR (Status)) {
It would still be worth including an ASSERT here, to let DEBUG builds report on point of failure rather than several steps up the chain.
/ Leif
- return NULL;
- } Buffer = AllocatePool (BlockIo->Media->BlockSize); if (Buffer != NULL) { BlockIo->ReadBlocks (
-- 2.9.5