Re: [PATCH] OvmfPkg/XenBusDxe: Close XenIoProtocol openned by childs

["Laszlo Ersek" <lersek@redhat.com>]

Hi Anthony,

the patch is good; please post a v2 with the following minor
improvements:

On 06/28/19 18:16, Anthony PERARD wrote:
> In XenBusDxe, the XenBusAddDevice() opens the gXenIoProtocolGuid on
> behalf of child controllers. It is never closed and prevent from

(1) s/prevent/prevents us/

> uninstalling the protocol.
>
> Close it were we stop all the childs in XenBusDxe->Stop().

(2) s/were/where/

(3) s/childs/children/ -- applies to the subject line as well

>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenBusDxe/XenBusDxe.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
> index 0e63707f50..fac6f3a09d 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
> @@ -453,6 +453,11 @@ XenBusDxeDriverBindingStop (
>        continue;
>      }
>
> +    Status = gBS->CloseProtocol (Dev->ControllerHandle, &gXenIoProtocolGuid,
> +                                 Dev->This->DriverBindingHandle,
> +                                 ChildData->Handle);
> +    ASSERT_EFI_ERROR (Status);
> +

(4) The indentation of function call arguments is inconsistent in this
driver. Still, if it's not a lot of trouble, please correct the
indentation here. Please pick one of the two below:

(4a)

    Status = gBS->CloseProtocol (
                    Dev->ControllerHandle,
                    &gXenIoProtocolGuid,
                    Dev->This->DriverBindingHandle,
                    ChildData->Handle
                    );

(4b)

    Status = gBS->CloseProtocol (Dev->ControllerHandle, &gXenIoProtocolGuid,
                    Dev->This->DriverBindingHandle, ChildData->Handle);

With those updates, please add, to v2:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


(5) Side remark (no need to do anything about it in the scope of this
patch): I think the DisconnectController() call in
XenBusDxeDriverBindingStop() is superfluous. That kind of disconnection
is not the job of EFI_DRIVER_BINDING_PROTOCOL.Stop().

Thanks
Laszlo

>      Status = gBS->UninstallMultipleProtocolInterfaces (
>                 ChildData->Handle,
>                 &gEfiDevicePathProtocolGuid, ChildData->DevicePath,
>