I think this patch improves QemuVideoDxe, but I have a few remarks:
On 12/12/13 00:23, Chris Ruffin wrote:
> The QemuVideoDxe driver creates child controller handles, so it is acting
> as a hybrid bus driver. The child handles should open the parent's bus
> protocol BY_CHILD_CONTROLLER to properly maintain the protocol usage count.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chris Ruffin <[email protected]>
> ---
>
> Patch revision 3 also includes a minor edit to close protocol on
> the appropriate handle.
>
>
> OvmfPkg/QemuVideoDxe/Driver.c | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 53718e1..e295b25 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -419,6 +419,22 @@ QemuVideoControllerDriverStart (
> &Private->GraphicsOutput,
> NULL
> );
> + if (EFI_ERROR (Status)) {
> + goto Error;
> + }
> +
> + Status = gBS->OpenProtocol (
> + Controller,
> + &gEfiPciIoProtocolGuid,
> + &Private->Handle,
This seems to be in the right place.
(1) But, I think the third parameter should be a pointer to a new
variable that is not used for anything else, such as:
EFI_PCI_IO_PROTOCOL *ChildPciIo;
/* ... */
Status = gBS->OpenProtocol (
Controller,
&gEfiPciIoProtocolGuid,
(VOID **) &ChildPciIo, // here
This->DriverBindingHandle,
Private->Handle,
EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
);
Otherwise I think this call overwrites Private->Handle with the address
of Controller's PciIo protocol instance (ie. the same value as held in
Private->PciIo).
> + This->DriverBindingHandle,
> + Private->Handle,
> + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
> + );
> +
> + if (EFI_ERROR (Status)) {
> + goto Error;
> + }
> }
This goto is not necessary, because the Error label is right below. But
it doesn't hurt and might even prove helpful if further code is added.
>
> Error:
> @@ -440,7 +456,14 @@ Error:
> // Close the PCI I/O Protocol
> //
> gBS->CloseProtocol (
> - Private->Handle,
> + Controller,
> + &gEfiPciIoProtocolGuid,
> + This->DriverBindingHandle,
> + Controller
> + );
> +
> + gBS->CloseProtocol (
> + Controller,
> &gEfiPciIoProtocolGuid,
> This->DriverBindingHandle,
> Private->Handle
Seems fine. We can reach this point without having opened Controller's
PciIo with BY_CHILD_CONTROLLER, but the protocol database should catch
that and we can simply ignore the EFI_NOT_FOUND here.
> @@ -533,6 +556,13 @@ QemuVideoControllerDriverStop (
> Controller
> );
>
> + gBS->CloseProtocol (
> + Controller,
> + &gEfiPciIoProtocolGuid,
> + This->DriverBindingHandle,
> + Private->Handle
> + );
> +
> //
> // Free our instance data
> //
>
I believe this hunk is fine.
Can you please post a v4 with (1) fixed?
Thanks!
Laszlo
------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
