Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Ming Huang <ming.huang@linaro.org>]

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
>>>>>