Re: [PATCH 2/7] EmbeddedPkg: SiI3132: Add ScsiProtocol callbacks

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Create a new module that adds the callbacks to support
> the EFI SCSI pass-through protocol. These callbacks
> wrap around the existing ATA pass-through callbacks.
> In particular the SCSI command submission routine takes
> the SCSI command and wraps it with an SATA FIS and
> sets the protocol to ATAPI. It then forwards the FIS to
> a new routine we will break out of the ATA pass-through
> callback that manages the FIS submission to the adapter.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  .../Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c   | 401 +++++++++++++++++++++
>  1 file changed, 401 insertions(+)
>  create mode 100644 EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
>
> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
> new file mode 100644
> index 0000000..131b0af
> --- /dev/null
> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132ScsiPassThru.c
> @@ -0,0 +1,401 @@
> +/** @file
> +*  ATAPI support for the Silicon Image I3132
> +*
> +*  Copyright (c) 2016, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD License
> +*  which accompanies this distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +
> +#include "SataSiI3132.h"
> +
> +#include <IndustryStandard/Atapi.h>
> +#include <IndustryStandard/Scsi.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +EFI_STATUS
> +SiI3132IDiscoverAtapi (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN int Port

INT32 not int, please

> +  )
> +{
> +  SATA_SI3132_INSTANCE  *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +  SATA_SI3132_PORT      *SataPort = &SataInstance->Ports[Port];

Please replace with assignments.

> +  EFI_ATA_PASS_THRU_COMMAND_PACKET Packet;
> +  ATA_IDENTIFY_DATA     *Data;
> +  EFI_STATUS            Status;
> +  EFI_ATA_STATUS_BLOCK  *Asb;
> +  EFI_ATA_COMMAND_BLOCK *Acb;
> +
> +  Asb=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)), EFI_PAGE_SIZE);
> +  Acb=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_COMMAND_BLOCK)), EFI_PAGE_SIZE);
> +  Data=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(ATA_IDENTIFY_DATA)), EFI_PAGE_SIZE);

Spaces around '='

> +  ZeroMem (Acb, sizeof (EFI_ATA_COMMAND_BLOCK));
> +  Acb->AtaCommand = ATA_CMD_IDENTIFY_DEVICE;
> +  Acb->AtaDeviceHead = (UINT8) (BIT7 | BIT6 | BIT5 );
> +
> +  ZeroMem (&Packet, sizeof (EFI_ATA_PASS_THRU_COMMAND_PACKET));
> +  Packet.Acb=Acb;
> +  Packet.Asb=Asb;

Spaces

> +  Packet.InDataBuffer = Data;
> +  Packet.InTransferLength = sizeof (ATA_IDENTIFY_DATA);
> +  Packet.Protocol = EFI_ATA_PASS_THRU_PROTOCOL_PIO_DATA_IN;
> +  Packet.Length   = EFI_ATA_PASS_THRU_LENGTH_BYTES | EFI_ATA_PASS_THRU_LENGTH_SECTOR_COUNT;
> +
> +  Status = SiI3132AtaPassThruCommand(SataInstance, SataPort, 0, &Packet, NULL);

Space before (

> +  if (EFI_ERROR (Status)) {
> +    DEBUG((DEBUG_WARN, "SiI ATAPI IDENTIFY DEVICE FAILURE %d\n", Status));
> +  }
> +  else
> +  {
> +    Data->ModelName[39]=0;
> +  }
> +
> +  FreeAlignedPages(Data, EFI_SIZE_TO_PAGES(sizeof(ATA_IDENTIFY_DATA)));
> +  FreeAlignedPages(Acb, EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_COMMAND_BLOCK)));
> +  FreeAlignedPages(Asb, EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)));

Space before (

> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132ScsiPassRead (
> +  IN SATA_SI3132_INSTANCE *SataInstance,
> +  IN SATA_SI3132_PORT *SataPort,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet)
> +{
> +  EFI_STATUS              Status = EFI_SUCCESS;
> +  EFI_PCI_IO_PROTOCOL     *PciIo = SataInstance->PciIo;
> +  VOID*                   PciAllocMapping = NULL;

Assignments not initializers

> +  EFI_PHYSICAL_ADDRESS    PhysBuffer;
> +  UINTN InDataBufferLength = Packet->InTransferLength;
> +  VOID *ata_sense=AllocateAlignedPages(EFI_SIZE_TO_PAGES(sizeof(EFI_ATA_STATUS_BLOCK)),
> +                                       EFI_PAGE_SIZE);;
> +
> +  BOOLEAN request_sense=FALSE;
> +

EDK2 uses CamelCase, not whatever_the_other_thing_is_called

> +  DEBUG ((DEBUG_VERBOSE, "SiI3132ScsiPassRead() CDB[0]:%X len=%d\n",
> +          ((UINT8*)Packet->Cdb)[0], Packet->InTransferLength));
> +
> +  if (ata_sense) {
> +    if (Packet->InTransferLength) {
> +      Status = PciIo->Map (SataInstance->PciIo, EfiPciIoOperationBusMasterRead,
> +                           Packet->InDataBuffer, &InDataBufferLength,
> +                           &PhysBuffer, &PciAllocMapping);
> +
> +      if (EFI_ERROR (Status)) {
> +        DEBUG((DEBUG_ERROR, "SiI map() failure %d\n", Status));

Space before ((

> +        return Status;
> +      }
> +    }
> +    else {
> +      PhysBuffer=0;

Spaces around =

> +    }
> +    do {
> +      // SI "The host driver must populate the area normaly used for the first SGE
> +      // with the desired ATAPI command". AKA, put the SCSI CDB itself (not the address)
> +      // in the 12 bytes comprising the SGE[0].
> +      ZeroMem (&SataPort->HostPRB->Sge[0], sizeof(SATA_SI3132_SGE));
> +      CopyMem(&SataPort->HostPRB->Sge[0], (UINT8*)Packet->Cdb, Packet->CdbLength);
> +
> +      // The SGE for the data buffer
> +      SataPort->HostPRB->Sge[1].DataAddressLow = (UINT64)PhysBuffer;
> +      SataPort->HostPRB->Sge[1].DataAddressHigh = ((UINT64)(PhysBuffer) >> 32);
> +      SataPort->HostPRB->Sge[1].Attributes = SGE_TRM;
> +      SataPort->HostPRB->Sge[1].DataCount = Packet->InTransferLength;
> +
> +      // Create the ATA FIS
> +      SataPort->HostPRB->Control = (Packet->InTransferLength ? PRB_CTRL_PKT_READ:0);
> +      SataPort->HostPRB->ProtocolOverride = 0;
> +
> +      // This is an ATA PACKET command, which encapuslates the ATAPI(sorta SCSI) command
> +      SataPort->HostPRB->Fis.FisType  = SII_FIS_REGISTER_H2D; // Register - Host to Device FIS
> +      SataPort->HostPRB->Fis.Control  = SII_FIS_CONTROL_CMD;  // Is a command
> +      SataPort->HostPRB->Fis.Command  = ATA_CMD_PACKET;
> +      SataPort->HostPRB->Fis.Features = 1;
> +      SataPort->HostPRB->Fis.Fis[0]   = 0;
> +      SataPort->HostPRB->Fis.Fis[1]   = (UINT8) (((UINT64)PhysBuffer) & 0x00ff);
> +      SataPort->HostPRB->Fis.Fis[2]   = (UINT8) (((UINT64)PhysBuffer) >> 8);
> +      SataPort->HostPRB->Fis.Fis[3]   = 0x40;
> +
> +      // Issue this as an ATA command
> +      Status = SiI3132IssueCommand(SataPort, PciIo, Packet->Timeout, ata_sense);
> +
> +      if (request_sense) {
> +        // If we were trying to send a request sense in response to a failure
> +        // Check conditions are a normal part of SCSI operation, its expected
> +        // that most devices will do a 6/2900 (reset) and 2/2800 (media change)
> +        // at startup.
> +        UINT8 *sense = (UINT8 *)Packet->SenseData;
> +        request_sense=FALSE;
> +        Packet->InTransferLength = 0;
> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +        Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION;
> +        if ((Packet->SenseDataLength > 13) &&
> +            (sense[0] & EFI_SCSI_REQUEST_SENSE_ERROR)) {
> +            DEBUG ((DEBUG_INFO, "SiI3132ScsiPassRead() Key %X ASC(Q) %02X%02X\n",
> +                    EFI_SCSI_SK_VALUE(sense[2]), sense[12], sense[13]));
> +        }
> +        Status = EFI_SUCCESS;
> +      } else if (Status == EFI_SUCCESS) {

Please use !EFI_ERROR (Status). I know it means the same thing *now*
but that may change in the future.

> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +        Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +        ZeroMem (Packet->SenseData, Packet->SenseDataLength);
> +        Packet->SenseDataLength = 0;
> +      } else if (Status == EFI_TIMEOUT) {
> +        SiI3132HwResetPort (SataPort);
> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +        Packet->SenseDataLength = 0;
> +        Packet->InTransferLength = 0;
> +      }
> +      else {

Same line please

> +        // Assume for now, that if we didn't succeed that it was a check condition.
> +        // ATAPI can't autosense on SiI, so lets emulate it with an explicit
> +        // request sense into the sense buffer.
> +        Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +        // Packet->SenseData
> +        if ((request_sense == FALSE) && (Packet->SenseDataLength)) {
> +          if (Packet->SenseDataLength >= SI_MAX_SENSE)
> +            Packet->SenseDataLength = SI_MAX_SENSE - 1;
> +          Packet->CdbLength = 6; //Request Sense is a 6 byte CDB
> +          ZeroMem (Packet->Cdb, SI_MAX_CDB);
> +          ((char*)Packet->Cdb)[0] = EFI_SCSI_OP_REQUEST_SENSE;
> +          ((char*)Packet->Cdb)[4] = Packet->SenseDataLength;

Yuck. Please use a separate UINT8* variable

> +
> +          ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB));
> +          if (PciAllocMapping) {
> +            Status = PciIo->Unmap (PciIo, PciAllocMapping);

Why is it OK to ignore Status. And if it is, why do you need to assign
it in the first place?

> +          }
> +
> +          Packet->InTransferLength = Packet->SenseDataLength;
> +          InDataBufferLength = Packet->SenseDataLength;
> +          Status = PciIo->Map (SataInstance->PciIo, EfiPciIoOperationBusMasterRead,
> +                               Packet->SenseData, &InDataBufferLength, &PhysBuffer, &PciAllocMapping);
> +          if (EFI_ERROR (Status)) {
> +            DEBUG((DEBUG_ERROR, "SiI map() sense failure %d\n", Status));
> +            Packet->SenseDataLength = 0;
> +          } else {
> +            // Everything seems ok, lets issue a SCSI sense
> +            request_sense = TRUE;
> +          }
> +        }
> +      }
> +    } while (request_sense);
> +
> +    if (PciAllocMapping) {
> +      PciIo->Unmap (PciIo, PciAllocMapping);
> +    }
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "SCSI PassThru Unable to allocate sense buffer\n"));
> +    Status = EFI_OUT_OF_RESOURCES;
> +  }
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132ScsiPassThru (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN EFI_EVENT  Event OPTIONAL
> +  )
> +{
> +  EFI_STATUS              Status = EFI_SUCCESS;
> +  SATA_SI3132_INSTANCE    *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +  SATA_SI3132_PORT        *SataPort = &SataInstance->Ports[Target[0]];
> +
> +  ZeroMem (SataPort->HostPRB, sizeof (SATA_SI3132_PRB));
> +
> +  switch (Packet->DataDirection) {
> +    case EFI_EXT_SCSI_DATA_DIRECTION_READ:  {
> +        Status = SiI3132ScsiPassRead(SataInstance,SataPort, Packet);
> +    }
> +    break;
> +    case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
> +      // TODO, fill this in if we ever want to connect something
> +      // besides a simple CDROM/DVDROM
> +    default:
> +      DEBUG ((DEBUG_ERROR, "SCSI PassThru Unsupported direction\n"));
> +  }
> +
> +  return Status;
> +}
> +
> +UINT8 mScsiId[TARGET_MAX_BYTES] = {
> +  0xFF, 0xFF, 0xFF, 0xFF,
> +  0xFF, 0xFF, 0xFF, 0xFF,
> +  0xFF, 0xFF, 0xFF, 0xFF,
> +  0xFF, 0xFF, 0xFF, 0xFF
> +};
> +
> +// With ATA, there are potentially three levels of addressing: port, portmultiplier, lun
> +// although I'm not sure there are any actual multilun devices anymore (cd autoloaders/etc)
> +// on ATA the common disk/cdroms generally are single lun devices. So, lets skip the LUN
> +// scanning/addressing. Futher, the standard ATAPI spec doesn't support lun's.
> +EFI_STATUS
> +SiI3132GetNextTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN OUT UINT8 **Target,
> +  IN OUT UINT64 *Lun
> +  )
> +{
> +  EFI_STATUS Status = EFI_NOT_FOUND;
> +  SATA_SI3132_INSTANCE *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +  UINT8                 *Target8;
> +
> +  DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun L:%d\n",*Lun));
> +  if (!SataInstance) {
> +    DEBUG ((DEBUG_ERROR, "SCSI GetNextTargetLun no instance\n",Lun));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Target == NULL || Lun == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if (*Target == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (*Lun > 0) {
> +    DEBUG ((DEBUG_ERROR, "SCSI GetNextTargetLun Only supports lun0 at the moment\n",Lun));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Target8 = *Target;
> +
> +  // look for first device
> +  if (CompareMem(Target8, mScsiId, TARGET_MAX_BYTES) == 0) {
> +    BOOLEAN found = FALSE;
> +    int Index;
> +    Target8  = *Target;
> +    for (Index = 0; ((Index < SATA_SII3132_MAXPORT) && (!found)); Index++) {
> +      SATA_SI3132_PORT        *SataPort;
> +      SataPort = &(SataInstance->Ports[Index]);
> +      LIST_ENTRY              *List = SataPort->Devices.ForwardLink;
> +      int Multiplier = 0;
> +      while ((List != &SataPort->Devices) && (!found)) {
> +        SATA_SI3132_DEVICE* SataDevice = (SATA_SI3132_DEVICE*)List;
> +        if (SataDevice->Atapi) {
> +          found = TRUE;
> +          Target8[0] = Index;
> +          Target8[1] = Multiplier; //the device on this port (for port multipliers)
> +          DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun found device at %d %d\n",Index,Multiplier));
> +          SiI3132IDiscoverAtapi(This, Index);
> +
> +          Status = EFI_SUCCESS;
> +          break;
> +        }
> +        List = List->ForwardLink;
> +        Multiplier++;
> +      }
> +    }
> +  }
> +  else {
> +      //todo find the next device,
> +  }
> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132GetNextTargetLun2 (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> +  IN UINT8 *Target,
> +  IN UINT64 Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
> +  )
> +{
> +  EFI_STATUS Status = EFI_SUCCESS;
> +  DEBUG ((DEBUG_INFO, "SCSI GetNextTargetLun2\n"));
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132ScsiBuildDevicePath (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun,
> +  IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
> +  )
> +{
> +  SATA_SI3132_INSTANCE *SataInstance=INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +
> +  DEBUG ((DEBUG_INFO, "SCSI BuildDevicePath T:%d L:%d\n",*Target,Lun));
> +
> +  if (Lun<1)
> +    return SiI3132BuildDevicePath (&SataInstance->AtaPassThruProtocol,Target[0],Target[1],DevicePath);
> +  return EFI_NOT_FOUND;
> +}
> +
> +EFI_STATUS
> +SiI3132GetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN EFI_DEVICE_PATH_PROTOCOL                      *DevicePath,
> +  OUT UINT8                                        **Target,
> +  OUT UINT64                                       *Lun
> +  )
> +{
> +  EFI_STATUS Status = EFI_SUCCESS;
> +
> +  DEBUG ((DEBUG_INFO, "SCSI GetNextTarget T:%d L:%d\n",*Target,Lun));
> +  return Status;
> +}
> +
> +// So normally we would want to do a ATA port reset here (which is generally
> +// frowned on with modern SCSI transports (sas, fc, etc) unless all the
> +// attached devices are in an error state). But the EFI SCSI protocol isn't
> +// specific enough to specify a device for which we want to reset the port.
> +// This means we are basically stuck simulating it by resetting all the ports
> +// which is bad karma. So lets just claim its unsupported and if we discover
> +// that port resets are needed as part of the target/lun reset then consider
> +// doing it automatically as part of that path.
> +EFI_STATUS
> +SiI3132ResetChannel (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
> +  )
> +{
> +  EFI_STATUS Status = EFI_UNSUPPORTED;
> +
> +  DEBUG ((DEBUG_ERROR, "SCSI ResetChannel\n"));
> +  return Status;
> +}
> +
> +// Just do a device reset here, in the future if we find out that is insufficient
> +// try to just reset the SATA port the device is attached to as well.
> +EFI_STATUS
> +SiI3132ResetTargetLun (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN UINT8                                         *Target,
> +  IN UINT64                                        Lun
> +  )
> +{
> +  EFI_STATUS Status = EFI_NOT_FOUND;
> +  SATA_SI3132_INSTANCE *SataInstance = INSTANCE_FROM_SCSIPASSTHRU_THIS(This);
> +
> +  DEBUG ((DEBUG_ERROR, "SCSI ResetTargetLun\n"));
> +
> +  if (Lun<1)
> +    Status = SiI3132ResetDevice (&SataInstance->AtaPassThruProtocol,
> +                                 Target[0], Target[1]);
> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +SiI3132GetNextTarget (
> +  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
> +  IN OUT UINT8                                     **Target
> +  )
> +{
> +  EFI_STATUS Status = EFI_SUCCESS;
> +  DEBUG ((DEBUG_VERBOSE, "SCSI GetNextTarget\n"));
> +  return Status;
> +}
> --
> 2.5.5
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel