Re: [edk2-devel] [PATCH 04/35] EmbeddedPkg/Universal/MmcDxe: "fix" CloseProtocol() call in BindingStop()

2019-09-25 Thread Ard Biesheuvel
On Tue, 17 Sep 2019 at 21:49, Laszlo Ersek  wrote:
>
> The 3rd and 4th parameters of the CloseProtocol() call are wrong.
>
> Given that we're not dissociating a child controller from a parent
> controller (= closing a BY_CHILD_CONTROLLER open), but closing a BY_DRIVER
> open, the 4th parameter (ControllerHandle) should equal the 1st parameter
> (Handle).
>
> It's unclear why this code hasn't crashed before.
>
> Note that the patch doesn't fix the underlying driver model bug. I don't
> understand what the loop in MmcDriverBindingStop() attempts to do. Is this
> driver supposed to be a bus driver? It seems to create new handles, and to
> append device path nodes. But it doesn't set up proper parent/child
> protocol opens, and it doesn't close them.
>
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Signed-off-by: Laszlo Ersek 

This code predates my involvement in Tianocore, so I cannot answer
your questions.

For the change itself,

Acked-by: Ard Biesheuvel 

> ---
>
> Notes:
> build-tested only
>
>  EmbeddedPkg/Universal/MmcDxe/Mmc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.c 
> b/EmbeddedPkg/Universal/MmcDxe/Mmc.c
> index 2f9ec9c7e7c1..c6170880debd 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.c
> @@ -329,8 +329,9 @@ MmcDriverBindingStop (
>  // Close gEfiMmcHostProtocolGuid
>  Status = gBS->CloseProtocol (
>  Controller,
> -,(VOID **) >MmcHost,
> -This->DriverBindingHandle
> +,
> +This->DriverBindingHandle,
> +Controller
>  );
>
>  // Remove MMC Host Instance from the pool
> --
> 2.19.1.3.g30247aa5d201
>
>

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

View/Reply Online (#48038): https://edk2.groups.io/g/devel/message/48038
Mute This Topic: https://groups.io/mt/34180203/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



[edk2-devel] [PATCH 04/35] EmbeddedPkg/Universal/MmcDxe: "fix" CloseProtocol() call in BindingStop()

2019-09-17 Thread Laszlo Ersek
The 3rd and 4th parameters of the CloseProtocol() call are wrong.

Given that we're not dissociating a child controller from a parent
controller (= closing a BY_CHILD_CONTROLLER open), but closing a BY_DRIVER
open, the 4th parameter (ControllerHandle) should equal the 1st parameter
(Handle).

It's unclear why this code hasn't crashed before.

Note that the patch doesn't fix the underlying driver model bug. I don't
understand what the loop in MmcDriverBindingStop() attempts to do. Is this
driver supposed to be a bus driver? It seems to create new handles, and to
append device path nodes. But it doesn't set up proper parent/child
protocol opens, and it doesn't close them.

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Signed-off-by: Laszlo Ersek 
---

Notes:
build-tested only

 EmbeddedPkg/Universal/MmcDxe/Mmc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.c 
b/EmbeddedPkg/Universal/MmcDxe/Mmc.c
index 2f9ec9c7e7c1..c6170880debd 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Mmc.c
+++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.c
@@ -329,8 +329,9 @@ MmcDriverBindingStop (
 // Close gEfiMmcHostProtocolGuid
 Status = gBS->CloseProtocol (
 Controller,
-,(VOID **) >MmcHost,
-This->DriverBindingHandle
+,
+This->DriverBindingHandle,
+Controller
 );
 
 // Remove MMC Host Instance from the pool
-- 
2.19.1.3.g30247aa5d201



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

View/Reply Online (#47391): https://edk2.groups.io/g/devel/message/47391
Mute This Topic: https://groups.io/mt/34180203/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-