From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::244; helo=mail-io0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x244.google.com (mail-io0-x244.google.com [IPv6:2607:f8b0:4001:c06::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 824AA2034A870 for ; Fri, 27 Oct 2017 05:43:35 -0700 (PDT) Received: by mail-io0-x244.google.com with SMTP id f20so12423915ioj.9 for ; Fri, 27 Oct 2017 05:47:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=3elkUS+ozg+6a2lfKNj5b/s9DTEV+ipugcr+JWXWQGM=; b=GQm+It1uVrRGv6dz/rvw7zMFf/HpBWFZCcIKGO9SkBPj5NmSEtLJ8y79fWebRAGwIM fLJf85UeRWPOk7lplbW9c5UqmcndN0B7nyhKkJlGssrqhjAVERekhXuwaEfwcn+q+wS7 gVZTwqEO0t2y0m7+RUjAepG8rQYuF2wzy/mEs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=3elkUS+ozg+6a2lfKNj5b/s9DTEV+ipugcr+JWXWQGM=; b=nZTNncPyhAj9zWCR+CrCGfpkBsn2yo/z9Jf2K+7PJq62FgTVsuBBgZYd4b5DWsGlQ4 diHf0JcnzS6ZCy5rTNvsteis0gadhEWu9rydvAVxCc9JZdbyg8WjS6QJOg8ABDd9HYyh YZAd6eaPt67NMjnZNYrdfAo7OaL1Ll3+mtXN8WwkR12KrOd/0clVzmfF3+Pqs9FcV1cw ynfM+mz2UzkFZJE5zC4naEzA3JCtnnZOpHVCtJfXTgXPSLIPEb3r405uXYI86pGMb4Qa USSrpi+tvE7qU+tlckmLKRHXyx/GtB1vYKlK90RE2Q4Txr0S2h8/PUu0OI3kga/SyA4i h0qQ== X-Gm-Message-State: AMCzsaXD2KXtceaoWWyQlJ4bvNk/ubDRcws3aoG3hf4Ll5+WwaS56q1T Q/egbRNXrEZgt0mlU++JRcdc//U6lSea3ZjtYipxhA== X-Google-Smtp-Source: ABhQp+SXtXr6xZRpp2RAvnCiy+uXk442TIYBVqNLFSqW3jFjuVbluDAyTpEovkpo+w8jPAzqljgn8d/0puPQAQrLQjI= X-Received: by 10.107.133.91 with SMTP id h88mr452266iod.52.1509108441987; Fri, 27 Oct 2017 05:47:21 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.131.167 with HTTP; Fri, 27 Oct 2017 05:47:21 -0700 (PDT) In-Reply-To: <20171027053326.48815-5-daniil.egranov@arm.com> References: <20171027053326.48815-1-daniil.egranov@arm.com> <20171027053326.48815-5-daniil.egranov@arm.com> From: Ard Biesheuvel Date: Fri, 27 Oct 2017 13:47:21 +0100 Message-ID: To: Daniil Egranov Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH 4/4] Drivers/SataSiI3132Dxe: Fixed startup and shutdown procedures X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Oct 2017 12:43:35 -0000 Content-Type: text/plain; charset="UTF-8" On 27 October 2017 at 06:33, Daniil Egranov wrote: > Corrected memory allocation during startup. > Added driver stop procedure and exit boot event handler. > Added driver memory and protocols cleanup procedures. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Daniil Egranov Can you split up this patch please? > --- > EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c | 281 ++++++++++++++++++----- > EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 17 ++ > 2 files changed, 236 insertions(+), 62 deletions(-) > > diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c > index 50253b9160..063086c956 100644 > --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c > +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c > @@ -89,20 +89,26 @@ SataSiI3132Constructor ( > { > SATA_SI3132_INSTANCE *Instance; > EFI_ATA_PASS_THRU_MODE *AtaPassThruMode; > + EFI_STATUS Status; > > if (!SataSiI3132Instance) { > return EFI_INVALID_PARAMETER; > } > > - Instance = (SATA_SI3132_INSTANCE*)AllocateZeroPool (sizeof (SATA_SI3132_INSTANCE)); > - if (Instance == NULL) { > - return EFI_OUT_OF_RESOURCES; > + Status = gBS->AllocatePool (EfiBootServicesData, sizeof (SATA_SI3132_INSTANCE), (VOID **)&Instance); > + if (EFI_ERROR (Status)) { > + return Status; > } > + gBS->SetMem (Instance, sizeof (SATA_SI3132_INSTANCE), 0); > > Instance->Signature = SATA_SII3132_SIGNATURE; > Instance->PciIo = PciIo; > > - AtaPassThruMode = (EFI_ATA_PASS_THRU_MODE*)AllocatePool (sizeof (EFI_ATA_PASS_THRU_MODE)); > + Status = gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_ATA_PASS_THRU_MODE), (VOID **)&AtaPassThruMode); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > AtaPassThruMode->Attributes = EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL | EFI_ATA_PASS_THRU_ATTRIBUTES_LOGICAL; > AtaPassThruMode->IoAlign = 0x1000; > > @@ -200,7 +206,10 @@ SataSiI3132PortInitialization ( > } > > // Create Device > - Device = (SATA_SI3132_DEVICE*)AllocatePool (sizeof (SATA_SI3132_DEVICE)); > + Status = gBS->AllocatePool (EfiBootServicesData, sizeof (SATA_SI3132_DEVICE), (VOID **)&Device); > + if (EFI_ERROR (Status)) { > + return Status; > + } > Device->Index = Port->Index; //TODO: Could need to be fixed when SATA Port Multiplier support > Device->Port = Port; > Device->BlockSize = 0; > @@ -310,6 +319,118 @@ ON_EXIT: > } > > /** > + Free memory allocated by the driver. > + > + @param SataSiI3132Instance pointer to the driver's data structure. > + > +**/ > +STATIC > +VOID > +SataSiI3132DriverFreeMemory ( > + IN SATA_SI3132_INSTANCE* SataSiI3132Instance > + ) > +{ > + UINTN PortIndex; > + SATA_SI3132_PORT *Port; > + SATA_SI3132_DEVICE *Device; > + LIST_ENTRY *Node; > + EFI_STATUS Status; > + UINTN NumberOfBytes; > + > + if (SataSiI3132Instance == NULL) { > + return; > + } > + > + for (PortIndex = 0; PortIndex < SATA_SII3132_MAXPORT; PortIndex++) { > + Port = &(SataSiI3132Instance->Ports[PortIndex]); > + if (Port != NULL) { > + > + //Free Device memory on each port > + Node = Port->Devices.ForwardLink; > + while (Node != &Port->Devices) { > + Device = (SATA_SI3132_DEVICE*)Node; > + Node = Node->ForwardLink; > + RemoveEntryList (&Device->Link); > + gBS->FreePool (Device); > + } > + > + //Unmap and deallocate PCI IO memory for each port > + if (Port->PciAllocMappingPRB != NULL) { > + Status = SataSiI3132Instance->PciIo->Unmap (SataSiI3132Instance->PciIo, > + Port->PciAllocMappingPRB); > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverFreeMemory: unable to unmap memory"); > + } > + } > + > + if (Port->HostPRB != 0) { > + NumberOfBytes = sizeof (SATA_SI3132_PRB); > + Status = SataSiI3132Instance->PciIo->FreeBuffer (SataSiI3132Instance->PciIo, > + EFI_SIZE_TO_PAGES (NumberOfBytes), > + Port->HostPRB); > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverFreeMemory: unable to free memory"); > + } > + } > + } > + } > + > + if (SataSiI3132Instance->AtaPassThruProtocol.Mode != NULL) { > + gBS->FreePool (SataSiI3132Instance->AtaPassThruProtocol.Mode); > + } > +} > + > +/** > + Uninstall and close protocols used by the driver. > + > + @param SataSiI3132Instance pointer to the driver's data structure. > + > +**/ > +STATIC > +VOID > +SataSiI3132DriverCloseProtocols ( > + IN SATA_SI3132_INSTANCE* SataSiI3132Instance > + ) > +{ > + EFI_STATUS Status; > + > + if (SataSiI3132Instance == NULL) { > + return; > + } > + > + // Uninstall ATA Pass-Through Protocol > + Status = gBS->UninstallProtocolInterface (SataSiI3132Instance->Controller, > + &gEfiAtaPassThruProtocolGuid, > + &SataSiI3132Instance->AtaPassThruProtocol); > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverFreeMemory: unable to uninstall AtaPassThruProtocol"); > + } > + > + if (SataSiI3132Instance->PciIo != NULL) { > + if (SataSiI3132Instance->OriginalPciAttributesValid) { > + // Restore original PCI attributes > + Status = SataSiI3132Instance->PciIo->Attributes (SataSiI3132Instance->PciIo, > + EfiPciIoAttributeOperationSet, > + SataSiI3132Instance->OriginalPciAttributes, > + NULL); > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverFreeMemory: unable to restore PCI attributes"); > + } > + } > + > + // Close PCI IO Protocol > + Status = gBS->CloseProtocol (SataSiI3132Instance->Controller, > + &gEfiPciIoProtocolGuid, > + SataSiI3132Instance->SataDriver->DriverBindingHandle, > + SataSiI3132Instance->Controller); > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverFreeMemory: unable to close PCI IO protocol"); > + } > + } > + > +} > + > +/** > Starting the Pci SATA Driver. > > @param This Protocol instance pointer. > @@ -333,8 +454,6 @@ SataSiI3132DriverBindingStart ( > EFI_STATUS Status; > EFI_PCI_IO_PROTOCOL *PciIo; > UINT64 Supports; > - UINT64 OriginalPciAttributes; > - BOOLEAN PciAttributesSaved; > UINT32 PciID; > SATA_SI3132_INSTANCE *SataSiI3132Instance = NULL; > > @@ -369,7 +488,22 @@ SataSiI3132DriverBindingStart ( > return Status; > } > > - PciAttributesSaved = FALSE; > + // Create SiI3132 Sata Instance > + Status = SataSiI3132Constructor (PciIo, &SataSiI3132Instance); > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverBindingStart: failed to allocate driver structure"); > + gBS->CloseProtocol ( > + Controller, > + &gEfiPciIoProtocolGuid, > + This->DriverBindingHandle, > + Controller > + ); > + return Status; > + } > + > + SataSiI3132Instance->Controller = Controller; > + SataSiI3132Instance->SataDriver = This; > + > // > // Save original PCI attributes > // > @@ -377,12 +511,13 @@ SataSiI3132DriverBindingStart ( > PciIo, > EfiPciIoAttributeOperationGet, > 0, > - &OriginalPciAttributes > + &SataSiI3132Instance->OriginalPciAttributes > ); > if (EFI_ERROR (Status)) { > - goto CLOSE_PCIIO; > + goto QUIT_ERROR; > } > - PciAttributesSaved = TRUE; > + > + SataSiI3132Instance->OriginalPciAttributesValid = TRUE; > > Status = PciIo->Attributes ( > PciIo, > @@ -401,7 +536,7 @@ SataSiI3132DriverBindingStart ( > } > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_ERROR, "SataSiI3132DriverBindingStart: failed to enable controller\n")); > - goto CLOSE_PCIIO; > + goto QUIT_ERROR; > } > > // > @@ -416,7 +551,7 @@ SataSiI3132DriverBindingStart ( > ); > if (EFI_ERROR (Status)) { > Status = EFI_UNSUPPORTED; > - goto CLOSE_PCIIO; > + goto QUIT_ERROR; > } > > // > @@ -424,21 +559,13 @@ SataSiI3132DriverBindingStart ( > // > if (PciID != ((SATA_SII3132_DEVICE_ID << 16) | SATA_SII3132_VENDOR_ID)) { > Status = EFI_UNSUPPORTED; > - goto CLOSE_PCIIO; > + goto QUIT_ERROR; > } > > - // Create SiI3132 Sata Instance > - Status = SataSiI3132Constructor (PciIo, &SataSiI3132Instance); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > - SataSiI3132Instance->OriginalPciAttributes = OriginalPciAttributes; > - > // Initialize SiI3132 Sata Controller > Status = SataSiI3132Initialization (SataSiI3132Instance); > if (EFI_ERROR (Status)) { > - return Status; > + goto QUIT_ERROR; > } > > // Install Ata Pass Thru Protocol > @@ -449,49 +576,21 @@ SataSiI3132DriverBindingStart ( > &(SataSiI3132Instance->AtaPassThruProtocol) > ); > if (EFI_ERROR (Status)) { > - goto FREE_POOL; > + goto QUIT_ERROR; > } > > -/* // > - // Create event to stop the HC when exit boot service. > - // > - Status = gBS->CreateEventEx ( > - EVT_NOTIFY_SIGNAL, > - TPL_NOTIFY, > - EhcExitBootService, > - Ehc, > - &gEfiEventExitBootServicesGuid, > - &Ehc->ExitBootServiceEvent > - ); > - if (EFI_ERROR (Status)) { > - goto UNINSTALL_USBHC; > - }*/ > + Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK, > + &SataSiI3132DriverExitBoot, SataSiI3132Instance, &SataSiI3132Instance->ExitBootEvent); > Please check your coding style and line length. > - SATA_TRACE ("SataSiI3132DriverBindingStart() Success!"); > - return EFI_SUCCESS; > - > -FREE_POOL: > - //TODO: Free SATA Instance > - > -CLOSE_PCIIO: > - if (PciAttributesSaved) { > - // > - // Restore original PCI attributes > - // > - PciIo->Attributes ( > - PciIo, > - EfiPciIoAttributeOperationSet, > - OriginalPciAttributes, > - NULL > - ); > + if (!EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverBindingStart() Success!"); > + return EFI_SUCCESS; > } > > - gBS->CloseProtocol ( > - Controller, > - &gEfiPciIoProtocolGuid, > - This->DriverBindingHandle, > - Controller > - ); > +QUIT_ERROR: > + SataSiI3132DriverCloseProtocols (SataSiI3132Instance); > + SataSiI3132DriverFreeMemory (SataSiI3132Instance); > + gBS->FreePool (SataSiI3132Instance); > > return Status; > } > @@ -518,8 +617,66 @@ SataSiI3132DriverBindingStop ( > IN EFI_HANDLE *ChildHandleBuffer > ) > { > + SATA_SI3132_INSTANCE *SataSiI3132Instance; > + EFI_ATA_PASS_THRU_PROTOCOL *AtaPassThruProtocol; > + EFI_STATUS Status; > + > SATA_TRACE ("SataSiI3132DriverBindingStop()"); > - return EFI_UNSUPPORTED; > + > + Status = gBS->OpenProtocol ( > + Controller, > + &gEfiAtaPassThruProtocolGuid, > + (VOID **)&AtaPassThruProtocol, > + This->DriverBindingHandle, > + Controller, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL > + ); > + > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverBindingStop: driver is not started"); > + return Status; > + } > + > + SataSiI3132Instance = INSTANCE_FROM_ATAPASSTHRU_THIS (AtaPassThruProtocol); > + > + gBS->CloseEvent (SataSiI3132Instance->ExitBootEvent); > + > + SataSiI3132DriverCloseProtocols (SataSiI3132Instance); > + SataSiI3132DriverFreeMemory (SataSiI3132Instance); > + gBS->FreePool (SataSiI3132Instance); > + > + return EFI_SUCCESS; > +} > + > +/** > + Process exit boot event. > + > + @param [in] Event Event id. > + @param [in] Context Driver context. > + > +**/ > +VOID > +EFIAPI > +SataSiI3132DriverExitBoot ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + SATA_SI3132_INSTANCE *SataSiI3132Instance; > + EFI_STATUS Status; > + > + if (Context == NULL) { > + SATA_TRACE ("SataSiI3132DriverExitBoot: invalid Context parameter"); > + } else { > + SataSiI3132Instance = (SATA_SI3132_INSTANCE*)Context; > + Status = SataSiI3132Instance->SataDriver->Stop (SataSiI3132Instance->SataDriver, > + SataSiI3132Instance->Controller, > + 0, > + NULL); You cannot stop the driver at ExitBootServices, since you are not allowed to do anything that affects the memory map. All you should do here is clear the EFI_PCI_COMMAND_BUS_MASTER PciIo attribute, so the device can no longer access memory. > + if (EFI_ERROR (Status)) { > + SATA_TRACE ("SataSiI3132DriverExitBoot: driver stop failed"); > + } > + } > } > > /** > diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h > index a7bc956b19..ab4510b97b 100644 > --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h > +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h > @@ -144,6 +144,16 @@ typedef struct _SATA_SI3132_INSTANCE { > EFI_ATA_PASS_THRU_PROTOCOL AtaPassThruProtocol; > > EFI_PCI_IO_PROTOCOL *PciIo; > + > + EFI_DRIVER_BINDING_PROTOCOL *SataDriver; > + > + EFI_HANDLE Controller; > + > + UINT64 OriginalPciAttributes; > + > + BOOLEAN OriginalPciAttributesValid; > + > + EFI_EVENT ExitBootEvent; > } SATA_SI3132_INSTANCE; > > #define SATA_SII3132_SIGNATURE SIGNATURE_32('s', 'i', '3', '2') > @@ -211,6 +221,13 @@ SataSiI3132DriverBindingStop ( > IN EFI_HANDLE *ChildHandleBuffer > ); > > +VOID > +EFIAPI > +SataSiI3132DriverExitBoot ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ); > + > EFI_STATUS SiI3132AtaPassThruCommand ( > IN SATA_SI3132_INSTANCE *pSataSiI3132Instance, > IN SATA_SI3132_PORT *pSataPort, > -- > 2.11.0 >