[PATCH 0/4] SataSiI3132Dxe fixes

[PATCH 0/4] SataSiI3132Dxe fixes

[Daniil Egranov]

This set of patches fixes an issue with 64-bit DMA and implements
the missing exit boot event and driver stop functionality including
memory/protocols cleanup procedure.

Daniil Egranov (4):
  Drivers/SataSiI3132Dxe: Fixed PCI IO read and write operations
  Drivers/SataSiI3132Dxe: Allow 64-bit DMA transfer
  Drivers/SataSiI3132Dxe: Enable multi-controller support
  Drivers/SataSiI3132Dxe: Fixed startup and shutdown procedures

 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   | 301 ++++++++++++++++-----
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  17 ++
 .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    |   4 +-
 3 files changed, 252 insertions(+), 70 deletions(-)

-- 
2.11.0

[PATCH 1/4] Drivers/SataSiI3132Dxe: Fixed PCI IO read and write operations

[Daniil Egranov]

The ATA pass through read should use PCI IO bus master write operation
and ATA pass through write should use PCI IO bus master read operation
as the read and write operations executed from the bus master's point
of view.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
index 2fb5fd68db..a938563ebd 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
@@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand (
     }
 
     Status = PciIo->Map (
-               PciIo, EfiPciIoOperationBusMasterRead,
+               PciIo, EfiPciIoOperationBusMasterWrite,
                Packet->InDataBuffer, &InDataBufferLength, &PhysInDataBuffer, &PciAllocMapping
                );
     if (EFI_ERROR (Status)) {
@@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand (
     OutDataBufferLength = Packet->OutTransferLength * SataDevice->BlockSize;
 
     Status = PciIo->Map (
-               PciIo, EfiPciIoOperationBusMasterWrite,
+               PciIo, EfiPciIoOperationBusMasterRead,
                Packet->OutDataBuffer, &OutDataBufferLength, &PhysOutDataBuffer, &PciAllocMapping
                );
     if (EFI_ERROR (Status)) {
-- 
2.11.0

[PATCH 2/4] Drivers/SataSiI3132Dxe: Allow 64-bit DMA transfer

[Daniil Egranov]

Set a PCI IO attribute allowing 64-bit DMA transfer.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
index f4946552a0..c1760fdc1b 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
@@ -384,7 +384,7 @@ SataSiI3132DriverBindingStart (
                   &Supports
                   );
   if (!EFI_ERROR (Status)) {
-      Supports &= EFI_PCI_DEVICE_ENABLE;
+      Supports &= EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
       Status = PciIo->Attributes (
                         PciIo,
                         EfiPciIoAttributeOperationEnable,
-- 
2.11.0

[PATCH 3/4] Drivers/SataSiI3132Dxe: Enable multi-controller support

[Daniil Egranov]

Saved controller specific data into the driver's information structure.
Removed global variable indicating the driver's status and added
check for the driver's status based on the available protocol.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
index c1760fdc1b..50253b9160 100644
--- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
+++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
@@ -309,8 +309,6 @@ ON_EXIT:
   return Status;
 }
 
-BOOLEAN mbStarted = FALSE;
-
 /**
   Starting the Pci SATA Driver.
 
@@ -342,9 +340,18 @@ SataSiI3132DriverBindingStart (
 
   SATA_TRACE ("SataSiI3132DriverBindingStart()");
 
-  //TODO: Find a nicer way to do it !
-  if (mbStarted) {
-    return EFI_SUCCESS; // Don't restart me !
+  Status = gBS->OpenProtocol (
+        Controller,
+        &gEfiAtaPassThruProtocolGuid,
+        NULL,
+        This->DriverBindingHandle,
+        Controller,
+        EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+        );
+
+  if (!EFI_ERROR (Status)) {
+    SATA_TRACE ("SataSiI3132DriverBindingStart: driver already started");
+    return Status;
   }
 
   //
@@ -426,6 +433,8 @@ SataSiI3132DriverBindingStart (
     return Status;
   }
 
+  SataSiI3132Instance->OriginalPciAttributes = OriginalPciAttributes;
+
   // Initialize SiI3132 Sata Controller
   Status = SataSiI3132Initialization (SataSiI3132Instance);
   if (EFI_ERROR (Status)) {
@@ -458,8 +467,6 @@ SataSiI3132DriverBindingStart (
       goto UNINSTALL_USBHC;
   }*/
 
-  mbStarted = TRUE;
-
   SATA_TRACE ("SataSiI3132DriverBindingStart() Success!");
   return EFI_SUCCESS;
 
-- 
2.11.0

[PATCH 4/4] Drivers/SataSiI3132Dxe: Fixed startup and shutdown procedures

[Daniil Egranov]

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 <daniil.egranov@arm.com>
---
 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);
 
-  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);
+    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

Re: [PATCH 1/4] Drivers/SataSiI3132Dxe: Fixed PCI IO read and write operations

[Ard Biesheuvel]

On 27 October 2017 at 06:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
> The ATA pass through read should use PCI IO bus master write operation
> and ATA pass through write should use PCI IO bus master read operation
> as the read and write operations executed from the bus master's point
> of view.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
> index 2fb5fd68db..a938563ebd 100644
> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c
> @@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand (
>      }
>
>      Status = PciIo->Map (
> -               PciIo, EfiPciIoOperationBusMasterRead,
> +               PciIo, EfiPciIoOperationBusMasterWrite,
>                 Packet->InDataBuffer, &InDataBufferLength, &PhysInDataBuffer, &PciAllocMapping
>                 );
>      if (EFI_ERROR (Status)) {
> @@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand (
>      OutDataBufferLength = Packet->OutTransferLength * SataDevice->BlockSize;
>
>      Status = PciIo->Map (
> -               PciIo, EfiPciIoOperationBusMasterWrite,
> +               PciIo, EfiPciIoOperationBusMasterRead,
>                 Packet->OutDataBuffer, &OutDataBufferLength, &PhysOutDataBuffer, &PciAllocMapping
>                 );
>      if (EFI_ERROR (Status)) {
> --
> 2.11.0
>

Re: [PATCH 2/4] Drivers/SataSiI3132Dxe: Allow 64-bit DMA transfer

[Ard Biesheuvel]

On 27 October 2017 at 06:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
> Set a PCI IO attribute allowing 64-bit DMA transfer.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
> index f4946552a0..c1760fdc1b 100644
> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
> @@ -384,7 +384,7 @@ SataSiI3132DriverBindingStart (
>                    &Supports
>                    );
>    if (!EFI_ERROR (Status)) {
> -      Supports &= EFI_PCI_DEVICE_ENABLE;
> +      Supports &= EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
>        Status = PciIo->Attributes (
>                          PciIo,
>                          EfiPciIoAttributeOperationEnable,
> --
> 2.11.0
>

Re: [PATCH 3/4] Drivers/SataSiI3132Dxe: Enable multi-controller support

[Ard Biesheuvel]

On 27 October 2017 at 06:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
> Saved controller specific data into the driver's information structure.
> Removed global variable indicating the driver's status and added
> check for the driver's status based on the available protocol.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
> ---
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
> index c1760fdc1b..50253b9160 100644
> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c
> @@ -309,8 +309,6 @@ ON_EXIT:
>    return Status;
>  }
>
> -BOOLEAN mbStarted = FALSE;
> -
>  /**
>    Starting the Pci SATA Driver.
>
> @@ -342,9 +340,18 @@ SataSiI3132DriverBindingStart (
>
>    SATA_TRACE ("SataSiI3132DriverBindingStart()");
>
> -  //TODO: Find a nicer way to do it !
> -  if (mbStarted) {
> -    return EFI_SUCCESS; // Don't restart me !
> +  Status = gBS->OpenProtocol (
> +        Controller,
> +        &gEfiAtaPassThruProtocolGuid,
> +        NULL,
> +        This->DriverBindingHandle,
> +        Controller,
> +        EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> +        );
> +
> +  if (!EFI_ERROR (Status)) {
> +    SATA_TRACE ("SataSiI3132DriverBindingStart: driver already started");
> +    return Status;

Can you explain what this does? Does it check whether the protocol we
will install is already installed? Is this how other PCI drivers deal
with this?

>    }
>
>    //
> @@ -426,6 +433,8 @@ SataSiI3132DriverBindingStart (
>      return Status;
>    }
>
> +  SataSiI3132Instance->OriginalPciAttributes = OriginalPciAttributes;
> +

Does this belong in the same patch?

>    // Initialize SiI3132 Sata Controller
>    Status = SataSiI3132Initialization (SataSiI3132Instance);
>    if (EFI_ERROR (Status)) {
> @@ -458,8 +467,6 @@ SataSiI3132DriverBindingStart (
>        goto UNINSTALL_USBHC;
>    }*/
>
> -  mbStarted = TRUE;
> -
>    SATA_TRACE ("SataSiI3132DriverBindingStart() Success!");
>    return EFI_SUCCESS;
>
> --
> 2.11.0
>

Re: [PATCH 4/4] Drivers/SataSiI3132Dxe: Fixed startup and shutdown procedures

[Ard Biesheuvel]

On 27 October 2017 at 06:33, Daniil Egranov <daniil.egranov@arm.com> 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 <daniil.egranov@arm.com>

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
>

Re: [PATCH 0/4] SataSiI3132Dxe fixes

[Ard Biesheuvel]

On 27 October 2017 at 06:33, Daniil Egranov <daniil.egranov@arm.com> wrote:
> This set of patches fixes an issue with 64-bit DMA and implements
> the missing exit boot event and driver stop functionality including
> memory/protocols cleanup procedure.
>
> Daniil Egranov (4):
>   Drivers/SataSiI3132Dxe: Fixed PCI IO read and write operations
>   Drivers/SataSiI3132Dxe: Allow 64-bit DMA transfer
>   Drivers/SataSiI3132Dxe: Enable multi-controller support
>   Drivers/SataSiI3132Dxe: Fixed startup and shutdown procedures
>
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   | 301 ++++++++++++++++-----
>  EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  17 ++
>  .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    |   4 +-
>  3 files changed, 252 insertions(+), 70 deletions(-)
>

Hi Daniil,

Thanks for taking the time to fix this driver.

I will go ahead and push the first two patches, given that they are
self-contained and obvious bug fixes.

The remaining patches, please split them up, and please align more
closely with what other upstream PCI drivers do.

Regards,
Ard.

Re: [PATCH 0/4] SataSiI3132Dxe fixes

[Jeremy Linton]

Hi,

On 10/27/2017 12:33 AM, Daniil Egranov wrote:
> This set of patches fixes an issue with 64-bit DMA and implements
> the missing exit boot event and driver stop functionality including
> memory/protocols cleanup procedure.
> 
> Daniil Egranov (4):
>    Drivers/SataSiI3132Dxe: Fixed PCI IO read and write operations
>    Drivers/SataSiI3132Dxe: Allow 64-bit DMA transfer
>    Drivers/SataSiI3132Dxe: Enable multi-controller support
>    Drivers/SataSiI3132Dxe: Fixed startup and shutdown procedures
> 
>   EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   | 301 ++++++++++++++++-----
>   EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  17 ++
>   .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    |   4 +-
>   3 files changed, 252 insertions(+), 70 deletions(-)

This is generally good, but there remain quite a number of "errors" in 
the command submission path as well as the completely unnecessary 4k IO 
alignment requirement which has been known to break older grubs/etc. A 
few of those "errors" were fixed in this patch set (1) as well, so might 
be worthwhile if you are looking at this driver to integrate those fixes 
as well.

(1) https://lists.01.org/pipermail/edk2-devel/2017-March/008277.html

Thanks,

Re: [PATCH 0/4] SataSiI3132Dxe fixes

[Ard Biesheuvel]

On 27 October 2017 at 17:57, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Hi,
>
> On 10/27/2017 12:33 AM, Daniil Egranov wrote:
>>
>> This set of patches fixes an issue with 64-bit DMA and implements
>> the missing exit boot event and driver stop functionality including
>> memory/protocols cleanup procedure.
>>
>> Daniil Egranov (4):
>>    Drivers/SataSiI3132Dxe: Fixed PCI IO read and write operations
>>    Drivers/SataSiI3132Dxe: Allow 64-bit DMA transfer
>>    Drivers/SataSiI3132Dxe: Enable multi-controller support
>>    Drivers/SataSiI3132Dxe: Fixed startup and shutdown procedures
>>
>>   EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c   | 301
>> ++++++++++++++++-----
>>   EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h   |  17 ++
>>   .../Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c    |   4 +-
>>   3 files changed, 252 insertions(+), 70 deletions(-)
>
>
> This is generally good, but there remain quite a number of "errors" in the
> command submission path as well as the completely unnecessary 4k IO
> alignment requirement which has been known to break older grubs/etc. A few
> of those "errors" were fixed in this patch set (1) as well, so might be
> worthwhile if you are looking at this driver to integrate those fixes as
> well.
>

Yes, please. And apologies for forgetting about thise patches.

> (1) https://lists.01.org/pipermail/edk2-devel/2017-March/008277.html
>
> Thanks,