Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

["Zeng, Star" <star.zeng@intel.com>]

Laszlo,

I agree to revert this patch first since it breaks something.
Could you help do that with detailed revert log?

Anyway, have you found any bug in this patch itself?


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, June 27, 2017 8:47 AM
To: Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

On 06/23/17 12:09, Amit Kumar wrote:
> Change since v3:
> 1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and 
> Inteface = NULL case. [Reported by:star.zeng at intel.com]
> 
> Change Since v2:
> 1) Modified to use EFI_ERROR to get status code
> 
> Change since v1:
> 1) Fixed typo protocal to protocol
> 2) Fixed coding style
> 
> Modified source code to update Interface as per spec.
> 1) In case of Protocol is un-supported, interface should be returned NULL.
> 2) In case of any error, interface should not be modified.
> 3) In case of Test Protocol, interface is optional.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Amit Kumar <amit.ak@samsung.com>
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

This patch breaks *at least* the following drivers:
- IntelFrameworkModulePkg/.../IsaBusDxe
- IntelFrameworkModulePkg/.../IsaSerialDxe
- MdeModulePkg/.../TerminalDxe
- MdeModulePkg/.../ScsiBusDxe

I have patches for the first three. I'm too tired to complete the patch for the fourth module (and any modules that might still be affected, but I'd only see those in the OVMF boot process if I got past the ScsiBusDxe
problem.)

The issue was reported by Gabriel here:

https://lists.01.org/pipermail/edk2-devel/2017-June/011886.html

although he didn't get past of the first driver, of course.

Now, what this patch does is valid, as far as the UEFI spec is concerned. And the above (now broken) drivers are all incorrect to assume that EFI_ALREADY_STARTED guarantees that OpenProtocol() has filled in Interface on output.

However, such an intrusive fix must be accompanied by extensive testing; preferably using one of the in-tree emulation platforms, such as OvmfPkg running on QEMU. And, when testing uncovers the failures in those drivers, patches should be submitted to fix those drivers *first*, and then the patch for OpenProtocol() should be presented *last*.

I'm attaching the first three patches I have now. As I said I'm too tired to work on ScsiBusDxe (the SCSIBusDriverBindingStart() function has the same bug around EFI_ALREADY_STARTED). Which is why I'm not posting the first three patches normally, with git-send-email; the work is incomplete.

Honestly, I'm thinking that commit 45cfcd8dccf8 should be reverted now, then all the drivers used by OvmfPkg should be fixed up (not by me, obviously!), and once OVMF boots again, we can re-commit (= cherry-pick) commit 45cfcd8dccf8.

Thanks
Laszlo