From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from huawei.com (huawei.com [45.249.212.191]) by mx.groups.io with SMTP id smtpd.web11.6890.1590589333799834807 for ; Wed, 27 May 2020 07:22:24 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.191, mailfrom: huangming23@huawei.com) Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 57A5A38E2EACBD23C3E6; Wed, 27 May 2020 22:21:40 +0800 (CST) Received: from [127.0.0.1] (10.78.51.60) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.487.0; Wed, 27 May 2020 22:21:32 +0800 Subject: Re: [RFC edk2-platforms v1 2/3] Silicon/Hisilicon/Acpi: Add update sas address feature To: Leif Lindholm CC: , , , , , References: <1590072184-16219-1-git-send-email-huangming23@huawei.com> <1590072184-16219-3-git-send-email-huangming23@huawei.com> <20200526184929.GP1923@vanye> From: Ming Huang Message-ID: <3d18962d-e539-0880-5b66-acd802333fda@huawei.com> Date: Wed, 27 May 2020 22:21:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20200526184929.GP1923@vanye> X-Originating-IP: [10.78.51.60] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="gbk" Content-Transfer-Encoding: quoted-printable =D4=DA 2020/5/27 2:49, Leif Lindholm =D0=B4=B5=C0: > 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 >> --- >> 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; >> } >> =20 >> - Status =3D EthMacInit (); >> + Status =3D UpdateAcpiDsdtTable (); >> if (EFI_ERROR (Status)) { >> DEBUG ((DEBUG_ERROR, " UpdateAcpiDsdtTable Failed, Status =3D %r\= n", Status)); >> } >> diff --git a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Sili= con/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 >> =20 >> - Copyright (c) 2014, Applied Micro Curcuit Corporation. All rights r= eserved.
>> - Copyright (c) 2015, Hisilicon Limited. All rights reserved.
>> - Copyright (c) 2015, Linaro Limited. All rights reserved.
>> + Copyright (c) 2014 - 2020, Applied Micro Curcuit Corporation. All r= ights reserved.
>> + Copyright (c) 2015 - 2020, Hisilicon Limited. All rights reserved.<= BR> >> + Copyright (c) 2015 - 2020, Linaro Limited. All rights reserved.
>=20 > Same comment as for 1/3. Modify it in v2. >=20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >> =20 >> This driver is called to initialize the FW part of the PHY in prepa= ration >> @@ -30,6 +30,7 @@ >> #include >> #include >> #include >> +#include >=20 > Good lord this include block is a mess. > Nevertheless, a less offensive place to insert this new header would > be between > #include > and > #include > . Ok, modify it in v2. >=20 >> =20 >> #include >> =20 >> @@ -45,13 +46,20 @@ >> #define EFI_ACPI_MAX_NUM_TABLES 20 >> #define DSDT_SIGNATURE 0x54445344 >> =20 >> -#define D03_ACPI_ETH_ID "HISI00C2" >> - >> #define ACPI_ETH_MAC_KEY "local-mac-address" >> +#define ACPI_ETH_SAS_KEY "sas-addr" >=20 > Please consider column alignment. >=20 >> =20 >> #define PREFIX_VARIABLE_NAME L"MAC" >> #define PREFIX_VARIABLE_NAME_COMPAT L"RGMII_MAC" >> -#define MAC_MAX_LEN 30 >> +#define ADDRESS_MAX_LEN 30 >=20 > Please consider column alignment. Ok, modify it in v2. >=20 >> >> +CHAR8 *mHisiAcpiDevId[] =3D {"HISI00C1","HISI00C2","HISI0162"}; >> + >> +typedef enum { >> + DsdtDeviceUnknown, >> + DsdtDeviceLan, >> + DsdtDeviceSas >> +} DSDT_DEVICE_TYPE; >> =20 >> EFI_STATUS GetEnvMac( >> IN UINTN MacNextID, >> @@ -89,12 +97,35 @@ EFI_STATUS GetEnvMac( >> return EFI_SUCCESS; >> } >> =20 >> -EFI_STATUS _SearchReplacePackageMACAddress( >> +EFI_STATUS GetSasAddress ( >=20 > Function name should be on separate line. > If only used in this module, function should also be STATIC. Ok, modify it in v2. >=20 >> + IN UINT8 Index, >> + IN OUT UINT8 *SasAddrBuffer >> + ) >> +{ >> + if (SasAddrBuffer =3D=3D NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + SasAddrBuffer[0] =3D 0x50; >> + SasAddrBuffer[1] =3D 0x01; >> + SasAddrBuffer[2] =3D 0x88; >> + SasAddrBuffer[3] =3D 0x20; >> + SasAddrBuffer[4] =3D 0x16; >> + SasAddrBuffer[5] =3D 0x00; >> + SasAddrBuffer[6] =3D 0x00; >> + SasAddrBuffer[7] =3D Index; >=20 > 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. >=20 >> + >> + return EFI_SUCCESS; >> +} >> + >> +EFI_STATUS _SearchReplacePackageAddress( >=20 > 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. >=20 >> 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 =3D 0; >> =20 >> DBG("In Level:%d\n", Level); >> + Level1Handle =3D NULL; >> Status =3D EFI_SUCCESS; >> for (CurrentHandle =3D NULL; ;) { >> Status =3D AcpiTableProtocol->GetChild(ChildHandle, &CurrentHandl= e); >> + if (Level =3D=3D 1) { >> + Level1Handle =3D CurrentHandle; >> + } >> if (Level !=3D 3 && (EFI_ERROR(Status) || CurrentHandle =3D=3D NU= LL)) >> break; >> =20 >> @@ -143,11 +180,14 @@ EFI_STATUS _SearchReplacePackageMACAddress( >> DataSize, Data[0], DataSize > 1 ? Data[1] : 0); >> =20 >> Data =3D Buffer; >> - if (DataType !=3D EFI_ACPI_DATA_TYPE_STRING >> - || AsciiStrCmp((CHAR8 *) Data, ACPI_ETH_MAC_KEY) !=3D 0= ) >> + if ((DataType !=3D EFI_ACPI_DATA_TYPE_STRING) || >> + ((AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_MAC_KEY) !=3D 0) = && >> + (AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_SAS_KEY) !=3D 0)))= { >=20 > Indentation should help clarify the relationship between the differenc > comparison values: >=20 > if ((DataType !=3D EFI_ACPI_DATA_TYPE_STRING) || > ((AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_MAC_KEY) !=3D 0) && > (AsciiStrCmp ((CHAR8 *) Data, ACPI_ETH_SAS_KEY) !=3D 0))) { Ok, modify it in v2. >=20 >> + ChildHandle =3D Level1Handle; >> continue; >> + } >> =20 >> - DBG("_DSD Key Type %d. Found MAC address key\n", DataType); >> + DBG("_DSD Key Type %d. Found address key\n", DataType); >> =20 >> // >> // We found the node. >> @@ -157,13 +197,33 @@ EFI_STATUS _SearchReplacePackageMACAddress( >> } >> =20 >> if (Level =3D=3D 3 && *Found) { >> + AddressBuffer =3D AllocateZeroPool (ADDRESS_MAX_LEN); >> + if (AddressBuffer =3D=3D NULL) { >> + DEBUG ((DEBUG_ERROR, "%a:%d AllocateZeroPool failed\n", __FIL= E__, __LINE__)); >> + return EFI_OUT_OF_RESOURCES; >> + } >> =20 >> - //Update the MAC >> - Status =3D GetEnvMac(MacNextID, MACBuffer); >> - if (EFI_ERROR(Status)) >> + switch (FoundDev) { >> + case DsdtDeviceLan: >> + //Update the MAC >> + Status =3D GetEnvMac (DevNextID, AddressBuffer); >> + AddressByte =3D 6; >> + break; >> + case DsdtDeviceSas: >> + //Update SAS Address. >> + Status =3D GetSasAddress (DevNextID, AddressBuffer); >> + AddressByte =3D 8; >> + break; >> + default: >> + Status =3D EFI_INVALID_PARAMETER; >> + } >> + if (EFI_ERROR (Status)) { >> + FreePool (AddressBuffer); >> + AddressBuffer =3D NULL; >=20 > 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 programm= ing specification to do this. The rewriting is not need in reality, so I will delete it in v2. >=20 >> break; >> + } >> =20 >> - for (Count =3D 0; Count < 6; Count++) { >> + for (Count =3D 0; Count < AddressByte; Count++) { >> Status =3D AcpiTableProtocol->GetOption(CurrentHandle, 1, &Da= taType, &Buffer, &DataSize); >> if (EFI_ERROR(Status)) >> break; >> @@ -177,13 +237,15 @@ EFI_STATUS _SearchReplacePackageMACAddress( >> =20 >> // only need one byte. >> // FIXME: Assume the CPU is little endian >> - Status =3D AcpiTableProtocol->SetOption(CurrentHandle, 1, (VO= ID *)&MACBuffer[Count], sizeof(UINT8)); >> + Status =3D AcpiTableProtocol->SetOption (CurrentHandle, 1, Ad= dressBuffer + Count, sizeof(UINT8)); >> if (EFI_ERROR(Status)) >> break; >> Status =3D AcpiTableProtocol->GetChild(ChildHandle, &CurrentH= andle); >> if (EFI_ERROR(Status) || CurrentHandle =3D=3D NULL) >> break; >> } >> + FreePool (AddressBuffer); >> + AddressBuffer =3D NULL; >=20 > Same thing again - why rewrite the pointer after free? >=20 >> break; >> } >> =20 >> @@ -192,7 +254,13 @@ EFI_STATUS _SearchReplacePackageMACAddress( >> =20 >> //Search next package >> AcpiTableProtocol->Open((VOID *) Buffer, &NextHandle); >> - Status =3D _SearchReplacePackageMACAddress(AcpiTableProtocol, Nex= tHandle, Level + 1, Found, MacNextID); >> + Status =3D _SearchReplacePackageAddress( >=20 > Indentation looks wrong because of the function name. Drop the leading > _ and it will be fine. Ok, modify it in v2. >=20 >> + AcpiTableProtocol, >> + NextHandle, >> + Level + 1, >> + Found, >> + DevNextID, >> + FoundDev); >> AcpiTableProtocol->Close(NextHandle); >> if (!EFI_ERROR(Status)) >> break; >> @@ -201,22 +269,26 @@ EFI_STATUS _SearchReplacePackageMACAddress( >> return Status; >> } >> =20 >> -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 =3D FALSE; >> UINTN Level =3D 0; >> =20 >> - return _SearchReplacePackageMACAddress(AcpiTableProtocol, ChildHand= le, Level, &Found, MacNextID); >> + return _SearchReplacePackageAddress(AcpiTableProtocol, ChildHandle,= Level, >> + &Found, DevNextID, FoundDev); >> } >> =20 >> 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; >> =20 >> - // Get NameString ETHx >> + // Get NameString >> Status =3D AcpiTableProtocol->GetOption (ChildHandle, 1, &DataType,= &Buffer, &DataSize); >> if (EFI_ERROR (Status)) { >> DEBUG ((EFI_D_ERROR, "[%a:%d] Get NameString failed: %r\n", __FUN= CTION__, __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]); >> =20 >> Data[4] =3D '\0'; >> - if (DataSize !=3D 4 || >> - AsciiStrnCmp ("ETH", Data, 3) !=3D 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 !=3D 4) || >> + (Data[3] > '9' || Data[3] < '0')) { >> + DEBUG ((DEBUG_ERROR, "The NameString %a is not ETHn or SASn\n", D= ata)); >> return EFI_INVALID_PARAMETER; >> } >> =20 >> - *EthID =3D Data[3] - '0'; >> + if (AsciiStrnCmp ("ETH", Data, 3) =3D=3D 0) { >> + *FoundDev =3D DsdtDeviceLan; >> + } else if (AsciiStrnCmp ("SAS", Data, 3) =3D=3D 0) { >> + *FoundDev =3D DsdtDeviceSas; >> + } else { >> + DEBUG ((DEBUG_ERROR, "[%a:%d] The NameString %a is not ETHn or SA= Sn\n", >> + __FUNCTION__, __LINE__, Data)); >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + *DevID =3D Data[3] - '0'; >> return EFI_SUCCESS; >> } >> =20 >> @@ -257,8 +338,10 @@ EFI_STATUS ProcessDSDTDevice ( >> CONST VOID *Buffer; >> UINTN DataSize; >> EFI_ACPI_HANDLE DevHandle; >> - INTN Found =3D 0; >> - UINTN MacNextID; >> + DSDT_DEVICE_TYPE FoundDev =3D DsdtDeviceUnknown; >> + UINTN DevNextID; >> + BOOLEAN HisiAcpiDevNotFound; >> + UINTN Index; >> =20 >> Status =3D AcpiTableProtocol->GetOption(ChildHandle, 0, &DataType, = &Buffer, &DataSize); >> if (EFI_ERROR(Status)) >> @@ -280,7 +363,7 @@ EFI_STATUS ProcessDSDTDevice ( >> break; >> =20 >> // >> - // Search for _HID with Ethernet ID >> + // Search for _HID with Device ID >> // >> Status =3D AcpiTableProtocol->GetOption(DevHandle, 0, &DataType, = &Buffer, &DataSize); >> if (EFI_ERROR(Status)) >> @@ -312,23 +395,34 @@ EFI_STATUS ProcessDSDTDevice ( >> DBG("[%a:%d] - _HID =3D %a\n", __FUNCTION__, __LINE__, Data= ); >> =20 >> if (EFI_ERROR(Status) || >> - DataType !=3D EFI_ACPI_DATA_TYPE_STRING || >> - (AsciiStrCmp((CHAR8 *) Data, D03_ACPI_ETH_ID) !=3D 0)) = { >> - AcpiTableProtocol->Close(ValueHandle); >> - Found =3D 0; >> + DataType !=3D EFI_ACPI_DATA_TYPE_STRING) { >> + AcpiTableProtocol->Close (ValueHandle); >> + FoundDev =3D DsdtDeviceUnknown; >> + continue; >> + } >> + >> + HisiAcpiDevNotFound =3D TRUE; >> + for (Index =3D 0; Index < ARRAY_SIZE (mHisiAcpiDevId); Inde= x++) { >> + if (AsciiStrCmp ((CHAR8 *)Data, mHisiAcpiDevId[Index]) =3D= =3D 0) { >> + HisiAcpiDevNotFound =3D FALSE; >> + break; >> + } >> + } >> + if (HisiAcpiDevNotFound) { >> + AcpiTableProtocol->Close (ValueHandle); >> + FoundDev =3D DsdtDeviceUnknown; >> continue; >> } >> =20 >> - DBG("Found Ethernet device\n"); >> + DBG("Found device\n"); >> AcpiTableProtocol->Close(ValueHandle); >> - Status =3D GetEthID (AcpiTableProtocol, ChildHandle, &MacNe= xtID); >> + Status =3D GetDeviceInfo (AcpiTableProtocol, ChildHandle, &= DevNextID, &FoundDev); >> if (EFI_ERROR (Status)) { >> continue; >> } >> - Found =3D 1; >> - } else if (Found =3D=3D 1 && AsciiStrnCmp((CHAR8 *) Data, "_D= SD", 4) =3D=3D 0) { >> + } else if ((FoundDev !=3D DsdtDeviceUnknown) && AsciiStrnCmp(= (CHAR8 *) Data, "_DSD", 4) =3D=3D 0) { >> // >> - // Patch MAC address for open source kernel >> + // Patch DSD data >> // >> EFI_ACPI_HANDLE PkgHandle; >> Status =3D AcpiTableProtocol->GetOption(DevHandle, 2, &Data= Type, &Buffer, &DataSize); >> @@ -351,12 +445,30 @@ EFI_STATUS ProcessDSDTDevice ( >> // >> // Walk the _DSD node >> // >> - if (DataSize =3D=3D 1 && Data[0] =3D=3D AML_PACKAGE_OP) >> - Status =3D SearchReplacePackageMACAddress(AcpiTableProtoc= ol, PkgHandle, MacNextID); >> + if (DataSize =3D=3D 1 && Data[0] =3D=3D AML_PACKAGE_OP) { >> + Status =3D SearchReplacePackageAddress (AcpiTableProtocol= , PkgHandle, DevNextID, FoundDev); >> + } >> =20 >> AcpiTableProtocol->Close(PkgHandle); >> + } else if (AsciiStrnCmp ((CHAR8 *) Data, "_ADR", 4) =3D=3D 0)= { >> + Status =3D AcpiTableProtocol->GetOption (DevHandle, 2, &Dat= aType, &Buffer, &DataSize); >> + if (EFI_ERROR (Status)) { >> + break; >> + } >> + >> + if (DataType !=3D EFI_ACPI_DATA_TYPE_CHILD) { >> + continue; >> + } >> + >> + Status =3D GetDeviceInfo (AcpiTableProtocol, ChildHandle, &= DevNextID, &FoundDev); >> + >> + if (EFI_ERROR (Status)) { >> + continue; >> + } >> } >> } >> + } else if ((DataSize =3D=3D 2) && (Data[0] =3D=3D AML_EXT_OP) && = (Data[1] =3D=3D AML_EXT_DEVICE_OP)) { >> + ProcessDSDTDevice (AcpiTableProtocol, DevHandle); >> } >> } >> =20 >> @@ -457,7 +569,7 @@ AcpiCheckSum ( >> Buffer[ChecksumOffset] =3D CalculateCheckSum8 (Buffer, Table->Lengt= h); >> } >> =20 >> -EFI_STATUS EthMacInit(void) >> +EFI_STATUS UpdateAcpiDsdtTable(void) >=20 > Function name on its own line. Ok, modify it in v2. Thanks, Ming >=20 > / > Leif >=20 >> { >> EFI_STATUS Status; >> EFI_ACPI_SDT_PROTOCOL *AcpiTableProtocol; >> diff --git a/Silicon/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.h b/Sili= con/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_ >> =20 >> -EFI_STATUS EthMacInit(VOID); >> +EFI_STATUS UpdateAcpiDsdtTable (VOID); >> =20 >> #endif >> =20 >> --=20 >> 2.8.1 >> >=20 > . >=20