Re: [RFC edk2-platforms v1 2/3] Silicon/Hisilicon/Acpi: Add update sas address feature

[Ming Huang <huangming23@huawei.com>]

在 2020/5/27 2:49, Leif Lindholm 写道:
> On Thu, May 21, 2020 at 22:43:03 +0800, Ming Huang wrote:
>> The updating sas address feature is similar with apdating mac address.
>> Modify updating dsdt flow for add this feature.
>>
>> Signed-off-by: Ming Huang <huangming23@huawei.com>
>> ---
>>  Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatform.c |   2 +-
>>  Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c       | 200 +++++++++++++++-----
>>  Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.h       |   2 +-
>>  3 files changed, 158 insertions(+), 46 deletions(-)
>>
>> diff --git a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatform.c b/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatform.c
>> index 1ab55bc..d3ea051 100644
>> --- a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatform.c
>> +++ b/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatform.c
>> @@ -46,7 +46,7 @@ UpdateAcpiDsdt (
>>      return;
>>    }
>>  
>> -  Status = EthMacInit ();
>> +  Status = UpdateAcpiDsdtTable ();
>>    if (EFI_ERROR (Status)) {
>>      DEBUG ((DEBUG_ERROR, " UpdateAcpiDsdtTable Failed, Status = %r\n", Status));
>>    }
>> diff --git a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
>> index cd98506..205f2f9 100644
>> --- a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
>> +++ b/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
>> @@ -1,8 +1,8 @@
>>  /** @file
>>  
>> -  Copyright (c) 2014, Applied Micro Curcuit Corporation. All rights reserved.<BR>
>> -  Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
>> -  Copyright (c) 2015, Linaro Limited. All rights reserved.<BR>
>> +  Copyright (c) 2014 - 2020, Applied Micro Curcuit Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2015 - 2020, Hisilicon Limited. All rights reserved.<BR>
>> +  Copyright (c) 2015 - 2020, Linaro Limited. All rights reserved.<BR>
> 
> Same comment as for 1/3.

Modify it in v2.

> 
>>    SPDX-License-Identifier: BSD-2-Clause-Patent
>>  
>>    This driver is called to initialize the FW part of the PHY in preparation
>> @@ -30,6 +30,7 @@
>>  #include <Library/UefiRuntimeServicesTableLib.h>
>>  #include <IndustryStandard/Acpi.h>
>>  #include <IndustryStandard/AcpiAml.h>
>> +#include <Library/MemoryAllocationLib.h>
> 
> Good lord this include block is a mess.
> Nevertheless, a less offensive place to insert this new header would
> be between
> #include <Library/DebugLib.h>
> and
> #include <Library/PcdLib.h>
> .

Ok, modify it in v2.

> 
>>  
>>  #include <Protocol/HisiBoardNicProtocol.h>
>>  
>> @@ -45,13 +46,20 @@
>>  #define EFI_ACPI_MAX_NUM_TABLES         20
>>  #define DSDT_SIGNATURE                  0x54445344
>>  
>> -#define D03_ACPI_ETH_ID                     "HISI00C2"
>> -
>>  #define ACPI_ETH_MAC_KEY                "local-mac-address"
>> +#define ACPI_ETH_SAS_KEY                 "sas-addr"
> 
> Please consider column alignment.
> 
>>  
>>  #define PREFIX_VARIABLE_NAME            L"MAC"
>>  #define PREFIX_VARIABLE_NAME_COMPAT     L"RGMII_MAC"
>> -#define MAC_MAX_LEN                     30
>> +#define ADDRESS_MAX_LEN                     30
> 
> Please consider column alignment.

Ok, modify it in v2.

> 
>>
>> +CHAR8 *mHisiAcpiDevId[] = {"HISI00C1","HISI00C2","HISI0162"};
>> +
>> +typedef enum {
>> +  DsdtDeviceUnknown,
>> +  DsdtDeviceLan,
>> +  DsdtDeviceSas
>> +} DSDT_DEVICE_TYPE;
>>  
>>  EFI_STATUS GetEnvMac(
>>    IN          UINTN    MacNextID,
>> @@ -89,12 +97,35 @@ EFI_STATUS GetEnvMac(
>>    return EFI_SUCCESS;
>>  }
>>  
>> -EFI_STATUS _SearchReplacePackageMACAddress(
>> +EFI_STATUS GetSasAddress (
> 
> Function name should be on separate line.
> If only used in this module, function should also be STATIC.

Ok, modify it in v2.

> 
>> +  IN UINT8        Index,
>> +  IN OUT UINT8    *SasAddrBuffer
>> +  )
>> +{
>> +  if (SasAddrBuffer == NULL) {
>> +      return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  SasAddrBuffer[0] = 0x50;
>> +  SasAddrBuffer[1] = 0x01;
>> +  SasAddrBuffer[2] = 0x88;
>> +  SasAddrBuffer[3] = 0x20;
>> +  SasAddrBuffer[4] = 0x16;
>> +  SasAddrBuffer[5] = 0x00;
>> +  SasAddrBuffer[6] = 0x00;
>> +  SasAddrBuffer[7] = Index;
> 
> What does the above do?
> What are the hardcoded values?

In v2, add a protocol interface to get sas address and this SasAddrBuffer assignment
will only in error branch.

> 
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS _SearchReplacePackageAddress(
> 
> Function name should be on separate line.
> If only used in this module, function should also be STATIC.
> Functions should not have names starting with _.

Ok, modify it in v2.

> 
>>    IN EFI_ACPI_SDT_PROTOCOL  *AcpiTableProtocol,
>>    IN EFI_ACPI_HANDLE        ChildHandle,
>>    IN UINTN                  Level,
>>    IN OUT BOOLEAN            *Found,
>> -  IN UINTN                  MacNextID)
>> +  IN UINTN                  DevNextID,
>> +  IN DSDT_DEVICE_TYPE       FoundDev
>> +  )
>>  {
>>    // ASL template for ethernet driver:
>>  /*
>> @@ -117,12 +148,18 @@ EFI_STATUS _SearchReplacePackageMACAddress(
>>    UINTN               Count;
>>    EFI_ACPI_HANDLE     CurrentHandle;
>>    EFI_ACPI_HANDLE     NextHandle;
>> -  UINT8               MACBuffer[MAC_MAX_LEN];
>> +  EFI_ACPI_HANDLE     Level1Handle;
>> +  UINT8               *AddressBuffer;
>> +  UINT8               AddressByte = 0;
>>  
>>    DBG("In Level:%d\n", Level);
>> +  Level1Handle = NULL;
>>    Status = EFI_SUCCESS;
>>    for (CurrentHandle = NULL; ;) {
>>      Status = AcpiTableProtocol->GetChild(ChildHandle, &CurrentHandle);
>> +    if (Level == 1) {
>> +      Level1Handle = CurrentHandle;
>> +    }
>>      if (Level != 3 && (EFI_ERROR(Status) || CurrentHandle == NULL))
>>         break;
>>  
>> @@ -143,11 +180,14 @@ EFI_STATUS _SearchReplacePackageMACAddress(
>>                DataSize, Data[0], DataSize > 1 ? Data[1] : 0);
>>  
>>        Data = Buffer;
>> -      if (DataType != EFI_ACPI_DATA_TYPE_STRING
>> -              || AsciiStrCmp((CHAR8 *) Data, ACPI_ETH_MAC_KEY) != 0)
>> +      if ((DataType != EFI_ACPI_DATA_TYPE_STRING) ||
>> +            ((AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_MAC_KEY) != 0) &&
>> +            (AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_SAS_KEY) != 0))) {
> 
> Indentation should help clarify the relationship between the differenc
> comparison values:
> 
>       if ((DataType != EFI_ACPI_DATA_TYPE_STRING) ||
>           ((AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_MAC_KEY) != 0) &&
>            (AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_SAS_KEY) != 0))) {

Ok, modify it in v2.

> 
>> +        ChildHandle = Level1Handle;
>>          continue;
>> +      }
>>  
>> -      DBG("_DSD Key Type %d. Found MAC address key\n", DataType);
>> +      DBG("_DSD Key Type %d. Found address key\n", DataType);
>>  
>>        //
>>        // We found the node.
>> @@ -157,13 +197,33 @@ EFI_STATUS _SearchReplacePackageMACAddress(
>>      }
>>  
>>      if (Level == 3 && *Found) {
>> +      AddressBuffer = AllocateZeroPool (ADDRESS_MAX_LEN);
>> +      if (AddressBuffer == NULL) {
>> +        DEBUG ((DEBUG_ERROR, "%a:%d AllocateZeroPool failed\n", __FILE__, __LINE__));
>> +        return EFI_OUT_OF_RESOURCES;
>> +      }
>>  
>> -      //Update the MAC
>> -      Status = GetEnvMac(MacNextID, MACBuffer);
>> -      if (EFI_ERROR(Status))
>> +      switch (FoundDev) {
>> +        case DsdtDeviceLan:
>> +          //Update the MAC
>> +          Status = GetEnvMac (DevNextID, AddressBuffer);
>> +          AddressByte = 6;
>> +          break;
>> +        case DsdtDeviceSas:
>> +          //Update SAS Address.
>> +          Status = GetSasAddress (DevNextID, AddressBuffer);
>> +          AddressByte = 8;
>> +          break;
>> +        default:
>> +          Status = EFI_INVALID_PARAMETER;
>> +      }
>> +      if (EFI_ERROR (Status)) {
>> +        FreePool (AddressBuffer);
>> +        AddressBuffer = NULL;
> 
> These nested for loops could certainly benefit from having their
> contents broken down into a few helper functions, but fundamentally -
> this AddressBuffer is only used again on another time around the loop,
> and if so, it is overwritten by the call to AllocateZeroPool. Why are
> we rewriting the pointer?

Good idea, I will break down it into a new helper funtion.

The reason for rewriting the pointer is that there is a internal programming
specification to do this. The rewriting is not need in reality, so I will
delete it in v2.

> 
>>          break;
>> +      }
>>  
>> -      for (Count = 0; Count < 6; Count++) {
>> +      for (Count = 0; Count < AddressByte; Count++) {
>>          Status = AcpiTableProtocol->GetOption(CurrentHandle, 1, &DataType, &Buffer, &DataSize);
>>          if (EFI_ERROR(Status))
>>            break;
>> @@ -177,13 +237,15 @@ EFI_STATUS _SearchReplacePackageMACAddress(
>>  
>>          // only need one byte.
>>          // FIXME: Assume the CPU is little endian
>> -        Status = AcpiTableProtocol->SetOption(CurrentHandle, 1, (VOID *)&MACBuffer[Count], sizeof(UINT8));
>> +        Status = AcpiTableProtocol->SetOption (CurrentHandle, 1, AddressBuffer + Count, sizeof(UINT8));
>>          if (EFI_ERROR(Status))
>>            break;
>>          Status = AcpiTableProtocol->GetChild(ChildHandle, &CurrentHandle);
>>          if (EFI_ERROR(Status) || CurrentHandle == NULL)
>>            break;
>>        }
>> +      FreePool (AddressBuffer);
>> +      AddressBuffer = NULL;
> 
> Same thing again - why rewrite the pointer after free?
> 
>>        break;
>>      }
>>  
>> @@ -192,7 +254,13 @@ EFI_STATUS _SearchReplacePackageMACAddress(
>>  
>>      //Search next package
>>      AcpiTableProtocol->Open((VOID *) Buffer, &NextHandle);
>> -    Status = _SearchReplacePackageMACAddress(AcpiTableProtocol, NextHandle, Level + 1, Found, MacNextID);
>> +    Status = _SearchReplacePackageAddress(
> 
> Indentation looks wrong because of the function name. Drop the leading
> _ and it will be fine.

Ok, modify it in v2.

> 
>> +               AcpiTableProtocol,
>> +               NextHandle,
>> +               Level + 1,
>> +               Found,
>> +               DevNextID,
>> +               FoundDev);
>>      AcpiTableProtocol->Close(NextHandle);
>>      if (!EFI_ERROR(Status))
>>        break;
>> @@ -201,22 +269,26 @@ EFI_STATUS _SearchReplacePackageMACAddress(
>>    return Status;
>>  }
>>  
>> -EFI_STATUS SearchReplacePackageMACAddress(
>> +EFI_STATUS SearchReplacePackageAddress(
>>    IN EFI_ACPI_SDT_PROTOCOL  *AcpiTableProtocol,
>>    IN EFI_ACPI_HANDLE        ChildHandle,
>> -  IN UINTN                  MacNextID)
>> +  IN UINTN                  DevNextID,
>> +  IN DSDT_DEVICE_TYPE       FoundDev
>> +  )
>>  {
>>    BOOLEAN Found = FALSE;
>>    UINTN Level = 0;
>>  
>> -  return _SearchReplacePackageMACAddress(AcpiTableProtocol, ChildHandle, Level, &Found, MacNextID);
>> +  return _SearchReplacePackageAddress(AcpiTableProtocol, ChildHandle, Level,
>> +                                      &Found, DevNextID, FoundDev);
>>  }
>>  
>>  EFI_STATUS
>> -GetEthID (
>> +GetDeviceInfo (
>>    EFI_ACPI_SDT_PROTOCOL   *AcpiTableProtocol,
>>    EFI_ACPI_HANDLE         ChildHandle,
>> -  UINTN                   *EthID
>> +  UINTN                   *DevID,
>> +  DSDT_DEVICE_TYPE        *FoundDev
>>    )
>>  {
>>    EFI_STATUS Status;
>> @@ -225,7 +297,7 @@ GetEthID (
>>    CONST VOID          *Buffer;
>>    UINTN               DataSize;
>>  
>> -  // Get NameString ETHx
>> +  // Get NameString
>>    Status = AcpiTableProtocol->GetOption (ChildHandle, 1, &DataType, &Buffer, &DataSize);
>>    if (EFI_ERROR (Status)) {
>>      DEBUG ((EFI_D_ERROR, "[%a:%d] Get NameString failed: %r\n", __FUNCTION__, __LINE__, Status));
>> @@ -236,14 +308,23 @@ GetEthID (
>>    DBG("Size %p Data %02x %02x %02x %02x\n", DataSize, Data[0], Data[1], Data[2], Data[3]);
>>  
>>    Data[4] = '\0';
>> -  if (DataSize != 4 ||
>> -      AsciiStrnCmp ("ETH", Data, 3) != 0 ||
>> -      Data[3] > '9' || Data[3] < '0') {
>> -    DEBUG ((EFI_D_ERROR, "[%a:%d] The NameString %a is not ETHn\n", __FUNCTION__, __LINE__, Data));
>> +  if ((DataSize != 4) ||
>> +    (Data[3] > '9' || Data[3] < '0')) {
>> +    DEBUG ((DEBUG_ERROR, "The NameString %a is not ETHn or SASn\n", Data));
>>      return EFI_INVALID_PARAMETER;
>>    }
>>  
>> -  *EthID = Data[3] - '0';
>> +  if (AsciiStrnCmp ("ETH", Data, 3) == 0) {
>> +    *FoundDev = DsdtDeviceLan;
>> +  } else if (AsciiStrnCmp ("SAS", Data, 3) == 0) {
>> +    *FoundDev = DsdtDeviceSas;
>> +  } else {
>> +    DEBUG ((DEBUG_ERROR, "[%a:%d] The NameString %a is not ETHn or SASn\n",
>> +            __FUNCTION__, __LINE__, Data));
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  *DevID = Data[3] - '0';
>>    return EFI_SUCCESS;
>>  }
>>  
>> @@ -257,8 +338,10 @@ EFI_STATUS ProcessDSDTDevice (
>>    CONST VOID          *Buffer;
>>    UINTN               DataSize;
>>    EFI_ACPI_HANDLE     DevHandle;
>> -  INTN                Found = 0;
>> -  UINTN               MacNextID;
>> +  DSDT_DEVICE_TYPE    FoundDev = DsdtDeviceUnknown;
>> +  UINTN               DevNextID;
>> +  BOOLEAN             HisiAcpiDevNotFound;
>> +  UINTN               Index;
>>  
>>    Status = AcpiTableProtocol->GetOption(ChildHandle, 0, &DataType, &Buffer, &DataSize);
>>    if (EFI_ERROR(Status))
>> @@ -280,7 +363,7 @@ EFI_STATUS ProcessDSDTDevice (
>>        break;
>>  
>>      //
>> -    // Search for _HID with Ethernet ID
>> +    // Search for _HID with Device ID
>>      //
>>      Status = AcpiTableProtocol->GetOption(DevHandle, 0, &DataType, &Buffer, &DataSize);
>>      if (EFI_ERROR(Status))
>> @@ -312,23 +395,34 @@ EFI_STATUS ProcessDSDTDevice (
>>            DBG("[%a:%d] - _HID = %a\n", __FUNCTION__, __LINE__, Data);
>>  
>>            if (EFI_ERROR(Status) ||
>> -              DataType != EFI_ACPI_DATA_TYPE_STRING ||
>> -              (AsciiStrCmp((CHAR8 *) Data, D03_ACPI_ETH_ID) != 0)) {
>> -            AcpiTableProtocol->Close(ValueHandle);
>> -            Found = 0;
>> +              DataType != EFI_ACPI_DATA_TYPE_STRING) {
>> +            AcpiTableProtocol->Close (ValueHandle);
>> +            FoundDev = DsdtDeviceUnknown;
>> +            continue;
>> +          }
>> +
>> +          HisiAcpiDevNotFound = TRUE;
>> +          for (Index = 0; Index < ARRAY_SIZE (mHisiAcpiDevId); Index++) {
>> +            if (AsciiStrCmp ((CHAR8 *)Data, mHisiAcpiDevId[Index]) == 0) {
>> +              HisiAcpiDevNotFound = FALSE;
>> +              break;
>> +            }
>> +          }
>> +          if (HisiAcpiDevNotFound) {
>> +            AcpiTableProtocol->Close (ValueHandle);
>> +            FoundDev = DsdtDeviceUnknown;
>>              continue;
>>            }
>>  
>> -          DBG("Found Ethernet device\n");
>> +          DBG("Found device\n");
>>            AcpiTableProtocol->Close(ValueHandle);
>> -          Status = GetEthID (AcpiTableProtocol, ChildHandle, &MacNextID);
>> +          Status = GetDeviceInfo (AcpiTableProtocol, ChildHandle, &DevNextID, &FoundDev);
>>            if (EFI_ERROR (Status)) {
>>              continue;
>>            }
>> -          Found = 1;
>> -        } else if (Found == 1 && AsciiStrnCmp((CHAR8 *) Data, "_DSD", 4) == 0) {
>> +        } else if ((FoundDev != DsdtDeviceUnknown) && AsciiStrnCmp((CHAR8 *) Data, "_DSD", 4) == 0) {
>>            //
>> -          // Patch MAC address for open source kernel
>> +          // Patch DSD data
>>            //
>>            EFI_ACPI_HANDLE    PkgHandle;
>>            Status = AcpiTableProtocol->GetOption(DevHandle, 2, &DataType, &Buffer, &DataSize);
>> @@ -351,12 +445,30 @@ EFI_STATUS ProcessDSDTDevice (
>>            //
>>            // Walk the _DSD node
>>            //
>> -          if (DataSize == 1 && Data[0] == AML_PACKAGE_OP)
>> -            Status = SearchReplacePackageMACAddress(AcpiTableProtocol, PkgHandle, MacNextID);
>> +          if (DataSize == 1 && Data[0] == AML_PACKAGE_OP) {
>> +            Status = SearchReplacePackageAddress (AcpiTableProtocol, PkgHandle, DevNextID, FoundDev);
>> +          }
>>  
>>            AcpiTableProtocol->Close(PkgHandle);
>> +        } else if (AsciiStrnCmp ((CHAR8 *) Data, "_ADR", 4) == 0) {
>> +          Status = AcpiTableProtocol->GetOption (DevHandle, 2, &DataType, &Buffer, &DataSize);
>> +          if (EFI_ERROR (Status)) {
>> +            break;
>> +          }
>> +
>> +          if (DataType != EFI_ACPI_DATA_TYPE_CHILD) {
>> +            continue;
>> +          }
>> +
>> +          Status = GetDeviceInfo (AcpiTableProtocol, ChildHandle, &DevNextID, &FoundDev);
>> +
>> +          if (EFI_ERROR (Status)) {
>> +            continue;
>> +          }
>>          }
>>        }
>> +    } else if ((DataSize == 2) && (Data[0] == AML_EXT_OP) && (Data[1] == AML_EXT_DEVICE_OP)) {
>> +      ProcessDSDTDevice (AcpiTableProtocol, DevHandle);
>>      }
>>    }
>>  
>> @@ -457,7 +569,7 @@ AcpiCheckSum (
>>    Buffer[ChecksumOffset] = CalculateCheckSum8 (Buffer, Table->Length);
>>  }
>>  
>> -EFI_STATUS EthMacInit(void)
>> +EFI_STATUS UpdateAcpiDsdtTable(void)
> 
> Function name on its own line.

Ok, modify it in v2.

Thanks,
Ming

> 
> /
>     Leif
> 
>>  {
>>    EFI_STATUS              Status;
>>    EFI_ACPI_SDT_PROTOCOL   *AcpiTableProtocol;
>> diff --git a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.h b/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.h
>> index 0a3e811..a7e1eed 100644
>> --- a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.h
>> +++ b/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.h
>> @@ -10,7 +10,7 @@
>>  #ifndef _ETH_MAC_H_
>>  #define _ETH_MAC_H_
>>  
>> -EFI_STATUS EthMacInit(VOID);
>> +EFI_STATUS UpdateAcpiDsdtTable (VOID);
>>  
>>  #endif
>>  
>> -- 
>> 2.8.1
>>
> 
> .
>