OvmfPkg/XenBusDxe: Don't call DisconnectController in Stop()

[edk2-devel] [PATCH] OvmfPkg/XenBusDxe: Don't call DisconnectController in Stop()

Posted by Anthony PERARD 4 years, 9 months ago

Calling DisconnectController() on children isn't part of the job of
EFI_DRIVER_BINDING_PROTOCOL.Stop() as it only needs to deallocate
resources allocated in Start(). The disconnection will happen when
both DevicePath and XenBus protocols gets uninstalled.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    Please apply this patch after:
    "OvmfPkg/XenBusDxe: Close XenIoProtocol openned by children"

 OvmfPkg/XenBusDxe/XenBusDxe.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
index 7c07a96650..634c7b71eb 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.c
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
@@ -446,12 +446,6 @@ XenBusDxeDriverBindingStop (
       continue;

     }

     ChildData = XENBUS_PRIVATE_DATA_FROM_THIS (XenBusIo);

-    Status = gBS->DisconnectController (ChildData->Handle, NULL, NULL);

-    if (EFI_ERROR (Status)) {

-      DEBUG ((EFI_D_ERROR, "XenBusDxe: error disconnecting child: %r\n",

-              Status));

-      continue;

-    }

 

     Status = gBS->CloseProtocol (Dev->ControllerHandle, &gXenIoProtocolGuid,

                     Dev->This->DriverBindingHandle, ChildData->Handle);

-- 
Anthony PERARD


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

View/Reply Online (#43119): https://edk2.groups.io/g/devel/message/43119
Mute This Topic: https://groups.io/mt/32270256/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: Don't call DisconnectController in Stop()

Posted by Laszlo Ersek 4 years, 9 months ago

On 07/01/19 13:14, Anthony PERARD wrote:
> Calling DisconnectController() on children isn't part of the job of
> EFI_DRIVER_BINDING_PROTOCOL.Stop() as it only needs to deallocate
> resources allocated in Start(). The disconnection will happen when
> both DevicePath and XenBus protocols gets uninstalled.

Correct. In the spec, UninstallMultipleProtocolInterfaces() refers to
UninstallProtocolInterface(), and the latter says,

    [...] Before the protocol interface is removed, an attempt is made
    to force all the drivers that are consuming the protocol interface
    to stop consuming that protocol interface. This is done by calling
    the boot service EFI_BOOT_SERVICES.DisconnectController() for the
    driver that currently have the protocol interface open with an
    attribute of EFI_OPEN_PROTOCOL_BY_DRIVER or
    EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE. [...]

And, the Driver Writer's Guide states, in a Note,

    When an attempt is made to remove a protocol interface from a handle
    in the handle database, the UEFI core firmware checks to see if any
    other UEFI drivers are currently using the services of the protocol
    to be removed. If UEFI drivers are using that protocol interface,
    the UEFI core firmware attempts to stop those UEFI drivers with a
    call to DisconnectController(). This is a quick, legal, and safe way
    to shut down any protocols associated with this driver's stack.

> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     Please apply this patch after:
>     "OvmfPkg/XenBusDxe: Close XenIoProtocol openned by children"
> 
>  OvmfPkg/XenBusDxe/XenBusDxe.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
> index 7c07a96650..634c7b71eb 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
> @@ -446,12 +446,6 @@ XenBusDxeDriverBindingStop (
>        continue;
>      }
>      ChildData = XENBUS_PRIVATE_DATA_FROM_THIS (XenBusIo);
> -    Status = gBS->DisconnectController (ChildData->Handle, NULL, NULL);
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "XenBusDxe: error disconnecting child: %r\n",
> -              Status));
> -      continue;
> -    }
>  
>      Status = gBS->CloseProtocol (Dev->ControllerHandle, &gXenIoProtocolGuid,
>                      Dev->This->DriverBindingHandle, ChildData->Handle);
> 

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

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

View/Reply Online (#43126): https://edk2.groups.io/g/devel/message/43126
Mute This Topic: https://groups.io/mt/32270256/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: Don't call DisconnectController in Stop()

Posted by Laszlo Ersek 4 years, 9 months ago

On 07/01/19 13:14, Anthony PERARD wrote:
> Calling DisconnectController() on children isn't part of the job of
> EFI_DRIVER_BINDING_PROTOCOL.Stop() as it only needs to deallocate
> resources allocated in Start(). The disconnection will happen when
> both DevicePath and XenBus protocols gets uninstalled.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     Please apply this patch after:
>     "OvmfPkg/XenBusDxe: Close XenIoProtocol openned by children"
> 
>  OvmfPkg/XenBusDxe/XenBusDxe.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
> index 7c07a96650..634c7b71eb 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
> @@ -446,12 +446,6 @@ XenBusDxeDriverBindingStop (
>        continue;
>      }
>      ChildData = XENBUS_PRIVATE_DATA_FROM_THIS (XenBusIo);
> -    Status = gBS->DisconnectController (ChildData->Handle, NULL, NULL);
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "XenBusDxe: error disconnecting child: %r\n",
> -              Status));
> -      continue;
> -    }
>  
>      Status = gBS->CloseProtocol (Dev->ControllerHandle, &gXenIoProtocolGuid,
>                      Dev->This->DriverBindingHandle, ChildData->Handle);
> 

Commit 6a1f06fadb26.

Thanks
Laszlo

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

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