[Linaro-uefi,27/27] D03/D05: Change to access EEPROM data by checking page boundary

We would get incorrect data when accessing multiple bytes at one time
and going across EEPROM page boundary, so we split data when the length
greater than page boundary.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Peicong Li <lipeicong@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 .../Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c  | 35 ++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

On Thu, Nov 10, 2016 at 09:52:30PM +0800, Heyi Guo wrote:
> We would get incorrect data when accessing multiple bytes at one time
> and going across EEPROM page boundary, so we split data when the length
> greater than page boundary.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Peicong Li <lipeicong@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  .../Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c  | 35 ++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
> index 994ed6a..eaabb1d 100644
> --- a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
> +++ b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
> @@ -26,6 +26,7 @@
>  #include <Library/I2CLib.h>
>  
>  #define EEPROM_I2C_PORT 6
> +#define EEPROM_PAGE_SIZE 0x40
>  
>  EFI_STATUS
>  EFIAPI OemGetMac2P (IN OUT EFI_MAC_ADDRESS *Mac, IN UINTN Port);
> @@ -110,6 +111,8 @@ EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr)
>      UINT16     I2cOffset;
>      UINT16     crc16;
>      NIC_MAC_ADDRESS stMacDesc = {0};
> +    UINT16     RemainderMacOffset;
> +    UINT16     LessSizeOfPage;
>  
>      Status = I2CInit(0, EEPROM_I2C_PORT, Normal);
>      if (EFI_ERROR(Status))
> @@ -124,7 +127,20 @@ EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr)
>      stI2cDev.Port = EEPROM_I2C_PORT;
>      stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM;
>      stI2cDev.Socket = 0;
> -    Status = I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    RemainderMacOffset = I2cOffset % EEPROM_PAGE_SIZE;
> +    LessSizeOfPage = EEPROM_PAGE_SIZE - RemainderMacOffset;
> +    //The length of NIC_MAC_ADDRESS is 10 bytes long,
> +    //It surly less than EEPROM page size, so we could
> +    //code as bellow, check the address whether across the page boundary,
> +    //and split the data when across page boundary.
> +    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
> +      Status |= I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    } else {
> +      Status |= I2CRead(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);

No need for |=. Status is always clear if we get here.

> +      if (!(EFI_ERROR(Status))) {
> +      Status |= I2CRead(&stI2cDev, I2cOffset + LessSizeOfPage, sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage, (UINT8 *)&stMacDesc + LessSizeOfPage);
> +      }

Indentation.
Also, would make sense to wrap this long line.

Could write as

    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
      Status = I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
    } else {
      Status = I2CRead(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);
      Status |= I2CRead(
                        &stI2cDev,
                        I2cOffset + LessSizeOfPage,
                        sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage,
		        (UINT8 *)&stMacDesc + LessSizeOfPage
		       );
    }

Could also keep the "if (!EFI_ERROR" bit, if there is a problem with
performing a second access if the first failed. But there is no point
with the |= then, since we know Status is 0 if the code is executed.

> +    }
>      if (EFI_ERROR(Status))
>      {
>          DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cRead failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status));
> @@ -153,6 +169,8 @@ EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr)
>  
>  
>      stMacDesc.MacLen = MAC_ADDR_LEN;
> +    UINT16     RemainderMacOffset;
> +    UINT16     LessSizeOfPage;
>      gBS->CopyMem((VOID *)(stMacDesc.Mac), (VOID *)pucAddr, MAC_ADDR_LEN);
>  
>      stMacDesc.Crc16  = make_crc_checksum((UINT8 *)&(stMacDesc.MacLen), sizeof(stMacDesc.MacLen) + MAC_ADDR_LEN);
> @@ -170,7 +188,20 @@ EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr)
>      stI2cDev.Port = EEPROM_I2C_PORT;
>      stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM;
>      stI2cDev.Socket = 0;
> -    Status = I2CWrite(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    RemainderMacOffset = I2cOffset % EEPROM_PAGE_SIZE;
> +    LessSizeOfPage = EEPROM_PAGE_SIZE - RemainderMacOffset;
> +    //The length of NIC_MAC_ADDRESS is 10 bytes long,
> +    //It surly less than EEPROM page size, so we could
> +    //code as bellow, check the address whether across the page boundary,
> +    //and split the data when across page boundary.
> +    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
> +      Status |= I2CWrite(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    } else {
> +      Status |= I2CWrite(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);
> +      if (!(EFI_ERROR(Status))) {
> +      Status |= I2CWrite(&stI2cDev, I2cOffset + LessSizeOfPage, sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage, (UINT8 *)&stMacDesc + LessSizeOfPage);


Identical comments as for the Get function.

> +      }
> +    }
>      if (EFI_ERROR(Status))
>      {
>          DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cWrite failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status));
> -- 
> 1.9.1
>