OvmfPkg/XenBusDxe: Close XenIoProtocol openned by childs

[edk2-devel] [PATCH] OvmfPkg/XenBusDxe: Close XenIoProtocol openned by childs

Posted by Anthony PERARD 4 years, 9 months ago

In XenBusDxe, the XenBusAddDevice() opens the gXenIoProtocolGuid on
behalf of child controllers. It is never closed and prevent from
uninstalling the protocol.

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

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

+

     Status = gBS->UninstallMultipleProtocolInterfaces (

                ChildData->Handle,

                &gEfiDevicePathProtocolGuid, ChildData->DevicePath,

-- 
Anthony PERARD


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43031): https://edk2.groups.io/g/devel/message/43031
Mute This Topic: https://groups.io/mt/32243460/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 4 years, 9 months ago

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43049): https://edk2.groups.io/g/devel/message/43049
Mute This Topic: https://groups.io/mt/32243460/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Anthony PERARD 4 years, 9 months ago

On Sun, Jun 30, 2019 at 12:56:57PM +0200, Laszlo Ersek wrote:
> (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().

That sounds good and works fine without the call, I'll send a separate
patch.

-- 
Anthony PERARD

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43114): https://edk2.groups.io/g/devel/message/43114
Mute This Topic: https://groups.io/mt/32243460/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-